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

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Doug Turner (:dougt)]

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?

Comment 1

[Boris Zbarsky [:bzbarsky]]

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.  :(

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(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.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> 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?

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(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.

Comment 5

[Doug Turner (:dougt)]

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?

Comment 6

[Doug Turner (:dougt)]

Attachment: part 1. don't do work if we don't have to

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.

Comment 7

[Doug Turner (:dougt)]

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?

Comment 8

[Boris Zbarsky [:bzbarsky]]

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.  :(

Comment 9

[Doug Turner (:dougt)]

...and we do this on really slow hardware.

Comment 10

[Dão Gottwald [:dao]]

(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.

Comment 11

[Dão Gottwald [: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.

Comment 12

[Dão Gottwald [:dao]]

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.

Comment 13

[Justin Dolske [:Dolske]]

(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?).

Comment 14

[Doug Turner (:dougt)]

yes.

but we need to fix it for realz for b2g and other users.  maybe metro.

Comment 15

[Dão Gottwald [:dao]]

(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.

Comment 16

[Dão Gottwald [:dao]]

Attachment: patch: don't initialize UserAgentOverrides on desktop

Also made getOverrideForURI return early if gOverrides is empty, although it doesn't affect desktop anymore.

Comment 17

[Dão Gottwald [:dao]]

(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.

Comment 18

[Dão Gottwald [:dao]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/76a31c4050cc

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/76a31c4050cc