New issue
Advanced search Search tips

Issue 618664 link

Starred by 1 user

Issue metadata

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

Blocking:
issue 79180



Sign in to add a comment

17.3% regression in blink_perf.layout at 398526:398543

Project Member Reported by pmeenan@chromium.org, Jun 9 2016

Issue description

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

Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?keys=agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICg3Ie64ggM


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

chromium-rel-mac10
Cc: r...@igalia.com
Owner: r...@igalia.com

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

Hi rego@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] Percentage columns can always be resolved during layout
Author  : rego
Commit description:
  
The issue is that the inline size of the grid container
is only indefinite while we're computing the intrinsic sizes.
During layout we should be able to resolve the percentage tracks
against that size.
This makes Grid Layout compatible with regular blocks regarding
how inline percentages are resolved.

The patch passes the SizingOperation enum to LayoutGrid::gridTrackSize().
That way we can know if we're computing the intrinsic sizes or not.
It also gets rid of LayoutBox::hasDefiniteLogicalWidth()
as it was wrong and not needed actually.

Created a new test verifying the expected behavior. Updated the results
in a few tests too.

BUG= 616716 
TEST=fast/css-grid-layout/grid-container-percentage-columns.html

Review-Url: https://codereview.chromium.org/2033033002
Cr-Commit-Position: refs/heads/master@{#398535}
Commit  : 3043a48f261b5d1b4f175a2a8f2373ac22ae41f1
Date    : Wed Jun 08 13:37:01 2016


===== TESTED REVISIONS =====
Revision         Mean     Std Dev  N  Good?
chromium@398525  604.165  4.2259   5  good
chromium@398534  612.285  5.36715  5  good
chromium@398535  501.099  4.48606  5  bad    <--
chromium@398536  493.37   9.27206  5  bad
chromium@398537  488.018  12.4008  5  bad
chromium@398539  494.238  3.90131  5  bad
chromium@398543  499.734  2.40492  5  bad

Bisect job ran on: mac_10_10_perf_bisect
Bug ID: 618664

Test Command: src/tools/perf/run_benchmark -v --browser=release --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: 17.29%
Score: 99.9

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/mac_10_10_perf_bisect/builds/2144
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9010328094855230272


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

| 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 r...@igalia.com, Jun 9 2016

Blocking: 79180
Cc: cbiesin...@chromium.org svil...@igalia.com jfernan...@igalia.com
Components: Blink>Layout>Grid
Status: Started (was: Assigned)
Yes, second time I do the same mistake (see  bug #457606 ). I'm sorry about that. :-(

The problem is that with my patch we're calling too many times hasDefiniteLogicalHeight() which is quite expensive.

I got rid of that method in https://codereview.chromium.org/2037383002,
but the performance is still worse so I'll come back to the previous solution and avoid unneeded calls there.
FYI, I'm going to play around with an optimization for this case in flexbox: https://codereview.chromium.org/2056043002/

Comment 5 by r...@igalia.com, Jun 10 2016

@cbiesinger we've plans to do something similar for Grid too,
as we need to check if the height is indefinite in several places.
But that would be addressed in future patches.
Project Member

Comment 6 by bugdroid1@chromium.org, Jun 10 2016

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

commit 06fdd11aadb98dd6e25c6aaace831998b23eae86
Author: rego <rego@igalia.com>
Date: Fri Jun 10 07:23:53 2016

[css-grid] Avoid unneeded calls to percentageLogicalHeightIsResolvable()

r398535 was introducing a performance regression on grid layout tests.
The problem is that it was calling LayoutGrid::hasDefiniteLogicalSize()
which was very slow.

r398680 was getting rid of hasDefiniteLogicalSize() and replacing it
by calls to LayoutBox::percentageLogicalHeightIsResolvable().
Despite being faster it's still an expensive operation and
we don't need to do it always.

So in this patch we're avoid calling percentageLogicalHeightIsResolvable()
when the min and max track breadths are not percentages.

BUG= 618664 , 616716 

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

[modify] https://crrev.com/06fdd11aadb98dd6e25c6aaace831998b23eae86/third_party/WebKit/Source/core/layout/LayoutGrid.cpp

Comment 7 by r...@igalia.com, Jun 10 2016

Status: Fixed (was: Started)
I don't know whats going on with the perf trybots as I cannot see the error's page:
https://codereview.chromium.org/2050043005/
https://codereview.chromium.org/2056173002/

So, I've just decided to land this patch.
I'll keep an eye on the perfbots but the regression should be fixed with this.
Project Member

Comment 8 by bugdroid1@chromium.org, Jun 15 2016

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

commit 06fdd11aadb98dd6e25c6aaace831998b23eae86
Author: rego <rego@igalia.com>
Date: Fri Jun 10 07:23:53 2016

[css-grid] Avoid unneeded calls to percentageLogicalHeightIsResolvable()

r398535 was introducing a performance regression on grid layout tests.
The problem is that it was calling LayoutGrid::hasDefiniteLogicalSize()
which was very slow.

r398680 was getting rid of hasDefiniteLogicalSize() and replacing it
by calls to LayoutBox::percentageLogicalHeightIsResolvable().
Despite being faster it's still an expensive operation and
we don't need to do it always.

So in this patch we're avoid calling percentageLogicalHeightIsResolvable()
when the min and max track breadths are not percentages.

BUG= 618664 , 616716 

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

[modify] https://crrev.com/06fdd11aadb98dd6e25c6aaace831998b23eae86/third_party/WebKit/Source/core/layout/LayoutGrid.cpp

Sign in to add a comment