Process Promise resolution runnables outside of main event queue.

RESOLVED FIXED in mozilla36

Status

()

defect
RESOLVED FIXED
5 years ago
4 months ago

People

(Reporter: nsm, Assigned: Paolo)

Tracking

(Blocks 2 bugs)

unspecified
mozilla36
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
Is that what some spec wants? End-of-microtask vs. task is detectable from scripts.
Seems like Bug 1023547 is another reason to figure this bug out.
Depends on: 1038631
> Is that what some spec wants?

Per spec, promises run end-of-microtask afaict.
Blocks: 1038631
No longer depends on: 1038631
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.
Blocks: 885333
Posted patch Initial patch (obsolete) — Splinter Review
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.
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Attachment #8505708 - Flags: feedback?(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.
Attachment #8505708 - Flags: feedback?(bzbarsky) → feedback+
Depends on: 1083783
I guess Bug 1058695 will be easier to fix with this change right?
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...
Posted patch The patch (obsolete) — Splinter Review
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.
Attachment #8505708 - Attachment is obsolete: true
Attachment #8507907 - Flags: review?(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?
> 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 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!
Attachment #8507907 - Flags: review?(khuey)
Attachment #8507907 - Flags: review?(bzbarsky)
Attachment #8507907 - Flags: review+
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?
Attachment #8507907 - Flags: review?(khuey) → review+
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.
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).
Ah, yes.  I should have asked what the desired semantics were here :/
Depends on: 1086528
(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.
(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?
> 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.
Blocks: 1087326
Blocks: 1087330
Posted patch Updated patchSplinter Review
(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.
Attachment #8507907 - Attachment is obsolete: true
Attachment #8509502 - Flags: review?(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?
Attachment #8509502 - Flags: review?(khuey)
Attachment #8509502 - Flags: review?(bzbarsky)
Attachment #8509502 - Flags: review+
Depends on: 1088704
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
https://hg.mozilla.org/mozilla-central/rev/47a470e57e9d
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Depends on: 1090756
Duplicate of this bug: 1038631
Blocks: 1094208
Blocks: 1094248
Depends on: 1292685
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.