1682686 - Reflow time terribly slow when nest grid depth of DOM tree > 8

Status: RESOLVED FIXED

(Core :: Layout: Grid, defect)

Description

[daniel]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:83.0) Gecko/20100101 Firefox/83.0

Steps to reproduce:

Just did some tests on Firefox 83 and DOM reflow time grows exponentially with the depth of DOM tree. With grid elements of depth 10 single reflow takes seconds on my Ryzen laptop while on chromium is not noticeable at all.

Actual results:

Couple of seconds to render single added DOM element at the bottom of tree.

Expected results:

Adding elements at the bottom should be not noticeable as it is in Chromium

Comment 1

[daniel]

Did a performance profile, here is a result: https://share.firefox.dev/3npzcZA

Comment 2

[daniel]

Did an example test case: https://jsfiddle.net/fuh7c894/

Comment 3

[Daniel Holbert [:dholbert]]

Thanks for the bug report! We do indeed have some known performance issues for nested CSS Grid. bug 1524056 comment 8 has some ideas for where to start here. Marking as a dependency for now.

Comment 4

[Daniel Holbert [:dholbert]]

The jsfiddle testcase is useful, too; thanks for adding that. For anyone looking at this in the future, the steps to reproduce with that testcase are:

• click the text, which adds another copy of the text;
• click the last copy of the text that you see
• repeat.

After 7 or 8 iterations, I do see noticeable delays in the click response time.

Comment 5

[Sean Feng [:sefeng]]

I just tried the jsfiddle testcase since we now have 1591366 fixed. The performance is now much-improved withlayout.css.grid-item-baxis-measurement.enabled turned on (enabled by default on Nightly).

Can we close this bug?

Comment 6

[daniel]

Just tested it on Firefox 92 with layout.css.grid-item-baxis-measurement.enabled and it still performs bad.

Comment 7

[Sören Hentzschel]

Just tested it on Firefox 92 with layout.css.grid-item-baxis-measurement.enabled and it still performs bad.

Bug 1591366 was fixed in Firefox 94 Nightly, not Firefox 92.

Comment 8

[Daniel Holbert [:dholbert]]

This looks good at first, in Nightly, using the jsfiddle https://jsfiddle.net/fuh7c894/ , but it gets terrible again when a scrollbar has to appear (i.e. when I've clicked the last line enough times such that we have however many lines are needed to cause overflow).

So, that situation (the presence of scrollbars) seems to defeat our optimization here -- I would guess because we reflow with two sizes -- with & without scrollbars -- and maybe that subtle change in conditions/viewport-width causes us to purge the cached value. [EDIT: per my later down comments, I think this just has to do with the nesting level, and not really with the scrollbars]

Comment 9

[Daniel Holbert [:dholbert]]

Actually, I'm not sure whether the scrollbars matter; this is still kinda-bad without scrollbars, too.

I'm going to attach a relatively-static testcase based on the jsfiddle, with some notes...

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: testcase 1 (13 nested grids)

Comment 11

[Daniel Holbert [:dholbert]]

Two profiles of testcase 1:
Current Nightly, with layout.css.grid-item-baxis-measurement.enabled set to true: https://share.firefox.dev/3zACP4i (3.3sec jank)
Current Nightly, with layout.css.grid-item-baxis-measurement.enabled set to false: https://share.firefox.dev/3ogqArF (6.6sec jank)

So it looks like we've roughly cut the reflow time in half, but it's still ~seconds long (and still seems to perhaps have some sort of exponential layout cost, in terms of the nesting-depth).

Comment 12

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (slightly different example with nested grids)

Here's the original testcase from bug 1591366, which (per bug 1591366 comment 22) I think we should now just track as part of this bug.

As with Testcase 1 here, it seems like this testcase's load time is approximately cut in half by the layout.css.grid-item-baxis-measurement.enabled pref; but it still hangs for multiple seconds before rendering.

Comment 13

[Daniel Holbert [:dholbert]]

(I'm also clearing the version-specific affected flags since those aren't particularly useful here. This affects all Firefox versions that support CSS Grid, which goes back quite a while. And versions that don't support CSS grid are only unaffected in the sense that they don't render the testcases properly.)

Comment 14

[Sean Feng [:sefeng]]

I'll take a look

Comment 15

[Julius]

Hello.
We've also stumbled upon this issue with our spa app in production.
Our grid-based component is a core one, so dropping grid is not an option for us.

After testing for a while in different UA we've figured out this list of issues regarding performance:

1. (ff) expo performance degradation: nested display: grid; nodes, rendered recursively.
2. (ff) expo performance degradation: nested diplay: grid + with a single row defined with grid-template-rows nodes, rendered recursively.

After investigation these issues, we've managed to come with a hotfix which solves them all.
1, 2 -- the solution is to apply min-height: 0

This solution works on both test cases, which Daniel Holbert attached. Try setting min-height: 0 on grid in both demos. Lags fade away.
Solution tested even in the latest release 96.0.3 (64-bit)

• testcase 1 (13 nested grids)
• testcase 2 (slightly different example with nested grids).

I hope this temporary solution helps if anyone else is facing these issues on production.

Comment 16

[Julius]

Couldn't find the "add attachment" button, so i'll just leave a snapshot of a testcase 1 fix applied, just to be clear.
testcase 1

Comment 17

[Daniel Holbert [:dholbert]]

Thanks for letting us know. I'm glad to hear that min-height:0 solves the issue for you (and it makes sense, since typically the recursive blowup here is from having to lay the content out twice at each level; once to resolve the default min-height:auto value and once for the actual layout. If you use min-height:0 instead of the default auto, then that avoids the first half of the work at each level, which prevents the exponential blowup.)

(RE the "add attachment" -- there is normally an "Attach new file" button -- but for brand-new Bugzilla accounts, I think it's hidden in bugs-that-you-did-not-personally-file, to reduce abuse/mistakes. I've now added the relevant permissions to your Bugzilla account, though, so that it should show up for you now.)

Comment 18

[Sean Feng [:sefeng]]

Attachment: Bug 1682686 - Fix grid-item measurement cache to actually update the cache when the old value is no longer suitable

Comment 19

[Daniel Holbert [:dholbert]]

Coincidentally, this bug superficially looked like it went away in recent Linux Nightlies, but that was just due to bug 1147847 (because this bug happens to depend on us mistakenly getting our cache "stuck" with a cached value from our initial time through reflow with the initial scrollbar present/absent choice). If we use overlay scrollbars, then we skip the pass of reflowing with+without the vertical scrollbar and we mercifully don't end up triggering the bug.

I'll attach a modified version of testcase 1 that's still slow in today's Nightly, even with overlay scrollbars, which we can turn into a crashtest here. (I think it's fixed by Sean's patch that he attached earlier today.)

Comment 20

[Daniel Holbert [:dholbert]]

Attachment: testcase 1a (like testcase 1 but with an adjustment to the body `width` in the onload handler)

This testcase should exercise the bug regardless of whether the user has overlay scrollbars enabled.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2cdd91c2710f
Fix grid-item measurement cache to actually update the cache when the old value is no longer suitable r=dholbert

Comment 22

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/2cdd91c2710f