New issue
Advanced search Search tips

Issue 847252 link

Starred by 7 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 17
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 1
Type: Bug-Regression

Blocked on:
issue 867595
issue 869409

Blocking:
issue 581518



Sign in to add a comment

JankTracker regresses prepaint time

Project Member Reported by skobes@chromium.org, May 28 2018

Issue description

On pages with large numbers of visible layout objects, JankTracker spends a lot of time updating the jank region.

We also branched M68 after JankTracker landed, so the fix will need a merge.
 

Comment 1 by skobes@chromium.org, May 28 2018

Blocking: 581518

Comment 2 by skobes@chromium.org, May 28 2018

Cc: emilio@chromium.org skobes@chromium.org ellyjo...@chromium.org sunyunjia@chromium.org
 Issue 846074  has been merged into this issue.

Comment 3 by skobes@chromium.org, May 28 2018

Cc: fmea...@chromium.org
 Issue 846330  has been merged into this issue.

Comment 4 by skobes@chromium.org, May 28 2018

 Issue 845943  has been merged into this issue.

Comment 5 by skobes@chromium.org, May 28 2018

Issue 846841 has been merged into this issue.

Comment 6 by skobes@chromium.org, May 28 2018

Cc: weiliangc@chromium.org ajuma@chromium.org
 Issue 846419  has been merged into this issue.
I think it needs to be put behind a runtime flag also.
Project Member

Comment 8 by bugdroid1@chromium.org, May 29 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7ec934dfd98984fac8af897b3f29c0514a3ff159

commit 7ec934dfd98984fac8af897b3f29c0514a3ff159
Author: Steve Kobes <skobes@chromium.org>
Date: Tue May 29 03:26:08 2018

Limit jank region granularity.

This patch snaps jank regions to a grid whose cells are 1/60th of the
min(width, height) of the viewport.  This greatly reduces performance
overhead of JankTracker's calls to Region::Unite.

Jank tracking is still not entirely free:
https://i.imgur.com/UpizpGe.png

However the remaining cost is due primarily to coordinate mapping and
not combining regions;  crbug.com/842282  might help here.

