532552 - There is no need to load nsBlocklistService.js on startup

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Robert Strong (they/them - no direct email)]

I've seen time spent in nsBlocklistService.js on WinCE Firefox during startup of around 350 ms and there is no need for nsBlocklistService.js to load on startup so ima gonna make it not load during startup. ;)

Comment 1

[Robert Strong (they/them - no direct email)]

Attachment: patch in progress

Comment 2

[Mike Beltzner [:beltzner, not reading bugmail]]

Pre-approval to land on mozilla-central when a reviewed patch is ready.

Comment 3

[Robert Strong (they/them - no direct email)]

Attachment: patch rev1

Dave, this pretty well covers it. This might make it for 3.6 so if you could review this at your earliest convenience I'd appreciate it. I have a dentist appt in the morning and will come up with startup numbers on Firefox WinCE after I get in to the office.

Comment 4

[Robert Strong (they/them - no direct email)]

btw: if you prefer I can go with a simpler patch but this seems like the right thing to do.

Comment 5

[Dave Townsend [:mossop]]

Comment on attachment 415816 [details] [diff] [review]
patch rev1

Looks pretty good with one minor issue. Any chance you could also knock up an automated test for bug 469806 at the same time, I'd like for us to be sure that we aren't accidentally changing the blocklist URL whenever we make changes to the component. It really just needs an xpcshell tests with some hardcoded urls for each platform that checks that those were the urls requested.

>+XPCOMUtils.defineLazyGetter(this, "gLoggingEnabled", function bls_gLoggingEnabled() {
>+  return getPref("getBoolPref", PREF_EM_LOGGING_ENABLED, false);
>+});
>+
>+XPCOMUtils.defineLazyGetter(this, "gBlocklistEnabled", function bls_gBlocklistEnabled() {
>+  return getPref("getBoolPref", PREF_BLOCKLIST_ENABLED, true);
>+});
>+
>+XPCOMUtils.defineLazyGetter(this, "gBlocklistLevel", function bls_gBlocklistLevel() {
>+  return Math.min(getPref("getIntPref", PREF_BLOCKLIST_LEVEL, DEFAULT_LEVEL),
>+                  MAX_BLOCK_LEVEL);
>+});

There is a small chance that if these preferences were modified before the blocklist service ever read them then we'd try to set a value on the getter which would throw an error. You need to define a setter for each of them too that removes the getter first, see f.e. http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#114

Comment 6

[Robert Strong (they/them - no direct email)]

Attachment: patch rev2

I think it is a tad cleaner to not use getters for those three.

I'll take a look at bug 469806 after I get some perf numbers with and without this patch.

Comment 7

[Robert Strong (they/them - no direct email)]

Some initial numbers that are a little noisy... still looks like a guaranteed win even with the noise

log-0                   | Before | After  | Change
nsBlocklistService.js   | 266 ms |   0 ms | -266 ms
nsExtensionManager.js   | 508 ms | 538 ms |  +30 ms
nsUpdateTimerManager.js |  66 ms |  34 ms |  -32 ms
nsUpdateServiceStub.js  |  16 ms |  16 ms |    0 ms
Total                   | 856 ms | 578 ms | -278 ms

log-1                   | Before | After  | Change
nsBlocklistService.js   | 252 ms |   0 ms | -252 ms
nsExtensionManager.js   | 518 ms | 429 ms |  -89 ms
nsUpdateTimerManager.js |  49 ms |  30 ms |  -19 ms
nsUpdateServiceStub.js  |  16 ms |  15 ms |   -1 ms
Total                   | 835 ms | 474 ms | -361 ms

log-2                   | Before | After  | Change
nsBlocklistService.js   | 260 ms |   0 ms | -260 ms
nsExtensionManager.js   | 534 ms | 508 ms |  -26 ms
nsUpdateTimerManager.js |  67 ms |  31 ms |  -36 ms
nsUpdateServiceStub.js  |  15 ms |  12 ms |   -3 ms
Total                   | 876 ms | 551 ms | -325 ms

Comment 8

[Robert Strong (they/them - no direct email)]

Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/587962048b99

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 415898 [details] [diff] [review]
patch rev2

a192=beltzner

(every release I take a checkin late in the game that I feel nervous about; this is that checkin, in case folks are wondering!)

Comment 10

[Shawn Wilsher :sdwilsh]

