1763279 - SessionStore keeps copying all data into a huge array

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Oriol Brufau [:Oriol]]

When you restore the previous session, SessionStoreInternal.restoreTab is called for each tab.

It has this code https://searchfox.org/mozilla-central/rev/911d1ebfb02cce4ff20f7ef965def04becfee710/browser/components/sessionstore/SessionStore.jsm#4564-4571

    // In case we didn't collect/receive data for any tabs yet we'll have to
    // fill the array with at least empty tabData objects until |_tPos| or
    // we'll end up with |null| entries.
    for (let otherTab of Array.prototype.slice.call(
      tabbrowser.tabs,
      0,
      tab._tPos
    )) {
      let emptyState = { entries: [], lastAccessed: otherTab.lastAccessed };
      this._windows[window.__SSi].tabs.push(emptyState);
    }

I'm not completely sure what exactly it's trying to do, but this is horrible:

0. The array initially has tabbrowser.tabs.length items.
1. For the first tab, tab._tPos == 0, nothing happens
2. For the 2nd tab, tab._tPos == 1, it pushes 1 emptyState
3. For the 3rd tab, tab._tPos == 2, it pushes 2 emptyState
4. For the 3rd tab, tab._tPos == 3, it pushes 3 emptyState
5. ...

Testing the restoration of a window with 1000 tabs (a low number by my standards), I get

SessionStoreInternal._windows.window0.tabs.length; // 500500

Effectively, 1000 + 0 + 1 + 2 + ... + 999 = 1000*1001/2 = 500500.

There is some code that ends up truncating the array back to 1000 items, but this is absurd. No wonder I sometimes got "out of memory" errors when trying to restore a window with 10k tabs, but splitting them into 2 windows with 5k each was fine.

The comments says "In case we didn't collect/receive data for any tabs yet", so maybe start at this._windows[window.__SSi].tabs.length instead of 0? Also, why use Array.prototype.slice??

    const {tabs} = this._windows[window.__SSi];
    for (let i = tabs.length; i < tab._tPos; ++i) {
      const otherTab = tabbrowser.tabs[i];
      const emptyState = { entries: [], lastAccessed: otherTab.lastAccessed };
      tabs.push(emptyState);
    }

CC Steven MacLeod who reviewed the bad code in bug 1157220. The author is no longer active.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1157220

Comment 2

[Oriol Brufau [:Oriol]]

Attachment: Bug 1763279 - Fix broken SessionStoreInternal.restoreTab to avoid huge arrays, unnecessary clones, undefined array index. r?Gijs

Comment 3

[Oriol Brufau [:Oriol]]

Tested restoring a session with 5000 tabs.
Without my patch, _restoreWindowsInReversedZOrder takes ~12.3 seconds.
With my patch, it takes ~7.4 seconds, i.e. a ~40% improvement.

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:dao, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 5

[Mark Banner (:standard8)]

NI for self to remember to take a look at this again.

Comment 6

[Oriol Brufau [:Oriol]]

OK, confirmed, I had 2 windows with 5246 and 6043 tabs. I had to restart due to a background update. But then I was only getting a broken Firefox UI with "out of memory" errors in the console, or simply startup crashes. I applied my patch into the omni.ja, and then Firefox started quick and fine!

So you say my patch is just wallpapering and that the code is still wrong, but my patch is clearly improving the situation without breaking anything.

Bigger refactorings are more likely to break other things. So I would ask you to approve my patch, or that someone that knows SessionStore better than me takes over and fixes everything. But I'm kinda tired of getting slow starts and broken sessions from time to time.

Comment 7

[Oriol Brufau [:Oriol]]

Ping. I don't have the time nor expertize to fix the every single bug in SessionStore. This is a fix for a single bug. I fail to see why this is not enough, it's a clear perf and stability improvement. It's so bothersome when Firefox updates and I have to patch omni.ja manually in order to avoid a crash during startup. Please approve my patch or take over the bug to do the desired extra bugfixes, but this is just stalling for no reason.

Comment 8

[Mark Banner (:standard8)]

(In reply to Oriol Brufau [:Oriol] from comment #7)

Ping. I don't have the time nor expertize to fix the every single bug in SessionStore. This is a fix for a single bug. I fail to see why this is not enough, it's a clear perf and stability improvement.

As we have already mentioned in the patch, we are not prepared to take one workaround for another when we need to understand the fundamental issue and fix that. I am hoping to be able to spend some time on this within the next few weeks which is why I've left the needinfo open on myself.

Comment 9

[Oriol Brufau [:Oriol]]

This is the main offender that makes opening several tabs slow. The 2nd one is telemetry doing getOpenTabsAndWinsCounts() (bug 1810739).

Comment 10

[Sarah Clements [:sclements]]

Our team might be able to pick this up.

Comment 11

[:Gijs (he/him)]

Attachment: Bug 1763279 - fix session store tab data sanity checks to not be terrible for performance, r?Standard8

Comment 12

[:Gijs (he/him)]

Stealing this per comments on phab. I'm sorry it took so long to get a patch up.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/21534b5fb9d3
fix session store tab data sanity checks to not be terrible for performance, r=Standard8

Comment 14

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/21534b5fb9d3

Comment 15

[Dianna Smith [:diannaS]]

Backed out changeset 21534b5fb9d3 (bug 1763279) out of fx112.0b6 for causing bug 1822854, a=backout
backout link: https://hg.mozilla.org/releases/mozilla-beta/rev/6af708163bf7a84aec91906959a2b821f7a37de8
backout push: https://treeherder.mozilla.org/jobs?repo=mozilla-beta&revision=7e8d312fbc46a462da77b48e923ec46830e43f60

The fix Remains in fx113