Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1267093002:
add SVG zoom/scroll case to perf test (Closed)
|
Created:
5 years, 4 months ago by junchao.han Modified:
5 years, 3 months ago Reviewers:
mtklein_C, nednguyen, fs, Jin Yang, kouhei (in TOK), sullivan, pdr., william.xie1, yongnian.le CC:
blink-reviews Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
Descriptionadd SVG zoom/scroll case to perf test
This case contains SVG zoom/scroll animation including 20 frames zoom in,
20 frames scroll and 20 frames zoom out. Final result is total time used to
animate these 60 frames.
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201291
Patch Set 1 #Patch Set 2 : upload bm2.1 svg case #Patch Set 3 : try upload large file #Patch Set 4 : add raf workload #
Total comments: 17
Patch Set 5 : fix comments #
Total comments: 4
Patch Set 6 : fix comments for patch set 5 #Patch Set 7 : run total animation 5 times #
Total comments: 1
Patch Set 8 : fix comments for patch set 7 and reduce inner animation loop #Patch Set 9 : replace Banboo.svg with Cowboy.svg #Messages
Total messages: 18 (4 generated)
junchao.han@intel.com changed reviewers: + kouhei@chromium.org, mtklein@chromium.org, nednguyen@google.com, pdr@chromium.org, sullivan@chromium.org
pdr@chromium.org changed reviewers: + fs@opera.com
Right now we're using 20 loop iterations, whereas other tests such as SVGHitTesting, do 6000. Can you do a test to determine the variance of results from this test? I suspect we will need to increase the loop iterations significantly to get consistent results. That is, run the test 10 times, then compute the standard deviation and double-check it's reasonably small (or comparable to other svg perf tests). I'm not sure how to go about actually wiring this up so it's represented on chromeperf.appspot.com. @fs, do you know by chance? https://codereview.chromium.org/1267093002/diff/30001/PerformanceTests/SVG/Ba... File PerformanceTests/SVG/Bamboo_transform.html (right): https://codereview.chromium.org/1267093002/diff/30001/PerformanceTests/SVG/Ba... PerformanceTests/SVG/Bamboo_transform.html:10: var loop0 = 20; Lets change this to a static in all caps. Maybe var LOOP_ITERATIONS = 20;? https://codereview.chromium.org/1267093002/diff/30001/PerformanceTests/SVG/Ba... PerformanceTests/SVG/Bamboo_transform.html:28: endTime = Date.now(); Lets use performance.now if available: endTime = window.performance ? performance.now() : Date.now(); https://codereview.chromium.org/1267093002/diff/30001/PerformanceTests/SVG/Ba... PerformanceTests/SVG/Bamboo_transform.html:35: if (state=='zoomin'){ Lets replace these if(...) blocks with a switch statement (here, and above) https://codereview.chromium.org/1267093002/diff/30001/PerformanceTests/SVG/re... File PerformanceTests/SVG/resources/Bamboo.svg (right): https://codereview.chromium.org/1267093002/diff/30001/PerformanceTests/SVG/re... PerformanceTests/SVG/resources/Bamboo.svg:5: +<g id="svg-matrix" transform="matrix(1 0 0 1 0 0)"> This has the potential to affect other perf tests. I think the effect will be minor though, so lets go with it.
On 2015/08/19 17:28:36, pdr wrote: ... > I'm not sure how to go about actually wiring this up so it's represented on > http://chromeperf.appspot.com. @fs, do you know by chance? Hmm, sorry, no. https://codereview.chromium.org/1267093002/diff/30001/PerformanceTests/SVG/Ba... File PerformanceTests/SVG/Bamboo_transform.html (right): https://codereview.chromium.org/1267093002/diff/30001/PerformanceTests/SVG/Ba... PerformanceTests/SVG/Bamboo_transform.html:1: <pre id="log"></pre> Add <!DOCTYPE html>? (Who wants quirks mode anyway?) https://codereview.chromium.org/1267093002/diff/30001/PerformanceTests/SVG/Ba... PerformanceTests/SVG/Bamboo_transform.html:3: <embed id="svg-content" src="./resources/Bamboo.svg" width="400px" height="400px" type="image/svg+xml" /> Use <object> instead of <embed>? https://codereview.chromium.org/1267093002/diff/30001/PerformanceTests/SVG/Ba... PerformanceTests/SVG/Bamboo_transform.html:52: svgContent = document.getElementById('svg-content').getSVGDocument().documentElement; Prefer to not use getSVGDocument() - contentDocument ought to work as well. https://codereview.chromium.org/1267093002/diff/30001/PerformanceTests/SVG/Ba... PerformanceTests/SVG/Bamboo_transform.html:57: setTimeout(init,100); This looks a bit sketchy - listen for a load event instead? https://codereview.chromium.org/1267093002/diff/30001/PerformanceTests/SVG/re... File PerformanceTests/SVG/resources/Bamboo.svg (right): https://codereview.chromium.org/1267093002/diff/30001/PerformanceTests/SVG/re... PerformanceTests/SVG/resources/Bamboo.svg:5: +<g id="svg-matrix" transform="matrix(1 0 0 1 0 0)"> On 2015/08/19 17:28:36, pdr wrote: > This has the potential to affect other perf tests. I think the effect will be > minor though, so lets go with it. I'm not quite sure what this is actually used for... The JS sets inline style on documentElement - which will be the document root (<svg>) - same but different. Regardless, the attribute is redundant (because inline style is used, and 'transform' is not yet a presentation attribute.) Since we're reaching into the SVG document anyway, it'd be possible to wrap dynamically, but yeah hopefully one more in depth for most nodes won't be too visible...
junchao.han@intel.com changed reviewers: + jin.a.yang@intel.com, william.xie@intel.com, yongnian.le@intel.com
Comments fixed and I run the test case 5 time manually, here is result: data: 7414, 7365, 7459, 7507, 7403 avg: 7430 std: 55 std/svg: 0.7% https://codereview.chromium.org/1267093002/diff/30001/PerformanceTests/SVG/Ba... File PerformanceTests/SVG/Bamboo_transform.html (right): https://codereview.chromium.org/1267093002/diff/30001/PerformanceTests/SVG/Ba... PerformanceTests/SVG/Bamboo_transform.html:1: <pre id="log"></pre> On 2015/08/19 23:04:32, fs wrote: > Add <!DOCTYPE html>? (Who wants quirks mode anyway?) Done. https://codereview.chromium.org/1267093002/diff/30001/PerformanceTests/SVG/Ba... PerformanceTests/SVG/Bamboo_transform.html:3: <embed id="svg-content" src="./resources/Bamboo.svg" width="400px" height="400px" type="image/svg+xml" /> On 2015/08/19 23:04:31, fs wrote: > Use <object> instead of <embed>? Done. https://codereview.chromium.org/1267093002/diff/30001/PerformanceTests/SVG/Ba... PerformanceTests/SVG/Bamboo_transform.html:10: var loop0 = 20; On 2015/08/19 17:28:35, pdr wrote: > Lets change this to a static in all caps. Maybe var LOOP_ITERATIONS = 20;? Done. https://codereview.chromium.org/1267093002/diff/30001/PerformanceTests/SVG/Ba... PerformanceTests/SVG/Bamboo_transform.html:28: endTime = Date.now(); On 2015/08/19 17:28:35, pdr wrote: > Lets use performance.now if available: > endTime = window.performance ? performance.now() : Date.now(); Done. https://codereview.chromium.org/1267093002/diff/30001/PerformanceTests/SVG/Ba... PerformanceTests/SVG/Bamboo_transform.html:35: if (state=='zoomin'){ On 2015/08/19 17:28:35, pdr wrote: > Lets replace these if(...) blocks with a switch statement (here, and above) Done. https://codereview.chromium.org/1267093002/diff/30001/PerformanceTests/SVG/Ba... PerformanceTests/SVG/Bamboo_transform.html:52: svgContent = document.getElementById('svg-content').getSVGDocument().documentElement; On 2015/08/19 23:04:31, fs wrote: > Prefer to not use getSVGDocument() - contentDocument ought to work as well. Done. https://codereview.chromium.org/1267093002/diff/30001/PerformanceTests/SVG/Ba... PerformanceTests/SVG/Bamboo_transform.html:57: setTimeout(init,100); On 2015/08/19 23:04:32, fs wrote: > This looks a bit sketchy - listen for a load event instead? Done. https://codereview.chromium.org/1267093002/diff/30001/PerformanceTests/SVG/re... File PerformanceTests/SVG/resources/Bamboo.svg (right): https://codereview.chromium.org/1267093002/diff/30001/PerformanceTests/SVG/re... PerformanceTests/SVG/resources/Bamboo.svg:5: +<g id="svg-matrix" transform="matrix(1 0 0 1 0 0)"> On 2015/08/19 23:04:32, fs wrote: > On 2015/08/19 17:28:36, pdr wrote: > > This has the potential to affect other perf tests. I think the effect will be > > minor though, so lets go with it. > > I'm not quite sure what this is actually used for... The JS sets inline style on > documentElement - which will be the document root (<svg>) - same but different. > Regardless, the attribute is redundant (because inline style is used, and > 'transform' is not yet a presentation attribute.) Since we're reaching into the > SVG document anyway, it'd be possible to wrap dynamically, but yeah hopefully > one more in depth for most nodes won't be too visible... This change is no need. Removed.
If you havno bug to reference, you can drop the BUG= https://codereview.chromium.org/1267093002/diff/50001/PerformanceTests/SVG/Ba... File PerformanceTests/SVG/Bamboo_transform.html (right): https://codereview.chromium.org/1267093002/diff/50001/PerformanceTests/SVG/Ba... PerformanceTests/SVG/Bamboo_transform.html:2: <body> Redundant (the parser will insert one automatically on the next line) https://codereview.chromium.org/1267093002/diff/50001/PerformanceTests/SVG/Ba... PerformanceTests/SVG/Bamboo_transform.html:6: <param name="src" value="./resources/Bamboo.svg" Redundant.
On 2015/08/19 17:28:36, pdr wrote: > Right now we're using 20 loop iterations, whereas other tests such as > SVGHitTesting, do 6000. Can you do a test to determine the variance of results > from this test? I suspect we will need to increase the loop iterations > significantly to get consistent results. That is, run the test 10 times, then > compute the standard deviation and double-check it's reasonably small (or > comparable to other svg perf tests). > > I'm not sure how to go about actually wiring this up so it's represented on > http://chromeperf.appspot.com. @fs, do you know by chance? For blink_perf tests, when you add a new file to PerformanceTests/SVG, it automatically starts reporting the data to the dashboard and monitoring it. (If you add a new directory to PerformanceTests, you need to add that to https://code.google.com/p/chromium/codesearch#chromium/src/tools/perf/benchma...)
https://codereview.chromium.org/1267093002/diff/50001/PerformanceTests/SVG/Ba... File PerformanceTests/SVG/Bamboo_transform.html (right): https://codereview.chromium.org/1267093002/diff/50001/PerformanceTests/SVG/Ba... PerformanceTests/SVG/Bamboo_transform.html:2: <body> On 2015/08/20 09:06:29, fs wrote: > Redundant (the parser will insert one automatically on the next line) Done. https://codereview.chromium.org/1267093002/diff/50001/PerformanceTests/SVG/Ba... PerformanceTests/SVG/Bamboo_transform.html:6: <param name="src" value="./resources/Bamboo.svg" On 2015/08/20 09:06:29, fs wrote: > Redundant. Done.
On 2015/08/19 17:28:36, pdr wrote: > Right now we're using 20 loop iterations, whereas other tests such as > SVGHitTesting, do 6000. Can you do a test to determine the variance of results > from this test? I suspect we will need to increase the loop iterations > significantly to get consistent results. That is, run the test 10 times, then > compute the standard deviation and double-check it's reasonably small (or > comparable to other svg perf tests). > > I'm not sure how to go about actually wiring this up so it's represented on > http://chromeperf.appspot.com. @fs, do you know by chance? > > https://codereview.chromium.org/1267093002/diff/30001/PerformanceTests/SVG/Ba... > File PerformanceTests/SVG/Bamboo_transform.html (right): > > https://codereview.chromium.org/1267093002/diff/30001/PerformanceTests/SVG/Ba... > PerformanceTests/SVG/Bamboo_transform.html:10: var loop0 = 20; > Lets change this to a static in all caps. Maybe var LOOP_ITERATIONS = 20;? > > https://codereview.chromium.org/1267093002/diff/30001/PerformanceTests/SVG/Ba... > PerformanceTests/SVG/Bamboo_transform.html:28: endTime = Date.now(); > Lets use performance.now if available: > endTime = window.performance ? performance.now() : Date.now(); > > https://codereview.chromium.org/1267093002/diff/30001/PerformanceTests/SVG/Ba... > PerformanceTests/SVG/Bamboo_transform.html:35: if (state=='zoomin'){ > Lets replace these if(...) blocks with a switch statement (here, and above) > > https://codereview.chromium.org/1267093002/diff/30001/PerformanceTests/SVG/re... > File PerformanceTests/SVG/resources/Bamboo.svg (right): > > https://codereview.chromium.org/1267093002/diff/30001/PerformanceTests/SVG/re... > PerformanceTests/SVG/resources/Bamboo.svg:5: +<g id="svg-matrix" > transform="matrix(1 0 0 1 0 0)"> > This has the potential to affect other perf tests. I think the effect will be > minor though, so lets go with it. Outer side loop is added to run zoom/scroll animation 5 times. Here is testing output: ... [ RUN ] Bamboo_transform.html Time: values 7097.495, 7108.405, 7137.16, 7153.750000000004, 7136.485000000001 ms avg 7126.659000000001 ms median 7136.485000000001 ms stdev 23.046100375119135 ms min 7097.495 ms max 7153.750000000004 ms [ OK ] Bamboo_transform.html (40308 ms) ... RESULT Bamboo_transform: Bamboo_transform.html= [7097.495000,7108.405000,7137.160000,7153.750000,7136.485000] ms Avg Bamboo_transform: 7126.659000ms Sd Bamboo_transform: 23.046100ms *RESULT Bamboo_transform: Bamboo_transform= [7097.495000,7108.405000,7137.160000,7153.750000,7136.485000] ms Avg Bamboo_transform: 7126.659000ms Sd Bamboo_transform: 23.046100ms ...
I'm not sure anyone has added to these tests before which is why this review is sort of slow :/ Assuming there are no issues from reviewers, lets go ahead and land this once the comments below are addressed. I'll liaison with the perf sheriff for you. I verified this will automatically work through telemetry and will show up on the blink_perf.svg bots on chromeperf.appspot.com. I did notice that the test is an order of magnitude slower than other tests in the suite though. I think we'll need to get the total test time down a little before we can land this since it'll take ages to run this test on android: RESULT Bamboo_transform: Bamboo_transform.html= [2776.235000,2869.945000,2490.170000,1310.265000,1226.475000] ms Avg Bamboo_transform: 2134.618000ms Sd Bamboo_transform: 803.597687ms RESULT SvgCubics: SvgCubics.html= [13.505000,13.575000,11.895000,11.975000,12.730000] ms Avg SvgCubics: 12.736000ms Sd SvgCubics: 0.803387ms Any single draw of the bamboo will take a while so I don't think we can expect this to get to 12ms, but something around the 200ms/run range seems more reasonable. If bamboo is too slow, you may want to try another one of the svg files in resources/. https://codereview.chromium.org/1267093002/diff/90001/PerformanceTests/SVG/Ba... File PerformanceTests/SVG/Bamboo_transform.html (right): https://codereview.chromium.org/1267093002/diff/90001/PerformanceTests/SVG/Ba... PerformanceTests/SVG/Bamboo_transform.html:3: <object id="svg-content" data="./resources/Bamboo.svg" width="400px" height="400px" type="image/svg+xml"> Nit: just data="resources/Bamboo.svg"
On 2015/08/26 21:19:40, pdr wrote: > I'm not sure anyone has added to these tests before which is why this review is > sort of slow :/ Assuming there are no issues from reviewers, lets go ahead and > land this once the comments below are addressed. I'll liaison with the perf > sheriff for you. > > I verified this will automatically work through telemetry and will show up on > the blink_perf.svg bots on http://chromeperf.appspot.com. I did notice that the test is > an order of magnitude slower than other tests in the suite though. I think we'll > need to get the total test time down a little before we can land this since > it'll take ages to run this test on android: > > RESULT Bamboo_transform: Bamboo_transform.html= > [2776.235000,2869.945000,2490.170000,1310.265000,1226.475000] ms > Avg Bamboo_transform: 2134.618000ms > Sd Bamboo_transform: 803.597687ms > > RESULT SvgCubics: SvgCubics.html= > [13.505000,13.575000,11.895000,11.975000,12.730000] ms > Avg SvgCubics: 12.736000ms > Sd SvgCubics: 0.803387ms > > Any single draw of the bamboo will take a while so I don't think we can expect > this to get to 12ms, but something around the 200ms/run range seems more > reasonable. If bamboo is too slow, you may want to try another one of the svg > files in resources/. > > https://codereview.chromium.org/1267093002/diff/90001/PerformanceTests/SVG/Ba... > File PerformanceTests/SVG/Bamboo_transform.html (right): > > https://codereview.chromium.org/1267093002/diff/90001/PerformanceTests/SVG/Ba... > PerformanceTests/SVG/Bamboo_transform.html:3: <object id="svg-content" > data="./resources/Bamboo.svg" width="400px" height="400px" type="image/svg+xml"> > Nit: just data="resources/Bamboo.svg" Bamboo.svg has closet number of path elements as BM2.1 svg file, that is why it is chosen (show in following data). ---file name--- ---path elements count--- BM2.1 svg file 4885 Worldcup.svg 8042 Bamboo.svg 6341 Cactus.svg 1576 Cowboy.svg 1366 MtSaintHelens.svg 1321 GearFlowers.svg 1101 Debian.svg 434 UnderTheSee.svg 369 HarveyRayner.svg 338 WorldIso.svg 243 FlowerFromMyGarden.svg 236 HereGear.svg 216 DropsOnABlade.svg 136 France.svg 124 AzLizardBenjiPark.svg 122 CrawFishGanson.svg 81 FrancoBolloGnomeEzechi.svg 36 Samurai.svg 16 FoodLeifLodahl.svg 11 SierpinskiCarpet.svg 0 We found reproduction of some regressions requires large number of path elements. For example Blink cl https://codereview.chromium.org/802833003 which land in r309325. This patch remove the SVG paint culling optimization. If number of path elements is small, negative effect will not be obvious. Here is an experiment to replace Bamboo.svg with Cowbow.svg: Bamboo shows 11% regression while Cowbow only shows 4%. Cowbow Bamboo r309324 748 1514 r309325 777 1688 regression: 4% 11% Instead of changing svg file, we reduce inner animation loop count and compare between before/after change and most time costing cases Worldcup.svg. Measure time seems at an order of magnitude now. Is it ok for auto testing? before change: Avg Bamboo_transform: 7045.775000ms after change: Avg Bamboo_transform: 1670.930000ms reference: Avg Worldcup: 979.084000ms
On 2015/08/27 at 03:05:52, junchao.han wrote: > On 2015/08/26 21:19:40, pdr wrote: > > I'm not sure anyone has added to these tests before which is why this review is > > sort of slow :/ Assuming there are no issues from reviewers, lets go ahead and > > land this once the comments below are addressed. I'll liaison with the perf > > sheriff for you. > > > > I verified this will automatically work through telemetry and will show up on > > the blink_perf.svg bots on http://chromeperf.appspot.com. I did notice that the test is > > an order of magnitude slower than other tests in the suite though. I think we'll > > need to get the total test time down a little before we can land this since > > it'll take ages to run this test on android: > > > > RESULT Bamboo_transform: Bamboo_transform.html= > > [2776.235000,2869.945000,2490.170000,1310.265000,1226.475000] ms > > Avg Bamboo_transform: 2134.618000ms > > Sd Bamboo_transform: 803.597687ms > > > > RESULT SvgCubics: SvgCubics.html= > > [13.505000,13.575000,11.895000,11.975000,12.730000] ms > > Avg SvgCubics: 12.736000ms > > Sd SvgCubics: 0.803387ms > > > > Any single draw of the bamboo will take a while so I don't think we can expect > > this to get to 12ms, but something around the 200ms/run range seems more > > reasonable. If bamboo is too slow, you may want to try another one of the svg > > files in resources/. > > > > https://codereview.chromium.org/1267093002/diff/90001/PerformanceTests/SVG/Ba... > > File PerformanceTests/SVG/Bamboo_transform.html (right): > > > > https://codereview.chromium.org/1267093002/diff/90001/PerformanceTests/SVG/Ba... > > PerformanceTests/SVG/Bamboo_transform.html:3: <object id="svg-content" > > data="./resources/Bamboo.svg" width="400px" height="400px" type="image/svg+xml"> > > Nit: just data="resources/Bamboo.svg" > > Bamboo.svg has closet number of path elements as BM2.1 svg file, that is why it > is chosen (show in following data). > > ---file name--- ---path elements count--- > BM2.1 svg file 4885 > > Worldcup.svg 8042 > Bamboo.svg 6341 > Cactus.svg 1576 > Cowboy.svg 1366 > MtSaintHelens.svg 1321 > GearFlowers.svg 1101 > Debian.svg 434 > UnderTheSee.svg 369 > HarveyRayner.svg 338 > WorldIso.svg 243 > FlowerFromMyGarden.svg 236 > HereGear.svg 216 > DropsOnABlade.svg 136 > France.svg 124 > AzLizardBenjiPark.svg 122 > CrawFishGanson.svg 81 > FrancoBolloGnomeEzechi.svg 36 > Samurai.svg 16 > FoodLeifLodahl.svg 11 > SierpinskiCarpet.svg 0 > > We found reproduction of some regressions requires large number of path elements. > For example Blink cl https://codereview.chromium.org/802833003 which land in > r309325. This patch remove the SVG paint culling optimization. If number of path > elements is small, negative effect will not be obvious. Here is an experiment to > replace Bamboo.svg with Cowbow.svg: Bamboo shows 11% regression while Cowbow > only shows 4%. > > Cowbow Bamboo > r309324 748 1514 > r309325 777 1688 > regression: 4% 11% > > Instead of changing svg file, we reduce inner animation loop count and compare > between before/after change and most time costing cases Worldcup.svg. Measure > time seems at an order of magnitude now. Is it ok for auto testing? > > before change: > Avg Bamboo_transform: 7045.775000ms > after change: > Avg Bamboo_transform: 1670.930000ms > reference: > Avg Worldcup: 979.084000ms A 4% regression will still be visible on the bots so lets switch to Cowboy as well. That should get to around 600ms which might be tolerable on the bots. My reasoning here is that we're constrained on the number of Android perf bots we have. Can you also double-check the standard deviation after switching to Cowboy? Is it similar to other tests in blink_perf.svg?
On 2015/08/27 03:52:01, pdr wrote: > On 2015/08/27 at 03:05:52, junchao.han wrote: > > On 2015/08/26 21:19:40, pdr wrote: > > > I'm not sure anyone has added to these tests before which is why this review > is > > > sort of slow :/ Assuming there are no issues from reviewers, lets go ahead > and > > > land this once the comments below are addressed. I'll liaison with the perf > > > sheriff for you. > > > > > > I verified this will automatically work through telemetry and will show up > on > > > the blink_perf.svg bots on http://chromeperf.appspot.com. I did notice that > the test is > > > an order of magnitude slower than other tests in the suite though. I think > we'll > > > need to get the total test time down a little before we can land this since > > > it'll take ages to run this test on android: > > > > > > RESULT Bamboo_transform: Bamboo_transform.html= > > > [2776.235000,2869.945000,2490.170000,1310.265000,1226.475000] ms > > > Avg Bamboo_transform: 2134.618000ms > > > Sd Bamboo_transform: 803.597687ms > > > > > > RESULT SvgCubics: SvgCubics.html= > > > [13.505000,13.575000,11.895000,11.975000,12.730000] ms > > > Avg SvgCubics: 12.736000ms > > > Sd SvgCubics: 0.803387ms > > > > > > Any single draw of the bamboo will take a while so I don't think we can > expect > > > this to get to 12ms, but something around the 200ms/run range seems more > > > reasonable. If bamboo is too slow, you may want to try another one of the > svg > > > files in resources/. > > > > > > > https://codereview.chromium.org/1267093002/diff/90001/PerformanceTests/SVG/Ba... > > > File PerformanceTests/SVG/Bamboo_transform.html (right): > > > > > > > https://codereview.chromium.org/1267093002/diff/90001/PerformanceTests/SVG/Ba... > > > PerformanceTests/SVG/Bamboo_transform.html:3: <object id="svg-content" > > > data="./resources/Bamboo.svg" width="400px" height="400px" > type="image/svg+xml"> > > > Nit: just data="resources/Bamboo.svg" > > > > Bamboo.svg has closet number of path elements as BM2.1 svg file, that is why > it > > is chosen (show in following data). > > > > ---file name--- ---path elements count--- > > BM2.1 svg file 4885 > > > > Worldcup.svg 8042 > > Bamboo.svg 6341 > > Cactus.svg 1576 > > Cowboy.svg 1366 > > MtSaintHelens.svg 1321 > > GearFlowers.svg 1101 > > Debian.svg 434 > > UnderTheSee.svg 369 > > HarveyRayner.svg 338 > > WorldIso.svg 243 > > FlowerFromMyGarden.svg 236 > > HereGear.svg 216 > > DropsOnABlade.svg 136 > > France.svg 124 > > AzLizardBenjiPark.svg 122 > > CrawFishGanson.svg 81 > > FrancoBolloGnomeEzechi.svg 36 > > Samurai.svg 16 > > FoodLeifLodahl.svg 11 > > SierpinskiCarpet.svg 0 > > > > We found reproduction of some regressions requires large number of path > elements. > > For example Blink cl https://codereview.chromium.org/802833003 which land in > > r309325. This patch remove the SVG paint culling optimization. If number of > path > > elements is small, negative effect will not be obvious. Here is an experiment > to > > replace Bamboo.svg with Cowbow.svg: Bamboo shows 11% regression while Cowbow > > only shows 4%. > > > > Cowbow Bamboo > > r309324 748 1514 > > r309325 777 1688 > > regression: 4% 11% > > > > Instead of changing svg file, we reduce inner animation loop count and compare > > > between before/after change and most time costing cases Worldcup.svg. Measure > > time seems at an order of magnitude now. Is it ok for auto testing? > > > > before change: > > Avg Bamboo_transform: 7045.775000ms > > after change: > > Avg Bamboo_transform: 1670.930000ms > > reference: > > Avg Worldcup: 979.084000ms > > > A 4% regression will still be visible on the bots so lets switch to Cowboy as > well. That should get to around 600ms which might be tolerable on the bots. My > reasoning here is that we're constrained on the number of Android perf bots we > have. > > Can you also double-check the standard deviation after switching to Cowboy? Is > it similar to other tests in blink_perf.svg? Switch to Cowboy and standard deviation is similiar. Here is data: RESULT Cowboy: Cowboy.html= [224.200000,228.515000,220.735000,226.140000,227.850000] ms Avg Cowboy: 225.488000ms Sd Cowboy: 3.139141ms *RESULT Cowboy: Cowboy= [224.200000,228.515000,220.735000,226.140000,227.850000] ms Avg Cowboy: 225.488000ms Sd Cowboy: 3.139141ms RESULT Cowboy_transform: Cowboy_transform.html= [775.860000,747.690000,750.350000,758.630000,771.590000] ms Avg Cowboy_transform: 760.824000ms Sd Cowboy_transform: 12.540019ms *RESULT Cowboy_transform: Cowboy_transform= [775.860000,747.690000,750.350000,758.630000,771.590000] ms Avg Cowboy_transform: 760.824000ms Sd Cowboy_transform: 12.540019ms
On 2015/08/27 at 04:08:03, junchao.han wrote: > On 2015/08/27 03:52:01, pdr wrote: > > On 2015/08/27 at 03:05:52, junchao.han wrote: > > > On 2015/08/26 21:19:40, pdr wrote: > > > > I'm not sure anyone has added to these tests before which is why this review > > is > > > > sort of slow :/ Assuming there are no issues from reviewers, lets go ahead > > and > > > > land this once the comments below are addressed. I'll liaison with the perf > > > > sheriff for you. > > > > > > > > I verified this will automatically work through telemetry and will show up > > on > > > > the blink_perf.svg bots on http://chromeperf.appspot.com. I did notice that > > the test is > > > > an order of magnitude slower than other tests in the suite though. I think > > we'll > > > > need to get the total test time down a little before we can land this since > > > > it'll take ages to run this test on android: > > > > > > > > RESULT Bamboo_transform: Bamboo_transform.html= > > > > [2776.235000,2869.945000,2490.170000,1310.265000,1226.475000] ms > > > > Avg Bamboo_transform: 2134.618000ms > > > > Sd Bamboo_transform: 803.597687ms > > > > > > > > RESULT SvgCubics: SvgCubics.html= > > > > [13.505000,13.575000,11.895000,11.975000,12.730000] ms > > > > Avg SvgCubics: 12.736000ms > > > > Sd SvgCubics: 0.803387ms > > > > > > > > Any single draw of the bamboo will take a while so I don't think we can > > expect > > > > this to get to 12ms, but something around the 200ms/run range seems more > > > > reasonable. If bamboo is too slow, you may want to try another one of the > > svg > > > > files in resources/. > > > > > > > > > > https://codereview.chromium.org/1267093002/diff/90001/PerformanceTests/SVG/Ba... > > > > File PerformanceTests/SVG/Bamboo_transform.html (right): > > > > > > > > > > https://codereview.chromium.org/1267093002/diff/90001/PerformanceTests/SVG/Ba... > > > > PerformanceTests/SVG/Bamboo_transform.html:3: <object id="svg-content" > > > > data="./resources/Bamboo.svg" width="400px" height="400px" > > type="image/svg+xml"> > > > > Nit: just data="resources/Bamboo.svg" > > > > > > Bamboo.svg has closet number of path elements as BM2.1 svg file, that is why > > it > > > is chosen (show in following data). > > > > > > ---file name--- ---path elements count--- > > > BM2.1 svg file 4885 > > > > > > Worldcup.svg 8042 > > > Bamboo.svg 6341 > > > Cactus.svg 1576 > > > Cowboy.svg 1366 > > > MtSaintHelens.svg 1321 > > > GearFlowers.svg 1101 > > > Debian.svg 434 > > > UnderTheSee.svg 369 > > > HarveyRayner.svg 338 > > > WorldIso.svg 243 > > > FlowerFromMyGarden.svg 236 > > > HereGear.svg 216 > > > DropsOnABlade.svg 136 > > > France.svg 124 > > > AzLizardBenjiPark.svg 122 > > > CrawFishGanson.svg 81 > > > FrancoBolloGnomeEzechi.svg 36 > > > Samurai.svg 16 > > > FoodLeifLodahl.svg 11 > > > SierpinskiCarpet.svg 0 > > > > > > We found reproduction of some regressions requires large number of path > > elements. > > > For example Blink cl https://codereview.chromium.org/802833003 which land in > > > r309325. This patch remove the SVG paint culling optimization. If number of > > path > > > elements is small, negative effect will not be obvious. Here is an experiment > > to > > > replace Bamboo.svg with Cowbow.svg: Bamboo shows 11% regression while Cowbow > > > only shows 4%. > > > > > > Cowbow Bamboo > > > r309324 748 1514 > > > r309325 777 1688 > > > regression: 4% 11% > > > > > > Instead of changing svg file, we reduce inner animation loop count and compare > > > > > between before/after change and most time costing cases Worldcup.svg. Measure > > > time seems at an order of magnitude now. Is it ok for auto testing? > > > > > > before change: > > > Avg Bamboo_transform: 7045.775000ms > > > after change: > > > Avg Bamboo_transform: 1670.930000ms > > > reference: > > > Avg Worldcup: 979.084000ms > > > > > > A 4% regression will still be visible on the bots so lets switch to Cowboy as > > well. That should get to around 600ms which might be tolerable on the bots. My > > reasoning here is that we're constrained on the number of Android perf bots we > > have. > > > > Can you also double-check the standard deviation after switching to Cowboy? Is > > it similar to other tests in blink_perf.svg? > > Switch to Cowboy and standard deviation is similiar. Here is data: > > RESULT Cowboy: Cowboy.html= [224.200000,228.515000,220.735000,226.140000,227.850000] ms > Avg Cowboy: 225.488000ms > Sd Cowboy: 3.139141ms > *RESULT Cowboy: Cowboy= [224.200000,228.515000,220.735000,226.140000,227.850000] ms > Avg Cowboy: 225.488000ms > Sd Cowboy: 3.139141ms > RESULT Cowboy_transform: Cowboy_transform.html= [775.860000,747.690000,750.350000,758.630000,771.590000] ms > Avg Cowboy_transform: 760.824000ms > Sd Cowboy_transform: 12.540019ms > *RESULT Cowboy_transform: Cowboy_transform= [775.860000,747.690000,750.350000,758.630000,771.590000] ms > Avg Cowboy_transform: 760.824000ms > Sd Cowboy_transform: 12.540019ms Thanks! LGTM Please hold off on pressing the commit button just yet. I'll work with the perf sheriff tomorrow (PST timezone) to make sure this lands smoothly.
The CQ bit was checked by junchao.han@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1267093002/130001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1267093002/130001
Message was sent while issue was closed.
Committed patchset #9 (id:130001) as https://src.chromium.org/viewvc/blink?view=rev&revision=201291 |
