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

Issue 873452 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Sep 25
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 2
Type: Bug-Regression



Sign in to add a comment

Resize performance regression

Reported by t...@tobireif.com, Aug 11

Issue description

UserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/68.0.3440.106 Safari/537.36

Steps to reproduce the problem:
On MacOS, in stable Chrome, load https://tobireif.com/demos/grid/ , grab the right edge of the window, and resize forth and back (width various speeds, sometimes fast sometimes slower).

The performance is sufficient (and thus there's no need to change my code).

Now do the same in Canary - the performance is very bad, with a large black area showing at the right edge on upsize.

What is the expected behavior?
Great perf.

What went wrong?
Bad perf.

Did this work before? Yes 68

Does this work in other browsers? N/A

Chrome version: 68.0.3440.106  Channel: stable
OS Version: OS X 10.13.6
Flash Version: 

Please ensure that the performance of Chrome 68 is preserved (or improved) in future Chrome versions.

Please also check on other OSs.
 
Labels: Needs-Triage-M68 Needs-Bisect
Status: Untriaged (was: Unconfirmed)
Confirmed in Chrome 68 Stable and 70 Canary. (It is working better in Safari on macOS.)
Cc: krajshree@chromium.org
Components: Blink>Canvas
Labels: Triaged-ET Needs-Feedback
Tested the issue on mac 10.13.1 using chrome latest stable #68.0.3440.106 and latest canary #70.0.3517.0.
Attached a screen cast for reference.

Following are the steps followed to reproduce the issue.
------------
1. Navigated to https://tobireif.com/demos/grid/
2. Grabbed the right edge of the window, and resized forth and back.
3. Checked chrome task manager and observed that the CPU memory increases.
Note: From M-60, the same behavior is observed in chrome. The CPU memory increases till 50 MB, resulting in bad performance.
 
tobi@ - Could you please check the attached screen cast of M-60 and please let us know if it is the issue being faced by you.

Thanks...!!
873452.mp4
12.3 MB View Download
Components: -Blink>Canvas Blink>Layout>Grid
This is not canvas related, reassigning.
"Confirmed in Chrome 68 Stable and 70 Canary."

Thanks!

"It is working better in Safari on macOS."

Here (on the latest MacOS stable) the performance (regarding the reported issue) of Chrome stable 68 is better than Safari's. I hope that this great perf of Chrome 68 can be ensured for future Chrome versions (current Canary's perf is very bad). Ideally there'd be an automated test preventing repeat regressions.

"Could you please check the attached screen cast of M-60 and please let us know if it is the issue being faced by you."

Thanks! Yes it is. The issue also can be observed with slower upsizing, see the attached video (showing the issue in Canary).
video_480.mov
4.6 MB View Download
Is this a Mac only issue ? I fail to see any regression in 70, comparing to 68, at least in Linux.  

In both versions the Memory Footprint increases from 27MB to 75MB and CPU usage is quite similar in both versions.

Is there a more deterministic way to determine the poor performance levels ? eg, rendering time from dev tools. The description in the original report is quite vage; " with a large black area showing at the right edge on upsize." I haven't see such artifact in my tests, besides the back edge caused by reaching the maximum width of the demo, which is centered for bigger widths.
"Is this a Mac only issue ?"

That might well be the case. I observed it on MacOS. (It might also be observable on other OSs given the right type of hardware perhaps?)

I hope you can test and reproduce this confirmed regression on MacOS.

(Latest OS, but the hardware is not extremely recent/powerful: "MacBook Pro (Retina, 15-inch, Mid 2015)", "2,2 GHz Intel Core i7", "16 GB 1600 MHz DDR3", "Intel Iris Pro 1536 MB").

"I fail to see any regression in 70, comparing to 68, at least in Linux."

I hope you can reproduce the regression, eg on MacOS.

Also see comment #2:
"Confirmed in Chrome 68 Stable and 70 Canary."

Also see the video in Comment 3. The video can be stopped to show that there sometimes is a black area of substantial width at the right side of the page in window widths where there should be none.

"Is there a more deterministic way to determine the poor performance levels ?"

I hope you can benchmark Chrome 68 vs Chrome Canary in a way that fully shows you the performance difference during resizing, and the causes for that regression.

"The description in the original report is quite vage; " with a large black area showing at the right edge on upsize."

Not vague. Please see each of the above videos for illustration.
I guess https://bugs.chromium.org/u/1119298406/ used MacOS to reach this conclusion:
Comment #2:
"Confirmed in Chrome 68 Stable and 70 Canary."
First of all, I admit that the performance for this case is bad. Also, I don't deny the regression exists, only that I'm not able to perceive in Linux with my current hardware.  

