Closed Bug 1800496 Opened 2 years ago Closed 2 years ago

Revert cancellation behavior of workers

Categories

(Core :: DOM: Workers, defect, P2)

defect

Tracking

()

RESOLVED FIXED
109 Branch
Tracking Status
firefox109 --- fixed

People

(Reporter: yulia, Assigned: yulia)

References

Details

Attachments

(9 files)

The simplification of the worker cancellation hasn't been worthwhile, so I will be reverting it to return to https://searchfox.org/mozilla-central/rev/e0a7a014f3384ac1abc229d9961f244e3e70a52c/dom/workers/ScriptLoader.cpp#735-776

It led to a tricky to fix crash, and I don't think continuing to patch this ad-hoc is the right way to go. The simplification was not critical.

Most other changes will layer on to this.

Assignee: nobody → ystartsev

We will be using the mSyncLoopTarget for module promises, and for returning to the worker thread
when cleaning up for ThreadSafeRequestHandle.

This introduces a new datastructure that wraps the ScriptLoadRequest. Previously, we were using the WorkerLoadContext to access the request, and we made it not cycle collected in order to make it safe across threads. This, of course, broke module behavior, as the cycle needs to be broken in more places, and things get rather complex.

In the spirit of making everyone's life easier, the ThreadSafeRequestHandle exists as a way to move the request to the main thread, operate on it, and then return it to the worker. We no longer need to track and free the WorkerLoadContext. Once the ThreadSafeRequestHandle is returned to the worker, we release the request, and the handle is empty. There is a possibility that this will cause issues, so we should keep an eye on this. Alternatively, we can never release the ScriptLoadRequest and let the ThreadSafeRequestHandle clean it up on destruction.

In the case that we somehow end up releasing on the main thread, we will send a runnable to release the request correctly on the worker.

Depends on D162212

The non-cycle collected LoadContextBase caused failures for Module Workers. This reverts that change

Depends on D162213

This was removed as part of an attempt to simplify worker cancellaton. However, this channel cancellation is important, as without it the timing changes when we finish a request. Without it, we continue to load a script, even after a worker has been shut down.

This tracking will help us reimplement cancellation.

Depends on D162214

This returns most of the behavior from https://searchfox.org/mozilla-central/rev/e0a7a014f3384ac1abc229d9961f244e3e70a52c/dom/workers/ScriptLoader.cpp#735-776

What is missing at this point is batch dispatching, as we once again cancel one-at-a-time rather than as a group.

Depends on D162215

The ScriptLoaderRunnable used to do double duty as both the runnable used to load a script, and as the data structure to hold on to loading information (what kind of worker, debugger status, etc, if we failed). The second half of these responsibilities remains with the WorkerScriptLoader. WorkerScriptLoader acts as a persistent data structure. ScriptLoaderRunnable is per batch of requests (such as ImportScripts, or a single module load). It works together with the ThreadSafeRequestHandle to load the script.

In the future, when we move to a single threaded implementation, ThreadSafeRequestHandle will likely be merged with WorkerLoadContext, but ScriptLoaderRunnable will stay as a datastructure to represent the batched request, and most of the existing fields (mSyncTarget, the request list) will stay. There is still some massaging to do here, but it gets closer to a final vision of how this should look.

Depends on D162216

ArrayView is a data structure I removed as part of the simplification of worker cancellation, as it was only used for this purpose in the entire code base. I am bringing it back now, as we are re-implementing that same behavior.

Depends on D162217

This brings back the "group" dispatch that existed before in https://searchfox.org/mozilla-central/rev/e0a7a014f3384ac1abc229d9961f244e3e70a52c/dom/workers/ScriptLoader.cpp#1027-1085

In my view, this is probably an optimization, but it may result in significant behavioral changes as timing has been a major issue with the cancellation refactor. What this does is, if two requests resolve in an order other than post order, then the requests will wait untill all of their dependencies have loaded before firing the dispatch. That means in some cases, we will have only one dispatch instead of two.

Depends on D162218

Previously, in the cancellation refactor, we removed knowledge of the list of scripts being loaded on the main thread. As a result of this, we could no longer clear the cache creator on the main thread after iterating over all requests. We can now do that once again, and it simplifies the code.

Depends on D162219

Severity: -- → S2
Priority: -- → P2
Pushed by ystartsev@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/879667e24ed7 Prep: Use nsISerialEventTarget for WorkerScriptLoader classes; r=dom-workers-and-storage-reviewers,asuth https://hg.mozilla.org/integration/autoland/rev/9a5e5a1b529d Introduce ThreadSafeRequestHandle; r=asuth https://hg.mozilla.org/integration/autoland/rev/eafec005c1bf Re-introduce Cycle Collected LoadContextBase for Workers; r=jonco https://hg.mozilla.org/integration/autoland/rev/6118d850a96d Re-introduce channel tracking for cancellation; r=asuth https://hg.mozilla.org/integration/autoland/rev/3605681ebb5c Initial cancellation revert; r=asuth https://hg.mozilla.org/integration/autoland/rev/5e08534b0119 Re-introduce ScriptLoaderRunnable; r=asuth https://hg.mozilla.org/integration/autoland/rev/03e408b6ebbf Re-introduce ArrayView; r=asuth https://hg.mozilla.org/integration/autoland/rev/dfe78c41e4eb Re-implement DispatchProcessPendingRequests; r=asuth https://hg.mozilla.org/integration/autoland/rev/1034aeb0f8e2 Revert mCacheCreator to being defined as part of ScriptLoaderRunnable; r=asuth
Regressions: 1801344
Regressions: 1801619
Regressions: 1801833
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: