Closed Bug 896114 Opened 8 years ago Closed 8 years ago

UserAgentOverrides take ~9% of pageload time on a smugmug gallery

Categories

(Core :: Networking, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: dougt, Assigned: dao)

References

Details

(Keywords: perf)

Attachments

(1 file, 1 obsolete file)

on smugmug, a photo site, i have a page that has 6000 galleries.

This function accounts for ~8.9% of page load time.  (UserAgentOverrides.jsm)

  function (aHttpChannel) UserAgentOverrides.getOverrideForURI(aHttpChannel.URI)

OMGWTFBBQ?
Welcome to the wonderful world of JS components!

This was obviously a worry when we were doing bug 782453, but I'm a little surprised that it's being _that_ big a hit.  I assume this is an opt build?  How long does each call take?

It might be that we need something that tells necko which urls overrides _might_ exist for so we can optimize out the call.  :(
(In reply to Boris Zbarsky (:bz) from comment #1)
> Welcome to the wonderful world of JS components!

I'm not sure that's the biggest issue:

    let host = aURI.asciiHost;
    for (let domain in gOverrides) {
      if (host == domain ||
          host.endsWith("." + domain)) {
        return gOverrides[domain];
      }
    }

The worst case is the case where there is no override for the site. Then, we have to search through the entire list, doing a bunch of string comparisons. It seems like it would be very simple to optimize this into a binary search. The dumbest thing I can think of would be to store the overrides in an array, sorted by reverse(x) where x is the domain name in the override. Then you can do a binary suffix search.

if that were not enough, it would not be hard to write this to do a bsearch() in a hard-coded C++ array in C++, instead of search of a JS array that is read from a pref. We do this hard-coded C++ stuff in PSM:
https://mxr.mozilla.org/mozilla-central/source/security/manager/boot/src/nsSTSPreloadList.inc
https://mxr.mozilla.org/mozilla-central/source/security/manager/tools/getHSTSPreloadList.js

C++ would, of course, eliminate the XPConnect.
> Then, we have to search through the entire list

Sure, but which product are the numbers from comment 0 from?  gOverrides is the empty list on everything except b2g, as far as I can tell.

Doug, whare _are_ those numbers from?
Flags: needinfo?(doug.turner)
(In reply to Boris Zbarsky (:bz) from comment #3)
> > Then, we have to search through the entire list
> 
> Sure, but which product are the numbers from comment 0 from?  gOverrides is
> the empty list on everything except b2g, as far as I can tell.

Not to state the obvious, but we should stop loading this component in products that don't need it.
I am using smugmug.  I have 6000 'galleries' in one place.  When loading this page that lists the 6000 galleries, Firefox hangs.

So, this page has at least 6000 images.  So, not your typical website.  But I would not expect this simple bit of functionality to even show up.


What bsmith says -- we must do much better than this.  If gOverrides is empty -- we should not register this code.  We should also move this crap out of jsm.  Does that sound right bz?
Flags: needinfo?(doug.turner)
Dao,

This make a big improvement on firefox desktop.  It doesn't fix it when we have overrides.  That either can be done here, or a follow up.  Please review.
Attachment #778980 - Flags: review?(dao)
okay, dumb question -- where is site_specific_overrides used?  In bug 782453, it looks like it was used for aol...  but I don't see any overrides in mxr.  am i missing something?
Flags: needinfo?(dao)
http://mxr.mozilla.org/gaia/search?string=general.useragent.override.&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=gaia

Looks like various Google properties, live.com, yahoo, msn, linkedin, tumblr, orkut, bing, ebay, groupon, etc, etc, etc, etc.  I do wonder how much time those linear searches take against that list.  :(
...and we do this on really slow hardware.
(In reply to Doug Turner (:dougt) from comment #7)
> okay, dumb question -- where is site_specific_overrides used?

That pref was added for testing / QA. See bug 782453 comment 18.
Flags: needinfo?(dao)
Comment on attachment 778980 [details] [diff] [review]
part 1. don't do work if we don't have to

>@@ -33,14 +35,30 @@ this.UserAgentOverrides = {

>+  registerWithNecko: function uoa_registerWithNecko() {    

>+  unregisterWithNecko: function uoa_unregisterWithNecko() {

>+    unregisterWithNecko();

>+    registerWithNecko();

You implemented registerWithNecko / unregisterWithNecko as methods of the exported UserAgentOverrides object, but you're calling them as if they existed as functions in the jsm's global scope. This isn't going to work. You should actually implement them as functions in the global scope.

>+  if (gOverrides != {}) {

This is always going to be true, as two objects are never equal even if both are empty, i.e. ({} == {}) is false.
Attachment #778980 - Flags: review?(dao) → review-
Blocks: 896378
I filed bug 896378 for b2g so we can keep this bug focused on not observing http-on-modify-request if it's not needed.
Keywords: perf
No longer blocks: 896378
(In reply to Dão Gottwald [:dao] from comment #12)
> I filed bug 896378 for b2g so we can keep this bug focused on not observing
> http-on-modify-request if it's not needed.

Isn't the simplest thing to simply not ship this code at all on Desktop (and Android?).
yes.

but we need to fix it for realz for b2g and other users.  maybe metro.
(In reply to Justin Dolske [:Dolske] from comment #13)
> (In reply to Dão Gottwald [:dao] from comment #12)
> > I filed bug 896378 for b2g so we can keep this bug focused on not observing
> > http-on-modify-request if it's not needed.
> 
> Isn't the simplest thing to simply not ship this code at all on Desktop (and
> Android?).

Yes, the original idea was that the module shouldn't be loaded in the first place when not needed. We can do that on desktop. It's actually used on Android.
Assignee: doug.turner → dao
Also made getOverrideForURI return early if gOverrides is empty, although it doesn't affect desktop anymore.
Attachment #778980 - Attachment is obsolete: true
Attachment #781555 - Flags: review?(dolske)
(In reply to Dão Gottwald [:dao] from comment #16)
> Created attachment 781555 [details] [diff] [review]
> patch: don't initialize UserAgentOverrides on desktop
> 
> Also made getOverrideForURI return early if gOverrides is empty, although it
> doesn't affect desktop anymore.

It will help on Android phones once I fixed bug 788422, as Android needs its youtube override only on tablets.
Attachment #781555 - Flags: review?(dolske) → review+
https://hg.mozilla.org/mozilla-central/rev/76a31c4050cc
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Depends on: 933959
You need to log in before you can comment on or make changes to this bug.