I see now the black area you mentioned before; I couldn't reproduce such artifact in my environment. I'll try to evaluate using macOS.

Regarding comment #2 ("Confirmed in Chrome 68 Stable and 70 Canary.") I don't fully understand it, since it seems to imply the regressions affects both 68 and 70, but the original report indicates that performance in 68 was ok but it regressed in 70. 

I'd really need to know what version provides an acceptable performance for this case. 

Finally, we already have regressions tests to evaluate and detect performance regressions. However, those tests don't cover any possible grid use case. It's possible that this demo spotted a case that we are not monitoring now.

Anyway, I'm still working on this and will provide additional feedback as soon as possible.
The linux bots detected a regression in one of our tests, which evaluates resizes of huge grids with auto-sized tracks: 

https://chromeperf.appspot.com/report?sid=a59eac6929f7bc6f1c795ae6d28fa8b219931447d2a8e8f5484e27fcaa68936b&start_rev=554053&end_rev=569878

The regression was also detected by the mac bots (although less clearly because that bot has less data to work on):

https://chromeperf.appspot.com/report?sid=e19b4632d71d74c76612c0404641bc1edbfa1110be028b99d71e8a8343cc4d00&start_rev=553413&end_rev=563240

The root cause of this regression is probably this commit:

https://chromium.googlesource.com/chromium/src/+/1f4159ae4aa6d85d967bc4f96b3305e4a2f8ac31

The change identified above was related to the implementation of the Baseline Alignment algorithm as part of the track sizing, following the resolution the CSS WG took to get better results on grid items participating in baseline alignment inside auto-sized tracks.

The change points to #r562406, which initially landed in 69.0.3445.0. There were no changes specifically focused on improving the performance of this logic (although we added several that could help to improve performance in the general case), so it's likely that this regressions still exists in 70 Canary.

I guess that the grid case the demo implement doesn't use Baseline alignment, so it'd be expected that this new logic added wouldn't have affected. However, the changes in the spec regarding the track sizing, specially for the auto-sized tracks, imply additional operations that have to be done now that were not necessary before this change in the spec. 

I want to finish my comment stating that, even though we will have to assume an overhead due to the changes introduced in the spec, I think it's possible that we can work on improving the performance of this new logic and its impact on the track sizing algorithm.
Owner: jfernan...@igalia.com
Status: Started (was: Untriaged)
>The root cause of this regression is probably this commit:
>https://chromium.googlesource.com/chromium/src/+/1f4159ae4aa6d85d967bc4f96b3305e4a2f8ac31

Sorry, the commit reference was not the one I meant, but this one:

https://chromium.googlesource.com/chromium/src/+/6534acd9b94a260ccf88ccdfd7ab8b3859349082
Thanks for investigating!
"Regarding comment #2 ("Confirmed in Chrome 68 Stable and 70 Canary.") I don't fully understand it, since it seems to imply the regressions affects both 68 and 70, but the original report indicates that performance in 68 was ok but it regressed in 70."

Best to ask the poster https://bugs.chromium.org/u/1119298406/ , but I guess the comment meant to stated "Confirmed in Chrome 68 Stable [vs] 70 Canary" because that's what I had reported.

"I'd really need to know what version provides an acceptable performance for this case."

In the issue report I report that the perf is bad in Canary, and I state that "The performance is sufficient [in the mentioned Chrome 68]" and "Please ensure that the performance of Chrome 68 is preserved (or improved) in future Chrome versions."

I hope that helps.

"I think it's possible that we can work on improving the performance of this new logic and its impact on the track sizing algorithm"

Sounds great!
"meant to stated" → "meant to state"
I made some progress on this task; the CL https://crrev.com/c/1179897 has some experimental patches to evaluate the impact on performance of the several approaches I'm considering so far. 

I've got a 12% improvement that looks like promising, but I still needs some work.  
That does sound promising!

Thanks for working on this important issue.
Project Member

Comment 18 by bugdroid1@chromium.org, Sep 24

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

commit f0179abffe0534ee3487d73436b01c278777993e
Author: Javier Fernandez <jfernandez@igalia.com>
Date: Mon Sep 24 16:03:03 2018

[css-grid] Performance optimizations in the Baseline alignment code

Since we integrated the baseline alignment logic in the grid tracks
sizing algorithm, its impact on performance has grown considerably.

The analysis of the new logic added and its overhead, due to different
operations, shows that evaluating the item's participation in the
baseline alignment context is the most expensive one. It's specially
demanding the evaluation of the grid item's alignment properties.

Considering that currently we are doing this for every grid item, this
CL propose to reuse the loop we already have to clear the grid item's
override size to cache the items with a baseline value in their
alignment CSS properties.

