Issue metadata
Sign in to add a comment
|
Resize performance regression
Reported by
t...@tobireif.com,
Aug 11
|
||||||||||||||||||||||
Issue descriptionUserAgent: 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.
,
Aug 13
Confirmed in Chrome 68 Stable and 70 Canary. (It is working better in Safari on macOS.)
,
Aug 13
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...!!
,
Aug 13
This is not canvas related, reassigning.
,
Aug 14
"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).
,
Aug 17
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.
,
Aug 17
"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.
,
Aug 17
I guess https://bugs.chromium.org/u/1119298406/ used MacOS to reach this conclusion: Comment #2: "Confirmed in Chrome 68 Stable and 70 Canary."
,
Aug 17
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.
,
Aug 17
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.
,
Aug 17
,
Aug 17
>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
,
Aug 17
Thanks for investigating!
,
Aug 17
"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!
,
Aug 17
"meant to stated" → "meant to state"
,
Aug 28
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.
,
Aug 29
That does sound promising! Thanks for working on this important issue.
,
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
,
Sep 24
This issue should be FIXED now, but I'll wait for some weeks to see the results of the performance bots.
,
Sep 25
Based on the performance bot report bellow, I think we can mark this bug as FIXED. https://chromeperf.appspot.com/report?sid=a59eac6929f7bc6f1c795ae6d28fa8b219931447d2a8e8f5484e27fcaa68936b
,
Sep 25
To verify the regression is actually gone, use 71.0.3561.0 version.
,
Sep 25
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.
,
Sep 25
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.
,
Sep 26
> 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).
,
Sep 26
> 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.
,
Sep 27
And thank you! |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by swarnasree.mukkala@chromium.org
, Aug 12