1039500 - tabbrowser's _getTabForBrowser is very slow with lots of tabs

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

The file tabbrowser.xml defines a method `_getTabForBrowser` that is used by the component to find the browser matching a tab (see 

http://dxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#373

).

The implementation of this method doesn't scale up, as it involves scanning the entire set of tabs at every single call. We could rewrite it to be much more efficient. This is a pretty simple bug that can be fixed with 5 to 10 lines of code plus documentation, as follows:

1. create a field `_tabForBrowser` containing a `WeakMap`, which will serve to record the tab for each browser;
2. at the end of method `addTab`, call `this._tabForBrowser.set(b, t)` to record that `t` is the tab for `b`;
3. in method `_getTabForBrowser`, return `this._tabForBrowser.get(aBrowser)` to get the browser.

The garbage-collector will automatically take care of the rest.

Comment 1

[:Gijs (he/him)]

Bonus points: can we make it non-private (remove the underscores) and switch session store to use it instead of its own copy-pasted implementation? :-)

Comment 2

[David Teller [:Yoric] - still alive but not very active]

This is clearly the objective. See blocked bug.

Comment 3

[Dão Gottwald [:dao]]

(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo") from comment #0)
> 1. create a field `_tabForBrowser` containing a `WeakMap`, which will serve
> to record the tab for each browser;
> 2. at the end of method `addTab`, call `this._tabForBrowser.set(b, t)` to
> record that `t` is the tab for `b`;
> 3. in method `_getTabForBrowser`, return `this._tabForBrowser.get(aBrowser)`
> to get the browser.
> 
> The garbage-collector will automatically take care of the rest.

This seems a bit fragile. E.g. I'm not sure if updateBrowserRemoteness would break this.

Comment 4

[David Teller [:Yoric] - still alive but not very active]

I don't really see how it could break it.

Comment 5

[Dão Gottwald [:dao]]

I thought that method might be replacing the browser, but it just removes it and inserts it again...

Comment 6

[vikneshwar]

Attachment: tabbed-browser

try push failed https://tbpl.mozilla.org/?tree=Try&rev=2a3fe995f370

Comment 7

[David Teller [:Yoric] - still alive but not very active]

Apparently, the issue appears here: http://dxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#430-431

I'm not completely sure why, but there is a change we might be calling `this._tabForBrowser.set(b, t)` too late. Could you try and move this statement just below the line `t.linkedBrowser = b`?

Also, let's take the opportunity to add:
- `this._tabForBrowser.delete(aTab.linkedBrowser)` just above the line `aTab.linkedBrowser = null` in method `_endRemoveTab` ;
- `this._tabForBrowser.set(this.mCurrentTab.linkedBrowser, this.mCurrentBrowser)` just below the line `this.mCurrentTab.linkedBrowser = this.mCurrentBrowser` in the constructor

Comment 8

[vikneshwar]

Attachment: tabbrowser.patch

Try push https://tbpl.mozilla.org/?tree=Try&rev=3c088ea38495

Comment 9

[vikneshwar]

Try push with latest pull https://tbpl.mozilla.org/?tree=Try&rev=babebf5e65a

Comment 10

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8460556 [details] [diff] [review]
tabbrowser.patch

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

This looks good to me. However, apparently, this makes e10s unhappy.
Felipe, do you have any idea how/why this patch could cause the JP orange in the Try?

Comment 11

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8460556 [details] [diff] [review]
tabbrowser.patch

I'm away and won't be able to look at that soon. Sending it over to Bill. Bill, see comment 10

Comment 12

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

David, it looks like the e10s tests are failing right away, so it should be easy to reproduce:

mach mochitest-plain --e10s

I see an error in the console about removeProgressListener. Maybe it's related to that somehow.

Comment 13

[David Teller [:Yoric] - still alive but not very active]

Bill, does your comment on bug 1039506 apply here, too?

Comment 14

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

No. As far as I know, this patch by itself should work fine with e10s. The tab and the browser both live in the parent.

Comment 15

[tabmix.onemen]

does it possible to add id to each tab, add the tab id as attribute to the attached browser.

then you can use getElementById

Comment 16

[David Teller [:Yoric] - still alive but not very active]

Ok, found the issue. In the constructor, the call to
  this._tabForBrowser.set(this.mCurrentTab.linkedBrowser, this.mCurrentBrowser);
should have been
  this._tabForBrowser.set(this.mCurrentTab.linkedBrowser, this.mCurrentTab);

Try:
https://tbpl.mozilla.org/?tree=Try&rev=d6d89e12510f
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d6d89e12510f

Comment 17

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8460556 [details] [diff] [review]
tabbrowser.patch

Sounds like this was resolved.

Comment 18

[David Teller [:Yoric] - still alive but not very active]

Vikneshwar, can you publish a patch with the fix I highlight in comment 16?

Comment 19

[vikneshwar]

Attachment: tabbed_final

Hai , David really sorry for delaying this patch . I was quite busy and forgot about this bug . I made changes as in comment 16 . Please , check whether its right :) Thanks :)

