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

RESOLVED FIXED in mozilla25

Status

()

defect
RESOLVED FIXED
6 years ago
Last year

People

(Reporter: dougt, Assigned: dao)

Tracking

(Depends on 1 bug, {perf})

unspecified
mozilla25
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

6 years ago
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.
Reporter

Comment 5

6 years ago
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)
Reporter

Comment 6

6 years ago
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)
Reporter

Comment 7

6 years ago
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.  :(
Reporter

Comment 9

6 years ago
...and we do this on really slow hardware.
Assignee

Comment 10

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

Comment 11

6 years ago
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-
Assignee

Updated

6 years ago
Blocks: 896378
Assignee

Comment 12

6 years ago
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.
Assignee

Updated

6 years ago
Keywords: perf
Assignee

Updated

6 years ago
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?).
Reporter

Comment 14

6 years ago
yes.

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

Comment 15

6 years ago
(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

Updated

6 years ago
Assignee: doug.turner → dao
Assignee

Comment 16

6 years ago
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)
Assignee

Comment 17

6 years ago
(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: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25

Updated

6 years ago
Depends on: 933959
You need to log in before you can comment on or make changes to this bug.