901786 - Persist titlebar placeholder widths to reduce layout work

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Matthew N. [:MattN]]

Attachment: WIP 1. Give ids and persist width

Initial WIP focusing on Windows: http://compare-talos.mattn.ca/?oldRevs=69bbd8165e4e&newRev=67838efec1f6&submit=true
This showed a few percent (statistically significant) savings on all Windows tpaint and tspaint_places_generated_max along with ts_paint wins on Windows 7 only for the other ts_paint tests. There aren't huge wins but the numbers are all going in the right direction except for one (WINNT 5.1 ts_paint with 0.57% regression at the time of posting) which can probably be ignored.

I have pushed an updated patch which also persists the placeholder size for the fullscreen button (OS X only).

Comment 1

[Dão Gottwald [:dao]]

Comment on attachment 786076 [details] [diff] [review]
WIP 1. Give ids and persist width

>+      <hbox id="placeholder-menubar-caption-buttons" class="titlebar-placeholder" type="caption-buttons"
>+            ordinal="1000" persist="width"/>

>+      <hbox id="placeholder-TabsToolbar-captions-buttons" class="titlebar-placeholder" type="caption-buttons"
>+            persist="width"

I'd prefer slightly more structured and verbose ids for making them easier to read.

e.g. titlebar-placeholder-on-menubar-for-caption-buttons

Also, another nit, it probably makes sense to put that attribute on a separate line grouped with persist="width".

Comment 2

[Matthew N. [:MattN]]

