Implement gBrowser.visibleTabs cache

RESOLVED FIXED in Firefox 15

Status

()

Firefox
Tabbed Browser
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: fryn, Assigned: dao)

Tracking

({perf})

Trunk
Firefox 15
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
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
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.
(Assignee)

Comment 2

5 years ago
We already have a cache for browsers, it's not a big deal.
Summary: fast-path gBrowser.visibleTabs when Panorama isn't being used and no tab is closing → Implement gBrowser.visibleTabs cache
(Assignee)

Comment 3

5 years ago
Created attachment 620300 [details] [diff] [review]
patch
Assignee: nobody → dao
Status: NEW → ASSIGNED
Attachment #620300 - Flags: review?(ttaubert)
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.
(Assignee)

Comment 5

5 years ago
(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 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!
Attachment #620300 - Flags: review?(ttaubert) → review+
(Assignee)

Comment 7

5 years ago
http://hg.mozilla.org/mozilla-central/rev/b83c87e1a122
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 15
(Reporter)

Comment 8

5 years ago
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.
(Assignee)

Comment 9

5 years ago
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.
(Reporter)

Comment 10

5 years ago
(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.
You need to log in before you can comment on or make changes to this bug.