Closed Bug 471219 Opened 11 years ago Closed 11 years ago

Store timer registrations somewhere permanent

Categories

(Toolkit :: Application Update, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- beta1-fixed
fennec 1.0- ---

People

(Reporter: mossop, Assigned: robert.strong.bugs)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [ts])

Attachments

(4 files, 8 obsolete files)

21.44 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
38.72 KB, patch
mossop
: review+
beltzner
: approval1.9.2+
Details | Diff | Splinter Review
961 bytes, patch
Details | Diff | Splinter Review
69.95 KB, patch
Details | Diff | Splinter Review
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.
Blocks: 459117
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.
tracking-fennec: --- → 1.0b2+
(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?
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.
tracking-fennec: 1.0b2+ → 1.0-
Flags: wanted-fennec1.0+
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.
Blocks: 480212
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
Whiteboard: [ts]
I changed my mind and went with the category manager. Also, I just about have this finished.
Assignee: nobody → robert.bugzilla
Status: NEW → ASSIGNED
Attached patch patch in progress rev1 (obsolete) — Splinter Review
There are some changes in this patch that aren't really needed and I'll resubmit after I've removed them.
For all of your lazy getters, you should be using XPCOMUtils.defineLazy[Service]Getter.  See http://shawnwilsher.com/archives/319 for more details.
Attached patch patch in progress rev2 (obsolete) — Splinter Review
Attachment #399817 - Attachment is obsolete: true
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 on attachment 399837 [details] [diff] [review]
patch in progress rev2

I'll followup shortly with tests.
Attachment #399837 - Flags: review?(dtownsend)
Attached patch basic tests (obsolete) — Splinter Review
I'll add more tests later.
Attachment #400083 - Flags: review?(dtownsend)
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.
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 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.
Attachment #399837 - Flags: review?(dtownsend) → review-
(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
(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.
(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.
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.
Attachment #400083 - Attachment is obsolete: true
Attachment #400083 - Flags: review?(dtownsend)
(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
(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/
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.
(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
Can you also add a comment to nsIUpdateTimerManager.idl describing the category manager registration.
Attached patch patch rev2 (obsolete) — Splinter Review
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.
Attachment #399837 - Attachment is obsolete: true
Attachment #401153 - Flags: review?(dtownsend)
Attached patch tests rev2 (obsolete) — Splinter Review
Attachment #401154 - Flags: review?(dtownsend)
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.
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.
Filed bug 517156 to prevent multiple timers firing after startup.
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.
Attached patch patch rev3 (obsolete) — Splinter Review
addresses comments
Attachment #401153 - Attachment is obsolete: true
Attachment #401154 - Attachment is obsolete: true
Attachment #401176 - Flags: review?(dtownsend)
Attachment #401153 - Flags: review?(dtownsend)
Attachment #401154 - Flags: review?(dtownsend)
Attached patch tests rev3 (obsolete) — Splinter Review
Attachment #401177 - Flags: review?(dtownsend)
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?
Attachment #401176 - Flags: review?(dtownsend) → review-
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.
Attachment #401177 - Flags: review?(dtownsend) → review+
Attached patch patch rev4 (obsolete) — Splinter Review
(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
Attachment #401176 - Attachment is obsolete: true
Attachment #401177 - Attachment is obsolete: true
Attachment #401295 - Flags: review?(dtownsend)
Duplicate of this bug: 465003
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
Attached patch patch rev5Splinter Review
This has the aforementioned removal and a little cleanup to aus with associated test changes.
Attachment #401295 - Attachment is obsolete: true
Attachment #401387 - Flags: review?(dtownsend)
Attachment #401295 - Flags: review?(dtownsend)
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.
Attachment #401387 - Flags: review?(dtownsend) → review+
Thanks for the review!
Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/a66d6e710af2
http://hg.mozilla.org/mozilla-central/rev/9a727d92fefd
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [ts] → [ts][needs baking]
Does this improve Ts noticeably?
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 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
(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
(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.
I'm whiteboarding this as docs needed post 1.9.2; if that's incorrect, let me know.
Whiteboard: [ts][needs baking] → [ts][needs baking][doc-waiting-1.9.3]
Attachment #401387 - Flags: approval1.9.2?
This will hopefully land for 1.9.2 soon
Whiteboard: [ts][needs baking][doc-waiting-1.9.3] → [ts][doc-waiting-1.9.3]
Whiteboard: [ts][doc-waiting-1.9.3] → [ts][doc-waiting-landing]
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.
Attachment #401387 - Flags: approval1.9.2? → approval1.9.2+
Whiteboard: [ts][doc-waiting-landing] → [ts]
(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.
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.
(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?
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.
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.
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?
(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.
Filed bug 520284 for Search.
Blocks: 520526
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?
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()
Done.
No longer blocks: 477192
No longer blocks: 480696
You need to log in before you can comment on or make changes to this bug.