Issue metadata
Sign in to add a comment
|
17.3% regression in blink_perf.layout at 398526:398543 |
||||||||||||||||||||||
Issue descriptionSee the link to graphs below.
,
Jun 9 2016
=== 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!
,
Jun 9 2016
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.
,
Jun 9 2016
FYI, I'm going to play around with an optimization for this case in flexbox: https://codereview.chromium.org/2056043002/
,
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.
,
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
,
Jun 10 2016
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.
,
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 |
|||||||||||||||||||||||
Comment 1 by pmeenan@chromium.org
, Jun 9 2016