897218 - Cache tabsintitlebar placeholder sizes in a JSM

Status: RESOLVED WONTFIX

(Firefox :: Toolbars and Customization, defect)

Description

[:Gijs (he/him)]

Per the discussion in yesterday's Australis performance meeting, we should investigate moving this code to a JSM to improve performance of the calculation work we're doing for the second/third/... window.

Comment 1

[:Gijs (he/him)]

Pushed something to try:

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

Comment 2

[:Gijs (he/him)]

Attachment: Cache tabsintitlebar values in a JSM.

Comment 3

[Dão Gottwald [:dao]]

How aggressively would stuff be cached, when would the cache be invalidated? Note that windows don't necessarily share the same layout, extensions can add stuff in one window but not in the other. We might actually do something like this ourselves for private vs. non-private windows.

Comment 4

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #3)
> How aggressively would stuff be cached, when would the cache be invalidated?
> Note that windows don't necessarily share the same layout, extensions can
> add stuff in one window but not in the other. We might actually do something
> like this ourselves for private vs. non-private windows.

I took care about the menubar being hidden/shown already; otherwise I think the layout ought not to change much? The private/non-private windows is a good point, and should probably be updated when we actually show which windows are/aren't private again on all platforms (IIRC either OS X or Windows currently has no indicator - there's a bug on file for that already).

We could in principle invalidate when any of the included toolbar layouts change (by registering a CustomizableUI listener).

However, surprisingly, the preliminary TBPL data seems to suggest this doesn't buy us much if anything at all. Which means this might not be worth it. I should probably run a baseline in order to be able to check ts_paint and ts_dirtypaint numbers, but the tpaint numbers don't look significantly better, and if none of the others do either, this might not be worth the additional complexity.

Comment 5

[:Gijs (he/him)]

... except I broke menu height setting. Fixed: https://tbpl.mozilla.org/?tree=Try&rev=38006c142d4a

Comment 6

[Matthew N. [:MattN]]

Comment on attachment 780398 [details] [diff] [review]
Cache tabsintitlebar values in a JSM.

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

Some ideas once we know this will give us perf wins:
1) I would prefer if the jsm was an hg copy from browser.js to preserve blame (especially because this is somewhat complicated) and to make it easier to see what actually changed.
2) The name of the JSM is not accurate as it's not just about placeholders, there is also margins and paddings of the menubar and titlebar being set. I think it would be cleaner to move the whole TabsInTitlebar object to TabsInTitlebar.jsm unless this gets too dirty.

::: browser/modules/TitlebarPlaceholders.jsm
@@ +39,5 @@
> +  return val;
> +}
> +
> +let gHaveMeasures = false;
> +let gHaveMenubarMeasure = false;

Nit: globals together at the top

Comment 7

[Matthew N. [:MattN]]

There is a bug in the try push patch. STR:

1) Start Firefox on XP in a restored window with the menubar shown
2) Maximize the window
3) Optionally hide the menubar

Actual result:
toolbars are too high off the screen

Comment 8

[:Gijs (he/him)]

Matt, should we call this as WONTFIX?

Comment 9

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

Wait wait wait - why is this maybe a WONTFIX? According to the spreadsheet, this gave us a bit of a win on XP tpaint. Or did I miss something?

Comment 10

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) from comment #9)
> Wait wait wait - why is this maybe a WONTFIX? According to the spreadsheet,
> this gave us a bit of a win on XP tpaint. Or did I miss something?

I don't see it... 148? About the same as the other ones with the windows client hittest fixes, isn't it? (row 139 right?)

Comment 11

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

Yeah, 148. It's not big and dramatic, but it looks like a few milliseconds, no? Or am I reading this spreadsheet wrong?

Comment 12

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) from comment #11)
> Yeah, 148. It's not big and dramatic, but it looks like a few milliseconds,
> no? Or am I reading this spreadsheet wrong?

