1356153 - RemoveTabsToTheEndFrom is very slow

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, enhancement, P1)

Description

[Florian Quèze [:florian]]

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.

Comment 1

[Florian Quèze [:florian]]

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.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

We should also pre-sort the tabs to be closed based on nsITabParent.hasBeforeUnload (if available), and close those ones first.

Comment 3

[Florian Quèze [:florian]]

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

Comment 4

[Dão Gottwald [:dao]]

Florian, can you profile again?

Comment 5

[Florian Quèze [: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.

Comment 6

[Dão Gottwald [:dao]]

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

Comment 7

[Florian Quèze [:florian]]

(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

Comment 8

[Dão Gottwald [:dao]]

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

Comment 9

[Florian Quèze [:florian]]

Attachment: Patch

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

Comment 10

[Mike Conley (:mconley) (:⚙️)]

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?

Comment 11

[Florian Quèze [:florian]]

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

Comment 12

[Florian Quèze [:florian]]

Attachment: Patch v2

There was indeed a logic error for the non-e10s case.

Comment 13

[Florian Quèze [:florian]]

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 14

[Mike Conley (:mconley) (:⚙️)]

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!

Comment 15

[Pulsebot]

Pushed by [e-mail]:
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.

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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