662178 - Simplify timed callbacks

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Philipp von Weitershausen [:philikon]]

Right now we use Utils.delay for two kinds of things:
* execute something on the next event loop tick, Utils.delay(func, 0)
* defined a named timer on an object that might be overwritten until it has been fired

These are pretty distinct use cases. Using Utils.delay for the first one is a bit inefficient since it creates a bunch of objects we don't need. I'm proposing to implement two distinct helpers, Utils.nextTick and Utils.namedTimer, in its place.

Comment 1

[Philipp von Weitershausen [:philikon]]

Attachment: v1

Comment 2

[Richard Newman [:rnewman]]

Comment on attachment 537528 [details] [diff] [review]
v1

Review of attachment 537528 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Nits:

::: services/sync/modules/service.js
@@ +1136,5 @@
>      let interval = this._calculateBackoff(++attempts, 60 * 1000);
>      this._log.debug("Autoconnect failed: " + (reason || Status.login) +
>        "; retry in " + Math.ceil(interval / 1000) + " sec.");
> +    Utils.namedTimer(function() { this._autoConnect(); }, interval, this,
> +                     "_autoTimer");

Can't this simply be 

  Utils.namedTimer(this._autoConnect, ..), just like line 1102?

@@ +1577,5 @@
>      }
>  
>      this._log.trace("Next sync in " + Math.ceil(interval / 1000) + " sec.");
> +    Utils.namedTimer(function() { this.syncOnIdle(); }, interval, this,
> +                     "_syncTimer");

Same again.

