1590554 - Improve caching of tabbrowser-tab fragment

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, enhancement, P1)

Description

[Brian Grinstead [:bgrins]]

Right now it's caching it on each element: https://searchfox.org/mozilla-central/rev/8a63fc190b39ed6951abb4aef4a56487a43962bc/browser/base/content/tabbrowser-tab.js#74. In practice this isn't any better than not caching since we only run connectedCallback logic once per tab.

If we move this stored value onto the constructor it means we'd only need to call parseXULToFragment a max of once per document instead of once per new tab.

Comment 1

[Brian Grinstead [:bgrins]]

Attachment: Bug 1590554 - Cache the parsed tabbrowser-tab fragment on the constructor instead of on each element;r=Gijs

This means we only need to call parseXULToFragment a max of once per document
instead of a maximum of once per tab (which in practice isn't any better than
not caching since we only run connectedCallback logic once per tab).

Comment 2

[Brian Grinstead [:bgrins]]

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=a4686b3f6d8a766989d11fc3f1b6abe3eb1a2e25&newProject=try&newRevision=0889dc9078d5f9a6d10d41227e938e86ab252b7b&framework=1&showOnlyImportant=1

Comment 3

[:Gijs (out for now; he/him)]

I don't suppose we have perf numbers for the impact of fixing this and/or runtime cost we see right now for e.g. opening 1000 tabs?

Comment 4

[Brian Grinstead [:bgrins]]

(In reply to :Gijs (he/him) from comment #3)

I don't suppose we have perf numbers for the impact of fixing this and/or runtime cost we see right now for e.g. opening 1000 tabs?

The best I have is this (ongoing) talos push https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=a4686b3f6d8a766989d11fc3f1b6abe3eb1a2e25&newProject=try&newRevision=0889dc9078d5f9a6d10d41227e938e86ab252b7b&framework=1&showOnlyImportant=1.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b32c8b01a31f
Cache the parsed tabbrowser-tab fragment on the constructor instead of on each element;r=Gijs

Comment 6

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/b32c8b01a31f

Comment 7

[Brian Grinstead [:bgrins]]

I'd like to validate that this showed up as an improvement on talos after it hit m-c, since it was clear on try pushes: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=a4686b3f6d8a766989d11fc3f1b6abe3eb1a2e25&newProject=try&newRevision=0889dc9078d5f9a6d10d41227e938e86ab252b7b&framework=1&showOnlyImportant=1.

Comment 8

[Ionuț Goldan [:igoldan]]

(In reply to Brian Grinstead [:bgrins] from comment #7)

I'd like to validate that this showed up as an improvement on talos after it hit m-c, since it was clear on try pushes: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=a4686b3f6d8a766989d11fc3f1b6abe3eb1a2e25&newProject=try&newRevision=0889dc9078d5f9a6d10d41227e938e86ab252b7b&framework=1&showOnlyImportant=1.

Alex, be on the lookup for any Talos improvements that may related to this bug. Please reply back when/if you see something.