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

Issue 713128 link

Starred by 1 user

Issue metadata

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


Show other hotlists

Hotlists containing this issue:
Stylimations-OKR-2017-Q2


Sign in to add a comment

19.5% regression in blink_perf.paint at 462371:462400

Project Member Reported by alexclarke@chromium.org, Apr 19 2017

Issue description

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

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


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

android-nexus7v2
Project Member

Comment 3 by 42576172...@developer.gserviceaccount.com, Apr 20 2017

Cc: shend@chromium.org
Owner: shend@chromium.org

=== Auto-CCing suspected CL author shend@chromium.org ===

Hi shend@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 : shend
  Commit : 25809e1270b9eb4103f1efcc089c781df8465e39
  Date   : Thu Apr 06 06:58:14 2017
  Subject: Generate ComputedStyle::hasViewportUnits and hasRemUnits.

Bisect Details
  Configuration: android_nexus7_perf_bisect
  Benchmark    : blink_perf.paint
  Metric       : large-table-background-change-with-invisible-collapsed-borders/large-table-background-change-with-invisible-collapsed-borders
  Change       : 31.28% | 706.157833333 -> 927.05125

Revision             Result                  N
chromium@462370      706.158 +- 21.8691      6      good
chromium@462374      711.395 +- 22.2861      6      good
chromium@462376      715.675 +- 11.1529      6      good
chromium@462377      912.787 +- 9.31231      6      bad       <--
chromium@462378      907.774 +- 26.2508      6      bad
chromium@462385      918.342 +- 45.6486      6      bad
chromium@462400      927.051 +- 11.515       6      bad

To Run This Test
  src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests blink_perf.paint

Debug Info
  https://chromeperf.appspot.com/buildbucket_job_status/8981887889941166688

Is this bisect wrong?
  https://chromeperf.appspot.com/bad_bisect?try_job_id=6333394616582144


| O O | Visit http://www.chromium.org/developers/speed-infra/perf-bug-faq
|  X  | for more information addressing perf regression bugs. For feedback,
| / \ | file a bug with component Speed>Bisection.  Thank you!
Project Member

Comment 5 by 42576172...@developer.gserviceaccount.com, Apr 20 2017

Cc: scottmg@chromium.org
Owner: scottmg@chromium.org

=== Auto-CCing suspected CL author scottmg@chromium.org ===

Hi scottmg@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 : scottmg
  Commit : ffcbc0b2bda32309a8e799e146b934ce9b556b82
  Date   : Wed Apr 12 22:53:42 2017
  Subject: Force PlzNav and LoadingWithMojo on when using network service

Bisect Details
  Configuration: android_nexus7_perf_bisect
  Benchmark    : blink_perf.paint
  Metric       : large-table-background-change-with-invisible-collapsed-borders/large-table-background-change-with-invisible-collapsed-borders
  Change       : 22.84% | 936.542833333 -> 722.636916667

Revision             Result                  N
chromium@464165      936.543 +- 15.5878      6      good
chromium@464181      910.002 +- 31.3424      6      good
chromium@464185      906.321 +- 9.7155       6      good
chromium@464186      920.795 +- 17.3946      6      good
chromium@464187      726.578 +- 19.3117      6      bad       <--
chromium@464189      734.038 +- 17.6249      6      bad
chromium@464197      707.17 +- 12.6937       6      bad
chromium@464229      722.637 +- 29.7758      6      bad

To Run This Test
  src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests blink_perf.paint

Debug Info
  https://chromeperf.appspot.com/buildbucket_job_status/8981813874618225136

Is this bisect wrong?
  https://chromeperf.appspot.com/bad_bisect?try_job_id=5910920091402240


| O O | Visit http://www.chromium.org/developers/speed-infra/perf-bug-faq
|  X  | for more information addressing perf regression bugs. For feedback,
| / \ | file a bug with component Speed>Bisection.  Thank you!

Comment 6 by shend@chromium.org, Apr 20 2017

Cc: -scottmg@chromium.org
Owner: shend@chromium.org
-scottmg
Sorry, was just doing a separate bisect to help figure out what's going on.

Comment 7 by shend@chromium.org, Apr 27 2017

Cc: meade@chromium.org
Components: Blink>CSS
Labels: Update-Weekly Performance
Status: Assigned (was: Untriaged)
Ran perf bots on a revert patch: https://codereview.chromium.org/2838513002
We would gain a 5% speedup on Nexus 7 with the revert.
Interestingly, if we keep the generated members (which are unused) and add back the handwritten members in ComputedStyle: https://codereview.chromium.org/2828253004#ps00001
we would get a 17% speedup.

I think there are two factors at work here. When we generate hasViewport and hasRem, it causes the data layout to change for ComputedStyleBase. It just so happens that the new data layout:
- Moves the other fields around in a way that makes the test faster.
- But makes accesses to hasViewport and hasRem slower.

This is just a hypothesis and more investigation would be needed.

Personally, I don't think this is urgent, as the graph has since gone back down and the regression seems to be only on this test. A possible fix would be to allow the order of fields to be specified (in a JSON file), but this would conflict with the automatic packing that we're currently doing.

@meade, WDYT?

Comment 8 by meade@chromium.org, Apr 27 2017

I'm curious about what made the graph go down again - I couldn't see any obvious culprits in the range other than V8 rolls. Do you know?

But likewise I don't think this is urgent.

Comment 9 by shend@chromium.org, Apr 27 2017

I did a bisect on what made the graph go down, and it's in comment #5. Doesn't appear to be relevant... 
Status: WontFix (was: Assigned)
Closing since the graph has gone back down and the regression seems to have disappeared.
Project Member

Comment 11 by bugdroid1@chromium.org, Feb 16 2018

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/chromeos/chromeos-admin/+/bc0d6d92201bd68cf6d780b00239f465181ce296

commit bc0d6d92201bd68cf6d780b00239f465181ce296
Author: Prathmesh Prabhu <pprabhu@chromium.org>
Date: Fri Feb 16 22:09:17 2018

Sign in to add a comment