1472938 - Image assets in the new meatball menu flicker when the menu is first displayed

Status: RESOLVED FIXED

(DevTools :: General, defect, P1)

Description

[Brian Birtles (:birtles)]

We need to somehow preload these assets. I suspect they are currently not pre-loaded because they are referenced from a background-image property on a pseudo-element.

Comment 1

[Brian Birtles (:birtles)]

Emilio mentioned there is a chrome-only API similar to getComputedStyle for getting back image URLs which is easier to use than parsing the "url(...)" returned from getComputedStyle.

The idea is, in MenuItem, to fetch the computed style of the pseudo (perhaps only for items with the "iconic" class), get the image URL of the background-image, and pass it to new Image(). We should probably do that on componentDidUpdate and store the URL in a class member (_not_ state) to avoid redundant requests.

Comment 2

[Brian Birtles (:birtles)]

Attachment: WIP patch

Comment 3

[Brian Birtles (:birtles)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=62d6db7653de888f2888965ca060b71f1b5e7997

Comment 4

[Brian Birtles (:birtles)]

I ran a screen recorder on Windows before and after this change and I can observe a frame or two without the icons before applying this change, but after this change the icons appear the instant the menu does.

Comment 5

[Brian Birtles (:birtles)]

Attachment: Bug 1472938 - Convert MenuItem to a class;

This is so that we can hook into the componentDidMount method.

Review commit: https://reviewboard.mozilla.org/r/258722/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/258722/

Comment 6

[Brian Birtles (:birtles)]

Attachment: Bug 1472938 - Preload images used on MenuItem labels;

This is to avoid rendering for a frame or two without the menu item icons being
visible which causes flicker, particularly on lower-end machines.

Review commit: https://reviewboard.mozilla.org/r/258724/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/258724/

Comment 7

[Brian Birtles (:birtles)]

I decided to only hook into componentDidMount for now since we don't typically change the ID / icon of these items after creating them.

I'm not sure if it's worth the extra logic to support subsequent updates since it would involve storing the URLs in local state and doing a comparison to ensure we don't do redundant requests. That seems like a fair bit of work to support a case that doesn't seem common (and if it does occur, will simply produce some minor flicker). Maybe it is worthwhile though.

Comment 8

[Julian Descottes [:jdescottes]]

Comment on attachment 8994101 [details]
Bug 1472938 - Convert MenuItem to a class;

https://reviewboard.mozilla.org/r/258722/#review265608

Comment 9

[Brian Birtles (:birtles)]

Comment on attachment 8994102 [details]
Bug 1472938 - Preload images used on MenuItem labels;

https://reviewboard.mozilla.org/r/258724/#review265610

::: devtools/client/shared/components/menu/MenuItem.js:61
(Diff revision 1)
> +  componentDidMount() {
> +    if (!this.labelRef.current) {
> +      return;
> +    }
> +
> +    // Pre-fetch any backgrounds specified for the button.

This should probably be s/button/label/, actually.

Comment 10

[Julian Descottes [:jdescottes]]

Comment on attachment 8994102 [details]
Bug 1472938 - Preload images used on MenuItem labels;

https://reviewboard.mozilla.org/r/258724/#review265612

::: devtools/client/shared/components/menu/MenuItem.js:56
(Diff revision 1)
> +  componentDidMount() {
> +    if (!this.labelRef.current) {
> +      return;
> +    }
> +
> +    // Pre-fetch any backgrounds specified for the button.
> +    const win = this.labelRef.current.ownerDocument.defaultView;
> +    const backgrounds = win
> +      .getComputedStyle(this.labelRef.current, ":before")
> +      .getCSSImageURLs("background-image");
> +    for (const background of backgrounds) {
> +      const image = new Image();
> +      image.src = background;
> +    }
> +  }

Should we be worried about the performance impact of querying elements style, creating images and loading resources here? I believe those components are mounted immediately when the toolbox opens.

Would be interested in seeing a talos run for this change (./mach try -b o -p linux64 -u none -t damp-e10s --rebuild-talos 6
). 

We could also programmatically do this ourselves before showing the tooltip (with a "preload" prop on MenuButton for instance?)

Comment 11

[Brian Birtles (:birtles)]

> Should we be worried about the performance impact of querying elements style, creating images and loading resources here? I believe those components are mounted immediately when the toolbox opens.

Yeah, it's worth checking.

I don't expect any significant overhead from creating images (doesn't touch the DOM, just creates an orphaned element) and loading resources (happens async). The main cost that might occur is if something else running after us is going to invalidate style without triggering a style flush first.

If that proves to be the case, I'd rather stick this in a idle callback than try to do this at the point when the tooltip is about to be shown (since doing it then will lead to lag).

Comment 12

[Brian Birtles (:birtles)]

(In reply to Julian Descottes [:jdescottes][:julian] from comment #10)
> Would be interested in seeing a talos run for this change (./mach try -b o
> -p linux64 -u none -t damp-e10s --rebuild-talos 6
> ). 

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=8ade68fd259bff13c62267c30786dc3ef34fdd5b&newProject=try&newRevision=60915e5246afd8568fd0a7e8e564b565fedf19ef&framework=1

Seems ok to me.

Comment 13

[Julian Descottes [:jdescottes]]

I see a slight but consistent slowdown of open.DAMP tests: 

https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=8ade68fd259bff13c62267c30786dc3ef34fdd5b&newProject=try&newRevision=60915e5246afd8568fd0a7e8e564b565fedf19ef&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&filter=.open.DAMP&framework=1

The delta is small but some of them are with medium confidence. Whether this should be addressed or not I'm not sure. At least we have the data ready in case someone wants to act on this.

> If that proves to be the case, I'd rather stick this in a idle callback than 
> try to do this at the point when the tooltip is about to be shown (since doing
> it then will lead to lag).

I assumed we could plug the preload on mousedown, so that images have a good chance to be preloaded when the tooltip opens on click afterwards.

Comment 14

[Julian Descottes [:jdescottes]]

Comment on attachment 8994102 [details]
Bug 1472938 - Preload images used on MenuItem labels;

https://reviewboard.mozilla.org/r/258724/#review265954

Comment 15

[Brian Birtles (:birtles)]

Pushed another patch which defers to loading to an idle callback:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d55f4dcaac099991657b553a78f3e037dd6dd8d

Talos numbers comparing to baseline should appear here when ready:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=8ade68fd259bff13c62267c30786dc3ef34fdd5b&newProject=try&newRevision=4d55f4dcaac099991657b553a78f3e037dd6dd8d&framework=1

Showing just the effect of this extra patch:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=60915e5246afd8568fd0a7e8e564b565fedf19ef&newProject=try&newRevision=4d55f4dcaac099991657b553a78f3e037dd6dd8d&framework=1

Comment 16

[Brian Birtles (:birtles)]

Also, I was seeing some odd failures of the sort:

  devtools/client/inspector/test/browser_inspector_breadcrumbs_visibility.js | div#aSixthOneToExceedTheTruncationLimit crumb is visible - Got false, expected true

I don't know how that could be related to this patch. Maybe it's a problem with the underlying revision.

This try job should tell:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ade68fd259bff13c62267c30786dc3ef34fdd5b

Comment 17

[Brian Birtles (:birtles)]

(In reply to Brian Birtles (:birtles) from comment #15)
> Pushed another patch which defers to loading to an idle callback:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=4d55f4dcaac099991657b553a78f3e037dd6dd8d
> 
> Talos numbers comparing to baseline should appear here when ready:
> https://treeherder.mozilla.org/perf.html#/
> compare?originalProject=try&originalRevision=8ade68fd259bff13c62267c30786dc3e
> f34fdd5b&newProject=try&newRevision=4d55f4dcaac099991657b553a78f3e037dd6dd8d&
> framework=1
> 
> Showing just the effect of this extra patch:
> https://treeherder.mozilla.org/perf.html#/
> compare?originalProject=try&originalRevision=60915e5246afd8568fd0a7e8e564b565
> fedf19ef&newProject=try&newRevision=4d55f4dcaac099991657b553a78f3e037dd6dd8d&
> framework=1

Looks like it doesn't help.

Comment 18

[Brian Birtles (:birtles)]

I've looked into the perf impact here and I'm not sure it should block this. I've done a few extra runs with different configurations but the results I get are quite inconsistent.

The numbers also don't seem significant to me. For example, one of the most significant regressions I could see was in cold.inspector.open.DAMP (0.79%)[1] but in a subsequent run with *only* the first patch which simply converts the component to a class instead of a pure function I see a 0.89% regression[2] on that same measure.

We're using a PureComponent here which should give us an implementation of shouldComponentUpdate based on a shallow equal that is equivalent to using a pure function so I don't think we're doing anything wrong here.

One curiosity, however, is that devtools/client/inspector/test/browser_inspector_breadcrumbs_visibility.js fails on opt builds on try without the third patch that does the deferred load. I started digging into it but haven't found anything obvious yet. Let me submit that patch for review while I get an opt build going locally.

[1] https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=8ade68fd259bff13c62267c30786dc3ef34fdd5b&newProject=try&newRevision=60915e5246afd8568fd0a7e8e564b565fedf19ef&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&filter=.open.DAMP&framework=1

[2] https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=331bcc9d18aa1e2bab4092ef7f00adbcba023d01&newProject=try&newRevision=f9498a29a47ddf1aeb3a5bc19b8ecf3c3700e547&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&filter=.open.DAMP&framework=1

Comment 19

[Brian Birtles (:birtles)]

Oh, and on a further note, I noticed that on my slower computer at home, that the icons in the chevron menu for the React and Redux tabs take several _seconds_ to appear (at least five seconds). I am hoping this bug fixes that.

Comment 20

[Brian Birtles (:birtles)]

Attachment: Bug 1472938 - Defer image pre-loading until idle;

This should prevent introducing further delay when opening DevTools.

Review commit: https://reviewboard.mozilla.org/r/259256/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/259256/

Comment 21

[Brian Birtles (:birtles)]

Comment on attachment 8994101 [details]
Bug 1472938 - Convert MenuItem to a class;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/258722/diff/1-2/

Comment 22

[Brian Birtles (:birtles)]

Comment on attachment 8994102 [details]
Bug 1472938 - Preload images used on MenuItem labels;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/258724/diff/1-2/

Comment 23

[Brian Birtles (:birtles)]

I dug into the failure in browser_inspector_breadcrumbs_visibility.js a little. I've spent about an hour debugging it and decided it's not worth spending more time on it since part 3 fixes the test failure anyway.

Here's what I learned, however.

* The test failure is caused by the additional style flush triggered in componentDidMount (which part 3 defers until later).
* When the toolbox is opened, there is a smooth scroll performed by the breadcrumb component.
  Although the test sets:

    inspector.breadcrumbs.arrowScrollBox.scrollBehavior = "instant";

  That happens too late and the smooth scroll is already in motion by the time that gets set.

* The style flushes triggered by MenuItem happen while this smooth scroll is in motion.
* The test clicks the start button which causes an instant scroll of the breadcrumb widget and then checks the bounding box of a particular crumb.

  _Without_ the style flush we see a change of the bbox of the crumb from the scroll such as the following:

    BBox before: {"x":-23.5,"y":30.199996948242188,"width":299,"height":24,"top":30.199996948242188,"right":275.5,"bottom":54.19999694824219,"left":-23.5}
    BBox after: {"x":22,"y":30.199996948242188,"width":299,"height":24,"top":30.199996948242188,"right":321,"bottom":54.19999694824219,"left":22}

  _With_ the style flush we see a change of the bbox of the crumb from the scroll such as the following:

    BBox before: {"x":-288,"y":247,"width":299,"height":24,"top":247,"right":11,"bottom":271,"left":-288}
    BBox after: {"x":-196,"y":247,"width":299,"height":24,"top":247,"right":103,"bottom":271,"left":-196}

I believe there's some interaction between the initial smooth scroll (presumably APZ) and the style flush but I haven't dug into exactly what it is.

I'd actually like to dig into this further but I don't think it's worth the time at this point.

Comment 24

[Julian Descottes [:jdescottes]]

Comment on attachment 8994737 [details]
Bug 1472938 - Defer image pre-loading until idle;

https://reviewboard.mozilla.org/r/259256/#review266250

LGTM. Is the breadcrumbs test no longer failing with this changeset added?

Comment 25

[Julien Wajsberg [:julienw]]

(In reply to Brian Birtles (:birtles) from comment #18)
> We're using a PureComponent here which should give us an implementation of
> shouldComponentUpdate based on a shallow equal that is equivalent to using a
> pure function so I don't think we're doing anything wrong here.

Hey, chiming in here, just to say that in React a pure function (actually they call this a Stateless Component) doesn't have the behavior you describe. So using a PureComponent is actually a different behavior than a pure function and may have different performance characteristics. Usually this is better, but sometimes you don't need it. (In this case I think this is a good thing, if it's properly used -- see below).

While I was looking at it I also gave a look at how MenuItem is used in [1]. I see that the onClick function is sometimes given as an arrow function, which means a new instance is created at each render, and therefore for these cases `shouldComponentUpdate` will always return true. You should fix these cases.
I also see that the `key` prop isn't used in [1], but you really should because this helps React's tree reconciliation algorithm.

[1] https://searchfox.org/mozilla-central/rev/bdfd20ef30d521b57d5b6feeda71325e8b4cad66/devtools/client/framework/components/MeatballMenu.js#92

Comment 26

[Brian Birtles (:birtles)]

(In reply to Julian Descottes [:jdescottes][:julian] from comment #24)
> Comment on attachment 8994737 [details]
> Bug 1472938 - Defer image pre-loading until idle;
> 
> https://reviewboard.mozilla.org/r/259256/#review266250
> 
> LGTM. Is the breadcrumbs test no longer failing with this changeset added?

Yes.

(In reply to Julien Wajsberg [:julienw] from comment #25)
> (In reply to Brian Birtles (:birtles) from comment #18)
> > We're using a PureComponent here which should give us an implementation of
> > shouldComponentUpdate based on a shallow equal that is equivalent to using a
> > pure function so I don't think we're doing anything wrong here.
> 
> Hey, chiming in here, just to say that in React a pure function (actually
> they call this a Stateless Component) doesn't have the behavior you
> describe. So using a PureComponent is actually a different behavior than a
> pure function and may have different performance characteristics. Usually
> this is better, but sometimes you don't need it. (In this case I think this
> is a good thing, if it's properly used -- see below).

Oh, I didn't realize React still doesn't do a shallow comparison of props to avoid redundant re-renders of stateless components. Thanks!

> While I was looking at it I also gave a look at how MenuItem is used in [1].
> I see that the onClick function is sometimes given as an arrow function,
> which means a new instance is created at each render, and therefore for
> these cases `shouldComponentUpdate` will always return true. You should fix
> these cases.

Yes, that seems suboptimal. It appears to be a long-standing bug going back to ToolboxToolbar but it's probably easier to fix now that it's been moved to a more narrowly-scoped component.

> I also see that the `key` prop isn't used in [1], but you really should
> because this helps React's tree reconciliation algorithm.

Yes, good idea. (This too, is a carry-over bug from ToolboxToolbar.)

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/73618b0b53b9
Convert MenuItem to a class; r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/973058863827
Preload images used on MenuItem labels; r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/9b1637ae2172
Defer image pre-loading until idle; r=jdescottes

Comment 28

[Brian Birtles (:birtles)]

Filed bug 1478563 for the optimizations mentioned in comment 25.

Comment 29

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/73618b0b53b9
https://hg.mozilla.org/mozilla-central/rev/973058863827
https://hg.mozilla.org/mozilla-central/rev/9b1637ae2172