Closed Bug 891104 Opened 7 years ago Closed 6 years ago

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

Categories

(Firefox :: Theme, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: MattN, Assigned: jaws)

References

(Depends on 1 open bug)

Details

(Keywords: perf, Whiteboard: [Australis:M8])

Attachments

(1 file, 1 obsolete file)

Attached patch WIP measurement patch (obsolete) — Splinter Review
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
I'm going to look in to this.
Assignee: mconley → jaws
Attached patch PatchSplinter Review
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
Attachment #772310 - Attachment is obsolete: true
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.
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)
(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.
Summary: Consider skipping onOverflow to avoid a reflow on startup → Skip calling onOverflow during startup if there wasn't any overflowed content before the toolbar is fully initialized
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.
Attachment #781958 - Flags: review?(mconley) → review+
https://hg.mozilla.org/mozilla-central/rev/8637eb509182
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M8][fixed-in-ux] → [Australis:M8]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.