Bug 1799201 Comment 17 Edit History

Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.


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

***
Bug 1800496 - Introduce ThreadSafeRequestHandle; r=asuth


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.


***
Bug 1800496 - Re-introduce Cycle Collected LoadContextBase for Workers; r=jonco


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

***
Re-introduce channel tracking for cancellation; r=asuth


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.

***
Bug 1800496 - Initial cancellation revert; r=asuth


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.

***
Bug 1800496 - Re-introduce ScriptLoaderRunnable; r=asuth


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.

***
Bug 1800496 - Re-introduce ArrayView; r=asuth


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.

***
Bug 1800496 - Re-implement DispatchProcessPendingRequests; r=asuth


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.

***
Bug 1800496 - Revert mCacheCreator to being defined as part of ScriptLoaderRunnable; r=asuth


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.

***
Bug 1801344 - fix iteration over ThreadSafeRequestHandler; r=asuth


This fixes a build error. It was a bit tricker than I expected, comments are inline.

***
Bug 1801616 - Cancellation should abort network and cache loads if it has already completed;

***
Bug 1801833 - ensure release request is only ever done in a lock; r=asuth

Back to Bug 1799201 Comment 17