New issue
Advanced search Search tips

Issue 643359 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Sep 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression

Blocking:
issue 79180



Sign in to add a comment

14.8%-16.9% regression in blink_perf.layout at 415632:415693

Project Member Reported by mustaq@chromium.org, Sep 1 2016

Issue description

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

Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?keys=agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICg4YbU4goM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICg4Zi5vgoM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICgobSw7QgM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICg4ZunrQoM


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

chromium-rel-win7-gpu-ati
chromium-rel-win7-gpu-intel
chromium-rel-win7-gpu-nvidia
chromium-rel-win7-x64-dual
Cc: svil...@igalia.com
Owner: svil...@igalia.com

=== Auto-CCing suspected CL author svillar@igalia.com ===

Hi svillar@igalia.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 : [css-grid] Implement fit-content track size
Author  : svillar
Commit description:
  
This CL implements the new <fit-content> track size which is defined as
follows: "Represents the formula min(max-content, max(auto, argument)),
which is calculated similar to auto (i.e. minmax(auto, max-content)), except
that the track size is clamped at argument if it is greater than the auto
minimum."

From the parsing POV fit-content was implemented as a new type of function
which only takes one argument. That forced us to refactor some code because
minmax() was the only allowed function for <track-size>s so far.

The implementation key is a new attribute in GridTrack called growthLimitCap
which is precisely the attribute of fit-content(). Some parts of the track sizing
algorithm were adapted to this change like for example the sorting of tracks by
growth potential (we need to consider the caps).

BUG= 618972 

Review-Url: https://codereview.chromium.org/2287113004
Cr-Commit-Position: refs/heads/master@{#415676}
Commit  : 1993c05386140afe56921048784bf3ca449f4b63
Date    : Wed Aug 31 17:43:04 2016


===== TESTED REVISIONS =====
Revision         Mean     Std Dev   N  Good?
chromium@415642  749.684  3.25797   5  good
chromium@415660  747.97   3.57644   5  good
chromium@415669  753.54   2.84553   5  good
chromium@415674  753.499  5.09173   5  good
chromium@415675  751.241  5.47193   5  good
chromium@415676  627.738  0.990206  5  bad    <--
chromium@415678  622.63   9.92542   5  bad

Bisect job ran on: winx64nvidia_perf_bisect
Bug ID: 643359

Test Command: src/tools/perf/run_benchmark -v --browser=release_x64 --output-format=chartjson --upload-results --also-run-disabled-tests blink_perf.layout
Test Metric: auto-grid-lots-of-data/auto-grid-lots-of-data
Relative Change: 16.95%
Score: 99.9

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/winx64nvidia_perf_bisect/builds/1850
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9002691519892175760


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

| 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 4 by svil...@igalia.com, Sep 2 2016

Working on this

Comment 5 by svil...@igalia.com, Sep 5 2016

So after bisecting a bit the changes it turns out that the main issue is the condition I've added to maxTrackBreadth(). That is causing most of the issues because it's called a lot.

I've to think in a new way to represent the different track sizes without having to add extra conditions.
Project Member

Comment 6 by bugdroid1@chromium.org, Sep 7 2016

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

commit 8b54987d15a92a54796a357daf6e021f0ff9a2b6
Author: svillar <svillar@igalia.com>
Date: Wed Sep 07 18:25:40 2016

[css-grid] Fix performance regression in grid layout

Some of the changes added in crrev.com/415676 made layout of grid containers
a bit slower. That was because it modified some methods in GridTrackSize.h
which is a hot spot. That class is basically a cache of some values and it's
critical to keep it really fast. The aforementioned CL was adding an if
clause to a getter method, and that was causing the slowdown.

BUG= 643359 

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

[modify] https://crrev.com/8b54987d15a92a54796a357daf6e021f0ff9a2b6/third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp
[modify] https://crrev.com/8b54987d15a92a54796a357daf6e021f0ff9a2b6/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
[modify] https://crrev.com/8b54987d15a92a54796a357daf6e021f0ff9a2b6/third_party/WebKit/Source/core/style/GridTrackSize.h

Comment 7 by r...@igalia.com, Sep 7 2016

Blocking: 79180
Cc: jfernan...@igalia.com r...@igalia.com
Components: Blink>Layout>Grid

Comment 8 by svil...@igalia.com, Sep 14 2016

Is it better now?

Comment 9 by mustaq@chromium.org, Sep 14 2016

Status: Verified (was: Assigned)
Yes, all metrics have recovered from the regressions, in most cases to a better level than pre-Aug-21 level. Thanks svillar@.

Sign in to add a comment