@@ +1653,5 @@
>        this.nextHeartbeat = now + interval;
>  
>      this._log.trace("Setting up heartbeat, next ping in " +
>                      Math.ceil(interval / 1000) + " sec.");
> +    Utils.namedTimer(function() { this._doHeartbeat() }, interval, this,

Same again.

Comment 3

[Philipp von Weitershausen [:philikon]]

Good points! Landed with those fixes:
http://hg.mozilla.org/services/services-central/rev/f66e634eb6eb

Comment 4

[Philipp von Weitershausen [:philikon]]

Two problems with the patch as it landed:

* there's a perma-orange (or high frequency random orange) in test_utils_namedTimer.js on WinXP

* timers as created by Utils.nextTick are susceptible to GC because the timer might be GC'ed before it has fired (see bug 640629). This is highly unlikely since Utils.nextTick always uses a delay of 0, but it could happen. I propose Utils.nextTick returns the timer reference and the caller should assign it to a local variable.

Will work on those two.

Comment 5

[Philipp von Weitershausen [:philikon]]

Attachment: follow-up: ensure timers aren't GC'ed

Comment 6

[Philipp von Weitershausen [:philikon]]

Attachment: follow-up: ensure timers aren't GC'ed (v1.1)

Hrm, forgot to refresh the patch

Comment 7

[Richard Newman [:rnewman]]

Comment on attachment 537765 [details] [diff] [review]
follow-up: ensure timers aren't GC'ed (v1.1)

Review of attachment 537765 [details] [diff] [review]:
-----------------------------------------------------------------

Overview question: the second part of this patch should be to actually hold on to the return value. You said you were going to do this, but the code does not. Is this incomplete?

I will file a bug to make the necessary change in TPS, which also uses this code.

Will r+ given appropriate answers :D

::: services/sync/modules/util.js
@@ +945,3 @@
>     */
>    nextTick: function nextTick(callback, thisObj) {
> +    callback = callback.bind(thisObj);

I don't see anything in the definition of Function.bind that defines its behavior for this = undefined. Presumably you're always calling bind so that it acts like 'clone', but I feel compelled to question this to make sure it's right!

Comment 8

[Philipp von Weitershausen [:philikon]]

(In reply to comment #7)
> Overview question: the second part of this patch should be to actually hold
> on to the return value. You said you were going to do this, but the code
> does not. Is this incomplete?

No. Sorry, I should've mentioned that I changed my mind. Returning the timer and making the caller hold on to it is rather inconvenient in many cases, e.g. when you're spawning off timers in a loop, so I made the callback hold on to it.

That said, I should probably also delete the timer from the callback once the timer has fired so it can actually be GC'ed. Will upload a revised patch in a bit.

> I will file a bug to make the necessary change in TPS, which also uses this
> code.

Thanks, but this won't be necessary, sorry.

> ::: services/sync/modules/util.js
> @@ +945,3 @@
> >     */
> >    nextTick: function nextTick(callback, thisObj) {
> > +    callback = callback.bind(thisObj);
> 
> I don't see anything in the definition of Function.bind that defines its
> behavior for this = undefined.

Made me dig into the spec there! ECMA-262 5th Edition [1] says on page 34 that the value domain for the internal property [[BoundThis]] (this is what gets set with Function.prototype.bind) is *any*.

[1] http://www.ecma-international.org/publications/files/ECMA-ST/ECMA-262.pdf

> Presumably you're always calling bind so that it acts like 'clone',

Correct.

> but I feel compelled to question this to make sure it's right!

Thanks for being thorough!

Comment 9

[Philipp von Weitershausen [:philikon]]

(In reply to comment #8)
> (In reply to comment #7)
> > Overview question: the second part of this patch should be to actually hold
> > on to the return value. You said you were going to do this, but the code
> > does not. Is this incomplete?
> 
> No. Sorry, I should've mentioned that I changed my mind. Returning the timer
> and making the caller hold on to it is rather inconvenient in many cases,
> e.g. when you're spawning off timers in a loop, so I made the callback hold
> on to it.

Now that I think about it, that's not helping either. All I've done now is create a circular reference loop: the cloned callback holds on to the timer and what holds on to the cloned callback? Yep, the timer.

So hrm, supposedly nsTimerImpl "does not participate in cycle collection and so a JS callback closure that closes over the timer reference will therefore keep the timer alive" (quoting asuth from bug 640629 comment 6). But that would mean we would rely on a) nsTimerImpl staying this way and b) SpiderMonkey not optimizing closures in funny ways (bz raised this point in bug 611807, for instance.)

So, hrm, I guess we should indeed just return the timer and make sure the outside caller hangs on to it.

> That said, I should probably also delete the timer from the callback once
> the timer has fired so it can actually be GC'ed. Will upload a revised patch
> in a bit.
> 
> > I will file a bug to make the necessary change in TPS, which also uses this
> > code.
> 
> Thanks, but this won't be necessary, sorry.

Ignore me, it will. Thanks :)

Comment 10

[Philipp von Weitershausen [:philikon]]

Attachment: follow-up: ensure timers aren't GC'ed (v2)

Looking at some of the places where Utils.nextTick is used, it's really inconvenient to be hanging onto the timer past the lifetime of the outer callback. So here's an alternate proposal: we keep a reference to the timer in an object private to Utils.nextTick until the timer fires.

Sorry for the back and forth. Stuff like this makes me wish for more explicit control over the heap sometimes...

Comment 11

[Philipp von Weitershausen [:philikon]]

Attachment: follow-up: harden test_utils_namedTimer.js

Fixes the permaish orange on WinXP debug...

Comment 12

[Richard Newman [:rnewman]]

Comment on attachment 538001 [details] [diff] [review]
follow-up: ensure timers aren't GC'ed (v2)

Review of attachment 538001 [details] [diff] [review]:
-----------------------------------------------------------------

Other than the one question and two nits, this is good, and a bit like what I did for the TPS change.

::: services/sync/modules/util.js
@@ +945,3 @@
>     */
> +  _timer_id: 0,
> +  _timers: {},

I'm a tiny bit concerned (probably unduly) about how this will perform under persistent load. The _timers object will of course last long enough to be tenured, and we're constantly adding new keys to it and deleting old ones. I'd hate for that to have some kind of detrimental effect on memory management.

Any ideas?

@@ +945,4 @@
>     */
> +  _timer_id: 0,
> +  _timers: {},
> +  nextTick2: function nextTick(callback, thisObj) {

Shouldn't this be nextTick, not nextTick2?

@@ +945,5 @@
>     */
> +  _timer_id: 0,
> +  _timers: {},
> +  nextTick2: function nextTick(callback, thisObj) {
> +    // Ensure we keep a strong reference to the timer until has fired.

Should be

  // Ensure that we keep a strong reference to the timer until it has fired.

Comment 13

[Philipp von Weitershausen [:philikon]]

Attachment: follow-up: ensure timers aren't GC'ed (v3)

I *think* I've got it now. The best way to ensure timers aren't GC'ed is to ... not use timers in the first place! We can simply use mainThread.dispatch!

Comment 14

[Richard Newman [:rnewman]]

Comment on attachment 538029 [details] [diff] [review]
follow-up: ensure timers aren't GC'ed (v3)

Review of attachment 538029 [details] [diff] [review]:
-----------------------------------------------------------------

Oh, the lulz.

r=me if you switch to using currentThread instead of mainThread.

Comment 15

[Richard Newman [:rnewman]]

Comment on attachment 538002 [details] [diff] [review]
follow-up: harden test_utils_namedTimer.js

Review of attachment 538002 [details] [diff] [review]:
-----------------------------------------------------------------

Comment 16

[Philipp von Weitershausen [:philikon]]

Landed follow-ups with comment addressed:
http://hg.mozilla.org/services/services-central/rev/8eae98fbd3fe
http://hg.mozilla.org/services/services-central/rev/607c796463f2

Comment 17

[Philipp von Weitershausen [:philikon]]

http://hg.mozilla.org/mozilla-central/rev/f66e634eb6eb
http://hg.mozilla.org/mozilla-central/rev/8eae98fbd3fe
http://hg.mozilla.org/mozilla-central/rev/607c796463f2

Comment 18

[Ed Lee :Mardak]

(In reply to comment #0)
> Using Utils.delay for the first one is a bit inefficient since it creates a bunch of objects we don't need.
FYI, some of that is to keep the timer alive.

Comment 19

[Philipp von Weitershausen [:philikon]]

(In reply to comment #18)
> FYI, some of that is to keep the timer alive.

Sure, but using a timer for a delay of 0 isn't efficient, because you can simply use nsIThread::dispatch without creating new objects.

Also, Utils.delay wasn't actually doing a very good job of keeping the timer alive always. A bunch of code would use Utils.delay() without passing a 'thisObj', in which case Utils.delay would create a locally scoped one, thus hoping that the closure would keep it alive (which it didn't have to necessarily.)

Comment 20

[Ed Lee :Mardak]

(In reply to comment #19)
> use nsIThread::dispatch without creating new objects.
Neato. I'll probably copy that for extensions for compatibility 4-7+. (Surprise surprise, extensions on trunk using Utils.delay broke! :p)

Comment 21

[Philipp von Weitershausen [:philikon]]

(In reply to comment #20)
> (In reply to comment #19)
> > use nsIThread::dispatch without creating new objects.
> Neato. I'll probably copy that for extensions for compatibility 4-7+.
> (Surprise surprise, extensions on trunk using Utils.delay broke! :p)

Heh. Yeah, Utils.nextTick is just a shorter way of spelling

  Services.tm.mainThread.dispatch(callback, Ci.nsIThread.DISPATCH_NORMAL);

whereas Utils.namedTimer will now always require a thisObj + name.