Closed Bug 1039500 Opened 10 years ago Closed 10 years ago

tabbrowser's _getTabForBrowser is very slow with lots of tabs

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 35

People

(Reporter: Yoric, Assigned: lviknesh, Mentored)

References

Details

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

Attachments

(2 files, 6 obsolete files)

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...
Blocks: 1039506
Summary: tabbrowser's _getTabForBrowser is very slow → tabbrowser's _getTabForBrowser is very slow with lots of tabs
Assignee: nobody → lviknesh
Whiteboard: [lang=js][good first bug] → [lang=js][good first bug][put doc at https://developer.mozilla.org/docs/Mozilla/Tech/XUL/Method/getTabForBrowser]
Attached patch tabbed-browser (obsolete) — Splinter Review
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
Attached patch tabbrowser.patch (obsolete) — Splinter Review
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)
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)
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?
Attached patch tabbed_final (obsolete) — Splinter Review
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+
Attached patch removed-pdf.js-changes (obsolete) — Splinter Review
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)
Attachment #8485912 - Flags: feedback?(jmathies)
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)
Attached patch tabbrowser-final (obsolete) — Splinter Review
Attachment #8485912 - Attachment is obsolete: true
Attachment #8491680 - Flags: review?(dteller)
Flags: needinfo?(lviknesh)
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", {});
Attached patch tab.patch (obsolete) — Splinter Review
Attachment #8491680 - Attachment is obsolete: true
Flags: needinfo?(dteller)
Flags: needinfo?(dteller)
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)
Flags: needinfo?(lviknesh)
Attached patch rebasedSplinter Review
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]
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
Closed: 10 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.
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
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?
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+
Blocks: 1080903
(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.
So, vikneshwar, now that this bug is resolved, do you want to handle bug 1029471?
Flags: needinfo?(lviknesh)
started working on it , thanks :)
Flags: needinfo?(lviknesh)
See Also: → 1193238
You need to log in before you can comment on or make changes to this bug.