Issue metadata
Sign in to add a comment
|
10.3%-118.3% regression in blink_perf.css at 488898:488959 |
||||||||||||||||||||||
Issue descriptionSee the link to graphs below.
,
Jul 27 2017
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8972961460047420144
,
Jul 27 2017
=== 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
,
Jul 27 2017
Issue 749340 has been merged into this issue.
,
Jul 27 2017
Issue 749332 has been merged into this issue.
,
Jul 27 2017
Issue 749338 has been merged into this issue.
,
Jul 27 2017
Issue 749336 has been merged into this issue.
,
Jul 27 2017
,
Jul 27 2017
Issue 749339 has been merged into this issue.
,
Jul 28 2017
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.
,
Jul 28 2017
,
Aug 1 2017
,
Aug 3 2017
Issue 748528 has been merged into this issue.
,
Aug 3 2017
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.
,
Aug 3 2017
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?
,
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?
,
Aug 14 2017
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?
,
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/
,
Aug 16 2017
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 |
|||||||||||||||||||||||
Comment 1 by 42576172...@developer.gserviceaccount.com
, Jul 27 2017