RemoveTabsToTheEndFrom is very slow

RESOLVED FIXED in Firefox 57

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: florian, Assigned: florian)

Tracking

(Depends on 1 bug, Blocks 1 bug)

unspecified
Firefox 57
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [reserve-photon-performance])

Attachments

(1 attachment, 1 obsolete attachment)

See this profile: https://perfht.ml/2p94N6J

The UI thread isn't blocked, but the user stays several seconds without any visual feedback of anything happening. This is because we seem to close tabs sequentially, and any tab causing permitUnload to take a while blocks closing all the tabs to the right of it.

I think we should try to close tabs in parallel rather than sequentially.
Flags: qe-verify?
Priority: -- → P2
I just noticed that if the currently selected tab is the last one, removeTabsToTheEndFrom will cause each tab to be selected successively, causing us to spend a lot of time in _blurTab: https://perfht.ml/2pTKbzZ

We should move the selection to the tab from which the removeTabsToTheEndFrom action was started as soon as we close one tab to avoid this.
We should also pre-sort the tabs to be closed based on nsITabParent.hasBeforeUnload (if available), and close those ones first.
No longer blocks: photon-performance-triage
Here is a new profile: https://perf-html.io/public/335692bec25c22012c265e680a7468cae6924aa4/calltree/?implementation=js&invertCallstack&range=24.4910_30.8725~24.7688_29.6479&thread=0

This profile is a capture of when it took ~5s to close a bit more than 500 tabs on my fast macbook.

Now that permitUnload is no longer done for tabs that don't have onbeforeunload, it's get_scrollPosition from scrollbox.xml that dominates (90% of the time!).

Here is how the stack looks:
get_scrollPosition
_fillTrailingGap
_endRemoveTab
removeTab
removeTabsToTheEndFrom
oncommand
Depends on: 1358453
Depends on: 1364661
Florian, can you profile again?
Flags: needinfo?(florian)
Here is a new profile on the latest nightly with the same machine and about 20 tabs to close:
https://perf-html.io/public/50b97cf987a08b1a6aceb560767c8dc57e13ff94/calltree/?callTreeFilters=prefixjs-x.ax.bzMp&implementation=js&range=7.4159_8.4191~7.6970_8.3216&thread=0

77% of what remains is bug 1358453.

This is still profiling the easy case where the tab on which the context menu was hidden is selected, and with few onbeforeunload event listeners.

I think what remains to do here is to handle the edge case in comment 2 (last tab selected; we should change the selected tab only once before we start closing tabs), and maybe do something smarter when there are several tabs with onbeforeunload listeners.
Flags: needinfo?(florian)
Priority: P2 → P3
Whiteboard: [photon-performance] → [reserve-photon-performance]
Florian, can you profile again now that bug 1358453 is fixed? Wondering 1) if the remaining overhead is significant and 2) where it comes from, although I guess I could extract that from your previous profile by ignoring the 77%...
Flags: needinfo?(florian)
Depends on: 1368208
No longer depends on: 1368208
(In reply to Dão Gottwald [::dao] from comment #6)
> Florian, can you profile again now that bug 1358453 is fixed? Wondering 1)
> if the remaining overhead is significant and 2) where it comes from,
> although I guess I could extract that from your previous profile by ignoring
> the 77%...

Here is a new profile of closing 166 tabs with removeTabsToTheEnd on my fast macbook using the May 28th nightly:
https://perf-html.io/public/97c8878f6040ab72952c26bddf9f2dccde816b1a/calltree/?implementation=js&range=5.9780_7.7931&thread=0