>+XPCOMUtils.defineLazyGetter(this, "gPref", function bls_gPref() {
>+  return Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefService).
>+         QueryInterface(Ci.nsIPrefBranch2);
>+});
This could have been:
XPCOMUtils.defineLazyServiceGetter(this, "gPref",
                                   "@mozilla.org/preferences-service;1",
                                   "nsIPrefBranch2");

>+XPCOMUtils.defineLazyGetter(this, "gApp", function bls_gApp() {
>+  return Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULAppInfo).
>+         QueryInterface(Ci.nsIXULRuntime);
>+});
This could have been:
XPCOMUtils.defineLazyServiceGetter(this, "gApp",
                                   "@mozilla.org/xre/app-info;1",
                                   "nsIXULRuntime");


>+function getObserverService() {
>+  return Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService);
>+}
Why didn't you make this a lazy getter?

Comment 11

[Robert Strong (they/them - no direct email)]

(In reply to comment #10)
> 
> >+function getObserverService() {
> >+  return Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService);
> >+}
> Why didn't you make this a lazy getter?
Only used once on startup and once on shutdown so it didn't seem worth it.

Comment 12

[Robert Strong (they/them - no direct email)]

(In reply to comment #10)
> >+XPCOMUtils.defineLazyGetter(this, "gPref", function bls_gPref() {
> >+  return Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefService).
> >+         QueryInterface(Ci.nsIPrefBranch2);
> >+});
> This could have been:
> XPCOMUtils.defineLazyServiceGetter(this, "gPref",
>                                    "@mozilla.org/preferences-service;1",
>                                    "nsIPrefBranch2");
I recall trying this and it failing... I'll verify when I have the time to do so.

> >+XPCOMUtils.defineLazyGetter(this, "gApp", function bls_gApp() {
> >+  return Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULAppInfo).
> >+         QueryInterface(Ci.nsIXULRuntime);
> >+});
> This could have been:
> XPCOMUtils.defineLazyServiceGetter(this, "gApp",
>                                    "@mozilla.org/xre/app-info;1",
>                                    "nsIXULRuntime");
Didn't try this one but I will when I have the time to do so.

Comment 13

[Shawn Wilsher :sdwilsh]

