New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 737560 link

Starred by 1 user

Issue metadata

Status: WontFix
Owner:
Closed: Jun 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

30.7% regression in blink_perf.svg at 482854:482861

Project Member Reported by tdres...@chromium.org, Jun 28 2017

Issue description

See the link to graphs below.
 
All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=737560

Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?keys=agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDgrtnNmwgM


Bot(s) for this bug's original alert(s):

android-webview-nexus6
Project Member

Comment 3 by 42576172...@developer.gserviceaccount.com, Jun 29 2017

Cc: chrishtr@chromium.org
Owner: chrishtr@chromium.org

=== 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
Status: WontFix (was: Untriaged)
This is not an important regression. The same effect could be achieved
with compositing, so any developer who faces this has an easy workaround.
Cc: benhenry@chromium.org
+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?
Cc: kouhei@chromium.org f...@opera.com
Also cc-ing kouhei and fs, owners of blink_perf.svg.

Comment 7 by kouhei@chromium.org, Jun 30 2017

I'm not sure if I'm 100% on board.
Do we have traces before/after?
Components: Blink>Paint
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.
Thanks for the details.

I agree that we can probably accept this.

Comment 10 by f...@opera.com, 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)?
It's just additional paint time - generating display items. There is no
impact on layout.

Comment 12 by f...@opera.com, 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.
Thanks for the details! I'm very convinced that this is WONTFIX.

Sign in to add a comment