(In reply to Matthew N. [:MattN] from comment #0)
> I have pushed an updated patch which also persists the placeholder size for
> the fullscreen button (OS X only).

http://compare-talos.mattn.ca/?oldRevs=69bbd8165e4e&newRev=f8d43f37be85&submit=true

The results don't look too good anywhere despite what I saw earlier.

Comment 3

[:Gijs (he/him)]

Attachment: Persist titlebar placeholder sizes to reduce layout work.

Dao, I presume this is what you mean? Matt and I discussed and think we should only land this if we can get measurable perf improvements. I think the initial results actually look OK (but talos is noisy as hell), but if we use IDs we can also update the TabsInTitlebar._update code to use those rather than querySelector, which will shave off a bit more time again (I've seen those show up in profiles, although admittedly not on the order of multiple ms or anything like that). I'll add a separate patch for that.

Comment 4

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #3)
> use IDs we can also update the TabsInTitlebar._update code to use those
> rather than querySelector, which will shave off a bit more time again (I've
> seen those show up in profiles, although admittedly not on the order of
> multiple ms or anything like that). I'll add a separate patch for that.

The class name + type attribute setup is intentionally generic so that add-ons can mess with placeholders. Anything we do shows up in profiles. We should only worry about it if it's actually significant.

Comment 5

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #3)
> Created attachment 786183 [details] [diff] [review]
> Persist titlebar placeholder sizes to reduce layout work.
> 
> Dao, I presume this is what you mean? Matt and I discussed and think we
> should only land this if we can get measurable perf improvements. I think
> the initial results actually look OK (but talos is noisy as hell),

Can you trigger more runs so we can make a really informed decision here?

Comment 6

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #5)
> (In reply to :Gijs Kruitbosch from comment #3)
> > Created attachment 786183 [details] [diff] [review]
> > Persist titlebar placeholder sizes to reduce layout work.
> > 
> > Dao, I presume this is what you mean? Matt and I discussed and think we
> > should only land this if we can get measurable perf improvements. I think
> > the initial results actually look OK (but talos is noisy as hell),
> 
> Can you trigger more runs so we can make a really informed decision here?

I don't think more runs will actually help much; the variance on OS X on e.g. tpaint is on the order of 40ms. If this change got us more than 4ms, then I would be extremely surprised. My stats intuition would say we'd need to re-run these numbers at least 50 times to be more sure (and even then...). I don't think that's a good use of our (or the infra's) time. I can run some local tests with e.g. 1000 window opens on tpaint and report back, but it'll take me longer.

(In reply to Dão Gottwald [:dao] from comment #4)
> (In reply to :Gijs Kruitbosch from comment #3)
> > use IDs we can also update the TabsInTitlebar._update code to use those
> > rather than querySelector, which will shave off a bit more time again (I've
> > seen those show up in profiles, although admittedly not on the order of
> > multiple ms or anything like that). I'll add a separate patch for that.
> 
> The class name + type attribute setup is intentionally generic so that
> add-ons can mess with placeholders. Anything we do shows up in profiles. We
> should only worry about it if it's actually significant.

We can let them do that if we expose the arrays of IDs we use for these placeholders, too, similar to how we exposed inContentWhitelist + hideChromeForLocation.

Comment 7

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #6)
> > Can you trigger more runs so we can make a really informed decision here?
> 
> I don't think more runs will actually help much; the variance on OS X on
> e.g. tpaint is on the order of 40ms.

It doesn't need to be OS X. If this shows a win on any platform, that's sufficient to tell that it's a useful change.

> We can let them do that if we expose the arrays of IDs we use for these
> placeholders, too, similar to how we exposed inContentWhitelist +
> hideChromeForLocation.

Sure, there are always multiple ways to solve the same problem. The question is, is the change worth it?

Comment 8

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #7)
> (In reply to :Gijs Kruitbosch from comment #6)
> > > Can you trigger more runs so we can make a really informed decision here?
> > 
> > I don't think more runs will actually help much; the variance on OS X on
> > e.g. tpaint is on the order of 40ms.
> 
> It doesn't need to be OS X. If this shows a win on any platform, that's
> sufficient to tell that it's a useful change.

Right. I did retrigger the OS X runs a bit (there are no Windows/Linux runs on the with-patch test run). My memory of noise levels was wrong; it's ts_paint which is that noisy, tpaint is more like 15-25ms noise (still not great). So it's now showing the results of 6 tpaint runs for each platform, and 7 ts_dirtypaint runs.

Right now, the numbers look like we're improving:

tpaint by an average of 0.50% (about 1-2ms, depending on the OS version), ts_paint by an average of 0.24% (about 2ms as well),
ts_dirtypaint_max by an average of 0.63% (about 7ms averaged over the OSes), ts_dirtypaint_med by an average of 0.50% (about 6ms on average)

My local builds also show an improvement of 1-2ms on a 1000-run tpaint run.

So, I am fairly confident that there is a small win to be made here, despite talos being noisy and showing individual platforms as regressing slightly / winning greatly on 1 of the 4 tests, it averages out to a small gain.

This is all with just this patch. We can evaluate the change I suggested regarding avoiding querySelectorAll separately.

Comment 9

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #8)
> My local builds also show an improvement of 1-2ms on a 1000-run tpaint run.

This should have said, my local evaluation of the tinderbox builds (I just downloaded from the try directory).

Comment 10

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #8)
> ts_paint by an average of 0.24% (about 2ms as well),

I suppose ts_paint doesn't get a new profile for each run? Because if it did, there should be no win.

Comment 11

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #10)
> (In reply to :Gijs Kruitbosch from comment #8)
> > ts_paint by an average of 0.24% (about 2ms as well),
> 
> I suppose ts_paint doesn't get a new profile for each run? Because if it
> did, there should be no win.

It does not, indeed. If you look at any ts_paint log, it'll have something like:

/Users/cltbld/talos-slave/test/build/application/FirefoxUX.app/Contents/MacOS/firefox -foreground  -profile /var/folders/qd/srwd5f710sj0fcl9z464lkj00000gn/T/tmpwsjqKw/profile http://localhost/startup_test/tspaint_test.html?begin=

Repeated several times, with the same profile path.

Comment 12

[:Gijs (he/him)]

https://hg.mozilla.org/projects/ux/rev/cba0829065f5

Comment 13

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

Attachment: Followup to fix bc orange

We need to set skipintoolbarset on the new items, since they have IDs now.

Comment 14

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

Attachment 786387 [details] [diff] rs'd by Gijs over IRC. Pushed to UX:

https://hg.mozilla.org/projects/ux/rev/4a09078bea49

Comment 15

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/cba0829065f5
https://hg.mozilla.org/mozilla-central/rev/4a09078bea49