1710502 - Extend grid devtools struct to indicate whether the grid is a row/column subgrid

Status: RESOLVED FIXED

(Core :: Layout: Grid, enhancement)

Description

[Daniel Holbert [:dholbert]]

As discussed in bug 1707808 comment 11, our grid devtools need to know whether a particular grid is a subgrid.

We can easily return that via the precomputed struct that we return via our devtools getGridFragments API.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: WIP: Bug 1710502: Extend grid devtools API to indicate whether the grid is a row/column subgrid.

Comment 2

[Daniel Holbert [:dholbert]]

Patch posted (fairly trivial). I want to add some tests, but otherwise I think this is probably-done.

Comment 3

[Julian Descottes [:jdescottes]]

Thanks for working on this Daniel! But I'm not sure it will help if we still need to call getGridFragments. I'll try to rephrase what I said in https://bugzilla.mozilla.org/show_bug.cgi?id=1707808#c11.

We can modify devtools stop calling getGridFragments for all grid elements upfront. However we still need to know (upfront, for all grid elements) which ones are subgrids or not. Because we will hide subgrid elements from the list of displayed grids.

Currently we compute "isSubgrid" by fetching gridTemplateColumns and gridTemplateRows from the computed style. From my tests, this has the same performance cost as getGridFragments. When I removed calls to getGridFragments, the performance was still identical until I also removed the computation of isSubgrid.

We would need a separate API, which hopefully doesn't have the same performance impact as getGridFragments or as fetching gridTemplateColumns and gridTemplateRows from the computed style.

Comment 4

[Daniel Holbert [:dholbert]]

Thanks for clarifying that, and sorry for misunderstanding.

It's surprising that your computed-style accesses are slow (or rather, it's strange that they're all slow). Are you modifying the page's DOM or style at all between these accesses? If not, I would think the accesses should be free after the first one. (The first one should flush pending restyles, and subsequent accesses shouldn't have styles to flush.)

The only reason getGridFragments is slow (the first time it's called for a given element) is that it explicitly forces its own dedicated reflow, for the purpose of gathering additional data. But the general computed-style accessors don't have any such per-invocation cost.

Comment 5

[Daniel Holbert [:dholbert]]

If it's not too much trouble, would you mind capturing a profile of your "just checking gridTemplateColumns/gridTemplateRows" approach?

Intuitively, I would expect the first call to be maybe-a-little-expensive, and then for subsequent calls to be cheap (until the page's DOM/styles are modified). If that's not what we're seeing, then I think something's fishy.

Comment 6

[Julian Descottes [:jdescottes]]

Sure!

Using the STR from https://bugzilla.mozilla.org/show_bug.cgi?id=1707808, here's a profile with getGridFragments commented out: https://share.firefox.dev/2R4m2GC

The patch is:

# HG changeset patch
# User Julian Descottes <[e-mail]>
# Date 1620750264 -7200
#      Tue May 11 18:24:24 2021 +0200
# Node ID 5b6da1e8a87328cbde79880ae3976c9f937e12b8
# Parent  500e969f0c545e040c8e14ce8a18c5f3e5d000c9
Bug 1707808 - (investigation) stop fetching grid fragments

diff --git a/devtools/server/actors/layout.js b/devtools/server/actors/layout.js
--- a/devtools/server/actors/layout.js
+++ b/devtools/server/actors/layout.js
@@ -314,17 +314,17 @@ const GridActor = ActorClassWithSpec(gri
     this.containerEl = null;
     this.gridFragments = null;
     this.walker = null;
   },
 
   form() {
     // Seralize the grid fragment data into JSON so protocol.js knows how to write
     // and read the data.
-    const gridFragments = this.containerEl.getGridFragments();
+    const gridFragments = [];
     this.gridFragments = getStringifiableFragments(gridFragments);
 
     // Record writing mode and text direction for use by the grid outline.
     const {
       direction,
       gridTemplateColumns,
       gridTemplateRows,
       writingMode,

We can see that getGridFragments no longer shows up in the profile, instead we see get CSS2Properties.gridTemplateColumns

If in addition to this I also comment out gridTemplateColumns and gridTemplateRows, the performance problem goes away.
If needed, here's an additional profile after commenting them out https://share.firefox.dev/3f82DfP

Comment 7

[Julian Descottes [:jdescottes]]

Are you modifying the page's DOM or style at all between these accesses?

I don't think we modify the DOM in any way between the accesses. We are just calling them in a loop as we are going to return our Grid actors to the client. I measured the time taken by each call, and similarly to what I wrote for getGridFragments, 50% of the calls take around 100ms while the rest is instant.

Comment 8

[Daniel Holbert [:dholbert]]

Aha, thank you! That profile helped me see that our computed-style getters for grid-template-rows/columns are unusual in that they invoke the same underlying devtools function, under the hood (GetGridFrameWithComputedInfo):

already_AddRefed<CSSValue> nsComputedDOMStyle::DoGetGridTemplateColumns() {
  nsGridContainerFrame* gridFrame =
      nsGridContainerFrame::GetGridFrameWithComputedInfo(mInnerFrame);

https://searchfox.org/mozilla-central/rev/cca1566127a2fcc013e9c09f9d90ed70df2250a4/layout/style/nsComputedDOMStyle.cpp#1714

And GetGridFrameWithComputedInfo is the thing that triggers a synchronous reflow. So, querying the computed-style of these properties will indeed trigger reflow and will be as expensive as using the main devtools API.

So: if the only thing we need up-front is to know whether all of the grids are subgrids or not (and we don't need to make any sort of bulk queries to getGridFragments / grid-template-rows/columns), then it would probably make sense to add some sort of new lightweight API which flushes style but doesn't require reflow.

Comment 9

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1710502 - Add a helper for devtools to tell if something is a row/col subgrid or not. r=dholbert!,#devtools-reviewers!

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/2fd65c211146
https://hg.mozilla.org/integration/autoland/rev/edd205018096
Add a helper for devtools to tell if something is a row/col subgrid or not. r=dholbert,devtools-reviewers

Comment 11

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/edd205018096