Closed Bug 1242912 Opened 4 years ago Closed 3 months ago

Allow batch creating tabs for session restore

Categories

(Firefox :: Session Restore, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 74
Tracking Status
firefox47 --- wontfix
firefox74 --- fixed

People

(Reporter: Gijs, Assigned: emalysz)

References

(Depends on 1 open bug, Blocks 6 open bugs)

Details

(Keywords: perf, Whiteboard: [fxperf:p2])

Attachments

(1 file, 1 obsolete file)

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.
It's actually the opposite of bug 906076, right?
(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.
Keywords: perf
Depends on: ss-perf
Is this a duplicate of bug 900803? Or maybe the other way?
(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.
Duplicate of this bug: 900803
I assume the patch in bug 900803 has suffered severe bitrot at this point, so let's keep the bug with the fewest comments.

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.

Whiteboard: [fxperf]

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.

Assignee: nobody → emalysz
Whiteboard: [fxperf] → [fxperf:p2]
Attachment #9107051 - Attachment is obsolete: true
Attachment #9109743 - Attachment description: Bug 1242912, updated batch insert method → Bug 1242912, batch insert tabs during a session restore instead of adding tabs individually.

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.

(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.

See Also: → 1584935
Blocks: 1605487
Blocks: 1605486
Blocks: 1606633
Blocks: 1606737
Blocks: 1607370
Pushed by nerli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ea61d93b2616
batch insert tabs during a session restore instead of adding tabs individually. r=Gijs
Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 74
Regressions: 1607441
Blocks: ss-perf
No longer depends on: ss-perf
Depends on: 1607718
Depends on: 1609413
Regressions: 1608545
Depends on: 1610463
Regressions: 1618936
You need to log in before you can comment on or make changes to this bug.