846987 - Don't always perform promise and task operations on the same tick

Status: RESOLVED WONTFIX

(Toolkit :: General, defect)

Description

[Gregory Szorc [:gps]]

This is a sister bug to bug 842347. Essentially, the request is that promises and tasks should periodically schedule operations for a future tick instead of always chaining functions on the same tick.

While the use of the same tick could be seen as optimal, it is apparently causing stack blow out in the wild (bug 845842). In that bug, it is speculated that chaining a bunch of immediately resolved promises - Promise.resolve() - in a task has directly contributed to stack explosion and eventual exhaustion.

After a very extensive IRC conversion with jimb, he recommended that we periodically insert "pauses" between promise or task chaining by way of future tick scheduling as a way to avoid excessive stack accumulation. By scheduling the next operation on a future tick, the stack from the current tick is wiped out. This should keep stack growth in check (at the expensive of slight performance loss).

Here are is a non-complete list of ideas:

* Always have Promise.resolve() and Promise.reject() fulfill on a future tick, never immediately.

* Have Task.jsm insert an extra tick between receiving the result of a promise and sending it back into the generator.

If we implement this in the current promise and task implementation, we should be careful that it doesn't cause any weird regressions due to old users relying on same-tick operation!

While we're here, we should also consider the use of random delays between tick-separated operations as a form of fuzz testing (not in release builds, of course).

Comment 1

[Richard Newman [:rnewman]]

Two more options:

* Calculate a reasonable upper bound for the number of resolutions that can be performed in the current tick (perhaps even as an attribute of a Task instance?), and only nextTick after that many times. This strikes a balance between the cost of starting over on a new tick and the chance of exhausting the stack.

* If it's possible to efficiently introspect the size of the stack, delay until the next tick whenever necessary.


I'd also be interested to know if there's something odd we can do in the JS engine, such as magically unwinding to the top of the task's stack before proceeding with the promise -- a cheaper sibling of waiting for the next tick.

Comment 2

[Gregory Szorc [:gps]]

Attachment: Give Task.jsm a tick deferring mode, v1

I added a new argument to Task.Spawn that influences when operations are performed. I've activated this mode on some of my tasks and things appear to work.

I want to explicitly note that I don't like the double function creation for each iteration of the generator that resolves a promise. However, I don't think we can proxy the argument to the outer bound function to an inner one without a closure :/

Note that this is the simplest and least efficient form. We could add promise counting, etc if we really wanted. I wonder if there is a way for JS to know if it's on the same tick as before...

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 720888 [details] [diff] [review]
Give Task.jsm a tick deferring mode, v1

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

::: toolkit/content/Task.jsm
@@ +102,5 @@
> + * Schedule a function to be executed on a later tick.
> + */
> +function runLater(fn) {
> +  Services.tm.currentThread.dispatch(fn, Ci.nsIThread.DISPATCH_NORMAL);
> +}

I don't think that if this technique is affected by bug 776798. However, it would be important to check. Perhaps checking Components.utils.methodjit in a test.

@@ +132,5 @@
> +   * @param aRunLater
> +   *        If true, generator iterations and resolving of the generator are
> +   *        deferred until a later tick. This is useful to prevent stack
> +   *        explosion if lots of things are happening on the same tick. The
> +   *        default is false (for backwards compatibility.

For forward-compatibility, I believe that this argument should be an options object, instead of a simple boolean, e.g.:

@param aOptions (optional)
   This object, if specified, allows finer control upon the execution of the task. It may contain the following fields:
  - runLater (boolean) If |true|, generator iterations and resolving of the generator are deferred until a later tick [...]

@@ +138,4 @@
>     * @return A promise object where you can register completion callbacks to be
>     *         called when the task terminates.
>     */
> +  spawn: function (aTask, aRunLater=false) {

I see that you are carrying out anonymization of methods. Isn't this a bit early? Has the profiler already gained the ability to extrapolate method names from context?

@@ +228,5 @@
>          // condition.
> +        if (this._runLater) {
> +          yielded.then(
> +            function onSuccess(value) {
> +              runLater(this._run.bind(this, true, value));

I don't know which one is more readable between what you write and the following:

function onSuccess(value) {
  runLater((function() {
    this._run(true, value);
  }).bind(this));
}

Your call.

@@ +274,1 @@
>      }

Given the number of occurrences of |if (this._runLater)|, I come to believe that there must b e away to factor these into something more regular.

Comment 4

[Dave Camp (:dcamp)]

(In reply to Richard Newman [:rnewman] from comment #1)
> Two more options:
> 
> * Calculate a reasonable upper bound for the number of resolutions that can
> be performed in the current tick (perhaps even as an attribute of a Task
> instance?), and only nextTick after that many times. This strikes a balance
> between the cost of starting over on a new tick and the chance of exhausting
> the stack.
> 
> * If it's possible to efficiently introspect the size of the stack, delay
> until the next tick whenever necessary.

Sometimes resolve immediately and sometimes delaying to a next tick seems like a recipe for intermittent bugs.

Comment 5

[Richard Newman [:rnewman]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #3)

> > +  spawn: function (aTask, aRunLater=false) {
> 
> I see that you are carrying out anonymization of methods. Isn't this a bit
> early? Has the profiler already gained the ability to extrapolate method
> names from context?

I'm not sure about the profiler, but everything else does now: Bug 433529, Bug 805222.


(In reply to Dave Camp (:dcamp) from comment #4)

> Sometimes resolve immediately and sometimes delaying to a next tick seems
> like a recipe for intermittent bugs.

I wholeheartedly agree. But it's an option on the table if the always-extra-tick approach turns out to be too expensive -- FHR uses *a lot* of promises.

Comment 6

[David Teller [:Yoric] - still alive but not very active]

(In reply to Richard Newman [:rnewman] from comment #5)
> (In reply to David Rajchenbach Teller [:Yoric] from comment #3)
> 
> > > +  spawn: function (aTask, aRunLater=false) {
> > 
> > I see that you are carrying out anonymization of methods. Isn't this a bit
> > early? Has the profiler already gained the ability to extrapolate method
> > names from context?
> 
> I'm not sure about the profiler, but everything else does now: Bug 433529,
> Bug 805222.

Just checked: now, the profiler has gained that ability.

Comment 7

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Hey folks I'm not sure why there is needinfo from me here. I think proposed solution is reasonable. Also I do agree with dcamp that sometimes delay and sometimes not can be surprising.

If you need some more specific feedback from me please let me know & set "need more info" so I get back to it sooner.

Comment 8

[Gregory Szorc [:gps]]

Has this been invalidated by all the other promise work?

Comment 9

[David Teller [:Yoric] - still alive but not very active]

Pretty much, yes.