I think at the time I pushed that, we were doing 147/148/149 anyway, because of bug 894099. :-(

Comment 13

[Matthew N. [:MattN]]

Here is the comparison:
http://perf.snarkfest.net/compare-talos/index.html?oldRevs=732794f8a4d4&newRev=38006c142d4a&submit=true

It does seem to help us in most cases where it counts (tpaint). Note that XP ts_paint seems to have an outlier of 587.58 as the first result which makes this look like it regresses that but I think we can ignore that. Logically, this patch has to make things faster.

We still have to fix comment 7 if it's caused by this patch although I think Gijs might have found that it's an existing bug?

Comment 14

[Dão Gottwald [:dao]]

Can somebody refresh my memory on how the tpaint test works? How many windows does it open? Does it open that number of windows because that's supposed to reflect reality or in order to get a good average from fluctuating numbers? If it's the latter, then we shouldn't optimize for that.

Comment 15

[:Gijs (he/him)]

(In reply to Matthew N. [:MattN] from comment #13)
> Here is the comparison:
> http://perf.snarkfest.net/compare-talos/index.
> html?oldRevs=732794f8a4d4&newRev=38006c142d4a&submit=true
> 
> It does seem to help us in most cases where it counts (tpaint). Note that XP
> ts_paint seems to have an outlier of 587.58 as the first result which makes
> this look like it regresses that but I think we can ignore that. Logically,
> this patch has to make things faster.
> 
> We still have to fix comment 7 if it's caused by this patch although I think
> Gijs might have found that it's an existing bug?

No, I think that issue means I'd have to cache things separately for different sizemodes. Note also what Dao said... Getting this right where add-ons are involved might be quite hard. Furthermore, looking at that comparison, the OS X 10.7 regressions worry me (although the noise there is terrible so it's basically impossible to tell whether that's real or not).

Some other notes (which I think I already mentioned to Matt, but responding here for posterity / review clarity):

(In reply to Matthew N. [:MattN] from comment #6)
> Comment on attachment 780398 [details] [diff] [review]
> Cache tabsintitlebar values in a JSM.
> 
> Review of attachment 780398 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Some ideas once we know this will give us perf wins:
> 1) I would prefer if the jsm was an hg copy from browser.js to preserve
> blame (especially because this is somewhat complicated) and to make it
> easier to see what actually changed.

I disagree; this makes merges a lot harder, and I'm touching a lot of the lines in this function anyway. I think we should take this along with bug 885062, fwiw.

> 2) The name of the JSM is not accurate as it's not just about placeholders,
> there is also margins and paddings of the menubar and titlebar being set. I
> think it would be cleaner to move the whole TabsInTitlebar object to
> TabsInTitlebar.jsm unless this gets too dirty.

It gets rather dirty because there are window-specific menu observers in the TabsInTitlebar object. And because you can't disconnect an observer from a single node at a time, that means either keeping track of all the menus you're tracking (so you can disconnect the observer from all of them and then reconnect the ones you still care about), or keeping a map of windows to observers so you can disconnect them and lose their reference... it all becomes rather a nuisance. This is why I only put part of the object in its own JSM.

Comment 16

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #14)
> Can somebody refresh my memory on how the tpaint test works? How many
> windows does it open?

20

> Does it open that number of windows because that's
> supposed to reflect reality or in order to get a good average from
> fluctuating numbers? If it's the latter, then we shouldn't optimize for that.

I don't know. Either way, regarding optimizations, we have other tests (ts_paint*) that test strict startup-to-first-page-paint times. tpaint doesn't count its initial window opening, and as caching would help for any window after the first one (ie, even if the test opened 2 windows, and restarted 50 times to get better averages, this patch would theoretically help), I don't think these optimizations are 'wrong' from a realistic perspective, either.

Comment 17

[Dão Gottwald [:dao]]

They aren't wrong per se, but the question is how much they buy is in reality and whether that's worth the complexity and potential fragility.

Comment 18

[Dão Gottwald [:dao]]

I.e. if this was the last thing preventing the mozilla-central landing, we might as well say: "After exhausting all other options, the only way to close the remaining gap was to add complex and fragile code that doesn't realistically help our users, so instead we chose to accept this small regression as the cost of the improvements our changes bring."

Comment 19

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #18)
> I.e. if this was the last thing preventing the mozilla-central landing, we
> might as well say: "After exhausting all other options, the only way to
> close the remaining gap was to add complex and fragile code that doesn't
> realistically help our users, so instead we chose to accept this small
> regression as the cost of the improvements our changes bring."

I'm not sure how complex/fragile it'd be in the end, but I agree that we have lower-hanging fruit that should be exhausted first (particularly, bug 898126, bug 885062 (which I think I may be able to push a bit further still)).

Comment 20

[:Gijs (he/him)]

Unassigning because I'm no longer working on this. I think this should be WONTFIX.

Comment 21

[Dão Gottwald [:dao]]

I agree.