Closed Bug 816607 Opened 12 years ago Closed 2 years ago

SessionStore closes all tabs one by one, triggering synchronous reflow each time

Categories

(Firefox :: Session Restore, defect)

Product:
Firefox
Component:
Session Restore
Type:
defect

Tracking

()

Status:
RESOLVED WORKSFORME
Project Flags:
Performance Impact low

People

(Reporter: glandium, Unassigned)

References

Details

(Keywords: perf, perf:frontend, Whiteboard: [Snappy:P1])

Attachments

(1 obsolete file)

See this profile: http://bit.ly/WxccDR This comes from this code in restoreWindow in SessionStore.jsm: // when overwriting tabs, remove all superflous ones if (aOverwriteTabs && newTabCount < openTabCount) { Array.slice(tabbrowser.tabs, newTabCount, openTabCount) .forEach(tabbrowser.removeTab, tabbrowser); }
The global PB mode will hopefully go away soon (perhaps Firefox 20) so this is not worth fixing I don't think. That said, this is really a sessionstore bug. PB is only a consumer of the setBrowserState API which does this.
Component: Private Browsing → Session Restore
(In reply to Ehsan Akhgari [:ehsan] from comment #1) > The global PB mode will hopefully go away soon (perhaps Firefox 20) so this > is not worth fixing I don't think. That said, this is really a sessionstore > bug. PB is only a consumer of the setBrowserState API which does this. We should fix this in sessionstore, there are likely to be other consumers of this. No reason to keep bad code around.
(In reply to comment #2) > (In reply to Ehsan Akhgari [:ehsan] from comment #1) > > The global PB mode will hopefully go away soon (perhaps Firefox 20) so this > > is not worth fixing I don't think. That said, this is really a sessionstore > > bug. PB is only a consumer of the setBrowserState API which does this. > > We should fix this in sessionstore, there are likely to be other consumers of > this. No reason to keep bad code around. There are no other consumers on m-c (besides tests of course), but there are definitely a bunch of add-ons which use this API. <https://mxr.mozilla.org/addons/search?string=setBrowserState>
The code tries to re-use the current window, I guess potentially to avoid to much visual noise when resetting a session. That seems misguided - it would be simpler to just always use new windows, to avoid having to close many tabs individually.
Assignee: enndeakin → nobody
Per-window private browsing landed so this is not an issue anymore.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → WONTFIX
(In reply to Tim Taubert [:ttaubert] from comment #6) > Per-window private browsing landed so this is not an issue anymore. The API still has this problem, right? I think we should fix it even if it isn't used by PB.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Summary: Enabling private browsing closes all tabs one by one, triggering synchronous reflow each time → setBrowserState closes all tabs one by one, triggering synchronous reflow each time
(In reply to comment #7) > (In reply to Tim Taubert [:ttaubert] from comment #6) > > Per-window private browsing landed so this is not an issue anymore. > > The API still has this problem, right? I think we should fix it even if it > isn't used by PB. It is currently unused outside of the tests.
If we don't think the API is worth maintaining we should just remove it - but I don't think we're going to do that, because it's useful to add-ons and it's already been exposed to them. So we should maintain it.
(In reply to comment #9) > If we don't think the API is worth maintaining we should just remove it - but I > don't think we're going to do that, because it's useful to add-ons and it's > already been exposed to them. So we should maintain it. Sigh. There are indeed a few add-ons which use it: <https://mxr.mozilla.org/addons/search?string=setBrowserState>
Assignee: nobody → smacleod
This is a preliminary patch which has |restoreWindow()| create and insert tabs all at once. On large sessions with lots of tabs I see about a 50% reduction in FX_SESSION_RESTORE_RESTORE_WINDOW_MS (From ~5000ms to ~2500ms).
Attachment #784724 - Flags: feedback?(ttaubert)
(In reply to Steven MacLeod [:smacleod] from comment #11) > Created attachment 784724 [details] [diff] [review] > Patch - Make restoreWindow create tabs all at once Doesn't sound like it belongs in this bug ("setBrowserState closes all tabs one by one").
OK, per IRL discussion with Tim and Steven, re-summarizing this bug to more accurately fit the problem we should address here. The inefficient tab closing is not isolated to setBrowserState, it also affects the other SessionStore paths. And the "inefficiently closing tabs" issue is different enough that it makes sense to track it separately from "inefficiently opening tabs" problem Steven's patch is solving, for which he'll file a separate bug.
Summary: setBrowserState closes all tabs one by one, triggering synchronous reflow each time → SessionStore closes all tabs one by one, triggering synchronous reflow each time
Attachment #784724 - Flags: feedback?(ttaubert)
Comment on attachment 784724 [details] [diff] [review] Patch - Make restoreWindow create tabs all at once >+ let t = this._createTab(aURI, this.tabContainer, >+ {animate: animate}); _createTab doesn't expect this second argument. > if (animate) { >- mozRequestAnimationFrame(function () { >- this.tabContainer._handleTabTelemetryStart(t, aURI); >- >- // kick the animation off >- t.setAttribute("fadein", "true"); >- >- // This call to adjustTabstrip is redundant but needed so that >- // when opening a second tab, the first tab's close buttons >- // appears immediately rather than when the transition ends. >- if (this.tabs.length - this._removingTabs.length == 2) >- this.tabContainer.adjustTabstrip(); >- }.bind(this)); >+ this._animateTab(t, aURI); > } >+ <method name="_animateTab"> >+ <parameter name="aTab"/> >+ <parameter name="aURI"/> >+ <body> >+ <![CDATA[ >+ mozRequestAnimationFrame(function () { >+ this.tabContainer._handleTabTelemetryStart(aTab, aURI); >+ >+ // kick the animation off >+ aTab.setAttribute("fadein", "true"); >+ >+ // This call to adjustTabstrip is redundant but needed so that >+ // when opening a second tab, the first tab's close buttons >+ // appears immediately rather than when the transition ends. >+ if (this.tabs.length - this._removingTabs.length == 2) >+ this.tabContainer.adjustTabstrip(); >+ }.bind(this)); >+ ]]> >+ </body> >+ </method> Unrelated change? >+ // inject the new DOM nodes >+ this.tabContainer.appendChild(tabsFragment); >+ this.mPanelContainer.appendChild(browsersFragment); >+ this.tabContainer.updateVisibility(); >+ >+ for (let i = 0; i < tabs.length; i++) { >+ let t = tabs[i], b = browsers[i]; >+ >+ // wire up a progress listener for the new browser object. >+ let tabListener = this.mTabProgressListener(t, b, true); >+ const filter = Components.classes["@mozilla.org/appshell/component/browser-status-filter;1"] >+ .createInstance(Components.interfaces.nsIWebProgress); >+ filter.addProgressListener(tabListener, Components.interfaces.nsIWebProgress.NOTIFY_ALL); >+ b.webProgress.addProgressListener(filter, Components.interfaces.nsIWebProgress.NOTIFY_ALL); >+ this.mTabListeners[startPosition + i] = tabListener; >+ this.mTabFilters[startPosition + i] = filter; >+ >+ b.droppedLinkHandler = handleDroppedLink; >+ >+ // Dispatch a new tab notification. We do this once we're >+ // entirely done, so that things are in a consistent state >+ // even if the event listener opens or closes tabs. >+ let evt = document.createEvent("Events"); >+ evt.initEvent("TabOpen", true, false); >+ t.dispatchEvent(evt); You're dispatching TabOpen for the individual tabs at a point where you already added various other tabs that didn't get the TabOpen event yet. That's basically breaking the API.
(this patch was moved to bug 900803)
Attachment #784724 - Attachment is obsolete: true
Removing myself as assignee as I'm no longer working on this
Assignee: smacleod → nobody
Flags: firefox-backlog?
OS: Linux → All
Hardware: x86_64 → All
This is in the same state as bug 900803 and doesn't seem to be actionable enough to be put in the backlog.
Flags: firefox-backlog? → firefox-backlog-
Comment 5 seems actionable.
Flags: needinfo?(ttaubert)
(In reply to Dão Gottwald [:dao] from comment #18) > Comment 5 seems actionable. Well, yes. The question is whether the average user has enough tabs in a single window so that the overhead of closing and opening a new window is an acceptable tradeoff. If you have a window with let's say 10 tabs and you restore one with 2 tabs then we'll close 8 of them. The average user won't ever hit this case, esp. not at startup or even when undo-closing a window so this doesn't seem like something worth spending time on.
Flags: needinfo?(ttaubert)
Keywords: perf
A close-/shutdown-bug can't block an startup-bug, or?
No longer blocks: 582005
Whiteboard: [Snappy:P1] → [Snappy:P1][fxperf]
Whiteboard: [Snappy:P1][fxperf] → [Snappy:P1][fxperf:p3]
Severity: normal → S3

The severity field for this bug is relatively low, S3. However, the bug has 12 votes.
:dao, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Flags: needinfo?(dao+bmo)

The last needinfo from me was triggered in error by recent activity on the bug. I'm clearing the needinfo since this is a very old bug and I don't know if it's still relevant.

Flags: needinfo?(dao+bmo)

Sync style reflows when closing a tab are still there, and I can't find another extant bug for this, so I filed bug 1822532 for it.

AFAICT we don't really use overwriteTabs anymore, except on startup session restore - in which case there is only 1 tab in the window, which is selected, and doesn't get removed. So I'm going to close this out. If there is still a perf issue here we should have a new bug with more specifics on how to repro and what exactly the issue is.

Status: REOPENED → RESOLVED
Closed: 11 years ago → 2 years ago
Performance Impact: --- → low
Keywords: perf:frontend
Resolution: --- → WORKSFORME
Whiteboard: [Snappy:P1][fxperf:p3] → [Snappy:P1]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: