Issue metadata
Sign in to add a comment
|
30.7% regression in blink_perf.svg at 482854:482861 |
||||||||||||||||||||||
Issue descriptionSee the link to graphs below.
,
Jun 28 2017
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8975538719996720576
,
Jun 29 2017
=== Auto-CCing suspected CL author chrishtr@chromium.org === Hi chrishtr@chromium.org, the bisect results pointed to your CL, please take a look at the results. === BISECT JOB RESULTS === Perf regression found with culprit Suspected Commit Author : Chris Harrelson Commit : 2f502a91eb07a2c8f68f9c83d250a5d39c76327f Date : Wed Jun 28 02:24:08 2017 Subject: Refactor to only need to check cull rect changes up to containing transform. Bisect Details Configuration: android_webview_nexus6_aosp_perf_bisect Benchmark : blink_perf.svg Metric : Cowboy_transform/Cowboy_transform Change : 32.11% | 553.887666667 -> 731.768166667 Revision Result N chromium@482853 553.888 +- 11.3791 6 good chromium@482854 742.501 +- 12.9286 6 bad <-- chromium@482855 747.771 +- 26.1934 6 bad chromium@482857 739.425 +- 12.3349 6 bad chromium@482861 731.768 +- 34.8887 6 bad To Run This Test src/tools/perf/run_benchmark -v --browser=android-webview --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests blink_perf.svg More information on addressing performance regressions: http://g.co/ChromePerformanceRegressions Debug information about this bisect: https://chromeperf.appspot.com/buildbucket_job_status/8975538719996720576 For feedback, file a bug with component Speed>Bisection
,
Jun 29 2017
This is not an important regression. The same effect could be achieved with compositing, so any developer who faces this has an easy workaround.
,
Jun 30 2017
+benhenry@ - to add to his collection of perf regression justifications. Just for clarification, we're arguing that we're okay with a performance regression in rAF driven animations, because folks should be using composited animations instead? Would this regression impact animations on non-composited properties? Would it impact something like touch drag logic, where the transform is set based on the position of the user's finger?
,
Jun 30 2017
Also cc-ing kouhei and fs, owners of blink_perf.svg.
,
Jun 30 2017
I'm not sure if I'm 100% on board. Do we have traces before/after?
,
Jun 30 2017
I'm not saying a generic rAF regression is ok. The regression caused by my patch is on one test, this one: https://cs.chromium.org/chromium/src/third_party/WebKit/PerformanceTests/SVG/Cowboy_transform.html In this test, there is a complicated SVG inside of an <object>. The test animates the transform of the document element containing the svg inside of the <object>'s Document. The test does not actually capture the cost of re-rastering on every frame, just the cost of painting the SVG after transform update. My patch changed behavior to turn off an optimization to cull out painting of elements underneath a transform which are clipped out. In this test, parts of the cowboy are clipped out by the clip of the <object> document's LayoutView. The reason for my change is that (a) this kind of culling is not SPv2-compatible, due to us not knowing during paint whether the transform will end up composited, and (b) even for SPv1, it enables a significant cleanup and likely performance improvement in the pre-paint tree walk. Furthermore, the kind of animation used in this test - content under animated transform under clip - can be much more efficient if the transform is composited, because that will lead to no paint or raster at all on each frame. This is what I referred to in comment 4. Finally, note that if the transform were composited, painting would not cull out the content either, because culling clips don't cross composited layer boundaries (otherwise it would defeat the performance benefits of compositing). For all the above reasons, the regression in this bug is very specific to this particular animation and the benefits of the change outweigh the small cost to this one use-case.
,
Jun 30 2017
Thanks for the details. I agree that we can probably accept this.
,
Jun 30 2017
Not culling is a very reasonable thing to do - and I believe should allow making this raster-bound eventually. That being said though, it's important that we know what additional we're adding here - is it mostly paint (generating display items) or layout (or both)?
,
Jun 30 2017
It's just additional paint time - generating display items. There is no impact on layout.
,
Jun 30 2017
Right, that's what I expected. I guess someone(TM) should look at what type of overhead we have in cases like this.
,
Jul 1 2017
Thanks for the details! I'm very convinced that this is WONTFIX. |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by tdres...@chromium.org
, Jun 28 2017