1030075 - In Task.jsm, move the first call to `_run` to a promise callback.

Status: RESOLVED WONTFIX

(Toolkit :: Async Tooling, defect)

Description

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

Extract from bug 1023787
« So, apparently the _run function can re-enter when it's called directly by Task.spawn:

http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/Task.jsm#250

I think the best thing to do is to change _run to be called as a resolution callback:

Promise.resolve().then(() => this._run(true)); »

Comment 1

[:Irving Reid (No longer working on Firefox)]

I'm concerned with the latency added by always waiting for the next turn of the Promise loop -  I remember Gijs complaining a couple of months ago about a performance regression in a UI smoothness test, caused by the first step of the animation being put on the event queue instead of executed immediately.

Comment 2

[:Paolo Amadini]

(In reply to :Irving Reid (Away until July 21) from comment #1)
> I'm concerned with the latency added by always waiting for the next turn of
> the Promise loop -  I remember Gijs complaining a couple of months ago about
> a performance regression in a UI smoothness test, caused by the first step
> of the animation being put on the event queue instead of executed
> immediately.

I remember the regression as well. It was due to the move to asynchronous Promises, and the work done to address it will benefit this case as well. In other words, I expect the code changes needed to address this change to be much less work than the original Promises migration.

I expect this general pattern:

A;
Task.spawn(function* () {
  B;
  yield resolvedPromise;
  C;
});

Some code will expect A and B to be on the same tick, and C on the next one. After this bug is solved, B and C will be on the same later tick. The timing of C will not change - it is already on a later tick, because of the move to asynchronous Promises.

In case A and B must be on the same tick for performance or other reasons, this can be rewritten as:

A;
B;
Task.spawn(function* () {
  yield promise;
  C;
});

Fixing this will also prevent future incorrect uses of "Task.async" directly on event handlers. Task.spawn preceded by synchronous event responses must be used there in case the event must do asynchronous work.

Comment 3

[:Paolo Amadini]

Attachment: The patch

Here is the patch implementing the change.

This already contains a fix for one test that depended on exact timing.

A tryserver build to identify more failures is running here:

https://tbpl.mozilla.org/?tree=Try&rev=1f4df7a96443

Comment 4

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

Comment on attachment 8451521 [details] [diff] [review]
The patch

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

I believe that I prefer this semantics, if we can get all tests to pass.

I guess this also means that we can now write

let promise = Task.spawn(function*() {
  foo.bar = promise;
  // ...
});

Can't we? If so, it might be useful to add a test.

Comment 5

[:Paolo Amadini]

There are lots of test failures due to the timing changes.

I don't think I'll be able to work on this in the short term, but this is something we should do to make or Task implementation future-proof. I can act as a mentor in case someone wants to step up in the meantime, but from my past experience this kind of problem requires being at ease with working in different areas of the code base, thus it has a pretty high difficulty and should be considered by experienced contributors.

Comment 6

[:Paolo Amadini]

I wonder how this change interacts with the DOM promise scheduling introduced in bug 1094208. I expect that it should make things simpler here, as we're not waiting for a tick of the event loop anymore, while still returning from the "Task.spawn" call before invoking the generator. Let's see:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=85581a5d0b1f

Comment 7

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 8

[Mark Banner (:standard8)]

Per bug 1515695, we're moving away from Task.jsm, hence wontfix.