750980 - Implement gBrowser.visibleTabs cache

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Frank Yan (:fryn)]

Right now, gBrowser.visibleTabs is a getter that takes O(n) time every time we use it. I suspect it's worth it to fast-path the common case where no tabs are being closed and there is only one tab group.

Where we use it in tabbrowser.xml:
https://mxr.mozilla.org/mozilla-central/search?string=visibleTabs&find=tabbrowser.xml&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Hidden tabs are as much a core feature as pinned tabs, so it might not be sufficient to determine if Panorama is used or not. We could implement a cache that is cleared when gBrowser.showOnlyTheseTabs() and gBrowser.removeTab() have been called. Not sure if it's worth the maintenance overhead.

Comment 2

[Dão Gottwald [:dao]]

We already have a cache for browsers, it's not a big deal.

Comment 3

[Dão Gottwald [:dao]]

Attachment: patch

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 620300 [details] [diff] [review]
patch

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

::: browser/base/content/tabbrowser.xml
@@ +97,5 @@
>          <getter><![CDATA[
> +          if (!this._visibleTabs)
> +            this._visibleTabs = Array.filter(this.tabs,
> +                                             function (tab) !tab.hidden && !tab.closing);
> +          return this._visibleTabs;         

Nit: there are some trailing spaces here.

@@ +2136,5 @@
>            this.mCurrentTab._selected = false;
> +
> +          // invalidate caches
> +          this._browsers = null;
> +          this._visibleTabs = null;

Do we really need to do this when calling gBrowser.moveTabTo()? Seems like .hidden/.closing attributes aren't touched.

Comment 5

[Dão Gottwald [:dao]]

(In reply to Tim Taubert [:ttaubert] from comment #4)
> Do we really need to do this when calling gBrowser.moveTabTo()? Seems like
> .hidden/.closing attributes aren't touched.

It changes the order.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 620300 [details] [diff] [review]
patch

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

(In reply to Dão Gottwald [:dao] from comment #5)
> (In reply to Tim Taubert [:ttaubert] from comment #4)
> > Do we really need to do this when calling gBrowser.moveTabTo()? Seems like
> > .hidden/.closing attributes aren't touched.
> 
> It changes the order.

Right, good catch. Looks good, then!

Comment 7

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/b83c87e1a122

Comment 8

[Frank Yan (:fryn)]

Thanks for the quick patch, Dão! :)

What I actually had in mind was having gBrowser.visibleTabs just return gBrowser.tabs when there are no closing or hidden tabs. This way we ensure that gBrowser.tabs is fast every time for most users.
With the cache, we still get the O(n) hit rather often.

Comment 9

[Dão Gottwald [:dao]]

We don't keep track of hidden tabs, so I guess we'd have to either start doing that or disable the optimization as soon as hideTab is called. Also, we can't just return 'tabs', we'd have to at least convert it to an array.

Comment 10

[Frank Yan (:fryn)]

(In reply to Dão Gottwald [:dao] from comment #9)
> disable the optimization as soon as hideTab is called.

I'm okay with that.

(In reply to Dão Gottwald [:dao] from comment #9)
> we can't just return 'tabs', we'd have to at least convert it to an array.

I'm not sure how to get around this one, since Array.slice is O(n), except by making a Proxy, but I imagine that that wouldn't be much faster, given the overhead.