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

Issue 749335 link

Starred by 1 user

Issue metadata

Status: WontFix
Owner:
Not on Chrome anymore
Closed: Aug 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

10.3%-118.3% regression in blink_perf.css at 488898:488959

Project Member Reported by m...@chromium.org, Jul 27 2017

Issue description

See the link to graphs below.
 
Project Member

Comment 1 by 42576172...@developer.gserviceaccount.com, Jul 27 2017

All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=749335

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


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

android-nexus5X
android-nexus7v2
android-one
android-webview-nexus5X
android-webview-nexus6
chromium-rel-mac-retina
chromium-rel-mac11
chromium-rel-mac11-air
chromium-rel-mac11-pro
chromium-rel-mac12
chromium-rel-mac12-mini-8gb
chromium-rel-win10
chromium-rel-win7-gpu-ati
chromium-rel-win7-gpu-intel
chromium-rel-win7-gpu-nvidia
chromium-rel-win8-dual
Project Member

Comment 3 by 42576172...@developer.gserviceaccount.com, Jul 27 2017

Cc: nainar@chromium.org
Owner: nainar@chromium.org

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

Hi nainar@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 : Naina Raisinghani
  Commit : fd7f99c9125b7f9b054b3a32c0100b526792f74c
  Date   : Mon Jul 24 05:21:35 2017
  Subject: Remove code and tests using StyleSharing

Bisect Details
  Configuration: mac_10_11_perf_bisect
  Benchmark    : blink_perf.css
  Metric       : ChangeStyleChildElementSelectors/ChangeStyleChildElementSelectors
  Change       : 118.31% | 227.5755 -> 496.814

Revision             Result                  N
chromium@488902      227.576 +- 3.28135      6      good
chromium@488909      237.788 +- 11.5187      6      good
chromium@488912      237.451 +- 4.01528      6      good
chromium@488914      238.091 +- 4.09586      6      good
chromium@488915      493.461 +- 5.6315       6      bad       <--
chromium@488928      496.814 +- 11.401       6      bad

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

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

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


For feedback, file a bug with component Speed>Bisection
Project Member

Comment 4 by 42576172...@developer.gserviceaccount.com, Jul 27 2017

 Issue 749340  has been merged into this issue.
Project Member

Comment 5 by 42576172...@developer.gserviceaccount.com, Jul 27 2017

 Issue 749332  has been merged into this issue.
Project Member

Comment 6 by 42576172...@developer.gserviceaccount.com, Jul 27 2017

 Issue 749338  has been merged into this issue.
Project Member

Comment 7 by 42576172...@developer.gserviceaccount.com, Jul 27 2017

 Issue 749336  has been merged into this issue.

Comment 8 by m...@chromium.org, Jul 27 2017

Cc: -m...@chromium.org

Comment 9 by m...@chromium.org, Jul 27 2017

 Issue 749339  has been merged into this issue.
Components: Blink>CSS
Labels: Performance
Status: Assigned (was: Untriaged)
Hmm this is interesting. The code deleted here was dead code only used in LayoutTests. I don't understand why there is a performance cost of removing dead code. Looking into this right now. 
Labels: Update-Weekly
Cc: ashleymarie@chromium.org wangxianzhu@chromium.org
Issue 748528 has been merged into this issue.
Issue 748528 has been merged into this issue.
That CL also removed the runtime flag (previously set to experimental), which suggests that the perf tests might also run in that mode.

The good news is that it appears as though the regressions are all blink_perf micro benchmarks.
Cc: r...@opera.com
Here are where the flags are set for blink_perf benchmarks, experimental is turned on: https://cs.chromium.org/chromium/src/tools/perf/benchmarks/blink_perf.py?q=blink_perf.py&sq=package:chromium&ssfr=1&l=148

+rune, who owns the blink_perf.css benchmark: definitely looks like this change caused a regression. Should the experimental flags be on for this benchmark?

Comment 16 by r...@opera.com, Aug 7 2017

I didn't know experimental was on for perf-tests, but I think that makes sense when you introduce a new feature. Otherwise you wouldn't detect perf regression until you tried shipping. Here, we're removing a feature by first making it experimental. I don't know why we did that.

It's definitely not a surprise that micro benchmarks took a hit from removing style sharing. I just thought we had taken the hit already, not going via experimental.

Naina, could you take a look at the regressed tests and see if the regression numbers are roughly expected?

I made the feature experimental with the misunderstanding that it would pull the code out from real sites use and perfbots and only be used by LayoutTests. Clearly I was wrong. :/ 

Looking at the regression alerts here: https://chromeperf.appspot.com/group_report?bug_id=749335 the tests that suffer the most are tables with a large (in the hundreds) number of rows, columns. The others have less than 15% of regression which seems tolerable to me. Also no regression reports from any page cycler site. So that (and the fact that we haven't received any complaints from StyleSharing being off on beta) sounds to me like we can take this hit?

Comment 18 by r...@opera.com, Aug 14 2017

A lot of the ChangeStyle* test are between 1.5 (50%) and double the time spent (100%). They all utilize createDOMTree() from utils.js which creates a tree of divs without attributes which effectively used style sharing all over the place. This is expected.

(Note that the regressions in the alert url above are collapsed)

I haven't studied the other tests, but I suspect style sharing was also efficient for the large tables.

We are most likely looking at regressions in the range expected from the document on which the decision to remove style sharing was based: https://docs.google.com/document/d/1rNzD4-RXYujUjLCR9YfZHaZ19Wq2X5XvRXlyatYZYj0/

Status: WontFix (was: Assigned)
I looked at the tables tests and you are right. It is heavily leveraging stylesharing. shend@ and I wrote the ChangeStyle* tests so I discounted those but yup all elements have the default style there leveraging style sharing there. 

Closing this bug assuming no real sites or or page cycler suites file a perf regression. 

Sign in to add a comment