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

Issue 606262 link

Starred by 0 users

Issue metadata

Status: Verified
Owner:
NOT IN USE
Closed: Jul 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

15.8%-25.8% regression in blink_perf.layout at 389053:389070

Project Member Reported by toyoshim@chromium.org, Apr 25 2016

Issue description

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

Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?keys=agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICgpIenoAoM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICgpJi-rgoM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICgpPufqwoM


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

chromium-rel-win7-x64-dual
linux-release
win-zenbook
Project Member

Comment 2 by 42576172...@developer.gserviceaccount.com, Apr 25 2016

Cc: msten...@opera.com
Owner: msten...@opera.com

=== 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!

Comment 3 by msten...@opera.com, 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.
Cc: -msten...@opera.com durga.behera@chromium.org
Labels: Needs-Feedback TE-Triaged
mstensho@opera.com : Could you please take a lok into this and update further as the alerts are not recovered.

Comment 5 by msten...@opera.com, 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.
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
Project Member

Comment 7 by bugdroid1@chromium.org, 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

Project Member

Comment 8 by sheriffbot@chromium.org, Jun 1 2016

Labels: -M-52 M-53 MovedFrom-52
Moving this nonessential bug to the next milestone.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -performance-sheriff Performance-Sheriff
Ping, the graph is not recovered, any update?
Cc: nednguyen@chromium.org
I think this is recovered. Ned, did we look at the same graphs?
Status: Verified (was: Assigned)
Yes, this is recovered.

Sign in to add a comment