Last Comment Bug 750980 - Implement gBrowser.visibleTabs cache
: Implement gBrowser.visibleTabs cache
Status: RESOLVED FIXED
: perf
Product: Firefox
Classification: Client Software
Component: Tabbed Browser (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 15
Assigned To: Dão Gottwald [:dao]
:
: Dão Gottwald [:dao]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-01 18:13 PDT by Frank Yan (:fryn)
Modified: 2012-05-03 11:51 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (7.01 KB, patch)
2012-05-02 07:01 PDT, Dão Gottwald [:dao]
ttaubert: review+
Details | Diff | Splinter Review

Description Frank Yan (:fryn) 2012-05-01 18:13:14 PDT
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] 2012-05-02 05:26:19 PDT
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] 2012-05-02 07:01:03 PDT
We already have a cache for browsers, it's not a big deal.
Comment 3 Dão Gottwald [:dao] 2012-05-02 07:01:32 PDT
Created attachment 620300 [details] [diff] [review]
patch
Comment 4 Tim Taubert [:ttaubert] 2012-05-02 07:17:27 PDT
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] 2012-05-02 07:19:57 PDT
(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] 2012-05-02 07:21:14 PDT
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] 2012-05-02 11:41:21 PDT
http://hg.mozilla.org/mozilla-central/rev/b83c87e1a122
Comment 8 Frank Yan (:fryn) 2012-05-03 10:58:53 PDT
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] 2012-05-03 11:07:45 PDT
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) 2012-05-03 11:51:24 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.