New issue
Advanced search Search tips

Issue 654025 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

10%-11.7% regression in blink_perf.layout at 422666:422728

Project Member Reported by qyears...@chromium.org, Oct 7 2016

Issue description

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

Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?keys=agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICg9aa2oAoM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICgteWLpQoM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICgtbXYuQkM


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

chromium-rel-mac-hdd
chromium-rel-mac10
chromium-rel-mac11
Cc: malaykeshav@chromium.org
Owner: malaykeshav@chromium.org

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

Hi malaykeshav@chromium.org, 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 : Use scrollbar thickness to set layout
Author  : malaykeshav
Commit description:
  
BUG= 651940 
COMPONENT=Paint, ScrollableArea, Scrollbar
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2382103006
Cr-Commit-Position: refs/heads/master@{#422706}
Commit  : 8d329a8b3ca5f11c064d1e109404613758dc3b45
Date    : Tue Oct 04 04:34:57 2016


===== TESTED REVISIONS =====
Revision         Mean     Std Dev    N  Good?
chromium@422685  10.8171  0.13094    5  good
chromium@422705  10.7469  0.109503   5  good
chromium@422706  9.71921  0.130258   5  bad    <--
chromium@422707  9.57042  0.143319   4  bad
chromium@422708  9.68189  0.132258   5  bad
chromium@422710  9.74734  0.0927136  5  bad
chromium@422715  9.6386   0.122609   5  bad
chromium@422725  9.67201  0.147105   5  bad

Bisect job ran on: mac_hdd_perf_bisect
Bug ID: 654025

Test Command: src/tools/perf/run_benchmark -v --browser=release --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests blink_perf.layout
Test Metric: flexbox-lots-of-data/flexbox-lots-of-data
Relative Change: 10.59%
Score: 99.9

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/mac_hdd_perf_bisect/builds/839
Job details: https://chromeperf.appspot.com/buildbucket_job_status/8999432561586350672


Not what you expected? We'll investigate and get back to you!
  https://chromeperf.appspot.com/bad_bisect?try_job_id=5318517841199104

| 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!
Cc: wangxianzhu@chromium.org bsep@chromium.org osh...@chromium.org
@oshima
Performing a scaling operation on the values during every fetch of scrollbar thickness seems to be causing this regression.
Which CL is it? If it's the one that fixed the layout, it fixed the issue where blink *didn't layout* when it should, so it could be that the original result wasn't expected result.
Re #6 We missed layout on scale change and the CL did fix that. However, the test case doesn't contain the bug case. If the bisect is correct, the performance regression is an unexpected side-effect of the CL. Perhaps we should optimize scrollbarThickness by caching the result instead of calculating the value every time.


wangxianzhu@'s suggestion sounds like it could be a performance win; malaykeshav@, do you think you might want to try that?

If you do, you can keep this bug open, and use performance test try jobs to see if you're getting an improvement (see http://www.chromium.org/developers/telemetry/performance-try-bots for more about this).
Status: Assigned (was: Untriaged)
Yes, #7 should work. 
Looking into it.
Project Member

Comment 10 by bugdroid1@chromium.org, Oct 31 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a044d31c7809559896994c2903d45860e075591e

commit a044d31c7809559896994c2903d45860e075591e
Author: malaykeshav <malaykeshav@chromium.org>
Date: Mon Oct 31 23:48:30 2016

Caches the scrollbar thickness for the given control size
to revert the regression.

BUG= 654025 
COMPONENT=Scrollbar, Webkit

Review-Url: https://codereview.chromium.org/2421573002
Cr-Commit-Position: refs/heads/master@{#428868}

[modify] https://crrev.com/a044d31c7809559896994c2903d45860e075591e/third_party/WebKit/Source/platform/scroll/Scrollbar.cpp
[modify] https://crrev.com/a044d31c7809559896994c2903d45860e075591e/third_party/WebKit/Source/platform/scroll/Scrollbar.h

Status: Fixed (was: Assigned)

Sign in to add a comment