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

Issue 798316 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2018
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

99.5%-99.8% regression in blink_perf.layout at 526314:526362

Project Member Reported by alexclarke@chromium.org, Jan 2 2018

Issue description

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

(For debugging:) Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?sid=8d99fcbdf71573ed9f5fffbedc14d8ba4ae09d2bf407cb19f3f2736f659621d1


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

android-nexus5X
android-one
android-webview-nexus5X
chromium-rel-mac-retina
chromium-rel-mac11
chromium-rel-mac11-air
chromium-rel-mac11-pro
chromium-rel-win10
chromium-rel-win7-x64-dual
chromium-rel-win8-dual
linux-release
Cc: chrishtr@chromium.org
Owner: chrishtr@chromium.org
Status: Assigned (was: Untriaged)

=== 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 : b7e2306fd4d8590a41f6fd103dfcc1013d6ca85e
  Date   : Fri Dec 29 00:51:58 2017
  Subject: [CI] Migrate hit-testing code in PaintLayer to use GeometryMapper.

Bisect Details
  Configuration: android_webview_arm64_aosp_perf_bisect
  Benchmark    : blink_perf.layout
  Metric       : multicol_tall-content-short-columns-realistic/multicol_tall-content-short-columns-realistic
  Change       : 99.50% | 1433.30542717 -> 7.12828834306

Revision             Result                    N
chromium@526338      1433.31 +- 39.5293        6      good
chromium@526343      1441.47 +- 35.3808        6      good
chromium@526344      1444.56 +- 25.7329        6      good
chromium@526345      7.18591 +- 0.0819284      6      bad       <--
chromium@526347      7.17934 +- 0.082053       6      bad
chromium@526356      7.12829 +- 0.118707       6      bad

Please refer to the following doc on diagnosing blink_perf regressions:
  https://chromium.googlesource.com/chromium/src/+/master/docs/speed/benchmark_harnesses/blink_perf.md

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.layout

More information on addressing performance regressions:
  http://g.co/ChromePerformanceRegressions

Debug information about this bisect:
  https://chromeperf.appspot.com/buildbucket_job_status/8958520593328995920


For feedback, file a bug with component Speed>Bisection
This regression is because we are not updating the lifecycle to PrePaintClean
instead of just CompositingCleanPlusScrolling, On ever frame, layout is forced,
which also causes paint invalidation, and paint invalidation in this
case is quite slow.
Most of the cost is in FragmentsVisualRectBoundingBox, which iterates over
all fragments and unites their rects.
Fixing that increased performance by a factor of 3, but there is still a big
regression.
Next biggest problem is ContextForFragment, taking up about 1/3 of the total
time.
This is in turn caused by the example having a huge number of fragments
with slightly different offsets. We currently cap at 2000 fragments, perhaps that
should be lowered.
Project Member

Comment 9 by bugdroid1@chromium.org, Jan 5 2018

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

commit 644fbe520dc19514a80d86a61655ff81ea4b00d5
Author: Chris Harrelson <chrishtr@chromium.org>
Date: Fri Jan 05 04:37:55 2018

Reduce maximum number of fragments to 500.

This is to further mitigate pathological fragmentation scenarios.

Bug:  798316 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: Ic292ad6c28a6980e54441881ef4961f8d9772108
Reviewed-on: https://chromium-review.googlesource.com/850583
Reviewed-by: Morten Stenshorne <mstensho@chromium.org>
Commit-Queue: Chris Harrelson <chrishtr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#527218}
[modify] https://crrev.com/644fbe520dc19514a80d86a61655ff81ea4b00d5/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp

Project Member

Comment 10 by bugdroid1@chromium.org, Jan 7 2018

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

commit 15a89597a5c7e2cebc1ed5316a49156bf1207062
Author: Chris Harrelson <chrishtr@chromium.org>
Date: Sun Jan 07 18:20:09 2018

Only check the first fragment for already-invalidated heuristic.

FragmentsVisualRectBoundingBox can be expensive because it iterates over all
fragments and unions their rects.

Bug:  798316 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: Icce66d4a9426a95ef5fa5992e7e4ad9ebacd66e6
Reviewed-on: https://chromium-review.googlesource.com/853073
Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org>
Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#527541}
[modify] https://crrev.com/15a89597a5c7e2cebc1ed5316a49156bf1207062/third_party/WebKit/Source/core/paint/ObjectPaintInvalidator.cpp

Status: Fixed (was: Assigned)
The above two patches make a significant difference on this test - going from
about 11 to about 340 in score. Given this is an extreme and unlikely scenario,
going to mark the bug as fixed, since we traded some slowness in this particular
test for a significant upgrade elsewhere.

Sign in to add a comment