Thanks to this cache we can determine the item's participation in a
baseline alignment context in the different phases of the track sizing
algorithm, with almost no cost (compared to the current logic). It may
be possible to share the cache with the algorithm run for computing the
grid's intrinsic size; however, if the intrinsic size logic is run
before the grid's layout, we'll end up duplicating the cache.

Additionally, this cache is also used in the alignment phase of the
grid layout logic; this change helps to avoid the various issues we
have been suffering related to the different evaluations of the item's
participation in baseline during the different phases of the grid
layout algorithm.

BUG =  873452 

Change-Id: Ida27be11ae0f5c455e6077367a277981ab35cec1
Reviewed-on: https://chromium-review.googlesource.com/1179897
Commit-Queue: Javier Fernandez <jfernandez@igalia.com>
Reviewed-by: Manuel Rego <rego@igalia.com>
Cr-Commit-Position: refs/heads/master@{#593552}
[add] https://crrev.com/f0179abffe0534ee3487d73436b01c278777993e/third_party/WebKit/LayoutTests/external/wpt/css/css-grid/alignment/grid-alignment-style-changes-001.html
[add] https://crrev.com/f0179abffe0534ee3487d73436b01c278777993e/third_party/WebKit/LayoutTests/external/wpt/css/css-grid/alignment/grid-alignment-style-changes-002.html
[add] https://crrev.com/f0179abffe0534ee3487d73436b01c278777993e/third_party/WebKit/LayoutTests/external/wpt/css/css-grid/alignment/grid-alignment-style-changes-003.html
[add] https://crrev.com/f0179abffe0534ee3487d73436b01c278777993e/third_party/WebKit/LayoutTests/external/wpt/css/css-grid/alignment/grid-alignment-style-changes-004.html
[add] https://crrev.com/f0179abffe0534ee3487d73436b01c278777993e/third_party/WebKit/LayoutTests/external/wpt/css/css-grid/alignment/grid-alignment-style-changes-005.html
[add] https://crrev.com/f0179abffe0534ee3487d73436b01c278777993e/third_party/WebKit/LayoutTests/external/wpt/css/css-grid/alignment/grid-alignment-style-changes-006.html
[add] https://crrev.com/f0179abffe0534ee3487d73436b01c278777993e/third_party/WebKit/LayoutTests/external/wpt/css/css-grid/alignment/grid-alignment-style-changes-007.html
[add] https://crrev.com/f0179abffe0534ee3487d73436b01c278777993e/third_party/WebKit/LayoutTests/external/wpt/css/css-grid/alignment/grid-alignment-style-changes-008.html
[add] https://crrev.com/f0179abffe0534ee3487d73436b01c278777993e/third_party/WebKit/LayoutTests/external/wpt/css/css-grid/alignment/grid-self-baseline-not-applied-if-sizing-cyclic-dependency-003.html
[modify] https://crrev.com/f0179abffe0534ee3487d73436b01c278777993e/third_party/WebKit/LayoutTests/external/wpt/css/css-grid/alignment/support/style-change.js
[add] https://crrev.com/f0179abffe0534ee3487d73436b01c278777993e/third_party/WebKit/LayoutTests/paint/invalidation/css-grid-layout/align-items-change-from-baseline-expected.html
[add] https://crrev.com/f0179abffe0534ee3487d73436b01c278777993e/third_party/WebKit/LayoutTests/paint/invalidation/css-grid-layout/align-items-change-from-baseline-expected.txt
[add] https://crrev.com/f0179abffe0534ee3487d73436b01c278777993e/third_party/WebKit/LayoutTests/paint/invalidation/css-grid-layout/align-items-change-from-baseline.html
[add] https://crrev.com/f0179abffe0534ee3487d73436b01c278777993e/third_party/WebKit/LayoutTests/paint/invalidation/css-grid-layout/align-items-change-to-baseline-expected.html
[add] https://crrev.com/f0179abffe0534ee3487d73436b01c278777993e/third_party/WebKit/LayoutTests/paint/invalidation/css-grid-layout/align-items-change-to-baseline-expected.txt
[add] https://crrev.com/f0179abffe0534ee3487d73436b01c278777993e/third_party/WebKit/LayoutTests/paint/invalidation/css-grid-layout/align-items-change-to-baseline.html
[add] https://crrev.com/f0179abffe0534ee3487d73436b01c278777993e/third_party/WebKit/LayoutTests/paint/invalidation/css-grid-layout/align-self-change-from-baseline-expected.html
[add] https://crrev.com/f0179abffe0534ee3487d73436b01c278777993e/third_party/WebKit/LayoutTests/paint/invalidation/css-grid-layout/align-self-change-from-baseline-expected.txt
[add] https://crrev.com/f0179abffe0534ee3487d73436b01c278777993e/third_party/WebKit/LayoutTests/paint/invalidation/css-grid-layout/align-self-change-from-baseline.html
[add] https://crrev.com/f0179abffe0534ee3487d73436b01c278777993e/third_party/WebKit/LayoutTests/paint/invalidation/css-grid-layout/align-self-change-to-baseline-expected.html
[add] https://crrev.com/f0179abffe0534ee3487d73436b01c278777993e/third_party/WebKit/LayoutTests/paint/invalidation/css-grid-layout/align-self-change-to-baseline-expected.txt
[add] https://crrev.com/f0179abffe0534ee3487d73436b01c278777993e/third_party/WebKit/LayoutTests/paint/invalidation/css-grid-layout/align-self-change-to-baseline.html
[modify] https://crrev.com/f0179abffe0534ee3487d73436b01c278777993e/third_party/blink/renderer/core/layout/grid_baseline_alignment.cc
[modify] https://crrev.com/f0179abffe0534ee3487d73436b01c278777993e/third_party/blink/renderer/core/layout/grid_baseline_alignment.h
[modify] https://crrev.com/f0179abffe0534ee3487d73436b01c278777993e/third_party/blink/renderer/core/layout/grid_track_sizing_algorithm.cc
[modify] https://crrev.com/f0179abffe0534ee3487d73436b01c278777993e/third_party/blink/renderer/core/layout/grid_track_sizing_algorithm.h
[modify] https://crrev.com/f0179abffe0534ee3487d73436b01c278777993e/third_party/blink/renderer/core/layout/layout_grid.cc
[modify] https://crrev.com/f0179abffe0534ee3487d73436b01c278777993e/third_party/blink/renderer/core/layout/layout_grid.h

This issue should be FIXED now, but I'll wait for some weeks to see the results of the performance bots.  
Status: Fixed (was: Started)
Based on the performance bot report bellow, I think we can mark this bug as FIXED.

https://chromeperf.appspot.com/report?sid=a59eac6929f7bc6f1c795ae6d28fa8b219931447d2a8e8f5484e27fcaa68936b
To verify the regression is actually gone, use 71.0.3561.0 version.
Thank you!

I checked it in "Version 71.0.3561.0 (Official Build) canary (64-bit)", and on my machine (a "MacBook Pro (Retina, 15-inch, Mid 2015)") the issue can be considered reduced but is still there. The perf issue (as described earlier) is still very noticeable. In the latest Chrome stable the issue exists as well. Perhaps the performance of CSS Grid in Chromium can get revisited and improved some more at some point in the future.
The fix has been introduced in  71.0.3561.0 so I don't expect any improvement in latest Chrome stable. 

Additionally, the change fixed indeed a an important performance regression that affected specially to grids with auto-sized tracks. This performance regression is surely solved, as it's shown in the perf bots that evaluate the regression test we currently have.

However, it's perfectly possible that your specific case still works slower than you would like. What's is important for us to know is whether there was a version where the performance was noticeable better than the one provided by the version with my patch (71.0.3561.0). 

If that's the case, please, provide the specific version so we can investigate other potential performance regressions we may have introduced. At least, with the test we have now (not so many) I couldn't detect any other relevant performance regression.
> The fix has been introduced in  71.0.3561.0 so I don't expect any improvement in latest Chrome stable.

I also checked the page in the version you had recommended: Version 71.0.3561.0 (Official Build) canary (64-bit)

> the change fixed indeed a an important performance regression that affected specially to grids with auto-sized
> tracks. This performance regression is surely solved, as it's shown in the perf bots that evaluate the regression
> test we currently have.

I hadn't doubted that :) Thanks for that specific perf regression fix!

I wrote that "the [reported overall] issue can be considered reduced [because of your specific fix] but is still there." The perf issue (as described in the report and in subsequent comments) is still very noticeable.

> What's is important for us to know is whether there was a version where the performance was noticeable better than
> the one provided by the version with my patch (71.0.3561.0).

I don't know. But regardless of earlier versions, I hope that the performance of CSS Grid in Chromium can get revisited and improved some more at some point in the future (regarding the issue & page I had reported here, and also generally). There might be some room for improvement regarding overall CSS Grid perf (whether the respective perf issues are regressions or not).
> I don't know. But regardless of earlier versions, I hope that the performance > of CSS Grid in Chromium can get revisited and improved some more at some 
> point in the future (regarding the issue & page I had reported here, and also
> generally). There might be some room for improvement regarding overall CSS
> Grid perf (whether the respective perf issues are regressions or not).

Yes, I totally agree with you and we already have several ideas to improve performance. 

In any case, figuring out test cases, as much simplified as possible, to detect performance issues will help us to improve and avoid future regressions. 

So, thanks for this bug report.
And thank you!

Sign in to add a comment