471219 - Store timer registrations somewhere permanent

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Dave Townsend [:mossop]]

If we were to store timer registrations somewhere permanent (prefs, category manager, db) then we would not longer need to load components during app-startup purely to register with the timer manager. This basically only needs to be store a mapping between timer id and contract id for a service to load when the timer fires.

Comment 1

[(dormant account)]

More background info:
Some components like ExtensionManager and UpdateService need giant js files loaded and then proceed to only register with a timer for and do a few other misc things. Timer registration of components should save around 300ms(that's how long it takes to load those files into js interpreter and register components) of startup time for Fennec on N810. There may be some memory savings too.

Comment 2

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

(In reply to comment #1)
> More background info:
> Some components like ExtensionManager and UpdateService need giant js files
> loaded and then proceed to only register with a timer for and do a few other
> misc things. Timer registration of components should save around 300ms(that's
> how long it takes to load those files into js interpreter and register
> components) of startup time for Fennec on N810. There may be some memory
> savings too.
Is there data yet on what the cost is of loading these components via fastload without the extra work the components perform?

Comment 3

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

btw: I think we should do this gut I am concerned about the startup time savings won't be anywhere near what is suggested. For example, the ExtensionManager is called during startup to check for new / removed / etc. add-ons so if fast load is doing what it is should be doing it is already loaded.

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Also many of these things will be needed at some point "soon", maybe at worst when the timer fires.. so getting it out of the initial startup path doesn't save us much, if we then freeze right after when the timer fires.

Comment 5

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

Looked into this a bit tonight and think that out of the suggested methods of storing the registration (prefs, category manager, db) that prefs would be the best since the other require additional work by the callsite that wants to register. Looking at the current callsites it looks like

nsMicrosummaryService.js - would need slight changes including implementing nsITimerCallback beyond removing the call to register

nsSearchService.js - would need to implement nsITimerCallback

PlacesDBUtils.jsm - I'm not sure how this could take part in this as it stands. Perhaps the consumer of PlacesDBUtils.jsm could implement nsITimerCallback, etc.

The following shouldn't be a problem
nsExtensionManager.js.in
nsBlocklistService.js
nsUpdateService.js.in

Comment 6

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

I changed my mind and went with the category manager. Also, I just about have this finished.

Comment 7

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

Attachment: patch in progress rev1

There are some changes in this patch that aren't really needed and I'll resubmit after I've removed them.

Comment 8

[Shawn Wilsher :sdwilsh]

For all of your lazy getters, you should be using XPCOMUtils.defineLazy[Service]Getter.  See http://shawnwilsher.com/archives/319 for more details.

Comment 9

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

Attachment: patch in progress rev2

Comment 10

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

Comment on attachment 399837 [details] [diff] [review]
patch in progress rev2

note: there is additional cleanup that can be done but for some odd reason I'm getting leaks with them so I am going to hold off on it and do the additional cleanup in another bug.

Comment 11

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

Comment on attachment 399837 [details] [diff] [review]
patch in progress rev2

I'll followup shortly with tests.

Comment 12

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

Attachment: basic tests

I'll add more tests later.

Comment 13

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

Comment on attachment 399837 [details] [diff] [review]
patch in progress rev2

>+  notify: function TM_notify(timer) {
>+    const Cm = Components.manager;
>+    var now = Math.round(Date.now() / 1000);
>+    var catMan = Cc["@mozilla.org/categorymanager;1"].
>+                 getService(Ci.nsICategoryManager);
>+    var entries = catMan.enumerateCategory(CATEGORY_UPDATE_TIMER);
>+    while (entries.hasMoreElements()) {
>+      let entry = entries.getNext().QueryInterface(Ci.nsISupportsCString).data;
>+      let value = catMan.getCategoryEntry(CATEGORY_UPDATE_TIMER, entry);
>+      let [timerCID, timerID, prefInterval, defaultInterval] = value.split(",");
>+      defaultInterval = parseInt(defaultInterval);
>+      // timerCID is validated below when calling notify.
>+      if (!timerID || !prefInterval ||
note: I've removed the check that prefInterval is provided in case the consumer doesn't want to provide a pref.

Comment 14

[Dave Townsend [:mossop]]

All of the ifdefs MOZ_UPDATER make nsUpdateService pretty confusing. Where are we shipping without that enabled? Would it make more sense to split the timer manager out into its own file to avoid having to do that?

Comment 15

[Dave Townsend [:mossop]]

Comment on attachment 399837 [details] [diff] [review]
patch in progress rev2

There are a few things that I think need changes here. Some of them were present in the old code too I know but might as well fix them while we're here. The whole fudging thing bugs me about the timer service. I think we could instead do what we really mean and just only allow a single timer to fire every 10 minutes but I guess that is a different bug.

>diff --git a/toolkit/mozapps/extensions/test/unit/test_bug430120.js b/toolkit/mozapps/extensions/test/unit/test_bug430120.js
>@@ -55,16 +56,37 @@ var timerService = {
> 
>   timers: {},
> 
>-  registerTimer: function(id, callback, interval) {
>-    this.timers[id] = callback;
>-  },
>-
>   hasTimer: function(id) {
>-    return id in this.timers;
>+    var catMan = Components.classes["@mozilla.org/categorymanager;1"]
>+                           .getService(Components.interfaces.nsICategoryManager);
>+    var entries = catMan.enumerateCategory(CATEGORY_UPDATE_TIMER);
>+    while (entries.hasMoreElements()) {
>+      var entry = entries.getNext().QueryInterface(Components.interfaces.nsISupportsCString).data;
>+      var value = catMan.getCategoryEntry(CATEGORY_UPDATE_TIMER, entry);
>+      var [timerCID, timerID, prefInterval, defaultInterval] = value.split(",");
>+      if (id == timerID) {
>+        return true;
>+      }
>+    }
>+    return false;
>   },
> 
>   fireTimer: function(id) {
>-    this.timers[id].notify(null);
>+    var catMan = Components.classes["@mozilla.org/categorymanager;1"]
>+                           .getService(Components.interfaces.nsICategoryManager);
>+    var entries = catMan.enumerateCategory(CATEGORY_UPDATE_TIMER);
>+    while (entries.hasMoreElements()) {
>+      let entry = entries.getNext()
>+                         .QueryInterface(Components.interfaces.nsISupportsCString).data;
>+      let value = catMan.getCategoryEntry(CATEGORY_UPDATE_TIMER, entry);
>+      let [timerCID, prefInterval, defaultInterval, timerID] = value.split(",");
>+      if (id = timerID) {
>+        Components.manager
>+                  .getClassObject(Components.classes[timerCID], Components.interfaces.nsIFactory)
>+                  .createInstance(null, Components.interfaces.nsITimerCallback).notify(null);
>+        return;
>+      }
>+    }

Do you think there is much to be gained by replicating this behaviour in the test? We could just replace it with a call to gBlocklist.QueryInterface(Ci.nsITimerCallback).notify(null).

>diff --git a/toolkit/mozapps/update/src/nsUpdateService.js.in b/toolkit/mozapps/update/src/nsUpdateService.js.in

>   showUpdateInstalled: function UP_showUpdateInstalled() {
>     if (!this._enabled || this._getUpdateWindow() ||
>-        !getPref("getBoolPref", PREF_APP_UPDATE_SHOW_INSTALLED_UI, true))
>+        !getPref("getBoolPref", PREF_APP_UPDATE_SHOW_INSTALLED_UI, false))
>       return;

Is this change meant to be here?

>+    case "xpcom-shutdown":
>+      os.removeObserver(this, "xpcom-shutdown");
>+
>+      // Release everything we hold onto.
>+      for (var timerID in this._timers)
>+        delete this._timers[timerID];
>+      this._timer = null;

Do we need to cancel the timer too? Don't know if that just happens automatically anyway.


>+  notify: function TM_notify(timer) {
>+    const Cm = Components.manager;
>+    var now = Math.round(Date.now() / 1000);
>+    var catMan = Cc["@mozilla.org/categorymanager;1"].
>+                 getService(Ci.nsICategoryManager);
>+    var entries = catMan.enumerateCategory(CATEGORY_UPDATE_TIMER);
>+    while (entries.hasMoreElements()) {
>+      let entry = entries.getNext().QueryInterface(Ci.nsISupportsCString).data;
>+      let value = catMan.getCategoryEntry(CATEGORY_UPDATE_TIMER, entry);
>+      let [timerCID, timerID, prefInterval, defaultInterval] = value.split(",");
>+      defaultInterval = parseInt(defaultInterval);

What do you think to making defaultInterval optional and just defaulting to a day, since that is what most timers use anyway.

>+      let timerInterval = getPref("getIntPref", prefInterval, defaultInterval);
>+      let prefLastUpdate = PREF_APP_UPDATE_LASTUPDATETIME_FMT.replace(/%ID%/, timerID);
>+      let lastUpdateTime;
>+      if (gPref.prefHasUserValue(prefLastUpdate)) {
>+        lastUpdateTime = gPref.getIntPref(prefLastUpdate);
>+      } else {
>+        lastUpdateTime = Math.round(Date.now() / 1000);;

Just use now rather than calculating it again.

>+        // Fudge the lastUpdateTime by some random increment of the update
>+        // check interval (e.g. some random slice of 10 minutes) so that when
>+        // the time comes to check, we offset each client request by a random
>+        // amount so they don't all hit at once. app.update.timer is in
>+        // milliseconds, whereas app.update.lastUpdateTime is in seconds
>+        lastUpdateTime += Math.round(Math.random() * this._timerInterval / 1000);

You need to save this to the pref now (and probably just continue since it isn't going to fire this time around), otherwise this timer will never have a last update time and so will never fire.

>+      }
>+
>+      if ((now - lastUpdateTime) > timerInterval) {
>+        try {
>+          Cm.getClassObject(Cc[timerCID], Ci.nsIFactory).
>+          createInstance(null, Ci.nsITimerCallback).notify(timer);

This is going to create a new instance for services unless they register a custom factory to stop that (only the extension manager and update service do out of the ones registering for timers in this way I believe). It might be necessary to include a "service" flag in the parameters to know whether to use createInstance or getService.

>+          LOG("TimerManager", "notify - notified component id: " + timerCID);
>+        }
>+        catch (e) {
>+          LOG("TimerManager", "notify - error notifying component id: " +
>+              timerCID);
>+        }
>+        gPref.setIntPref(prefLastUpdate, now);

Fudge now by the random amount before saving it.

>+      }
>+    }
>+
>+    for (var timerID in this._timers) {
>+      var timerData = this._timers[timerID];
>+      var lastUpdateTime = timerData.lastUpdateTime;
>+
>+      // Fudge the lastUpdateTime by some random increment of the update
>+      // check interval (e.g. some random slice of 10 minutes) so that when
>+      // the time comes to check, we offset each client request by a random
>+      // amount so they don't all hit at once. app.update.timer is in milliseconds,
>+      // whereas app.update.lastUpdateTime is in seconds
>+      lastUpdateTime += Math.round(Math.random() * this._timerInterval / 1000);

Let's do the fudging when setting the pref rather than every 10 minutes. 

>+  registerTimer: function TM_registerTimer(id, callback, interval) {
>+    LOG("TimerManager", "registerTimer - id: " + id);
>+    var preference = PREF_APP_UPDATE_LASTUPDATETIME_FMT.replace(/%ID%/, id);
>+    var now = Math.round(Date.now() / 1000);

Don't need to get this unless the pref isn't set.

Comment 16

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

(In reply to comment #14)
> All of the ifdefs MOZ_UPDATER make nsUpdateService pretty confusing. Where are
> we shipping without that enabled? Would it make more sense to split the timer
> manager out into its own file to avoid having to do that?
Linux distros

I plan on doing so on trunk but not for 3.6

Comment 17

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

(In reply to comment #15)
> (From update of attachment 399837 [details] [diff] [review])
> There are a few things that I think need changes here. Some of them were
> present in the old code too I know but might as well fix them while we're here.
> The whole fudging thing bugs me about the timer service. I think we could
> instead do what we really mean and just only allow a single timer to fire every
> 10 minutes but I guess that is a different bug.
I thought about doing something like that but was concerned about short timers preventing other timers from firing in a timely fashion and having to keep track of which should be next... it seemed like overkill. Also, this timer really does mean to be intentionally sloppy about when it will fire so the concern about it being every 10 minutes is a non-issue as I see it.

Comment 18

[Dave Townsend [:mossop]]

(In reply to comment #17)
> (In reply to comment #15)
> > (From update of attachment 399837 [details] [diff] [review] [details])
> > There are a few things that I think need changes here. Some of them were
> > present in the old code too I know but might as well fix them while we're here.
> > The whole fudging thing bugs me about the timer service. I think we could
> > instead do what we really mean and just only allow a single timer to fire every
> > 10 minutes but I guess that is a different bug.
> I thought about doing something like that but was concerned about short timers
> preventing other timers from firing in a timely fashion and having to keep
> track of which should be next... it seemed like overkill. Also, this timer
> really does mean to be intentionally sloppy about when it will fire so the
> concern about it being every 10 minutes is a non-issue as I see it.

Perhaps. I'm just thinking of Fennec. If the browser is closed for a couple of days, 10 minutes after it next starts up every single registered timer is going to fire and do work.

Comment 19

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

That scenario in general I agree with. I was thinking about either changing the timer to fire every five minutes or having a second timer for firing other timers after x minutes, only firing one consumer timer at a time, and likely keeping track of the timers.

Comment 20

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

(In reply to comment #15)
>...
> >+      }
> >+
> >+      if ((now - lastUpdateTime) > timerInterval) {
> >+        try {
> >+          Cm.getClassObject(Cc[timerCID], Ci.nsIFactory).
> >+          createInstance(null, Ci.nsITimerCallback).notify(timer);
> 
> This is going to create a new instance for services unless they register a
> custom factory to stop that (only the extension manager and update service do
> out of the ones registering for timers in this way I believe). It might be
> necessary to include a "service" flag in the parameters to know whether to use
> createInstance or getService.
This is the factory object
https://developer.mozilla.org/en/NsIFactory

which only has a createInstance method and corresponds to
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/src/nsExtensionManager.js.in#5392
and
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/src/nsUpdateService.js.in#1029

Comment 21

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

(In reply to comment #20)
>...
> This is the factory object
> https://developer.mozilla.org/en/NsIFactory
> 
> which only has a createInstance method and corresponds to
s/which only has a createInstance method/which only has createInstance and lockFactory methods/

Comment 22

[Dave Townsend [:mossop]]

Yeah I believe we should instead use Cc[contract].createInstance or Cc[contract].getService depending on an argument in the category manager. Unless we just demand that services registering it handle it themselves like the EM and update manager do. I'm not too concerned which way to do it, but if you choose the latter you'll have to make the microsummary service and blocklist service implement a custom factory.

Comment 23

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

(In reply to comment #15)
>...
> >diff --git a/toolkit/mozapps/extensions/test/unit/test_bug430120.js b/toolkit/mozapps/extensions/test/unit/test_bug430120.js
> >@@ -55,16 +56,37 @@ var timerService = {
>...
> Do you think there is much to be gained by replicating this behaviour in the
> test? We could just replace it with a call to
> gBlocklist.QueryInterface(Ci.nsITimerCallback).notify(null).
No... I cleaned it up a bit

> >diff --git a/toolkit/mozapps/update/src/nsUpdateService.js.in b/toolkit/mozapps/update/src/nsUpdateService.js.in
> 
> >   showUpdateInstalled: function UP_showUpdateInstalled() {
> >     if (!this._enabled || this._getUpdateWindow() ||
> >-        !getPref("getBoolPref", PREF_APP_UPDATE_SHOW_INSTALLED_UI, true))
> >+        !getPref("getBoolPref", PREF_APP_UPDATE_SHOW_INSTALLED_UI, false))
> >       return;
> 
> Is this change meant to be here?
Yes, all major apps have set it to false and newer apps like fennec end up with bugs to add the pref so I'd like to flip it to default to false.

> >+    case "xpcom-shutdown":
> >+      os.removeObserver(this, "xpcom-shutdown");
> >+
> >+      // Release everything we hold onto.
> >+      for (var timerID in this._timers)
> >+        delete this._timers[timerID];
> >+      this._timer = null;
> 
> Do we need to cancel the timer too? Don't know if that just happens
> automatically anyway.
I suspect it will handle its own destruction on shutdown but it shouldn't hurt to add it so I did.

> >+  notify: function TM_notify(timer) {
> >+    const Cm = Components.manager;
> >+    var now = Math.round(Date.now() / 1000);
> >+    var catMan = Cc["@mozilla.org/categorymanager;1"].
> >+                 getService(Ci.nsICategoryManager);
> >+    var entries = catMan.enumerateCategory(CATEGORY_UPDATE_TIMER);
> >+    while (entries.hasMoreElements()) {
> >+      let entry = entries.getNext().QueryInterface(Ci.nsISupportsCString).data;
> >+      let value = catMan.getCategoryEntry(CATEGORY_UPDATE_TIMER, entry);
> >+      let [timerCID, timerID, prefInterval, defaultInterval] = value.split(",");
> >+      defaultInterval = parseInt(defaultInterval);
> 
> What do you think to making defaultInterval optional and just defaulting to a
> day, since that is what most timers use anyway.
I don't like the idea of the timer manager having a default since if it is ever changed consumers that decided to go with this default might have problems and it isn't any pain on the consumers to provide a default.

The remaining comments are either fixed or reworked to address the comments

Comment 24

[Dave Townsend [:mossop]]

Can you also add a comment to nsIUpdateTimerManager.idl describing the category manager registration.

Comment 25

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

Attachment: patch rev2

I've got a couple of ideas on how to handle preventing firing multiple timers 10 minutes after startup but I'll do that in a followup bug.

Comment 26

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

Attachment: tests rev2

Comment 27

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

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

>diff --git a/toolkit/mozapps/extensions/test/unit/test_bug430120.js b/toolkit/mozapps/extensions/test/unit/test_bug430120.js
>--- a/toolkit/mozapps/extensions/test/unit/test_bug430120.js
>+++ b/toolkit/mozapps/extensions/test/unit/test_bug430120.js
>@@ -53,18 +54,23 @@ var gBlocklist;
>...
>   fireTimer: function(id) {
>-    this.timers[id].notify(null);
>+    gBlocklist.QueryInterface(Components.interfaces.nsITimerCallback).notify(null)
added ; to the end of the above


>diff --git a/toolkit/mozapps/update/public/nsIUpdateTimerManager.idl b/toolkit/mozapps/update/public/nsIUpdateTimerManager.idl
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/mozapps/update/public/nsIUpdateTimerManager.idl
>@@ -0,0 +1,88 @@
>...
>+   * Note: to avoid having to instantiate a component to call registerTimer
>+   * the component can intead register an update-timer category with comma
>+   * separated values as a single string representing the timer as follows.
>+   *
>+   * _xpcom_categories: [{ category: "update-timer",
>+   *                       value: "contractID," +
>+   *                              "method," +
>+   *                              "interface," +
>+   *                              "id," +
>+   *                              "preference," +
>+   *                              "interval" }],
>+   * the values are as follows
>+   *   contractID : the contract ID for the component.
>+   *   method     : the method used to instanciate the interface. This should be
>+   *                either getService or createInstance depending on your
>+   *                component.
>+   *   interface  : the interface name without 'Components.interfaces.'.
>+   *   id         : the id that identifies the interval, used for persistence.
>+   *   preference : the preference to for timer interval. This value can be
>+   *                optional by specifying an empty string for the value.

added missing interval comment
>+   *   interval   : the default interval in seconds for the timer.

Not sure how best to add a comment to the idl for this new functionality... perhaps what I've done suffices? If not, suggestions are very welcome.

Comment 28

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

bah... I missed a couple of the initial review comments. I'll resubmit but if you get a chance to review before I do I'd appreciate the feedback.

Comment 29

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

Filed bug 517156 to prevent multiple timers firing after startup.

Comment 30

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

note to self: the places-maintenance-timer registers late enough that it shouldn't have to be rewritten. It looks like it would be good to get engine-update-timer to take advantage of this.

Comment 31

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

Attachment: patch rev3

addresses comments

Comment 32

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

Attachment: tests rev3

Comment 33

[Dave Townsend [:mossop]]

Comment on attachment 401176 [details] [diff] [review]
patch rev3

Couple of minor bits left I think. After this should be good to go.

>diff --git a/toolkit/mozapps/extensions/test/unit/test_bug430120.js b/toolkit/mozapps/extensions/test/unit/test_bug430120.js

>   fireTimer: function(id) {
>-    this.timers[id].notify(null);
>+    gBlocklist.QueryInterface(Components.interfaces.nsITimerCallback).notify(null)
>   },

Nit: put a ; on the end there.

>diff --git a/toolkit/mozapps/update/public/nsIUpdateTimerManager.idl b/toolkit/mozapps/update/public/nsIUpdateTimerManager.idl

>+   * Note: to avoid having to instantiate a component to call registerTimer
>+   * the component can intead register an update-timer category with comma
>+   * separated values as a single string representing the timer as follows.
>+   *
>+   * _xpcom_categories: [{ category: "update-timer",
>+   *                       value: "contractID," +
>+   *                              "method," +
>+   *                              "interface," +
>+   *                              "id," +
>+   *                              "preference," +
>+   *                              "interval" }],
>+   * the values are as follows
>+   *   contractID : the contract ID for the component.
>+   *   method     : the method used to instanciate the interface. This should be
>+   *                either getService or createInstance depending on your
>+   *                component.

"instantiate"

>+   *   interface  : the interface name without 'Components.interfaces.'.

Why do we need this information? Can't the timer service just use Ci.nsITimerCallback?

>diff --git a/toolkit/mozapps/update/src/nsUpdateService.js.in b/toolkit/mozapps/update/src/nsUpdateService.js.in

>+  /**
>+   * The amount to fudge the lastUpdateTime where fudge is a random increment of
>+   * the update check interval (e.g. some random slice of 10 minutes). When the
>+   * time comes to notify a timer or a timer is first registered the timer is
>+   * offset by this amount to lessen the number of timers firing at the same
>+   * time. this._timerInterval is in milliseconds, whereas the lastUpdateTime is
>+   * in seconds so this._timerInterval is divided by 1000.
>+   */
>+  get _fudge() { return Math.round(Math.random() * this._timerInterval / 1000); },

I find myself suddenly wondering if fudging all the times by an amount smaller than the timer interval actually has any effect on spacing the timers out at all, or just moves them all back one timer interval. Something for the other bug though.

>+  notify: function TM_notify(timer) {
>+    const Cm = Components.manager;

Cm is no longer used.

>+      if ((now - lastUpdateTime) > interval) {
>+        try {
>+          Components.classes[cid][method](Ci[iface]).
>+          QueryInterface(Ci.nsITimerCallback).notify(timer);

Yeah I think you can just use this and get rid of the iface parameter:

Cc[cid][method](Ci.nsITimerCallback).notify(timer);

>+    for (var timerID in this._timers) {
>+      var timerData = this._timers[timerID];
>+
>+      if ((now - timerData.lastUpdateTime) > timerData.interval) {
>+        if (timerData.callback instanceof Ci.nsITimerCallback) {

Is this test necessary? The IDL ensures we are only passed an nsITimerCallback for a callback.

>+function FakeTimer() {
>+}
>+FakeTimer.prototype = {
>+  cancel: function() { throw Cr.NS_ERROR_NOT_IMPLEMENTED; },
>+  type: null,
>+  delay: null,
>+  target: null,
>+};

Accidentally included?

Comment 34

[Dave Townsend [:mossop]]

Comment on attachment 401177 [details] [diff] [review]
tests rev3

Couple of suggestions but basically fine.

>diff --git a/toolkit/mozapps/update/test_timermanager/unit/test_0010_timermanager.js b/toolkit/mozapps/update/test_timermanager/unit/test_0010_timermanager.js

>+  gUTM = Cc["@mozilla.org/updates/timer-manager;1"].
>+         getService(Ci.nsIUpdateTimerManager).
>+         QueryInterface(Ci.nsIObserver);
>+  gUTM.observe(null, "profile-after-change", "");
>+
>+  do_timeout(0, "run_test1thru5()");

We have do_execute_soon(callback) now which would probably be better here.

>+  // already has a last update time
>+  gPref.setIntPref(PREF_BRANCH_LAST_UPDATE_TIME + TESTS[3].timerID, 1);
>+  gCompReg.registerFactory(TESTS[3].classID, TESTS[3].desc,
>+                           TESTS[3].contractID, gTest4Factory);
>+  gCatMan.addCategoryEntry(CATEGORY_UPDATE_TIMER, TESTS[3].desc,
>+                           [TESTS[3].contractID, TESTS[3].method,
>+                            "nsITimerCallback", TESTS[3].timerID,
>+                            TESTS[3].prefInterval,
>+                            TESTS[3].defaultInterval].join(","), false, true);

How about adding one that has a last update time set in the future and making sure it doesn't fire?

>+var gTest1TimerCallback = {
>+  notify: function T1CB_notify(aTimer) {
>+    gCatMan.deleteCategoryEntry(CATEGORY_UPDATE_TIMER, TESTS[0].desc, true);
>+    TESTS[0].notified = true;
>+    finished_test1thru5();

Rather than calling finished how about a do_throw since getting here would be an error.

>+var gTest2TimerCallback = {
>+  notify: function T2CB_notify(aTimer) {
>+    gCatMan.deleteCategoryEntry(CATEGORY_UPDATE_TIMER, TESTS[1].desc, true);
>+    TESTS[1].notified = true;
>+    finished_test1thru5();

Same here.

Comment 35

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

Attachment: patch rev4

(In reply to comment #33)
> (From update of attachment 401176 [details] [diff] [review])
>...
> Nit: put a ; on the end there.
fixed

> "instantiate"
fixed

> Why do we need this information? Can't the timer service just use
> Ci.nsITimerCallback?
fixed

> I find myself suddenly wondering if fudging all the times by an amount smaller
> than the timer interval actually has any effect on spacing the timers out at
> all, or just moves them all back one timer interval. Something for the other
> bug though.
saw that as well and agree that it should be taken care of in the other bug

> Cm is no longer used.
fixed

> Yeah I think you can just use this and get rid of the iface parameter:
> 
> Cc[cid][method](Ci.nsITimerCallback).notify(timer);
fixed

> Is this test necessary? The IDL ensures we are only passed an nsITimerCallback
> for a callback.
I'd like to be cautious and investigate / take care of this in the other bug

> Accidentally included?
fixed

Comment 36

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

Attachment: tests rev4 - comments addressed

Comment 38

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

Comment on attachment 401295 [details] [diff] [review]
patch rev4

>diff --git a/toolkit/mozapps/update/src/nsUpdateService.js.in b/toolkit/mozapps/update/src/nsUpdateService.js.in
>--- a/toolkit/mozapps/update/src/nsUpdateService.js.in
>+++ b/toolkit/mozapps/update/src/nsUpdateService.js.in
>@@ -3260,9 +3156,193 @@ UpdatePrompt.prototype = {
>...
>+const TimerManagerFactory = {
>+  _instance: null,
>+  createInstance: function (outer, iid) {
>+    if (outer != null)
>+      throw Components.results.NS_ERROR_NO_AGGREGATION;
>+    return this._instance == null ? this._instance = new TimerManager() :
>+                                    this._instance;
>+  }
>+};
not used and removed locally

Comment 39

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

Attachment: patch rev5

This has the aforementioned removal and a little cleanup to aus with associated test changes.

Comment 40

[Dave Townsend [:mossop]]

Comment on attachment 401387 [details] [diff] [review]
patch rev5

>diff --git a/browser/components/microsummaries/src/nsMicrosummaryService.js b/browser/components/microsummaries/src/nsMicrosummaryService.js

>+  _xpcom_categories: [{ category: "update-timer",
>+                        value: "@mozilla.org/microsummary/service;1," +
>+                               "getService,microsummary-generator-update-timer," + 
>+                               "browser.microsummary.generatorUpdateInterval," +
>+                               GENERATOR_INTERVAL }],

You have a trailing whitespace here, but otherwise this is fine.

Comment 41

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

Thanks for the review!

Comment 42

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

Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/a66d6e710af2
http://hg.mozilla.org/mozilla-central/rev/9a727d92fefd

Comment 43

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

Attachment: Followup test only fix landed

Comment 44

[Brian Crowder]

Does this improve Ts noticeably?

Comment 45

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

Not from my observations using standalone talos. I suspect any improvement would be more easily noticed on arm devices especially WinCE and WinMo. I'm keeping an eye on it and will post to this bug if there is a noticeable change

Comment 46

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

Comment on attachment 401505 [details] [diff] [review]
Followup test only fix landed

Followup test only fix pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/8b60809857c3

Comment 47

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

(In reply to comment #44)
> Does this improve Ts noticeably?
Looks like there was an increase in TS prior to this being checked in as well
http://tinyurl.com/klwak8

Comment 48

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

(In reply to comment #44)
> Does this improve Ts noticeably?
It appears that this does not. It is the right thing for this code to do though so not all is lost and it may improve things for WinCE and WinMo.

Comment 49

[Eric Shepherd [:sheppy]]

I'm whiteboarding this as docs needed post 1.9.2; if that's incorrect, let me know.

Comment 50

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

This will hopefully land for 1.9.2 soon

Comment 51

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

Comment on attachment 401387 [details] [diff] [review]
patch rev5

Drivers, requesting a1.9.2 for the wanted‑fennec1.0+. This is IMO the right thing to do though there hasn't been an improvement in TS on desktops since there might be TS improvements on arm devices and it has baked on the trunk for several days. It also provides more options for other components to rework there startup with the goal of improving TS.

Comment 52

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

Comment on attachment 401387 [details] [diff] [review]
patch rev5

a192=beltzner

Comment 53

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

Attachment: patch as landed on the 1.9.2 branch due to bug 508850 not being on branch

Comment 54

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

pushed to mozilla-1.9.2
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/bded48284d02

Comment 55

[(dormant account)]

(In reply to comment #0)
> If we were to store timer registrations somewhere permanent (prefs, category
> manager, db) then we would not longer need to load components during
> app-startup purely to register with the timer manager. This basically only
> needs to be store a mapping between timer id and contract id for a service to
> load when the timer fires.

I just did a profile of a fennec build from yesterday and posted a log in 311965. I thought this bug would make it possible to not load nsUpdateManager during startup.

Comment 56

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

That is used to resume downloading an update when the app quits before the download finishes and cleaning up an active update when its state is STATE_NONE during the final-ui-startup notification. I haven't looked into optimizing that specifically and would appreciate any insight as to why you thought it would be possible to not load it.

Comment 57

[(dormant account)]

(In reply to comment #56)
> That is used to resume downloading an update when the app quits before the
> download finishes and cleaning up an active update when its state is STATE_NONE
> during the final-ui-startup notification. I haven't looked into optimizing that
> specifically and would appreciate any insight as to why you thought it would be
> possible to not load it.

I don't know anything about this process. My main motivation for this bug has been to avoid loading updatemanager altogether. Can this mechanism be delayed until after startup? Or maybe this functionality is small/self-countained enough that it can be broken into a separate(small) component?

Comment 58

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

I personally don't think small / self-contained separate components are going to help when fastload is taken into consideration but I would like to be educated on how it would. I'm going to focus on where it eats up time per your latest log in bug 311965, removing the extra observer notification in UpdateService, and bug 517156.

Comment 59

[Dietrich Ayala (:dietrich)]

Taras said 99ms is just to get the JS from that file ready to run. That's after being pulled from the fastload cache, and before any JS is actually run.

The landing of session restore caused a huge startup regression, and we fixed it basically by doing this: making a smaller component with only the code that was essential for startup, and loading everything else in a separate component later in delayedStartup().

(In reply to comment #56)
> That is used to resume downloading an update when the app quits before the
> download finishes and cleaning up an active update when its state is STATE_NONE
> during the final-ui-startup notification. I haven't looked into optimizing that
> specifically and would appreciate any insight as to why you thought it would be
> possible to not load it.

those tasks sound like they could be kicked off after first window is up.

We should take this talk into bug 311965.

Comment 60

[Dietrich Ayala (:dietrich)]

related to this bug: are there any other services we know of that are loaded in the startup path solely to register with the timer manager?

Comment 61

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

(In reply to comment #60)
> related to this bug: are there any other services we know of that are loaded in
> the startup path solely to register with the timer manager?
The only one left that does this during startup is nsSearchService.js. PlacesDBUtils.jsm actually registers after startup. In this bug four callsites were fixed.

Comment 62

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

Filed bug 520284 for Search.

Comment 63

[Eric Shepherd [:sheppy]]

All I can find in here that obviously impacts developer documentation is the stuff about registering using the update-timer category. I've added documentation on that here:

https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIUpdateTimerManager#registerTimer()

Is there anything I've missed?

Comment 64

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

The following should be changed
id
    The ID that identifies the interval; this will be passed into registerTimer().
Change:
this will be passed into registerTimer()
To:
this is the same as the id that would be passed into registerTimer()

preference
    The preference for the timer interval; this is used to save the preference across launches of the browser if needed. This can be an empty string if cross-launch persistence isn't needed.
Change:
this is used to save the preference across launches of the browser if needed. This can be an empty string if cross-launch persistence isn't needed.
To:
this allows a preference to override the default interval. This can be an empty string if the default interval should be be overridden.

interval
    The default interval, in seconds, for the timer; this will be passed into registerTimer().
Change:
this will be passed into registerTimer()
To:
this is the same as the id that would be passed into registerTimer()

Comment 65

[Eric Shepherd [:sheppy]]

Done.