(In reply to comment #12)
> I recall trying this and it failing... I'll verify when I have the time to do
> so.
I know it works with nsIPrefBranch, so I'd expect it to work with nsIPrefBranch2 (unless it has a strange QueryInterface).

Comment 14

[Robert Strong (they/them - no direct email)]

Pushed to mozilla-1.9.2
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/b343b3557268

Comment 15

[Dave Townsend [:mossop]]

(In reply to comment #12)
> (In reply to comment #10)
> > >+XPCOMUtils.defineLazyGetter(this, "gPref", function bls_gPref() {
> > >+  return Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefService).
> > >+         QueryInterface(Ci.nsIPrefBranch2);
> > >+});
> > This could have been:
> > XPCOMUtils.defineLazyServiceGetter(this, "gPref",
> >                                    "@mozilla.org/preferences-service;1",
> >                                    "nsIPrefBranch2");
> I recall trying this and it failing... I'll verify when I have the time to do
> so.

Should work since nsIPrefBranch2 extends nsIPrefBranch

> > >+XPCOMUtils.defineLazyGetter(this, "gApp", function bls_gApp() {
> > >+  return Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULAppInfo).
> > >+         QueryInterface(Ci.nsIXULRuntime);
> > >+});
> > This could have been:
> > XPCOMUtils.defineLazyServiceGetter(this, "gApp",
> >                                    "@mozilla.org/xre/app-info;1",
> >                                    "nsIXULRuntime");
> Didn't try this one but I will when I have the time to do so.

Probably won't work since nsIXULRuntime and nsIXULAppInfo are unrelated interfaces, unless that component happens to implement nsIClassInfo properly

Comment 16

[Robert Strong (they/them - no direct email)]

(In reply to comment #15)
> (In reply to comment #12)
> > (In reply to comment #10)
>...
> > > >+XPCOMUtils.defineLazyGetter(this, "gApp", function bls_gApp() {
> > > >+  return Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULAppInfo).
> > > >+         QueryInterface(Ci.nsIXULRuntime);
> > > >+});
> > > This could have been:
> > > XPCOMUtils.defineLazyServiceGetter(this, "gApp",
> > >                                    "@mozilla.org/xre/app-info;1",
> > >                                    "nsIXULRuntime");
> > Didn't try this one but I will when I have the time to do so.
> 
> Probably won't work since nsIXULRuntime and nsIXULAppInfo are unrelated
> interfaces, unless that component happens to implement nsIClassInfo properly
Actually, I recall this from a while back

Comment 17

[Robert Strong (they/them - no direct email)]

I borrowed dolske's tegra and logged startup on it with the following results... they were much less noisy than the oban board. I expected some of the time initializing services to transfer to the next component that runs during startup which is the extension manager and saw exactly that in these results (don't know why this wasn't seen when using the oban board).

log-0                   | Before | After  | Change
nsBlocklistService.js   | 110 ms |   0 ms | -110 ms
nsExtensionManager.js   | 153 ms | 194 ms |  +41 ms
nsUpdateTimerManager.js |  19 ms |  24 ms |   +5 ms
nsUpdateServiceStub.js  |   8 ms |  10 ms |   +2 ms
Total                   | 290 ms | 228 ms |  -62 ms

log-1                   | Before | After  | Change
nsBlocklistService.js   | 112 ms |   0 ms | -112 ms
nsExtensionManager.js   | 146 ms | 191 ms |  +45 ms
nsUpdateTimerManager.js |  21 ms |  23 ms |   +2 ms
nsUpdateServiceStub.js  |  12 ms |  10 ms |   -2 ms
Total                   | 291 ms | 224 ms |  -67 ms

log-2                   | Before | After  | Change
nsBlocklistService.js   | 118 ms |   0 ms | -118 ms
nsExtensionManager.js   | 146 ms | 191 ms |  +45 ms
nsUpdateTimerManager.js |  18 ms |  20 ms |   +2 ms
nsUpdateServiceStub.js  |   9 ms |   8 ms |   -1 ms
Total                   | 291 ms | 219 ms |  -72 ms

The tegra is faster than the oban so the total TS millisecond win is less but even so there is still a nice win on WinCE (I suspect maemo as well) with this patch. I'll zip up and attach the logs to this bug. There are 30 without the patch and 30 with the patch.

Comment 18

[Robert Strong (they/them - no direct email)]

Attachment: WinCE Firefox logs

Comment 19

[Robert Strong (they/them - no direct email)]

Looks like this did have a slight win on desktop as well... I thought it would be too small and be inside the noise.
Talos Improvement: Ts, MAX Dirty Profile decrease 0.53% on Linux Firefox3.6

Comment 20

[Robert Strong (they/them - no direct email)]

(In reply to comment #15)
> (In reply to comment #12)
> > (In reply to comment #10)
> > > >+XPCOMUtils.defineLazyGetter(this, "gPref", function bls_gPref() {
> > > >+  return Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefService).
> > > >+         QueryInterface(Ci.nsIPrefBranch2);
> > > >+});
> > > This could have been:
> > > XPCOMUtils.defineLazyServiceGetter(this, "gPref",
> > >                                    "@mozilla.org/preferences-service;1",
> > >                                    "nsIPrefBranch2");
> > I recall trying this and it failing... I'll verify when I have the time to do
> > so.
> 
> Should work since nsIPrefBranch2 extends nsIPrefBranch
It doesn't... the calls to getDefaultBranch fail.

TEST-UNEXPECTED-FAIL | (xpcshell/head.js) | [Exception... "'[JavaScript Error: "gPref.getDefaultBranch is not a function" {file: "file:///d:/moz/_1_mozilla-central/ff-dbg/dist/bin/components/nsBlocklistService.js" line: 214}]' when calling method: [nsITimerCallback::notify]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: d:/moz/_1_mozilla-central/ff-dbg/_tests/xpcshell/test_extensionmanager/unit/test_bug455906.js :: load_blocklist :: line 238"  data: yes]

Comment 21

[:Gavin Sharp [email: [e-mail]]]

Yeah, nsIPrefBranch2 inherits from nsIPrefBranch, but it doesn't (and shouldn't, conceptually) inherit from nsIPrefService, so you need two separate QIs. Would be nice if defineLazyServiceGetter supported that, I guess, but it's a bit of an edge case I guess.

Comment 22

[Shawn Wilsher :sdwilsh]

(In reply to comment #21)
> Yeah, nsIPrefBranch2 inherits from nsIPrefBranch, but it doesn't (and
> shouldn't, conceptually) inherit from nsIPrefService, so you need two separate
> QIs. Would be nice if defineLazyServiceGetter supported that, I guess, but it's
> a bit of an edge case I guess.
What we probably want to do is make a Prefs.jsm that presents a better API around our XPCOM one like NetUtil.jsm does.