891104 - Skip calling onOverflow during startup if there wasn't any overflowed content before the toolbar is fully initialized

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Matthew N. [:MattN]]

Attachment: WIP measurement patch

Perhaps this isn't possible but there may be heuristics we can use that don't require a reflow.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=73fccc85c313

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

I'm going to look in to this.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

This patch only runs the onOverflow code if we've actually received an overflow event.

Try push with patch: https://tbpl.mozilla.org/?tree=Try&rev=caa53f9af310
Try push w/out patch: https://tbpl.mozilla.org/?tree=Try&rev=3ea895acccce

Comment 3

[Matthew N. [:MattN]]

My try push to skip this altogether showed slight tpaint wins on most platforms but it also regressed tresize and dirty places by a larger magnitude.

Perf: http://perf.snarkfest.net/compare-talos/index.html?oldRevs=515129449f3e&newRev=3c89703e6993&submit=true

Try run: https://tbpl.mozilla.org/?tree=Try&rev=3c89703e6993

Patch: https://hg.mozilla.org/try/rev/22bb2d9fabb8

Any ideas why that would cause regressions? Perhaps your version won't show the same.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Running this using mconley's local tpaint with 1,000 iterations:

With patch: 197.31 (175 - 385.59)
W/out patch: 213.45 (177.38 - 437.45)

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Matthew N. [:MattN] from comment #3)
> My try push to skip this altogether showed slight tpaint wins on most
> platforms but it also regressed tresize and dirty places by a larger
> magnitude.
> 
> Any ideas why that would cause regressions? Perhaps your version won't show
> the same.

Well, your patch completely disables the overflow code. I could take a guess that when resizing to a smaller window, once we place an element in the overflow widget then resizing is less work as there are less elements to move around.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 781958 [details] [diff] [review]
Patch

Review of attachment 781958 [details] [diff] [review]:
-----------------------------------------------------------------

I think this works really nicely. Update that one comment I pointed out, and it's r=me. Thanks jaws!

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +2211,3 @@
>  
>      // The toolbar could initialize in an overflowed state, in which case
>      // the 'overflow' event may have been fired before the handler was registered.

This comment isn't really reflective of the code anymore. Instead, it should say that the 'overflow' event may have been fired before init was called.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/projects/ux/rev/8637eb509182

Comment 8

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/8637eb509182