Closed Bug 906075 Opened 11 years ago Closed 11 years ago

Only send toolbars through buildArea if they're not in their default state

Categories

(Firefox :: Toolbars and Customization, defect)

x86_64
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: mconley, Assigned: mconley)

References

Details

(Whiteboard: [Austrails:M8])

Attachments

(2 files, 2 obsolete files)

According to my data, we spend about 2.5ms inside buildArea per each window for the tpaint test.

But buildArea's sole purpose is to add widgets to toolbars if they don't exist (if a user put them there) and to remove things that are there that the user had customized away.

But if the toolbar was *never changed*, this function, I believe, serves no purpose except to waste us time.

I'm experimenting with detecting if a toolbar is in a "dirty" state or not, and only running it through buildArea if it's dirty.

I've got a proof-of-concept patch baking on try.

Baseline: https://tbpl.mozilla.org/?tree=Try&rev=e736fa7b4de5
Patch: https://tbpl.mozilla.org/?tree=Try&rev=dbba7e3627f1
Attached patch Patch v1 (obsolete) — Splinter Review
Assignee: nobody → mconley
Status: NEW → ASSIGNED
Attached patch Patch v1.1 (obsolete) — Splinter Review
Fixing self-review nit.
Attachment #791459 - Attachment is obsolete: true
Comment on attachment 791462 [details] [diff] [review]
Patch v1.1

What do you think of this, Jared?
Attachment #791462 - Flags: review?(jaws)
Here's the compare talos. This maybe gains us something like 0.5ms on XP? (Smaller than I expected, but that's about par for the course).

http://compare-talos.mattn.ca/?oldRevs=e736fa7b4de5&newRev=dbba7e3627f1&submit=true
Attachment #791462 - Flags: review?(jaws) → review+
jaws: +1 on fast turnaround.

https://hg.mozilla.org/projects/ux/rev/5a2fbb868edd
Whiteboard: [Austrails:M8][fixed-in-ux]
Backed this out because I tried the patch on my OS X machine and it seemed to break CustomizableUI in unexpected ways. Needs further investigation.

https://hg.mozilla.org/projects/ux/rev/e7977c9175f2
Whiteboard: [Austrails:M8][fixed-in-ux] → [Austrails:M?]
This seemed to cause an XP tpaint regression with 5a2fbb868edd and it went away the the backout.
Yep, noticed that too. Clearly there's something wrong with the patch, which is strange, because I swore I had it working fine on my Windows box at work.

But when I tried it on my Mac tonight, everything was horribly busted. :/ So, clearly, this needs to more investigation. I still think this is a viable avenue for a performance win, albeit a small one.
Attached patch Patch v1.2Splinter Review
Turns out I forgot to close one of the last-minute comments I added, and accidentally commented out a huge chunk of code. Classic.

Going to push this to try and see what we get.
Attachment #791462 - Attachment is obsolete: true
This is what we got:

https://tbpl.mozilla.org/?tree=Try&rev=39c003c08bf6

Looks like a win - and local testing seems to show this new patch is A-OK. Let's try this again.
Re-landed here:

https://hg.mozilla.org/projects/ux/rev/f1b79ee158c9
Whiteboard: [Austrails:M?] → [Austrails:M8][fixed-in-ux]
After a quick discussion with Matt on IRC, I decided to land this followup patch:

https://hg.mozilla.org/projects/ux/rev/0cf7690c8428

which I presume should have been part of this bug.

Sadly, the patch means that previously, we will have always fast-pathed, whereas now we may or may not. We should, but we may not. Let's keep an eye on tpaint/ts_paint numbers.
Ouch. My mistake. :/
https://hg.mozilla.org/mozilla-central/rev/f1b79ee158c9
https://hg.mozilla.org/mozilla-central/rev/0cf7690c8428
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [Austrails:M8][fixed-in-ux] → [Austrails:M8]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.