Bug:  847252 
Change-Id: I86f202b8ab3a7be609c668ab0285e06745b41e10
Reviewed-on: https://chromium-review.googlesource.com/1075520
Commit-Queue: Steve Kobes <skobes@chromium.org>
Reviewed-by: Timothy Dresser <tdresser@chromium.org>
Cr-Commit-Position: refs/heads/master@{#562327}
[modify] https://crrev.com/7ec934dfd98984fac8af897b3f29c0514a3ff159/third_party/blink/renderer/core/layout/jank_tracker.cc
[modify] https://crrev.com/7ec934dfd98984fac8af897b3f29c0514a3ff159/third_party/blink/renderer/core/layout/jank_tracker_test.cc

Comment 9 by skobes@chromium.org, May 29 2018

Re #7: I thought about that, but I'm not sure what value a flag provides here.  There's no observable change to platform behavior, and we don't expect it to take multiple milestones to stabilize.  If we had a flag, this perf regression might not have been discovered until much later.
In addition, this feature doesn't do anything useful if tracing is not turned
on, right? Therefore JankTracker should not even collect any data unless
the right trace category is on.
The eventual goal is to gather jank scores in the wild and expose them through UKM.
But before that time we should be careful not to impact performance at all, especially if the code doesn't do anything observable. If you want to know
whether it is performant it should be in a finch experiment.
Why would a performance impact that is acceptable when the UKM is collected be unacceptable before the UKM is collected?
It's not, but currently you don't know the performance impact at all, right?
You can collect this data and use it to prioritize optimizations, based on
Finch.
Ok I am convinced that JankTracker should be behind a REF.  Filed  issue 847581  for this.
😿 Pinpoint job stopped with an error.
https://pinpoint-dot-chromeperf.appspot.com/job/165aa2b3a40000

The swarming task expired. The bots are likely overloaded, dead, or misconfigured.
😿 Pinpoint job stopped with an error.
https://pinpoint-dot-chromeperf.appspot.com/job/1629d4b5a40000

The swarming task expired. The bots are likely overloaded, dead, or misconfigured.
😿 Pinpoint job stopped with an error.
https://pinpoint-dot-chromeperf.appspot.com/job/12c63647a40000

Buildbucket says the build completed successfully, but Pinpoint can't find the isolate hash.
Blockedon: 869409 867595
😿 Pinpoint job stopped with an error.
https://pinpoint-dot-chromeperf.appspot.com/job/16c63647a40000

Buildbucket says the build completed successfully, but Pinpoint can't find the isolate hash.
Blockedon: 868926
Blockedon: -868926
"The swarming task expired. The bots are likely overloaded, dead, or misconfigured.": all the Nexus 5X bots went down ( issue 867595 ). We'll want to wait for today's migration before trying again.

"Buildbucket says the build completed successfully, but Pinpoint can't find the isolate hash.": The pixel2 bots are currently FYI only, and we don't have bisect support yet. (There are no builds and no devices.) There shouldn't be any alerts on it, filed issue 869485,
📍 Job complete. See results below.
https://pinpoint-dot-chromeperf.appspot.com/job/16b240e8640000
📍 Job complete. See results below.
https://pinpoint-dot-chromeperf.appspot.com/job/178c2d10640000
📍 Job complete. See results below.
https://pinpoint-dot-chromeperf.appspot.com/job/13dcc68c640000
The regressions in frame_times for tough_animation stories (balls_css_key_frame_animations, balls_css_transition_2_properties, and balls_javascript_css) no longer reproduce; they were likely fixed by r562327.

https://pinpoint-dot-chromeperf.appspot.com/job/16b240e8640000

The regressions in mean_pixels_checkerboarded for tough_scrolling stories also no longer reproduce:

https://pinpoint-dot-chromeperf.appspot.com/job/178c2d10640000

JankTracker still has a small impact on a few tests in blink_perf.paint (+18% for containment-resize, +12% for paint-offset-changes).  I am working on a patch to add a movement distance threshold, which should help.

https://pinpoint-dot-chromeperf.appspot.com/job/11617130640000

Note that the blink_perf.py harness enables experimental web platform features, so the bots are testing blink_perf.paint with jank tracking enabled (unlike the rendering.* benchmarks).
Project Member

Comment 35 by bugdroid1@chromium.org, Aug 15

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/db004f1535ceae194451729b357d5160ff0fe773

commit db004f1535ceae194451729b357d5160ff0fe773
Author: Steve Kobes <skobes@chromium.org>
Date: Wed Aug 15 15:32:37 2018

JankTracker: Early exit for small movements and small sizes.

This change ignores movements of less than 3px, which are small enough
to not be distracting.

It also adds an early exit for objects smaller than the jank region
granularity.  This is a performance optimization, since these objects
will not affect the jank region after snapping.

This should mostly eliminate JankTracker's impact on blink_perf.paint
benchmarks (already reduced by crrev.com/562327).

Bug: 581518, 847252 
Change-Id: I3979421e7a25654e67d3eeba7a2cd5d217ed3f03
Reviewed-on: https://chromium-review.googlesource.com/1175109
Commit-Queue: Steve Kobes <skobes@chromium.org>
Reviewed-by: Timothy Dresser <tdresser@chromium.org>
Cr-Commit-Position: refs/heads/master@{#583255}
[modify] https://crrev.com/db004f1535ceae194451729b357d5160ff0fe773/third_party/blink/renderer/core/layout/jank_tracker.cc
[modify] https://crrev.com/db004f1535ceae194451729b357d5160ff0fe773/third_party/blink/renderer/core/layout/jank_tracker_test.cc

Status: Fixed (was: Started)
Graphs show blink_perf.paint recovering after r583255:

https://chromeperf.appspot.com/report?sid=187dafad55174ba05eee38b34c99140a1b6d280344596a7293d101370c9d94d0

Sign in to add a comment