Comment 20

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8481831 [details] [diff] [review]
tabbed_final

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

This looks good to me.
Can you get rid of the changes to PdfStreamConverter.jsm for the moment and then request a review from dao? He's one of the owners of this part of the code, while I'm not.

::: browser/extensions/pdfjs/content/PdfStreamConverter.jsm
@@ +67,5 @@
>  function getFindBar(domWindow) {
>    var browser = getContainingBrowser(domWindow);
>    try {
>      var tabbrowser = browser.getTabBrowser();
> +    var tab = tabbrowser.getTabForBrowser(browser);

Actually, I'm not completely sure of how the development of pdf.js works, so let's not change this file for the time being. In the worst case, this will display a warning that can be fixed in another bug.

Comment 21

[vikneshwar]

Attachment: removed-pdf.js-changes

Removed changes in PDF.js

Comment 22

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8485912 [details] [diff] [review]
removed-pdf.js-changes

I don't think you need my review at this stage, only Dao's.

Comment 23

[Jim Mathies [:jimm]]

Comment on attachment 8485912 [details] [diff] [review]
removed-pdf.js-changes

Testing with this, didn't see any issues. I don't see any issues with the code either.

Comment 24

[David Teller [:Yoric] - still alive but not very active]

Dao?

Comment 25

[Dão Gottwald [:dao]]

Comment on attachment 8485912 [details] [diff] [review]
removed-pdf.js-changes

>@@ -3145,16 +3163,17 @@
>           window.addEventListener("sizemodechange", this, false);
> 
>           var uniqueId = this._generateUniquePanelID();
>           this.mPanelContainer.childNodes[0].id = uniqueId;
>           this.mCurrentTab.linkedPanel = uniqueId;
>           this.mCurrentTab._tPos = 0;
>           this.mCurrentTab._fullyOpen = true;
>           this.mCurrentTab.linkedBrowser = this.mCurrentBrowser;
>+          this._tabForBrowser.set(this.mCurrentTab.linkedBrowser, this.mCurrentTab);

nit: use this.mCurrentBrowser instead of this.mCurrentTab.linkedBrowser

looks good otherwise, thanks!

Comment 26

[David Teller [:Yoric] - still alive but not very active]

vikneshwar, can you apply Dao's feedback? Once this is done, we will launch the tests one last time and, if things go as planned, we will land your patch.

Comment 27

[vikneshwar]

Attachment: tabbrowser-final

Comment 28

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8491680 [details] [diff] [review]
tabbrowser-final

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

You didn't need to ask for my review on this.
Let's just check if it works:
 https://tbpl.mozilla.org/?tree=Try&rev=41e9be86248e

Comment 29

[David Teller [:Yoric] - still alive but not very active]

Tests don't look too good.
Now the good part is that, as far as I can tell, all the errors are
"Deprecated.warning is not a function at chrome://browser/content/tabbrowser.xml:409".

let Deprecated = Components.utils.import("resource://gre/modules/Deprecated.jsm", {});

should be replaced with

let Deprecated = Components.utils.import("resource://gre/modules/Deprecated.jsm", {}).Deprecated;

Comment 30

[Tim Taubert [:ttaubert] (inactive)]

Or:

let {Deprecated} = Components.utils.import("resource://gre/modules/Deprecated.jsm", {});

Comment 31

[vikneshwar]

Attachment: tab.patch

Comment 32

[vikneshwar]

sorry for messing up with needinfo

Comment 33

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8493885 [details] [diff] [review]
tab.patch

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

Looks good, let's see how it works: https://tbpl.mozilla.org/?tree=Try&rev=d7524168d27c

Comment 34

[David Teller [:Yoric] - still alive but not very active]

Tests look good. Vikneshwar, can you confirm that you are ready to land this?

Comment 35

[vikneshwar]

Attachment: rebased

Rebased :)

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/047f52c44212

Comment 37

[tabmix.onemen]

why you changed '_getTabForBrowser' to 'getTabForBrowser' ?

you missed few places:
gBrowser._getTabForContentWindow
browser.js > RedirectLoad
webrtcUI.jsm > webrtcUI.getActiveStreams
FxAccountsOAuthClient.jsm > FxAccountsOAuthClient.prototype._registerChannel
PdfjsChromeUtils.jsm > PdfjsFindbarWrapper
PdfStreamConverter.jsm > getFindBar


SessionStore.jsm use copy of the old _getTabForBrowser function are you going to fix it to use the new _tabForBrowser WeakMap ?

Comment 38

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/047f52c44212

Comment 39

[David Teller [:Yoric] - still alive but not very active]

(In reply to tabmix.onemen from comment #37)
> why you changed '_getTabForBrowser' to 'getTabForBrowser' ?

Because, by convention, `_` means private. This new method is meant to be public.

> you missed few places:
> gBrowser._getTabForContentWindow
> browser.js > RedirectLoad
> webrtcUI.jsm > webrtcUI.getActiveStreams
> FxAccountsOAuthClient.jsm > FxAccountsOAuthClient.prototype._registerChannel
> PdfjsChromeUtils.jsm > PdfjsFindbarWrapper
> PdfStreamConverter.jsm > getFindBar

Oh. Can you file a bug?

> SessionStore.jsm use copy of the old _getTabForBrowser function are you
> going to fix it to use the new _tabForBrowser WeakMap ?

Indeed, that's bug 1029471.

Comment 40

[Eric Shepherd [:sheppy]]

I believe the doc changes here are primarily that there is now a getTabForBrowser() method which is public, and that caling the old _getTabForBrowser() is deprecated.

getTabForBrowser() needs to be added to https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/tabbrowser -- including both adding it to the method list and adding the macro call below to insert its docs into the page.

We also need to create https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Method/getTabForBrowser.

Comment 41

[Eric Shepherd [:sheppy]]

Created:

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Method/getTabForBrowser

Updated:

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/tabbrowser

And added a mention of this change to the Firefox 35 for developers page, here:

https://developer.mozilla.org/en-US/Firefox/Releases/35#XUL

Docs should be done at this point.

Comment 42

[TheRave]

addtab results in DEPRECATION WARNING: _getTabForBrowser` is now deprecated, please use `getTabForBrowser
You may find more details about this deprecation at: https://developer.mozilla.org/docs/Mozilla/Tech/XUL/Method/getTabForBrowser

Shall we open a new bug?

Comment 43

[Dave Townsend [:mossop]]

Attachment: Fix remaining callers

Comment 44

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8502864 [details] [diff] [review]
Fix remaining callers

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

Complaining on IRC really goes get you everything you want. Lesson learnt.

In the future, can we not land deprecation notices until we fix our own common uses in the tree? This has been flooding my console so much I've been unable to use it.

Comment 45

[Dave Townsend [:mossop]]

https://hg.mozilla.org/integration/fx-team/rev/8f632f1754a7

Comment 46

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8f632f1754a7

Comment 47

[David Teller [:Yoric] - still alive but not very active]

(In reply to Blair McBride [:Unfocused] from comment #44)
> Comment on attachment 8502864 [details] [diff] [review]
> Fix remaining callers
> 
> Review of attachment 8502864 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Complaining on IRC really goes get you everything you want. Lesson learnt.
> 
> In the future, can we not land deprecation notices until we fix our own
> common uses in the tree? This has been flooding my console so much I've been
> unable to use it.

My bad, I failed to check that we had fixed all uses. I wonder how we could automatize this kind of check in the future.

Comment 48

[:Gijs (he/him)]

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #47)
> (In reply to Blair McBride [:Unfocused] from comment #44)
> > Comment on attachment 8502864 [details] [diff] [review]
> > Fix remaining callers
> > 
> > Review of attachment 8502864 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Complaining on IRC really goes get you everything you want. Lesson learnt.
> > 
> > In the future, can we not land deprecation notices until we fix our own
> > common uses in the tree? This has been flooding my console so much I've been
> > unable to use it.
> 
> My bad, I failed to check that we had fixed all uses. I wonder how we could
> automatize this kind of check in the future.

Make Deprecated.jsm's warnings fatal on debug builds?

Comment 49

[David Teller [:Yoric] - still alive but not very active]

That sounds difficult, but I filed this as bug bug 1081048, bug 1081049.

Comment 50

[David Teller [:Yoric] - still alive but not very active]

So, vikneshwar, now that this bug is resolved, do you want to handle bug 1029471?

Comment 51

[vikneshwar]

started working on it , thanks :)