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)
Tracking
()
RESOLVED
FIXED
Firefox 28
People
(Reporter: mconley, Assigned: mconley)
References
Details
(Whiteboard: [Austrails:M8])
Attachments
(2 files, 2 obsolete files)
35.44 KB,
image/png
|
Details | |
12.54 KB,
patch
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•11 years ago
|
||
Assignee: nobody → mconley
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•11 years ago
|
||
Fixing self-review nit.
Attachment #791459 -
Attachment is obsolete: true
Assignee | ||
Comment 3•11 years ago
|
||
Comment on attachment 791462 [details] [diff] [review] Patch v1.1 What do you think of this, Jared?
Attachment #791462 -
Flags: review?(jaws)
Assignee | ||
Comment 4•11 years ago
|
||
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
Updated•11 years ago
|
Attachment #791462 -
Flags: review?(jaws) → review+
Assignee | ||
Comment 5•11 years ago
|
||
jaws: +1 on fast turnaround. https://hg.mozilla.org/projects/ux/rev/5a2fbb868edd
Whiteboard: [Austrails:M8][fixed-in-ux]
Assignee | ||
Comment 6•11 years ago
|
||
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?]
Comment 7•11 years ago
|
||
This seemed to cause an XP tpaint regression with 5a2fbb868edd and it went away the the backout.
Assignee | ||
Comment 8•11 years ago
|
||
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.
Assignee | ||
Comment 9•11 years ago
|
||
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
Assignee | ||
Comment 10•11 years ago
|
||
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.
Assignee | ||
Comment 11•11 years ago
|
||
Re-landed here: https://hg.mozilla.org/projects/ux/rev/f1b79ee158c9
Whiteboard: [Austrails:M?] → [Austrails:M8][fixed-in-ux]
Comment 12•11 years ago
|
||
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.
Assignee | ||
Comment 13•11 years ago
|
||
Ouch. My mistake. :/
Assignee | ||
Comment 14•11 years ago
|
||
Data's come in - I think we're in the clear. tpaint: https://datazilla.mozilla.org/?start=1378482638&stop=1379087438&product=Firefox&repository=UX-Non-PGO&test=tpaint&page=tpaint&tr_id=2810330&graph=win%206.1.7601&project=talos ts_paint: https://datazilla.mozilla.org/?start=1378482638&stop=1379087438&product=Firefox&repository=UX-Non-PGO&test=ts_paint&page=ts_paint&tr_id=2810330&graph=win%206.1.7601&project=talos I don't see any troubling spikes.
Comment 15•11 years ago
|
||
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.
Description
•