1707808 - Use getGridContainerType instead of reading style.gridTemplateRows/Columns

Status: RESOLVED FIXED

(DevTools :: Inspector: Layout, defect, P3)

Description

[Karl Dubost💡 :karlcow]

What were you doing?

1. open https://csgohub.com/
2. open devtools

What happened?

As reported in https://github.com/webcompat/web-bugs/issues/70395

tab hang and devtools takes 2 minutes to pop into existence.
As soon as I open devtools, I get 87s worth of jank with a ton of reflows.

What should have happened?

Open ready to explore the code.

Anything else we should know?

Browser / Version: Firefox Release 87.0 (64-bit)/ Firefox Nightly 89.0a1 (2021-04-15)
Operating System: Ubuntu 20.4 LTS x64

Comment 1

[Julian Descottes [:jdescottes]]

Some more information would be great.
It seems to be especially slow if you open DevTools on the Inspector panel, with the Layout side panel visible.

With any other panel (or with the inspector without the Layout panel visible), DevTools opens rather quickly.

A quick profile indicates that we spend a lot of time collecting grid fragments: https://share.firefox.dev/3u8tv5q

Comment 2

[David Silva]

(In reply to Julian Descottes [:jdescottes] from comment #1)

Some more information would be great.
It seems to be especially slow if you open DevTools on the Inspector panel, with the Layout side panel visible.

With any other panel (or with the inspector without the Layout panel visible), DevTools opens rather quickly.

A quick profile indicates that we spend a lot of time collecting grid fragments: https://share.firefox.dev/3u8tv5q

Hey Julian, what info do you need? Hoping I can be of assistance here, indeed from my profile as well, it does seem that Firefox takes a lot of time on the grid fragments as soon as you open devtools with the Layout side panel open and closing it does make it heaps faster. Also as noted on the webcompat ticket, it hogs a full CPU core at least when this occurs.

Comment 3

[Julian Descottes [:jdescottes]]

(In reply to David Silva from comment #2)

Hey Julian, what info do you need?

Mostly to make sure we are looking at the right issue. From the answers here and on the GH ticket, it looks like this really is about the Grid info in the layout panel.

If you see a performance issue that on this website that doesn't involve the Layout sidepanel, make sure to mention it here.

For now we will focus on those STRs

• open https://csgohub.com/
• open DevTools > Inspector > Layout

As there seems to be an obvious performance issue with this feature on this website.

Thanks!

Comment 4

[David Silva]

(In reply to Julian Descottes [:jdescottes] from comment #3)

(In reply to David Silva from comment #2)

Hey Julian, what info do you need?

Mostly to make sure we are looking at the right issue. From the answers here and on the GH ticket, it looks like this really is about the Grid info in the layout panel.

If you see a performance issue that on this website that doesn't involve the Layout sidepanel, make sure to mention it here.

For now we will focus on those STRs

• open https://csgohub.com/
• open DevTools > Inspector > Layout

As there seems to be an obvious performance issue with this feature on this website.

Thanks!

This seems to be spot on to me!

Thanks,
David

Comment 5

[Julian Descottes [:jdescottes]]

ni? to investigate and see how many calls are performed to getGridFragments (which is clearly highlighted by the profile). This should help to see if there is a performance issue with getGridFragments or with the DevTools usage of this API

Comment 6

[Julian Descottes [:jdescottes]]

On this page, we perform 350+ calls to getGridFragments:

• 50% of the calls are instant (0 or 1ms).
• the other 50% take 100ms on average for each call (on my machine)

If I isolate an element on which getGridFragments was slow, calling getGridFragments a second time on it is instant.
Looking at the profile, it seems that calling getGridFragments triggers a lot of reflows (https://share.firefox.dev/3ecG764)

Trying to run the following script from the browser console (targeting the content process for the webpage)

{
  const els = tabs[0].docShell.document.querySelectorAll("*");
  const win = els[0].ownerGlobal;
  const start = win.performance.now();
  for (const el of els) {
    const gridFragments = el.getGridFragments();
  }
  const duration = win.performance.now() - start;
  win.console.log(duration)
}

Takes around 16 seconds on my machine, similar to the hang I get from the inspector.

Daniel: is there anything on this page which could explain the poor performance of some of the getGridFragments calls?

Comment 7

[Daniel Holbert [:dholbert]]

getGridFragments is backed by some data that we populate/update during reflow, and we only set that data lazily (i.e. only after getGridFragments has been called once for the frame in question). The first time getGridFragments is called, it marks the frame as dirty and triggers an immediate synchronous reflow, to fill out the data.

If we do this for 350+ grid elements back-to-back, it will take some appreciable amount of time. This is partly because our grid reflow has some known inefficiencies, tracked in e.g. bug 1591366 (this is why some of the calls are particularly slow, I think). But it's also because the way we're calling/implementing this devtools API feels a bit silly in this particular case with 350+ grid elements.

Does DevTools really intend to call this API synchronously for all of the grid elements in the page, when the layout panel is shown? That's a reasonable thing to do, but it's not really how the API is designed to be called right now -- e.g. right now, in the API's implementation, each grid frame individually tracks whether the API has ever been called on it (and hence whether to generate the lazy data), and each grid frame triggers its own lazy reflow in the first call to the API in order to generate its own data. This approach is quite inefficient if we're going to call this API in bulk on all grids at once.

Julian, if you could confirm that this is indeed the way that devtools calls this API, then the simplest fix here would be to redesign the implementation to use a single state bit for the document which triggers the lazy computation (for grid as well as flex). Then, we're likely to coalesce a lot more of the data-generation into happening in the first few calls to the API. (i.e. The first grid frame's synchronous reflow is likely to reflow some of the other grids and generate some of their data so that it'll be ready when we call the API on them.)

Comment 8

[Daniel Holbert [:dholbert]]

Basically: if getGridFragments were only ever called on the currently-selected node in the DevTools inspector (to lazily populate the layout panel for that node), then our current implementation approach would make sense.

But if we intentionally call getGridFragments in bulk for all of the grids back-to-back, then we should absolutely change our approach to take advantage of that and to coalesce some of the redundant work.

Comment 9

[Brad Werth [:bradwerth]]

If devtools team wants to propose a new API for this (with a new calling pattern), I'm happy to do the implementation work. generateComputedLayoutInfo on the Document? Something like that?

Comment 10

[Julian Descottes [:jdescottes]]

Thanks for the feedback!

A quick look at our use case here:

The Grid inspector calls getAllGrids on all the Layout fronts (we have one layout per "target", so more or less per process).
It can be called:

• on every reflow (500ms throttling) (searchfox)
• on navigation (searchfox)
• when the panel becomes visible (searchfox)
• when the panel is initialized (searchox)

Two things:

• the onReflow codepath is concerning here. It calls getAllGrids once on its own, but then can perform up to 2 additional calls to updateGridPanel (searchfox) which will also call getAllGrids via updateGridPanel
• we also have an issue with the init code, which can lead to call getAllGrids twice, once explicitly, and once because of the "navigation" handler

The first issue, while it seems bad, will normally only have one slow call to getAllGrids. The lazy data gets generated by the first call, so the 2 additional ones are fast.
I can easily fix the second issue, but similarly, we only pay the cost of initializing the lazy data once.

But it shows that we are really carelessly calling the getAllGrids API. Now on the server/actor side getAllGrids will call the following snippets on all documents

    const gridElements = node.getElementsWithGrid();
    let gridActors = gridElements.map(n => new GridActor(this, n));

Which means we create a GridActor for all grid elements, and when we return it to the client, we extract the following info

    const gridFragments = this.containerEl.getGridFragments();
    const { gridTemplateColumns, gridTemplateRows } = 
      CssLogic.getComputedStyle(this.containerEl);

(from https://searchfox.org/mozilla-central/rev/6371054f6260a5f8844846439297547f7cfeeedd/devtools/server/actors/layout.js#331)

We find getGridFragments here.

On the client, gridFragments are used in 2 spots which could retrieve the data on-demand:

• grid-outline (searchfox). We only display one of those outlines, for the selected node
• rule view autocomplete (searchfox) to propose the line/area names in the autocomplete

And in one spot which can't easily retrieve the data on-demand:

• grid-inspector's haveCurrentFragmentsChanged (searchfox). Used on reflow to check if we should update the UI even though we have the same list of grid elements.

And then we have gridTemplateColumns and gridTemplateRows. I mention them because retrieving them seem to have the same performance impact as getGridFragments. If I remove the call to getGridFragments and return an empty array instead, the performance is the same because of those properties. On DevTools side they are only used to compute the isSubgrid property (searchfox). This property is used when building the list of grid elements in the grid inspector. We don't include subgrids in the list because they are rendered by their parent.

So we have at least 2 usage where we "need" to get the information for all the grid elements:

• gridFragments to know if something changed on reflow
• gridTemplateColumns and gridTemplateRows to exclude subgrids

For the first item, the reason this was added is only to update the grid outline (see https://bugzilla.mozilla.org/show_bug.cgi?id=1374587#c6). So even though we compare the grid fragments for all grid elements, I think it would be fine to get the information only for the currently displayed grid outline.

So I think we could update DevTools to stop fetching gridFragments by default, and only fetch it on demand.

However for the "isSubgrid" check that's something that is really used to build the list of grid elements. So we would need another way to compute this data.

Would it be possible to either exclude subgrids from getGridElements? Or to compute isSubgrid on the platform side for a grid element?

Comment 11

[Julian Descottes [:jdescottes]]

Sorry for the long comment above, but basically I'm confident DevTools can stop using getGridFragments when retrieving the list of all GridActors and instead fetch it on demand.

However we'd still need to find a way to stop fetching gridTemplateColumns and gridTemplateRows which we use to know if an element is a subgrid or not (see previous comment for more details).

Would platform be able to help for the "subgrid" part?

Comment 12

[Daniel Holbert [:dholbert]]

Yeah, we could absolutely include isSubgrid as a piece of information on the getGridFragments()-returned struct.

(Note that it's possible for an element to be a subgrid in only one axis and not-a-subgrid in the other axis; so really, you want to ask "isColSubgrid" and "isRowSubgrid", I think, to determine whether to fetch gridTemplateColumns / gridTemplateRows.)

We could just add these as bits of information on https://searchfox.org/mozilla-central/source/dom/grid/Grid.h (which is the thing returned by this API). I filed bug 1710502 to do so.

Comment 14

[Julian Descottes [:jdescottes]]

Sadly the original page doesn't seem to heavily use grid anymore and I can't reproduce in an isolated test case for now.

Comment 15

[Daniel Holbert [:dholbert]]

Good news, we've got a new page that seems to trigger the same issue in bug 1987193.

Comment 16

[Julian Descottes [:jdescottes]]

Perfect! I'll try to update devtools to use your patches from bug 1710502 then.

Comment 17

[Julian Descottes [:jdescottes]]

Attachment: Bug 1707808 - [devtools] Improve performance of layout panel on pages with many grid elements

Comment 18

[Julian Descottes [:jdescottes]]

Attachment: Bug 1707808 - [devtools] Use getGridContainerType instead of reading style.gridTemplateRows/Columns

Comment 19

[Phabricator Automation]

Comment on attachment 9512328 [details]
Bug 1707808 - [devtools] Improve performance of layout panel on pages with many grid elements

Revision D264256 was moved to bug 1988868. Setting attachment 9512328 [details] to obsolete.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/03fb2a8c40c7
https://hg.mozilla.org/integration/autoland/rev/771f09ca8058
[devtools] Use getGridContainerType instead of reading style.gridTemplateRows/Columns r=devtools-reviewers,nchevobbe

Comment 21

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/771f09ca8058