1013625 - Process Promise resolution runnables outside of main event queue.

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Promises post a significant number of runnables to the event queue. These delay promise resolution to their position in the event queue and flood the event queue when chains of Promises are created or resolved.

The idea is to have a separate nsTArray of dummy runnables and process them in nsXPConnect::AfterProcessNextEvent and nsContentUtils::LeaveMicroTask.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Is that what some spec wants? End-of-microtask vs. task is detectable from scripts.

Comment 2

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Seems like Bug 1023547 is another reason to figure this bug out.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> Is that what some spec wants?

Per spec, promises run end-of-microtask afaict.

Comment 4

[Alex Vincent [:WeirdAl]]

I'm definitely willing to help implement this, and make some time available.  But I'll need some guidance; it'd be nice to partner with someone on this.

Comment 5

[:Paolo Amadini]

Attachment: Initial patch

I'm taking this bug as part of the Promises Debugging work week, for obtaining Promise.jsm parity.

Boris, this change breaks the following test:

http://mxr.mozilla.org/mozilla-central/source/dom/promise/tests/test_promise.html?force=1#701

It now shows 1-1,2-1,1-2,1-3,2-2,2-3. Maybe there is now something scheduled slightly differently. Ideas?

I also still need to free the memory allocated for the microtask queue.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8505708 [details] [diff] [review]
Initial patch

Paolo and I figured out the test failure.  The issue is that microtask checkpoints happen at the end of the then callbacks.  So if a promise has two things waiting on it (two calls to then() on the same promise, returning two promises P1 and P2), we get into a situation where we invoke the first then callback, queue up P1, then call the second then callback and before we get a chance to queue up P2 we process P1 and if anything was waiting on it we queue it up.  Hence the test failure.

If we change this to do the "then and queue" all inside a microtask, then the failure mode doesn't really change: we call the then callback for P1, then queue P1, then the microtask ends, we process the queue, call the then callback for anything waiting on P1 and queue it up.  All before we get to P2.

The fundamental problem is that we have things waiting to have their then callbacks invoked that are not represented in the queue in any way.

We have two options here, I think.  

1)  More short-term, don't process the queue from microtask checkpoints; only process it from the event loop.  That would fix the above, kinda, but leave other issues that we have right now (e.g. spinning the event loop in a then callback blocks other then callbacks for the same promise from running).

2)  Move to a model more like the current spec, where what's queued is not the resolution of a promise per se but rather the invocation of its then callbacks, and there is a separate entry in the queue for each then callback.  In that situation we never have a "pending but not in the queue yet" situation.  This is what the spec actually says to do.

We're going to take a shot at #2.

Comment 7

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

I guess Bug 1058695 will be easier to fix with this change right?

Comment 8

[Boris Zbarsky [:bzbarsky]]

I don't think this change will affect bug 1058695, since the queue is planned to be per-thread at the moment, not per-global.

But note the es-discuss discussion about that bug.  Maybe a viable thing to do is to simply not run callbacks for promises whose global is in some "unloaded" state...

Comment 9

[:Paolo Amadini]

Attachment: The patch

The funny thing is that bug 1083783 does not solve the issue about the microtask checkpoint - it just makes the test output 1-1,2-1,2-2,2-3,1-2,1-3.

This is because we still perform the microtask checkpoint just after returning from the "then" callback, but before we enqueue the task corresponding to the resolution of the promise on which "then" was called.

I think we should do the change in bug 1083783 anyways, and for the moment only perform the promise microtask checkpoint at the end of the current event (solution 1 from comment 6).

I don't know if you still want to keep the PerformMainThreadMicrotaskCheckpoint function now that it only handles DOM mutations.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> The funny thing is that bug 1083783 does not solve the issue about the microtask
> checkpoint - it just makes the test output 1-1,2-1,2-2,2-3,1-2,1-3.

Is that still true if you do a microtask around the combination of calling the then callback and quing the task?

Comment 11

[Boris Zbarsky [:bzbarsky]]

> I don't know if you still want to keep the PerformMainThreadMicrotaskCheckpoint function 

Oh, and for this, I think we do want it no matter what.

Comment 12

[Boris Zbarsky [:bzbarsky]]

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

Could you please file a bug on :wchen about maybe moving ProcessBaseElementQueue to PerformMainThreadMicrotaskCheckpoint?

>+  nsTArray<nsRefPtr<nsIRunnable>>* microtaskQueue =

I think it would make sense for GetPromiseMicrotaskQueue to return a reference, not a pointer.

