New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 595361 link

Starred by 0 users

Issue metadata

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



Sign in to add a comment

3.9% regression in thread_times.key_silk_cases at 381372:381387

Project Member Reported by pras...@chromium.org, Mar 16 2016

Issue description

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

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


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

android-nexus5
Project Member

Comment 2 by 42576172...@developer.gserviceaccount.com, Mar 16 2016

Cc: cbiesin...@chromium.org
Owner: cbiesin...@chromium.org

=== 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!
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.
Cc: tabatkins@chromium.org
Any update on this?
Still waiting for a response as per comment 3 :(
Any update on this bug?
Any update  cbiesinger@?
I have some ideas on how to optimize this but it'll take me a little bit to get to that
Project Member

Comment 10 by bugdroid1@chromium.org, 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

There is no recover around #386234. cbiesinger@, can you update the status of this bug?
Agreed, not seeing recovery here. cbiesinger, any updates?
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.
Project Member

Comment 14 by 42576172...@developer.gserviceaccount.com, 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!
@cbiesinger -- Could you please respond to comment# 14 and update the issue.
Thanks in Advance.
This bisect looks pretty clear - cbiesinger@, can you confirm that this is your patch?
Ping cbiesinger@. What should we do about this regression?
Working on another change that might help here.
Project Member

Comment 19 by bugdroid1@chromium.org, 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

Labels: Merge-Request-52
I don't know how much that last fix helped, but it is a safe fix that I'd like to merge to 52.

Comment 21 by tin...@google.com, Jun 7 2016

Labels: -Merge-Request-52 Merge-Approved-52 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M52 (branch: 2743)
Project Member

Comment 22 by bugdroid1@chromium.org, Jun 7 2016

Labels: -merge-approved-52 merge-merged-2743
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

Project Member

Comment 23 by bugdroid1@chromium.org, 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

Labels: -performance-sheriff Performance-Sheriff
Status: Fixed (was: Assigned)
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.
Project Member

Comment 25 by bugdroid1@chromium.org, 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

Project Member

Comment 26 by bugdroid1@chromium.org, 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

Project Member

Comment 27 by bugdroid1@chromium.org, Jul 8 2016

Labels: merge-merged-2785
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