tabbrowser's _getTabForBrowser is very slow with lots of tabs

RESOLVED FIXED in Firefox 35

Status

()

Firefox
Tabbed Browser
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: Yoric, Assigned: vikneshwar, Mentored)

Tracking

({dev-doc-complete, perf})

unspecified
Firefox 35
dev-doc-complete, perf
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=js][good first bug][put doc at https://developer.mozilla.org/docs/Mozilla/Tech/XUL/Method/getTabForBrowser])

Attachments

(2 attachments, 6 obsolete attachments)

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.
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? :-)
This is clearly the objective. See blocked bug.
(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.
I don't really see how it could break it.
I thought that method might be replacing the browser, but it just removes it and inserts it again...

Updated

3 years ago
Blocks: 1039506

Updated

3 years ago
Summary: tabbrowser's _getTabForBrowser is very slow → tabbrowser's _getTabForBrowser is very slow with lots of tabs
(Assignee)

Updated

3 years ago
Assignee: nobody → lviknesh
Keywords: dev-doc-needed
Whiteboard: [lang=js][good first bug] → [lang=js][good first bug][put doc at https://developer.mozilla.org/docs/Mozilla/Tech/XUL/Method/getTabForBrowser]
(Assignee)

Comment 6

3 years ago
Created attachment 8458963 [details] [diff] [review]
tabbed-browser

try push failed https://tbpl.mozilla.org/?tree=Try&rev=2a3fe995f370
Attachment #8458963 - Flags: feedback?(dteller)
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
(Assignee)

Comment 8

3 years ago
Created attachment 8460556 [details] [diff] [review]
tabbrowser.patch

Try push https://tbpl.mozilla.org/?tree=Try&rev=3c088ea38495
Attachment #8458963 - Attachment is obsolete: true
Attachment #8458963 - Flags: feedback?(dteller)
Attachment #8460556 - Flags: feedback?(dteller)
(Assignee)

Comment 9

3 years ago
Try push with latest pull https://tbpl.mozilla.org/?tree=Try&rev=babebf5e65a
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?
Attachment #8460556 - Flags: feedback?(felipc)
Attachment #8460556 - Flags: feedback?(dteller)
Attachment #8460556 - Flags: feedback+
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
Attachment #8460556 - Flags: feedback?(felipc) → feedback?(wmccloskey)
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.
Bill, does your comment on bug 1039506 apply here, too?
Flags: needinfo?(wmccloskey)
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.
Flags: needinfo?(wmccloskey)

Comment 15

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

then you can use getElementById
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 on attachment 8460556 [details] [diff] [review]
tabbrowser.patch

Sounds like this was resolved.
Attachment #8460556 - Flags: feedback?(wmccloskey)
Vikneshwar, can you publish a patch with the fix I highlight in comment 16?
(Assignee)

Comment 19

3 years ago
Created attachment 8481831 [details] [diff] [review]
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 :)
Attachment #8460556 - Attachment is obsolete: true
Attachment #8481831 - Flags: review?(dteller)
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.
Attachment #8481831 - Flags: review?(dteller) → feedback+
(Assignee)

Comment 21

3 years ago
Created attachment 8485912 [details] [diff] [review]
removed-pdf.js-changes

Removed changes in PDF.js
Attachment #8481831 - Attachment is obsolete: true
Attachment #8485912 - Flags: review?(dao)
Flags: needinfo?(dteller)
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.
Attachment #8485912 - Flags: review?(dao)
Flags: needinfo?(dteller)
Attachment #8485912 - Flags: review?(dao)

Updated

3 years ago
Attachment #8485912 - Flags: feedback?(jmathies)

Comment 23

3 years ago
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.
Attachment #8485912 - Flags: feedback?(jmathies) → feedback+
Dao?
Flags: needinfo?(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!
Attachment #8485912 - Flags: review?(dao) → review+
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.
Flags: needinfo?(dao) → needinfo?(lviknesh)
(Assignee)

Comment 27

3 years ago
Created attachment 8491680 [details] [diff] [review]
tabbrowser-final
Attachment #8485912 - Attachment is obsolete: true
Attachment #8491680 - Flags: review?(dteller)
Flags: needinfo?(lviknesh)
(Assignee)

Updated

3 years ago
Flags: needinfo?(dteller)
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
Attachment #8491680 - Flags: review?(dteller)
Flags: needinfo?(dteller)
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;
Or:

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

Comment 31

3 years ago
Created attachment 8493885 [details] [diff] [review]
tab.patch
Attachment #8491680 - Attachment is obsolete: true
Flags: needinfo?(dteller)
(Assignee)

Updated

3 years ago
Flags: needinfo?(dteller)
(Assignee)

Comment 32

3 years ago
sorry for messing up with needinfo
Flags: needinfo?(dteller)
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
Attachment #8493885 - Flags: review+
Flags: needinfo?(dteller)
Tests look good. Vikneshwar, can you confirm that you are ready to land this?
Flags: needinfo?(lviknesh)
Keywords: checkin-needed
(Assignee)

Updated

3 years ago
Flags: needinfo?(lviknesh)
(Assignee)

Comment 35

3 years ago
Created attachment 8500151 [details] [diff] [review]
rebased

Rebased :)
Attachment #8493885 - Attachment is obsolete: true
https://hg.mozilla.org/integration/fx-team/rev/047f52c44212
Keywords: checkin-needed
Whiteboard: [lang=js][good first bug][put doc at https://developer.mozilla.org/docs/Mozilla/Tech/XUL/Method/getTabForBrowser] → [lang=js][good first bug][put doc at https://developer.mozilla.org/docs/Mozilla/Tech/XUL/Method/getTabForBrowser][fixed-in-fx-team]

Comment 37

3 years ago
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 ?
https://hg.mozilla.org/mozilla-central/rev/047f52c44212
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Whiteboard: [lang=js][good first bug][put doc at https://developer.mozilla.org/docs/Mozilla/Tech/XUL/Method/getTabForBrowser][fixed-in-fx-team] → [lang=js][good first bug][put doc at https://developer.mozilla.org/docs/Mozilla/Tech/XUL/Method/getTabForBrowser]
Target Milestone: --- → Firefox 35
(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

3 years ago
dev-doc-info
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.
Depends on: 1078665
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.
Keywords: dev-doc-needed → dev-doc-complete

Comment 42

3 years ago
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?
Created attachment 8502864 [details] [diff] [review]
Fix remaining callers
Attachment #8502864 - Flags: review?(bmcbride)
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.
Attachment #8502864 - Flags: review?(bmcbride) → review+
https://hg.mozilla.org/integration/fx-team/rev/8f632f1754a7
Blocks: 1080903
https://hg.mozilla.org/mozilla-central/rev/8f632f1754a7
(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.
(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?
That sounds difficult, but I filed this as bug bug 1081048, bug 1081049.
Blocks: 1084158
So, vikneshwar, now that this bug is resolved, do you want to handle bug 1029471?
Flags: needinfo?(lviknesh)
(Assignee)

Comment 51

3 years ago
started working on it , thanks :)
Flags: needinfo?(lviknesh)

Updated

2 years ago
See Also: → bug 1193238
You need to log in before you can comment on or make changes to this bug.