Here is the stack that dominates (1.5s) :
get_scrollPosition
_updateScrollButtonsDisabledState
_endRemoveTab
removeTab
removeTabsToTheEndFrom
oncommand
Flags: needinfo?(florian)
Depends on: 1368208
(In reply to Florian Quèze [:florian] [:flo] (away until Monday May 29th) from comment #7)
> (In reply to Dão Gottwald [::dao] from comment #6)
> > Florian, can you profile again now that bug 1358453 is fixed? Wondering 1)
> > if the remaining overhead is significant and 2) where it comes from,
> > although I guess I could extract that from your previous profile by ignoring
> > the 77%...
> 
> Here is a new profile of closing 166 tabs with removeTabsToTheEnd on my fast
> macbook using the May 28th nightly:
> https://perf-html.io/public/97c8878f6040ab72952c26bddf9f2dccde816b1a/
> calltree/?implementation=js&range=5.9780_7.7931&thread=0
> 
> Here is the stack that dominates (1.5s) :
> get_scrollPosition
> _updateScrollButtonsDisabledState

Bug 1368208 should fix that.
Flags: qe-verify? → qe-verify-
Keywords: meta
Priority: P3 → --
Summary: removeTabsToTheEndFrom is very slow → [meta] removeTabsToTheEndFrom is very slow
Whiteboard: [reserve-photon-performance] → [photon-performance]
Depends on: 1379174
Posted patch Patch (obsolete) — Splinter Review
I think we have covered the things that are actually slow (in terms of CPU usage) in dependent bugs, and what remains to do here is improve the perception:
- remove the tabs without beforeunload listener first
- avoid changing the selection for each closed tab (ie. once we are closing a selected tab, select instead of tab that we will keep once we are done closing all the other tabs)
- close tabs with beforeunload listeners from left to right so that there's immediately something that moves on the tabstrip, giving user feedback that something is happening / in progress.

We should also send the PermitUnload message to multiple tabs at once when they are in different content processes, but that seems too risky for 57, and already has a separate bug covering the idea (bug 1379174).
Attachment #8907761 - Flags: review?(mconley)
Assignee: nobody → florian
Status: NEW → ASSIGNED
Comment on attachment 8907761 [details] [diff] [review]
Patch

Review of attachment 8907761 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/tabbrowser.xml
@@ +3078,5 @@
> +      <method name="_hasBeforeUnload">
> +        <parameter name="aTab"/>
> +        <body>
> +          <![CDATA[
> +            let browser = aTab.linkedBrowser;

Hm. What about the non-e10s tab case? We're going to assume that there are no beforeunload listeners. Intentional?
(In reply to Mike Conley (:mconley) (:⚙️) from comment #10)
> Comment on attachment 8907761 [details] [diff] [review]
> Patch
> 
> Review of attachment 8907761 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/tabbrowser.xml
> @@ +3078,5 @@
> > +      <method name="_hasBeforeUnload">
> > +        <parameter name="aTab"/>
> > +        <body>
> > +          <![CDATA[
> > +            let browser = aTab.linkedBrowser;
> 
> Hm. What about the non-e10s tab case? We're going to assume that there are
> no beforeunload listeners. Intentional?

Assuming there's no beforeunload listeners means not changing anything to the current behavior, so I think that's OK for non-e10s.
Keywords: meta
Priority: -- → P1
Summary: [meta] removeTabsToTheEndFrom is very slow → RemoveTabsToTheEndFrom is very slow
Whiteboard: [photon-performance] → [reserve-photon-performance]
Posted patch Patch v2Splinter Review
There was indeed a logic error for the non-e10s case.
Attachment #8908114 - Flags: review?(mconley)
Attachment #8907761 - Attachment is obsolete: true
Attachment #8907761 - Flags: review?(mconley)
Try looks happy with the 2nd version of the patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=70fd3631f6b59ae0c971f285af602893db0a96d5&filter-tier=1 (all the oranges were green after re-triggering)
Comment on attachment 8908114 [details] [diff] [review]
Patch v2

Review of attachment 8908114 [details] [diff] [review]:
-----------------------------------------------------------------

The check in _beginRemoveTab for isRemoteBrowser addresses my concerns from before. Thanks!
Attachment #8908114 - Flags: review?(mconley) → review+
Pushed by florian@queze.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb57f4702727
when closing multiple tabs, remove the tabs without beforeunload listener first, and then close what remains from left to right for immediate user feedback, r=mconley.
https://hg.mozilla.org/mozilla-central/rev/bb57f4702727
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
You need to log in before you can comment on or make changes to this bug.