Closed Bug 1682686 Opened 4 years ago Closed 3 years ago

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

Categories

(Core :: Layout: Grid, defect)

defect

Tracking

()

RESOLVED FIXED
99 Branch
Tracking Status
firefox99 --- fixed

People

(Reporter: daniel, Assigned: sefeng)

References

()

Details

(Keywords: perf, testcase)

Attachments

(4 files)

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

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

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

Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: perf, testcase
Component: Untriaged → Layout: Grid
Product: Firefox → Core

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.

Severity: -- → S3
Depends on: 1524056
OS: Unspecified → All
Hardware: Unspecified → All

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.

Depends on: 1591366
No longer depends on: 1524056

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?

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

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.

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]

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...

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).

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.

(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.)

Version: Firefox 83 → Trunk

I'll take a look

Flags: needinfo?(sefeng)

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.

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

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.)

Assignee: nobody → sefeng
Status: NEW → ASSIGNED

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.)

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

Attachment #9264468 - Attachment description: Bug 1682686 - Fix updating cached baxis measurement for grid isn't updating it r=dholbert → Bug 1682686 - Fix grid-item measurement cache to actually update the cache when the old value is no longer suitable
Pushed by sefeng@mozilla.com: 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
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 99 Branch
Regressions: 1756303
Flags: needinfo?(sefeng)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: