404222 - getBrowserIndexForDocument caller optimization

Status: RESOLVED FIXED

(Firefox :: General, defect, P4)

Description

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: reduces number of calls to getBrowserIndexForDocument

tabbrowser.xml::getBrowserIndexForDocument shows up as one of the more costly methods in a Tp run profiled with calltime.d.

A look through its callers reveals that pageShowEventHandlers calls it on pageshow and then throws away the result, so that call can be removed.  Its second caller is DOMLinkHandler::onLinkAdded, which calls it when an icon link is added to update the icon on the tab.  That call isn't as simple to optimize, but it might not happen that frequently, either.

And its third caller is tabbrowser.xml::getBrowserForDocument, which is itself called in a variety of places, including ContentPrefSink::_handleDOMContentLoaded (which is being removed in bug 385224), in FeedHandler::onPageHide, whose functionality could be moved to the browser-specific pagehide handler in browser.xml <http://lxr.mozilla.org/mozilla/source/toolkit/content/widgets/browser.xml#450>, and in other methods that at first glance don't look like they get called very often.

After applying the patch in bug 385224, removing the unused code from pageShowEventHandlers, and moving the logic in FeedHandler::onPageHide into the browser-specific pagehide handler, getBrowserIndexForDocument costs much less.

Inclusive function elapsed times (us),
   FILE                 TYPE       NAME                                TOTAL
Before:
   tabbrowser.xml       func       getBrowserIndexForDocument        3308607
After:
   tabbrowser.xml       func       getBrowserIndexForDocument         219806

Timing the test runs with "time" also shows improvement, although these numbers are probably not reliable:

Completed test: tp

real	10m22.205s
user	5m37.308s
sys	2m2.331s

Completed test: tp

real	9m36.466s
user	5m31.259s
sys	1m56.808s

Note: a look at getBrowserAtIndex and getBrowserIndexForDocument (and by extension getBrowserForDocument) suggests they might be faster if they used the cache of browser objects in the tabbrowser's "browsers" property.  getBrowser could then be simply:

            return this.browsers[i];

While getBrowserIndexForDocument could be:

	    let browsers = this.browsers;
            for (var i = 0; i < browsers.length; i++)
              if (browsers[i].contentDocument == aDocument)
                return i;
            return -1;

But the cache gets invalidated when tabs are added or removed and possibly in some other situations as well, and it's not entirely clear when these functions get called relative to when the cache is invalidated, so I'm not sure using the cache is actually an optimization.  Further testing is needed.

Here's a patch that makes the changes measured above (excepting the changes made by the patch in bug 385224).  Requesting wanted-firefox3 for this performance improvement.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

Note: the pageShowEventHandlers code became unused once we dropped pre-Places bookmarks code in revision 1.815:

1.815	mozilla.mano%sent.com	2007-07-17 14:08	 	Bug 386392 - Drop pre places/places-bookmarks support from browser/. r=gavin.

Also, when I said "getBrowser could then be simply: |return this.browsers[i];|" I meant "getBrowserAtIndex could then be simply: |return this.browsers[aIndex]|"

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Attachment: optimizations for the methods themselves

Making the functions use the "browsers" cache does show an improvement in the Tp run:

Before:

Count,
   tabbrowser.xml       func       getBrowserForDocument                1802
   tabbrowser.xml       func       getBrowserIndexForDocument           2062
   tabbrowser.xml       func       getBrowserAtIndex                    3116
Exclusive function elapsed times (us),
   tabbrowser.xml       func       getBrowserAtIndex                  142235
   tabbrowser.xml       func       getBrowserForDocument              164047
   tabbrowser.xml       func       getBrowserIndexForDocument         295874
Inclusive function elapsed times (us),
   tabbrowser.xml       func       getBrowserAtIndex                  192200
   tabbrowser.xml       func       getBrowserIndexForDocument         884685
   tabbrowser.xml       func       getBrowserForDocument             1176728

real	9m22.465s
user	5m30.903s
sys	1m58.284s

After:

Count,
   tabbrowser.xml       func       getBrowserAtIndex                    1054
   tabbrowser.xml       func       getBrowserForDocument                1802
   tabbrowser.xml       func       getBrowserIndexForDocument           2062
Exclusive function elapsed times (us),
   tabbrowser.xml       func       getBrowserAtIndex                   41935
   tabbrowser.xml       func       getBrowserForDocument              157169
   tabbrowser.xml       func       getBrowserIndexForDocument         204464
Inclusive function elapsed times (us),
   tabbrowser.xml       func       getBrowserAtIndex                   51124
   tabbrowser.xml       func       getBrowserIndexForDocument         503894
   tabbrowser.xml       func       getBrowserForDocument              842778

real	9m20.899s
user	5m29.368s
sys	1m57.803s

Without better understanding when these methods get called relative to when the cache gets invalidated, however, it's hard for me to say whether this will speed things up in real world usage.  Nevertheless, here's a patch that makes the change.

Comment 3

[Myk Melez [:myk] [@mykmelez]]

Note: the reason the numbers in comment 2 are higher than those in comment 0 is that the numbers in comment 2 were generated without the patch in bug 385224 applied.  What matters is the relative difference between the before and after numbers in comment 2.

Comment 4

[Mike Connor [:mconnor]]

Comment on attachment 289206 [details] [diff] [review]
optimizations for the methods themselves

r=me, good catch!

Comment 5

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 289203 [details] [diff] [review]
patch v1: reduces number of calls to getBrowserIndexForDocument

Requesting approval to land this perf optimization, which does two things:

1. removes code that isn't being used since we removed pre-Places bookmarks code, which is very low risk;

2. when moving between pages, destroys the feed cache for the old page from the browser object in browser.xml's pagehide handler instead of browser.js's pagehide handler, which also seems low-risk (potential regression brainstorm: some code depends on which point in pagehide that cache gets destroyed, or some other browser.xml consumer stores something else in the "feed" property).

Comment 6

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 289206 [details] [diff] [review]
optimizations for the methods themselves

Requesting approval to land this perf optimization, which makes getBrowserAtIndex and getBrowserIndexForDocument retrieve browser objects from the browsers cache instead of the tab container.  The browsers cache <http://lxr.mozilla.org/mozilla/source/browser/base/content/tabbrowser.xml#1636> gets populated from the tab container the same way these two functions access browsers from the tab container, so this seems like a low risk and obvious consolidation of logic.

However, if there's a bug in the cache population and invalidation, then these two functions will now inherit that bug.  And it's possible (although it seems unlikely) that this could actually hurt performance in some edge cases where the cache was often being invalidated before these two methods were called.

Comment 7

[Mike Schroepfer]

Comment on attachment 289206 [details] [diff] [review]
optimizations for the methods themselves

Go go tabbrowser perf!

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Checked in both patches:

Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.895; previous revision: 1.894
done
Checking in toolkit/content/widgets/browser.xml;
/cvsroot/mozilla/toolkit/content/widgets/browser.xml,v  <--  browser.xml
new revision: 1.109; previous revision: 1.108
done

Checking in browser/base/content/tabbrowser.xml;
/cvsroot/mozilla/browser/base/content/tabbrowser.xml,v  <--  tabbrowser.xml
new revision: 1.250; previous revision: 1.249
done

Comment 9

[Stephen Donner [:stephend] Not actively reading bugmail]

Myk, is it possible that this caused bug 407116?

Comment 10

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #9)
> Myk, is it possible that this caused bug 407116?

Yes, this is responsible for that regression.  I now have a patch to fix the regression, which I've attached to bug 407116.