1242912 - Allow batch creating tabs for session restore

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[:Gijs (he/him)]

Looking at a profile for this stuff, it seems like creating the tabs and their linked browsers in one go in a documentfragment, and only appending them when we need their XBL stuff to be bound, would help a lot with bug 1135214 (admittedly an extreme case!) and startup perf when restoring tabs generally. Related to bug 906076, but not the same. That also doesn't look like it'll be ready to land for a while, and would also require a lot of add-on work. This is a perf improvement specifically for startup itself, and would help without requiring anything else from add-ons.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

It's actually the opposite of bug 906076, right?

Comment 2

[:Gijs (he/him)]

(In reply to David Rajchenbach-Teller [:Yoric] (please use "needinfo") from comment #1)
> It's actually the opposite of bug 906076, right?

No?

bug 906076 does nothing about how we create <tab> elements themselves, it only deals with the linked browser for each tab.

Adding 1500 tabs one at a time causes a lot of churn in how we do a lot of things in addTab() in the tabbrowser that are optimized for adding a new tab using the new tab button (like setting positional / selected attributes).

Adding all the tabs in one go and only updating those attributes once would be faster.

Similarly, instead of adding the linked browsers one at a time as we do now, we could add them all in one go. Of course, bug 906076 might obviate the need to do that at all if we stop creating linked browsers as soon as we create a <tab>. Even so, judging by the profile from the bug this blocks, just optimizing how we create <tab>s should help.

Comment 3

[Florian Quèze [:florian]]

Is this a duplicate of bug 900803? Or maybe the other way?

Comment 4

[:Gijs (he/him)]

(In reply to Florian Quèze [:florian] from comment #3)
> Is this a duplicate of bug 900803? Or maybe the other way?

Yes, one way or another. I don't feel strongly about which way to dupe. I imagine the relative perf gain here is now more significant than when the patches for bug 900803 were written, given the existence of lazy browsers - spending less time creating browsers means more of the time we do spend is spent on the tabs.

Comment 6

[Florian Quèze [:florian]]

I assume the patch in bug 900803 has suffered severe bitrot at this point, so let's keep the bug with the fewest comments.

Comment 7

[:Gijs (he/him)]

Glandium radar'd a profile of their startup of a profile with about 2000 tabs on #firefox on slack.

We end up spending a full second on the repeated querySelector calls for tab:hover, which on startup is probably not needed anyway. The next biggest hitter is checking _visibleTabs, which requires getting all tabs and filtering on the hidden attribute, as well as checking multiselected - all from _setPositionalAttributes.

Batch-inserting them would at least avoid calling _setPositionalAttributes more than once per window. The main thing I'm unsure about at a glance is whether we would want to create a dedicated method or figure out a param we can pass (or are already passing) to addTab to allow us to skip expensive bits and just do them once at the end... I suspect a dedicated thing to bulk-insert tabs would be best, but I'm not sure how messy it would be and/or how much code we'd be duplicating / maintenance hassle we'd create. Fortunately, the add-on work is probably more manageable now than when I filed this bug given the demise of chrome/xul/js/xpcom add-ons.

Comment 8

[Florian Quèze [:florian]]

There are also lots of insertBefore calls. I wonder if we could insert the tabs to a document fragment so that all tabs are inserted at once into the document.

Comment 9

[Emma Malysz]

Attachment: Bug 1242912, batch insert tabs upon session restore

Comment 10

[Emma Malysz]

Attachment: Bug 1242912, batch insert tabs during a session restore instead of adding tabs individually.

Comment 11

[Emma Malysz]

I loaded 400 tabs and restored them.
With patch: https://perfht.ml/2Rk8VyG
Without patch: https://perfht.ml/34QnbTE

There is a 1,301 ms improvement within ssi_restoreWindow, but this is reduced depending on the number of tabs we load.

Comment 12

[Florian Quèze [:florian]]

(In reply to Emma Malysz from comment #11)

I loaded 400 tabs and restored them.
With patch: https://perfht.ml/2Rk8VyG
Without patch: https://perfht.ml/34QnbTE

There is a 1,301 ms improvement within ssi_restoreWindow, but this is reduced depending on the number of tabs we load.

These profiles both show a lot of time spent in Servo_TraverseSubtree (triggered from nsCSSFrameConstructor::ContentAppended) for every document fragment we insert into the document. We are doing sequential styling there, but the refresh driver ticks triggers parallel styling.
I asked emilio about it, and he said it's because there's a hack for XUL (actually I think this was to support XBL binding attachment) at https://searchfox.org/mozilla-central/rev/ea63a0888d406fae720cf24f4727d87569a8cab5/layout/base/nsCSSFrameConstructor.cpp#6412

He thinks we should see a pretty massive difference if we remove that hack. This is covered by bug 1584935.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ea61d93b2616
batch insert tabs during a session restore instead of adding tabs individually. r=Gijs

Comment 14

[Oana Pop-Rus]

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