>+    // This function can re-enter, so we remove the element before calling.

I'm a bit torn here.

What you have has the benefit of releasing the runnables expeditiously and avoiding the queue getting too long but involves memmoves which can take a bunch of time when the queue is long.  Iterating along the queue would avoid those, but at the cost of maybe keeping the runnables or (if we null them out as we go) still leading to a possibly-very-large array.

Could you please file a followup bug on doing this on top of nsDeque or equivalent?  We may want to create a nice templated deque class in the process, which I'd rather we didn't block this bug on.

I'd like Kyle to double-check the WorkerPrivate bit here.

We should probably check whether this patch in fact fixes bug 1038631 and if so add a test for that behavior.

r=me.  Thank you for doing this!

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

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

The WorkerPrivate bit is fine, but do we really want the O(N^2) behavior in PerformMicrotaskCheckpoint?

Comment 14

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I guess bz covered that in comment 12.

Comment 15

[Boris Zbarsky [:bzbarsky]]

I was just talking to Ben and he pointed out that we probably want to only do the promise bit if aRecursionDepth == 1 so we don't do it during sync XHR and importScripts and the like.

Comment 16

[Boris Zbarsky [:bzbarsky]]

And a test for the sync-xhr case might be good; basically verifying that promise callbacks don't happen until after the XHR is done (and _do_ happen before it with the patch as written).

Comment 17

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Ah, yes.  I should have asked what the desired semantics were here :/

Comment 18

[:Paolo Amadini]

(In reply to Boris Zbarsky [:bz] from comment #10)
> > The funny thing is that bug 1083783 does not solve the issue about the microtask
> > checkpoint - it just makes the test output 1-1,2-1,2-2,2-3,1-2,1-3.
> 
> Is that still true if you do a microtask around the combination of calling
> the then callback and quing the task?

I assume you mean "do a microtask around doing a microtask (for each callback) around the combination of calling the then callback and queuing the next task"... which I've not tried. But even if it passed the test case, I wouldn't be comfortable with it for more complex "then" recursions (and it's not what the specification says anyways).

The microtask specification does define a mechanism to prevent microtask processing recursion, but in order to support nested event loops we'd need to also support the part that says that microtasks can be promoted to tasks in that case... and it looks like it's too much complexity for what we need right now.

Comment 19

[:Paolo Amadini]

(In reply to Boris Zbarsky [:bz] from comment #15)
> I was just talking to Ben and he pointed out that we probably want to only
> do the promise bit if aRecursionDepth == 1 so we don't do it during sync XHR
> and importScripts and the like.

Wouldn't that also prevent us from processing all Promise callbacks on the main thread, including chrome code, during "alert"s?

Comment 20

[Boris Zbarsky [:bzbarsky]]

> But even if it passed the test case, I wouldn't be comfortable with it for more complex
> "then" recursions (and it's not what the specification says anyways).

The specification pretty clearly says that the mere act of returning from the then callback should not trigger processing of the queue.  Insofar as the specification says anything at all, of course, since right now the ES6 job queue is not hooked up with anything in the web platform in spec terms...

But ok, let's just do the promise processing off the event loop for now, until the specs sort themselves out here.

> Wouldn't that also prevent us from processing all Promise callbacks on the main thread,

Sorry, I was unclear.  The aRecursionDepth check should only be in WorkerPrivate::AfterProcessNextEvent and should not affect mainthread behavior at all.

Comment 21

[:Paolo Amadini]

Attachment: Updated patch

(In reply to Boris Zbarsky [:bz] from comment #15)
> if aRecursionDepth == 1 so we don't do it during sync XHR
> and importScripts and the like.

I've not debugged it, but it turns out that someone is calling AfterProcessNextEvent with aRecursionDepth == 1 during operations like sync XHR. The simplest thing to do was to place the call directly inside the outer worker event loop implementation. Requesting review again because of this change and the new XHR test.

I've also clarified the existing async tests, including a new test for bug 1038631, that definitely fails without this and bug 1083783 applied.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8509502 [details] [diff] [review]
Updated patch

r=me on all the bits except WorkerPrivate::DoRunLoop.

Kyle, can you please take a look at that part?

Comment 23

[:Paolo Amadini]

The tryserver build looks good, the intermittent failure on Android looks like bug 1085627 and has a similarly high rate on the fx-team branch:

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

Comment 24

[:Paolo Amadini]

https://hg.mozilla.org/integration/fx-team/rev/47a470e57e9d

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/47a470e57e9d