Issue metadata
Sign in to add a comment
|
15.8%-25.8% regression in blink_perf.layout at 389053:389070 |
||||||||||||||||||||
Issue descriptionSee the link to graphs below.
,
Apr 25 2016
=== Auto-CCing suspected CL author mstensho@opera.com === Hi mstensho@opera.com, the bisect results pointed to your CL below as possibly causing a regression. Please have a look at this info and see whether your CL be related. ===== BISECT JOB RESULTS ===== Status: completed ===== SUSPECTED CL(s) ===== Subject : Spec-compliant parsing and initial values for 'orphans' and 'widows'. Author : mstensho Commit description: The initial values for these properties should be '2', not 'auto'. 'auto' isn't even an allowed value in the spec. So remove support for that completely. FWIW: 'auto' used to mean pretty much the same as '1'. Quite a few tests have to be updated because of this change, typically because they assume that there are no orphans and widows requirements, meaning that there'd be no breaking restrictions between lines. In those cases, now that the initial value is '2', we need to set 'orphans' and 'widows' to '1' explicitly if we don't want any restrictions. There are also some non-layout tests that expect the initial value to be 'auto' or '1'. In those cases we need to just update the expectations to be '2' instead. BUG= 473509 Review URL: https://codereview.chromium.org/1909233002 Cr-Commit-Position: refs/heads/master@{#389061} Commit : b25fd3116d7cb9366a24af711666fc63a27f91b4 Date : Fri Apr 22 09:20:09 2016 ===== TESTED REVISIONS ===== Revision Mean Value Std. Dev. Num Values Good? chromium@389057 36.802947 1.011621 5 good chromium@389060 37.486168 0.258171 5 good chromium@389061 30.337944 0.426913 5 bad <- chromium@389062 30.636437 0.18875 5 bad Bisect job ran on: linux_perf_bisect Bug ID: 606262 Test Command: src/tools/perf/run_benchmark -v --browser=release --output-format=chartjson --upload-results --also-run-disabled-tests blink_perf.layout Test Metric: multicol_lots-of-text-balanced/multicol_lots-of-text-balanced Relative Change: 16.76% Score: 99.9 Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/linux_perf_bisect/builds/6452 Job details: https://chromeperf.appspot.com/buildbucket_job_status/9014427179476301632 Not what you expected? We'll investigate and get back to you! https://chromeperf.appspot.com/bad_bisect?try_job_id=606262 | 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 Tests>AutoBisect. Thank you!
,
Apr 25 2016
Sounds likely. Having orphans and especially widows set to something larger than 1 is more expensive, since it may cause an additional layout pass. I suppose we could update the tests to specify "orphans:1; widows:1;". Or just live with the regression. Or - do both: Update the old tests with the aforementioned declarations, but also create new tests that don't have them, since it's interesting to test performance with orphans and widows requirements too.
,
May 6 2016
mstensho@opera.com : Could you please take a lok into this and update further as the alerts are not recovered.
,
May 10 2016
I cannot reproduce this performance regression at all, it seems. It's kind of surprising that the orphans/widows initial value change doesn't affect performance, though. I'll change the existing test to use orphans:1; widows:1; (which would make the test behave like it did before my patch) and add another new test that has higher values for those properties. Let's see if that clears the alerts.
,
May 10 2016
mstensho: the regression may be specific to the hardware we're using. You can try using the perf try bots to test out CLs on the exact hardware: https://www.chromium.org/developers/telemetry/performance-try-bots
,
May 10 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e1665788d89da9aeeb80ee6b2e0a9c8c65fbce08 commit e1665788d89da9aeeb80ee6b2e0a9c8c65fbce08 Author: mstensho <mstensho@opera.com> Date: Tue May 10 16:38:37 2016 Set orphans and widows to 1 in lots-of-text-balanced multicol performance test. https://codereview.chromium.org/1909233002 changed the initial values for orphans and widows to match the spec. This could very well affect performance slightly, although I cannot reproduce it myself. Speculatively set orphans/widows to 1 and see if that helps. Add a new test that uses higher values for orphans and widows, so that we still get to test that too. BUG= 606262 Review-Url: https://codereview.chromium.org/1964473003 Cr-Commit-Position: refs/heads/master@{#392625} [add] https://crrev.com/e1665788d89da9aeeb80ee6b2e0a9c8c65fbce08/third_party/WebKit/PerformanceTests/Layout/multicol/lots-of-text-balanced-orphans-widows.html [modify] https://crrev.com/e1665788d89da9aeeb80ee6b2e0a9c8c65fbce08/third_party/WebKit/PerformanceTests/Layout/multicol/lots-of-text-balanced.html
,
Jun 1 2016
Moving this nonessential bug to the next milestone. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jun 24 2016
Ping, the graph is not recovered, any update?
,
Jul 1 2016
I think this is recovered. Ned, did we look at the same graphs?
,
Jul 5 2016
Yes, this is recovered. |
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by toyoshim@chromium.org
, Apr 25 2016