Closed Bug 1420816 Opened 6 years ago Closed 6 years ago

Land new Microtask/Promise scheduling but disable it by default

Categories

(Core :: DOM: Core & HTML, enhancement, P2)

enhancement

Tracking

()

RESOLVED INVALID

People

(Reporter: bevis, Assigned: bevis)

References

Details

Attachments

(2 files, 2 obsolete files)

After further summary, in the patch of bug 1193394, there are following reasons to perform microtask check point:
enum ReasonToPerform {
  BeforeProcessTask,
  AfterProcessTask,
  CleanupScript,
  AsyncMutation,
  Timeout,
};

Maybe we could filter in BeforeProcessTask, AfterProcessTask, Timeout for Promise callbacks to land the fix part 1 of bug 1193394 earlier with a variable to enable this new scheduling later.
carry patch part 1 from bug 1193394.
Attachment #8932036 - Flags: review+
There are several new crashes, failures and unexpected pass on try:
FAIL | dom/animation/test/chrome/test_animation_observers_async.html | [single_transition_cancelled_property] records after transition end - number of records
got +0, expected 1

PASS (expected FAIL) | dom/promise/tests/test_promise.html

FAIL | browser/components/contextualidentity/test/browser/browser_usercontextid_tabdrop.js | Test timed out

PASS (expected FAIL) | /IndexedDB/transaction-deactivation-timing.html

PROCESS-CRASH | /css/css-tables/table-model-fixup-2.html | application crashed [@ mozilla::net::nsSocketTransport::InitiateSocket]
Comment on attachment 8932038 [details] [diff] [review]
(WIP) Patch: Part 2: Disable new Microtask/Promise scheduling by default

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

I'd like to ask for earlier feedback of this plan if we think this is doable and will be better for us to enable it later after all failure test cases in bug 1193394 are clear and more user trial on nightly or manual test from QA are done.

Although, there are still some new orange factors on comment 3 introduced by these changes to break down in advance.
Attachment #8932038 - Flags: review?(bugs)
(In reply to Bevis Tseng[:bevis][:btseng] from comment #0)
> After further summary, in the patch of bug 1193394, there are following
> reasons to perform microtask check point:
> enum ReasonToPerform {
>   BeforeProcessTask,
>   AfterProcessTask,
>   CleanupScript,
>   AsyncMutation,
>   Timeout,
> };
If there is something still in Time or so, that is leftover.
And I don't know what CleanupScript or AsyncMutation mean
Comment on attachment 8932038 [details] [diff] [review]
(WIP) Patch: Part 2: Disable new Microtask/Promise scheduling by default

If this causes failures, it means this isn't giving us the current Promise or MutationObserver scheduling.
Attachment #8932038 - Flags: review?(bugs) → review-
(Oops, just realized that I used the r? for the feedback instead of f?, sorry. :p)
Add MOZ_ASSERT(NS_IsMainThread()) before suppressed.push(runnable) to clarify that microtasks in worker shall never be suppressed.
Otherwise, mPendingMicroTaskRunnables will be replaced later with all suppressed tasks in mDebuggerMicroTaskQueue unexpectedly.
Attachment #8932036 - Attachment is obsolete: true
Attachment #8933150 - Flags: review+
This patch has identified all the differences except the ordering of the tasks between mutation observers and promise callbacks.
This patch also fixed all failures except dom/animation/test/chrome/test_animation_observers_async.html [1] which strongly depends on the executing order between mutation observers and promise callbacks [2] in CycleCollectedJSContext::AfterProcessTask() because I can see the same failure if we call PerformMicroTaskCheckPoint() after Promise::PerformMicroTaskCheckpoint() in current m-c build.

Unfortunately, this is the biggest blocker preventing us moving further in this bug:
In the new scheduling patch, there is only single microtask queue, so it increases the complexity too much to simulate the ordering of mutation observers and promise callbacks in old fashion and I don't think it's reasonable to land a patch that is hard to maintain and read. :-\

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=a3f71e9cd474fb6cad5af9bbb71cb81daaf97bce&selectedJob=148429975
[2] https://searchfox.org/mozilla-central/rev/9f3bd430c2b132c86c46126a0548661de876799a/xpcom/base/CycleCollectedJSContext.cpp#359,361
Attachment #8932038 - Attachment is obsolete: true
Change status to INVALID because of the reason explained in comment 9.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → INVALID
It's still worth to summarize the differences in details between new/old microtask scheduling:

[Micotask check points]
In new approach, all microtasks will only be performed at:
1. The return of outermost JS callback[1].
2. The end of current task[2].
(The recursive depths of microtasks is respected.)
(The recursive depths of tasks in worker thread are respected.)
In old approach in m-c:
1. Microtasks dispatched via CycleCollectedJSContext::DispatchMicroTaskRunnable() will behave the same as the new approach.
2. Microtasks dispatched via CycleCollectedJSContext::DispatchToMicroTask() will be performed at:
   a. The end of of current task[3]. (Only the recursive depth of the tasks in worker is respected[4].)
   b. The end of performing a JS Timeout Callback[5][6].

[Microtask queues]
1. In new approach, there is only single queue for all kinds of microtasks.
2. In old approach, 
   - CycleCollectedJSContext::mPendingMicroTaskRunnables for tasks via DispatchMicroTaskRunnable() and CycleCollectedJSContext::mPromiseMicroTaskQueue for tasks via DispatchToMicroTask() or a direct push on it.
   - mPendingMicroTaskRunnables will be cleaned up before mPromiseMicroTaskQueue according to [7].

[1] https://searchfox.org/mozilla-central/rev/be78e6ea9b10b1f5b2b3b013f01d86e1062abb2b/xpcom/base/CycleCollectedJSContext.h#225
[2] https://searchfox.org/mozilla-central/rev/9f3bd430c2b132c86c46126a0548661de876799a/xpcom/base/CycleCollectedJSContext.cpp#359
[3] https://searchfox.org/mozilla-central/rev/9f3bd430c2b132c86c46126a0548661de876799a/xpcom/base/CycleCollectedJSContext.cpp#360-364
[4] https://searchfox.org/mozilla-central/rev/9f3bd430c2b132c86c46126a0548661de876799a/dom/workers/RuntimeService.cpp#1114
[5] https://searchfox.org/mozilla-central/rev/be78e6ea9b10b1f5b2b3b013f01d86e1062abb2b/dom/base/nsGlobalWindowInner.cpp#6412
[6] https://searchfox.org/mozilla-central/rev/be78e6ea9b10b1f5b2b3b013f01d86e1062abb2b/dom/workers/WorkerPrivate.cpp#6708
[7] https://searchfox.org/mozilla-central/rev/9f3bd430c2b132c86c46126a0548661de876799a/xpcom/base/CycleCollectedJSContext.cpp#359,361
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: