Issue metadata
Sign in to add a comment
|
3.9% regression in thread_times.key_silk_cases at 381372:381387 |
||||||||||||||||||||
Issue descriptionSee the link to graphs below.
,
Mar 16 2016
=== Auto-CCing suspected CL author cbiesinger@chromium.org === Hi cbiesinger@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 : [css-flexbox] Correctly handle percentage sizing in the main axis Author : cbiesinger Commit description: Flex items should have a definite size subject to certain conditions: https://drafts.csswg.org/css-flexbox/#definite-sizes This patch implements part 2 of that section. This also fixes a bug in definite-cross-sizes; I put the flex-one class on the wrong div. flex-flow-padding has percentage children of flexboxes and thus needed updating. BUG= 346275 NOPRESUBMIT=true Review URL: https://codereview.chromium.org/1807653002 Cr-Commit-Position: refs/heads/master@{#381385} Commit : ab4d700467b65a395a64c02d1a7d2777950ec0cd Date : Wed Mar 16 02:38:43 2016 ===== TESTED REVISIONS ===== Revision Mean Value Std. Dev. Num Values Good? chromium@381371 3.180248 0.013996 5 good chromium@381379 3.209537 0.007898 5 good chromium@381383 3.209999 0.015488 8 good chromium@381384 3.20806 0.014088 8 good chromium@381385 3.32911 0.017369 5 bad chromium@381387 3.338911 0.014334 8 bad Bisect job ran on: android_nexus5_perf_bisect Bug ID: 595361 Test Command: src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --also-run-disabled-tests thread_times.key_silk_cases Test Metric: thread_renderer_compositor_cpu_time_per_frame/thread_renderer_compositor_cpu_time_per_frame Relative Change: 5.07% Score: 99.9 Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_nexus5_perf_bisect/builds/3501 Job details: https://chromeperf.appspot.com/buildbucket_job_status/9018015492610753616 | 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 label Cr-Tests-AutoBisect. Thank you!
,
Mar 17 2016
Well that sucks. I'm waiting for a response to https://lists.w3.org/Archives/Public/www-style/2016Feb/0316.html, which may let us get an easy speedup here.
,
Mar 17 2016
,
Mar 25 2016
Any update on this?
,
Mar 26 2016
Still waiting for a response as per comment 3 :(
,
Apr 1 2016
Any update on this bug?
,
Apr 8 2016
Any update cbiesinger@?
,
Apr 8 2016
I have some ideas on how to optimize this but it'll take me a little bit to get to that
,
Apr 8 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/37e278b6d096600cc1351a40aefd0e7a71601167 commit 37e278b6d096600cc1351a40aefd0e7a71601167 Author: cbiesinger <cbiesinger@chromium.org> Date: Fri Apr 08 22:51:17 2016 [css-flexbox] slight improvements to mainSizeForPercentageResolution We can skip a check if flexBasis has a percentage (see comment in patch). Also, reusing mainAxisLengthIsDefinite will be slightly more correct. R=leviw@chromium.org CC=dgrogran@chromium.org BUG= 595361 Review URL: https://codereview.chromium.org/1870163004 Cr-Commit-Position: refs/heads/master@{#386234} [modify] https://crrev.com/37e278b6d096600cc1351a40aefd0e7a71601167/third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp
,
Apr 15 2016
There is no recover around #386234. cbiesinger@, can you update the status of this bug?
,
Apr 22 2016
Agreed, not seeing recovery here. cbiesinger, any updates?
,
Apr 29 2016
Looking at the graphs for these alerts (https://chromeperf.appspot.com/group_report?bug_id=595361), I think we can see two things: The results for "thread_total_fast_path_cpu_time_per_frame/inbox_app.html?stress_hidey_bars" were always 0.0 before, suggesting that something was broken before, and then it got fixed. This is not a performance regression, but a fix in functionality. The results for "thread_renderer_compositor_cpu_time_per_frame" regressed, but by a relatively small margin compared to the noise. So small that we can't say for sure that it isn't noise. Therefore, I'll start another bisect job for "thread_renderer_compositor_cpu_time_per_frame" to see if it can be reproduced again, and if it can't be clearly reproduced, then that suggests that there may not have been a real performance regression in the first place.
,
Apr 30 2016
=== Auto-CCing suspected CL author cbiesinger@chromium.org === Hi cbiesinger@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 : [css-flexbox] Correctly handle percentage sizing in the main axis Author : cbiesinger Commit description: Flex items should have a definite size subject to certain conditions: https://drafts.csswg.org/css-flexbox/#definite-sizes This patch implements part 2 of that section. This also fixes a bug in definite-cross-sizes; I put the flex-one class on the wrong div. flex-flow-padding has percentage children of flexboxes and thus needed updating. BUG= 346275 NOPRESUBMIT=true Review URL: https://codereview.chromium.org/1807653002 Cr-Commit-Position: refs/heads/master@{#381385} Commit : ab4d700467b65a395a64c02d1a7d2777950ec0cd Date : Wed Mar 16 02:38:43 2016 ===== TESTED REVISIONS ===== Revision Mean Std Dev N Good? chromium@381377 3.8244 0.0186766 5 good chromium@381383 3.83755 0.0126326 5 good chromium@381384 3.81867 0.0137765 5 good chromium@381385 3.9248 0.0258171 5 bad <-- chromium@381386 3.91619 0.0428122 5 bad chromium@381388 3.90964 0.0277142 5 bad Bisect job ran on: android_s5_perf_bisect Bug ID: 595361 Test Command: src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --also-run-disabled-tests thread_times.key_silk_cases Test Metric: thread_renderer_compositor_cpu_time_per_frame/thread_renderer_compositor_cpu_time_per_frame Relative Change: 2.23% Score: 99.8 Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_s5_perf_bisect/builds/634 Job details: https://chromeperf.appspot.com/buildbucket_job_status/9014015667757815376 Not what you expected? We'll investigate and get back to you! https://chromeperf.appspot.com/bad_bisect?try_job_id=5846572055658496 | 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!
,
May 6 2016
@cbiesinger -- Could you please respond to comment# 14 and update the issue. Thanks in Advance.
,
May 10 2016
This bisect looks pretty clear - cbiesinger@, can you confirm that this is your patch?
,
May 20 2016
Ping cbiesinger@. What should we do about this regression?
,
May 27 2016
Working on another change that might help here.
,
May 27 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/237e6796e049700ccb7adf105d3f95ba9572e66c commit 237e6796e049700ccb7adf105d3f95ba9572e66c Author: cbiesinger <cbiesinger@chromium.org> Date: Fri May 27 22:20:14 2016 [css-flexbox] logical widths are always definite. Per guidance from the CSS working group we should always treat widths as definite, which allows us to simplify & speed up some code in Flexbox. See also https://lists.w3.org/Archives/Public/www-style/2016May/0135.html which I believe covers all the cases where they might not be (=shrinkwrapping). Because this means that for widths we always use the actual width of the element for percentage calculations, we can remove some code from LayoutBox too. BUG= 595361 Review-Url: https://codereview.chromium.org/2022553002 Cr-Commit-Position: refs/heads/master@{#396584} [modify] https://crrev.com/237e6796e049700ccb7adf105d3f95ba9572e66c/third_party/WebKit/Source/core/layout/LayoutBox.cpp [modify] https://crrev.com/237e6796e049700ccb7adf105d3f95ba9572e66c/third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp [modify] https://crrev.com/237e6796e049700ccb7adf105d3f95ba9572e66c/third_party/WebKit/Source/core/layout/LayoutFlexibleBox.h
,
Jun 7 2016
I don't know how much that last fix helped, but it is a safe fix that I'd like to merge to 52.
,
Jun 7 2016
Your change meets the bar and is auto-approved for M52 (branch: 2743)
,
Jun 7 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/41265a04a2f3c3db5988012aae575b3667639674 commit 41265a04a2f3c3db5988012aae575b3667639674 Author: Christian Biesinger <cbiesinger@chromium.org> Date: Tue Jun 07 17:55:05 2016 [css-flexbox] logical widths are always definite. Per guidance from the CSS working group we should always treat widths as definite, which allows us to simplify & speed up some code in Flexbox. See also https://lists.w3.org/Archives/Public/www-style/2016May/0135.html which I believe covers all the cases where they might not be (=shrinkwrapping). Because this means that for widths we always use the actual width of the element for percentage calculations, we can remove some code from LayoutBox too. BUG= 595361 Review-Url: https://codereview.chromium.org/2022553002 Cr-Commit-Position: refs/heads/master@{#396584} (cherry picked from commit 237e6796e049700ccb7adf105d3f95ba9572e66c) Review URL: https://codereview.chromium.org/2047743002 . Cr-Commit-Position: refs/branch-heads/2743@{#265} Cr-Branched-From: 2b3ae3b8090361f8af5a611712fc1a5ab2de53cb-refs/heads/master@{#394939} [modify] https://crrev.com/41265a04a2f3c3db5988012aae575b3667639674/third_party/WebKit/Source/core/layout/LayoutBox.cpp [modify] https://crrev.com/41265a04a2f3c3db5988012aae575b3667639674/third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp [modify] https://crrev.com/41265a04a2f3c3db5988012aae575b3667639674/third_party/WebKit/Source/core/layout/LayoutFlexibleBox.h
,
Jun 15 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/41265a04a2f3c3db5988012aae575b3667639674 commit 41265a04a2f3c3db5988012aae575b3667639674 Author: Christian Biesinger <cbiesinger@chromium.org> Date: Tue Jun 07 17:55:05 2016 [css-flexbox] logical widths are always definite. Per guidance from the CSS working group we should always treat widths as definite, which allows us to simplify & speed up some code in Flexbox. See also https://lists.w3.org/Archives/Public/www-style/2016May/0135.html which I believe covers all the cases where they might not be (=shrinkwrapping). Because this means that for widths we always use the actual width of the element for percentage calculations, we can remove some code from LayoutBox too. BUG= 595361 Review-Url: https://codereview.chromium.org/2022553002 Cr-Commit-Position: refs/heads/master@{#396584} (cherry picked from commit 237e6796e049700ccb7adf105d3f95ba9572e66c) Review URL: https://codereview.chromium.org/2047743002 . Cr-Commit-Position: refs/branch-heads/2743@{#265} Cr-Branched-From: 2b3ae3b8090361f8af5a611712fc1a5ab2de53cb-refs/heads/master@{#394939} [modify] https://crrev.com/41265a04a2f3c3db5988012aae575b3667639674/third_party/WebKit/Source/core/layout/LayoutBox.cpp [modify] https://crrev.com/41265a04a2f3c3db5988012aae575b3667639674/third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp [modify] https://crrev.com/41265a04a2f3c3db5988012aae575b3667639674/third_party/WebKit/Source/core/layout/LayoutFlexibleBox.h
,
Jun 24 2016
Most of the alerts on this bug seem to be fixing a test, since the numbers went from 0 to something reasonable. I'm going to mark this closed.
,
Jul 7 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e31affc47d4167415327e54c48da086e9152ee10 commit e31affc47d4167415327e54c48da086e9152ee10 Author: cbiesinger <cbiesinger@chromium.org> Date: Thu Jul 07 03:05:43 2016 [css-flexbox] Don't over-invalidate flex items dirtyForLayoutFromPercentageHeightDescendants invalidates too much. We already have code to handle this case, so remove this call. We did have a bug in that code though -- we do need to force layout on flex items even if did not lay them out already: such percentages are only resolvable if the flexbox has a definite height. However, whether the flexbox height is definite may have changed since the last layout. So, we need to relayout in either case. The similar code in applyStretchAlignment does not have this issue because percentages in the cross axis can be resolved independent of whether the flexbox height itself is definite. This is covered by an existing test; the call to dirtyForLayoutFromPercentageHeightDescendants was previously covering this case but is slower. This caused a massive performance regression (many minutes to lay out a flexbox-based website instead of a few seconds). TEST=css3/flexbox/definite-cross-sizes.html BUG= 595361 , 617792 R=dgrogan@chromium.org,eae@chromium.org Review-Url: https://codereview.chromium.org/2128093003 Cr-Commit-Position: refs/heads/master@{#404056} [modify] https://crrev.com/e31affc47d4167415327e54c48da086e9152ee10/third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp
,
Jul 8 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/447be33eb6e7761a91cc6c40603ab23897233724 commit 447be33eb6e7761a91cc6c40603ab23897233724 Author: Christian Biesinger <cbiesinger@chromium.org> Date: Fri Jul 08 16:46:23 2016 [css-flexbox] Don't over-invalidate flex items dirtyForLayoutFromPercentageHeightDescendants invalidates too much. We already have code to handle this case, so remove this call. We did have a bug in that code though -- we do need to force layout on flex items even if did not lay them out already: such percentages are only resolvable if the flexbox has a definite height. However, whether the flexbox height is definite may have changed since the last layout. So, we need to relayout in either case. The similar code in applyStretchAlignment does not have this issue because percentages in the cross axis can be resolved independent of whether the flexbox height itself is definite. This is covered by an existing test; the call to dirtyForLayoutFromPercentageHeightDescendants was previously covering this case but is slower. This caused a massive performance regression (many minutes to lay out a flexbox-based website instead of a few seconds). TEST=css3/flexbox/definite-cross-sizes.html BUG= 595361 , 617792 R=dgrogan@chromium.org,eae@chromium.org Review-Url: https://codereview.chromium.org/2128093003 Cr-Commit-Position: refs/heads/master@{#404056} (cherry picked from commit e31affc47d4167415327e54c48da086e9152ee10) Review URL: https://codereview.chromium.org/2135733002 . Cr-Commit-Position: refs/branch-heads/2743@{#599} Cr-Branched-From: 2b3ae3b8090361f8af5a611712fc1a5ab2de53cb-refs/heads/master@{#394939} [modify] https://crrev.com/447be33eb6e7761a91cc6c40603ab23897233724/third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp
,
Jul 8 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/291a41e2a82664720953ddc7bc6b3f4e86f07b83 commit 291a41e2a82664720953ddc7bc6b3f4e86f07b83 Author: Christian Biesinger <cbiesinger@chromium.org> Date: Fri Jul 08 16:52:02 2016 [css-flexbox] Don't over-invalidate flex items dirtyForLayoutFromPercentageHeightDescendants invalidates too much. We already have code to handle this case, so remove this call. We did have a bug in that code though -- we do need to force layout on flex items even if did not lay them out already: such percentages are only resolvable if the flexbox has a definite height. However, whether the flexbox height is definite may have changed since the last layout. So, we need to relayout in either case. The similar code in applyStretchAlignment does not have this issue because percentages in the cross axis can be resolved independent of whether the flexbox height itself is definite. This is covered by an existing test; the call to dirtyForLayoutFromPercentageHeightDescendants was previously covering this case but is slower. This caused a massive performance regression (many minutes to lay out a flexbox-based website instead of a few seconds). TEST=css3/flexbox/definite-cross-sizes.html BUG= 595361 , 617792 R=dgrogan@chromium.org,eae@chromium.org Review-Url: https://codereview.chromium.org/2128093003 Cr-Commit-Position: refs/heads/master@{#404056} (cherry picked from commit e31affc47d4167415327e54c48da086e9152ee10) Review URL: https://codereview.chromium.org/2136473003 . Cr-Commit-Position: refs/branch-heads/2785@{#59} Cr-Branched-From: 68623971be0cfc492a2cb0427d7f478e7b214c24-refs/heads/master@{#403382} [modify] https://crrev.com/291a41e2a82664720953ddc7bc6b3f4e86f07b83/third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp |
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by pras...@chromium.org
, Mar 16 2016