1742438 - Use ScriptLoadRequest in Workers

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, task, P3)

Description

[Yulia Startsev [:yulia]]

ScriptLoadRequest has a lot of common functionality with ScriptLoadInfo. There are some patterns in the Worker implementation that make this difficult to use, but those can be addressed. We will be able to easily reuse the ModuleLoader after this.

Comment 1

[Yulia Startsev [:yulia]]

Attachment: WIP: Bug 1742438 - (WIP) refactor worker scriptLoadInfo as ScriptLoadRequest - intermediate patch

Depends on D134041

Comment 2

[Yulia Startsev [:yulia]]

Attachment: WIP: Bug 1742438 - (WIP) use ScriptLoadRequestList in worker ScriptLoaderRunnable

Depends on D134042

Comment 3

[Yulia Startsev [:yulia]]

Attachment: WIP: Bug 1742438 - (WIP) Implement use of mScriptData in Workers

Depends on D134043

Comment 4

[Yulia Startsev [:yulia]]

Attachment: WIP: Bug 1742438 - (WIP) stop using custom worker fields for general script loading behavior

Depends on D134044

Comment 5

[Yulia Startsev [:yulia]]

Attachment: WIP: Bug 1742438 - (WIP) Exclusively use ScriptLoadRequest in Worker ScriptLoader

Depends on D134045

Comment 6

[Yulia Startsev [:yulia]]

Attachment: WIP: Bug 1742438 - (WIP) remove mChannel use for cancellation in Worker ScriptLoader

Cancel using the same pattern as in DOM ScriptLoader

Depends on D134046

Comment 7

[Yulia Startsev [:yulia]]

Attachment: WIP: Bug 1742438 - (WIP) remove nsflags as part of loadinfo reduction

Depends on D134047

Comment 8

[Yulia Startsev [:yulia]]

Attachment: WIP: Bug 1742438 - (WIP) remove mReservedInfo from worker ScriptLoader LoadInfo

Depends on D134048

Comment 9

[Yulia Startsev [:yulia]]

Attachment: WIP: Bug 1742438 - (WIP) Cleanup naming of refactored objects

To be broken up into the appropriat spots

Depends on D134049

Comment 10

[Yulia Startsev [:yulia]]

Attachment: Bug 1742438 - Prep Part 1: Use references, not addresses for ScriptLoadInfo; r=asuth

The goal of this patch stack is to transition from using the ScriptLoadInfo to represent script
requests for workers, to ScriptLoadRequest and its partner datastructure ScriptLoadRequestList.
There are a number of differences between these two datastructures, but they fundamentally represent
the same thing: a request which is either about to be loaded, or about to be executed.

To get there, in a reasonable manner, is a bit tricky. ScriptLoadRequest et all are refcounted, and
restrict which thread can delete or add them. The decision I've made in this transformation is to
make the Worker thread the home of ScriptLoadRequests. This means that we have to change how we are
doing things on the WorkerScriptLoader so that the MainThread is largely unaware of the
ScriptLoadRequestList, and it only operates in terms of ScriptLoadRequests (or, ScriptLoadInfos as
they are known for now)

The following need to be changed:

• We need to prepare everything to operate on pointers (Prep Patches 1-3)
• Cancellation must no longer iterate over a list (Prep patches 4-8, removing the mChannel reference
  is a bonus)
• List management must only be done on the Worker side (Prep Patches 9-10)

This first patch transitions from using the ScriptLoadInfo directly to passing around references.

You will find a similar explaination for the actual transition in the patches that follow the prep
patches. This is bundled in the same bug as the goal here remains the same.

Depends on D145439

Comment 11

[Yulia Startsev [:yulia]]

Attachment: Bug 1742438 - Prep Part 2: Create an array of references for ScriptLoadInfos; r=asuth

This patch introduces a temporary array for the purposes of this refactoring. First, I'll
demonstrate the new loading strategy using the nsTArray datastructure, before moving everything over
to a ScriptLoadRequestList and ScriptLoadRequest datastructure. The impact of this change will only
exist for the course of this bug.

Depends on D145441

Comment 12

[Yulia Startsev [:yulia]]

Attachment: Bug 1742438 - Prep Part 3: Make Executable list of ScriptLoadInfos into a list of references; r=asuth

Much like Part 2, we are replacing the prior Span structure with a list of pointers. This also
reflects the ultimate ScriptLoadRequestList implementation: We will be modifying lists, moving the
head of one list to the tail of the other, in order to preserve the ImportScripts invariant around
post order == evaluation order.

Depends on D145442

Comment 13

[Yulia Startsev [:yulia]]

Attachment: WIP: Bug 1742438 - Prep Part 4: Make LoadHandlers inherit from nsIIncrementalStreamLoader

This is actually optional. The reason I included it is that it might improve the cancellation story
a little bit, and it brings us closer to the DOM ScriptLoadHandler.

What this does: It transitions both {Cache,Network}LoadHandler to inheriting from
nsIIncrementalStreamLoader. In doing this, we will be able to incrementally parse (which we are not
doing, but would likely be beneficial 1), but more importantly, eagerly abort consuming data.
This directly impacts part 5 of the prep patch queue.

It isn't strictly necessary, if this looks too weird we can omit it.

Depends on D145443

Comment 14

[Yulia Startsev [:yulia]]

Attachment: WIP: Bug 1742438 - Prep Part 5: Remove ScriptResponseHeaderProcessor{.h,.cpp}

As a direct result of Prep Part 4, ScriptResponseHeaderProcessor is no longer necessary.

Depends on D145444

Comment 15

[Yulia Startsev [:yulia]]

Attachment: Bug 1742438 - Prep Part 4: Change cancellation to not rely on iterating over mLoadingRequests r=asuth

The joys of multi-threaded programming, in other words.

As you likely noticed, we now have list modification where previously we didn't.
We need to do this list modification on the worker side, not the main thread side.
The only reason this isn't showing up on thread sanitizer now, is because the main
thread is acting as the owner of the the lists.

In order to implement Worker ownership, we need to remove all list traversal from the main thread
runnable. There are two significant cases in which we traverse lists: Cancellation, and Dispatching.
Here we handle cancellation.

The strategy we take is fundamentally the same as in the DOM Script Loader -- We use the
"mCanceledMainThread" flag to indicate to our load handlers that the thread has been cancelled. The
cancellation is then handled in onStreamCompleted and OnIncrementalData. This is taken directly from
the DOM loader 1.

Depends on D145445

Comment 16

[Yulia Startsev [:yulia]]

Attachment: Bug 1742438 - Prep Part 5: Remove mChannel from ScriptLoadInfo; r=asuth

This is mostly a freebee: Since we no longer use mChannel for cancellation, it can disappear.

This cancellation pattern was introduced in 1. It prevents a copy2, which as mentioned in the prior
patch, we can achieve in using OnIncrementalData.

Depends on D145446

Comment 17

[Yulia Startsev [:yulia]]

Attachment: Bug 1742438 - Prep Part 6: CacheCreator is managed by ScriptLoadInfo, not WorkerScriptLoader; r=asuth

This follows up the cancellation requirements -- CacheCreator is a main thread only datastructure,
which must be cleaned up once the last ScriptLoadInfo is dispatched. Previously, we determined this
via iteration. Now, we manage it as part of ScriptLoadInfo, and release our reference to it when we
dispatch a given ScriptLoadInfo. Later, once we transition to ScriptLoadRequest and LoadContexts,
the cache information can live on a ServiceWorkerLoadContext1. This will help use differentiate
between regular worker loader environments and service worker environments.

Depends on D145447

Comment 18

[Yulia Startsev [:yulia]]

Attachment: Bug 1742438 - Prep Part 7: Move list management to the worker side; r=asuth

This is the last piece that we need in order to move ahead with managing the lists by the worker
thread. This patch moves the last iteration over mLoadingRequests over to the worker thread.

Depends on D145448

Comment 19

[Yulia Startsev [:yulia]]

Attachment: Bug 1742438 - Prep Part 8: Implement all scripts executed on WorkerScriptLoader; r=asuth

Last item before moving ahead with ScriptLoadRequest: tracking if the scripts have been executed can
now be done on the worker side without information from the main thread. This also introduces a name
change: We are really interested in whether the scripts are all totally finished whatever they were
doing, not if they are ready to be run.

Depends on D145449

Comment 20

[Yulia Startsev [:yulia]]

Attachment: Bug 1742438 - Prep Part 9: Encode URIs on worker script loader creation; r=asuth

This step is not strictly necessary, and if this is too ugly we can think of something else. The
reason for this change is that ScriptLoadRequests are initialized by a URI, and the owner of these
objects will be the worker thread. However, the current strategy requires that we create the URI
only on the main thread. We always have this information however, when we create the loader, so
there isn't any reason to defer this step until we bounce back into the main thread.

This is another spot where we can reduce reliance on the main thread, which is an eventual goal, so
it seems like this change is an improvement even outside of the goal of moving to ScriptLoadRequests
as a representation.

Depends on D145450

Comment 21

[Yulia Startsev [:yulia]]

Attachment: Bug 1742438 - Prep Part 10: Make TriggeringPrincipal optional for ScriptFetchOptions; r=jonco

I didn't realize that this would only be used in the DOM script loader, so this is now optional. In
a later stage we might move this over to the ScriptLoadContext as it looks like this might need to
propogate through the module script tree.

Depends on D146170

Comment 22

[Yulia Startsev [:yulia]]

Attachment: Bug 1742438 - Part 1: Use ScriptLoadRequestList and have ScriptLoadInfo inherit from ScriptLoadRequest; r=asuth

This section of the patch queue starts the migration from ScriptLoadInfo to
ScriptLoadRequest/LoadContext pairs.

We will be making use of the ModuleLoader, and in the future we will have a unified ScriptLoader
implementation (currently, the skeleton for this is in ScriptLoaderInterface). ScriptLoadRequest has
been rewritten to be generic to all contexts, with a companion object "LoadContext" that will handle
specialized behavior required to load the request. That is exactly the case we have with workers,
most of the fields are generic with a couple of specialized fields associated with service workers.

The first part of this focuses on getting things into place without using them, we rely on
ScriptLoadInfo (later renamed to WorkerLoadContext) for patches 1-7. Then we migrate all shared
data to the standard utilization used by other loaders (pathes 8-13). The final patch is a small
cleanup, as we can split WorkerLoadContext into WorkerLoadContext and ServiceWorkerLoadContext at
some future point.

Depends on D146172

Comment 23

[Yulia Startsev [:yulia]]

Attachment: Bug 1742438 - Part 2: Rename ScriptLoadInfo references from {a,m}LoadInfo to {a,m}Request; r=asuth

Preparation for using ScriptLoadRequest: Doing the rename here so it doesn't make it confusing later
on.

Depends on D146173

Comment 24

[Yulia Startsev [:yulia]]

Attachment: Bug 1742438 - Part 3: Split ScriptLoadInfo into ScriptLoadRequest and ScriptLoadInfo (as a LoadContext) classes; r=asuth,jonco

Here we split between "what is being loaded" and "how it is being loaded in a worker context" with
two classes representing what ScriptLoadInfo used to represent.

Depends on D146174

Comment 25

[Yulia Startsev [:yulia]]

Attachment: Bug 1742438 - Part 4: Rename ScriptLoadInfo files to WorkerLoadContext; r=asuth

Depends on D146175

Comment 26

[Yulia Startsev [:yulia]]

Attachment: Bug 1742438 - Part 5: Rename ScriptLoadInfo to WorkerLoadContext; r=asuth

Depends on D146176

Comment 27

[Yulia Startsev [:yulia]]

Attachment: Bug 1742438 - Part 6: Factor out ScriptDecoder class from ScriptLoadHandler; r=arai,smaug

This patch splits out the decoding from the ScriptLoadHandler. This will be reused by the worker
ScriptLoader for loading scripts via both NetworkLoadHandler and CacheLoadHandler.

Depends on D146177

Comment 28

[Yulia Startsev [:yulia]]

Attachment: WIP: Bug 1742438 - Part 7: Move ScriptDecoder methods together

To avoid difficulty reviewing, this has been split out a separate change. We are just moving one
method here to be with other ScriptDecoder methods.

Depends on D146178

Comment 29

[Yulia Startsev [:yulia]]

Attachment: Bug 1742438 - Part 8: Use mScriptData instead of custom load context field; r=arai,asuth

This is the most substantial change. ScriptLoadRequests can have their data incrementally loaded, so
it is already fully decoded and ready to go by the time that we create the source buffer for worker
scripts. This simplifies some of the code, and we can add incremental loading when we are ready.

Depends on D146179

Comment 30

[Yulia Startsev [:yulia]]

Attachment: Bug 1742438 - Part 9: Replace mLoadingFinished with State::Ready in ScriptLoadRequest; r=asuth

The "mLoadingFinished" state that we are using a boolean for can be represented by State::Ready in
ScriptLoadRequest.

Depends on D146180

Comment 31

[Yulia Startsev [:yulia]]

Attachment: Bug 1742438 - Part 10: Remove mExecutionResult; r=asuth

This field is no longer necessary, as we are removing executed scripts from our list of scripts to
execute, so we cannot enter a state where something may be executed twice.

Depends on D146181

Comment 32

[Yulia Startsev [:yulia]]

Attachment: Bug 1742438 - Part 11: Replace Finished() check with IsAwaitingPromise() and move state change to worker thread; r=asuth,jonco

This adds a state to executions in ScriptLoadRequest, allowing us to indicate that we have entered a
new point in the state machine.

Depends on D146182

Comment 33

[Yulia Startsev [:yulia]]

Attachment: Bug 1742438 - Part 12: Remove WorkerLoadContext mSourceMapURL and use ScriptLoadRequest mSourceMapURL; r=asuth

The simplest change: these two fields were identical across the representations

Depends on D146183

Comment 34

[Yulia Startsev [:yulia]]

Attachment: Bug 1742438 - Cleanup: Move generic WorkerLoadContext fields together, document WorkerLoadContext; r=asuth

And we are done!

Depends on D146184

Comment 35

[Yulia Startsev [:yulia]]

Attachment: Bug 1742438 - Part 7: Make ScriptDecoder an independent class rather than a base class; r=arai,smaug

Now that the Worker ScriptLoader largely conforms to the same shape as the DOM ScriptLoader, and
represents requests in the same way, we can start sharing some code. The ScriptLoadHandler isn't
entirely common to both cases, as it handles preloads (which are not possible in workers). This
patch splits out the common class, ScriptDecoder, as it's own thing. A demonstration of its use is
in Part 8 of this patch queue.

Depends on D146178

Comment 36

[Yulia Startsev [:yulia]]

Attachment: Bug 1742438 - Cleanup: Remove nsTArrayView; r=asuth

This class is no longer used, after the removal from the WorkerScriptLoader. It can be removed,
unelss we want to keep it for future use. This cleanup is optional.

Depends on D146185

Comment 37

[Yulia Startsev [:yulia]]

Attachment: Bug 1742438 - Part 13: Use ScriptLoadRequest's mURL instead of WorkerLoadContext's; r=asuth,jonco

Depends on D146184

Comment 38

[Yulia Startsev [:yulia]]

Attachment: WIP: Bug 1742438 - Part X: Use Load Context as a cross thread refcounted object

Depends on D146175

Comment 39

[Yulia Startsev [:yulia]]

Attachment: WIP: Bug 1742438 - Part X: Move Cycle Collection from LoadContextBase to derived classes

Depends on D149722

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d556d3775e9e
Prep Part 1: Use references, not addresses for ScriptLoadInfo; r=asuth
https://hg.mozilla.org/integration/autoland/rev/ac92b01963ab
Prep Part 2: Create an array of references for ScriptLoadInfos; r=asuth
https://hg.mozilla.org/integration/autoland/rev/a07a748d5fd8
Prep Part 3: Make Executable list of ScriptLoadInfos into a list of references; r=asuth
https://hg.mozilla.org/integration/autoland/rev/8be847627149
Prep Part 4: Change cancellation to not rely on iterating over mLoadingRequests r=asuth
https://hg.mozilla.org/integration/autoland/rev/ce97d6f62b05
Prep Part 5: Remove mChannel from ScriptLoadInfo; r=asuth
https://hg.mozilla.org/integration/autoland/rev/c59a40e54e30
Prep Part 6: CacheCreator is managed by ScriptLoadInfo, not WorkerScriptLoader; r=asuth
https://hg.mozilla.org/integration/autoland/rev/a835a0f73714
Prep Part 7: Move list management to the worker side; r=asuth
https://hg.mozilla.org/integration/autoland/rev/b0c14823e829
Prep Part 8: Implement all scripts executed on WorkerScriptLoader; r=asuth
https://hg.mozilla.org/integration/autoland/rev/492bd49620ae
Prep Part 9: Encode URIs on worker script loader creation; r=asuth
https://hg.mozilla.org/integration/autoland/rev/66940c72c03b
Prep Part 10: Make TriggeringPrincipal optional for ScriptFetchOptions; r=jonco
https://hg.mozilla.org/integration/autoland/rev/367679be762e
Part 1: Use ScriptLoadRequestList and have ScriptLoadInfo inherit from ScriptLoadRequest; r=asuth
https://hg.mozilla.org/integration/autoland/rev/cf65036c47de
Part 2: Rename ScriptLoadInfo references from {a,m}LoadInfo to {a,m}Request; r=asuth
https://hg.mozilla.org/integration/autoland/rev/afdf554510e5
Part 3: Split ScriptLoadInfo into ScriptLoadRequest and ScriptLoadInfo (as a LoadContext) classes; r=asuth,jonco
https://hg.mozilla.org/integration/autoland/rev/14eb61468ec7
Part 4: Rename ScriptLoadInfo files to WorkerLoadContext; r=asuth
https://hg.mozilla.org/integration/autoland/rev/1428ff83cb43
Part 5: Rename ScriptLoadInfo to WorkerLoadContext; r=asuth
https://hg.mozilla.org/integration/autoland/rev/dea6f4bb3494
Part 6: Factor out ScriptDecoder class from ScriptLoadHandler; r=arai
https://hg.mozilla.org/integration/autoland/rev/adc701ccb87c
Part 7: Make ScriptDecoder an independent class rather than a base class; r=arai
https://hg.mozilla.org/integration/autoland/rev/06cd5943a51c
Part 8: Use mScriptData instead of custom load context field; r=arai,asuth
https://hg.mozilla.org/integration/autoland/rev/26f520aa56a4
Part 9: Replace mLoadingFinished with State::Ready in ScriptLoadRequest; r=asuth
https://hg.mozilla.org/integration/autoland/rev/d81f6ead8f9c
Part 10: Remove mExecutionResult; r=asuth
https://hg.mozilla.org/integration/autoland/rev/151e92f813fc
Part 11: Replace Finished() check with IsAwaitingPromise() and move state change to worker thread; r=asuth,jonco
https://hg.mozilla.org/integration/autoland/rev/23ea9963b359
Part 12: Remove WorkerLoadContext mSourceMapURL and use ScriptLoadRequest mSourceMapURL; r=asuth
https://hg.mozilla.org/integration/autoland/rev/3a1faab53f2b
Part 13: Use ScriptLoadRequest's mURL instead of WorkerLoadContext's; r=asuth,jonco,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/bfe2e791de6e
Cleanup: Move generic WorkerLoadContext fields together, document WorkerLoadContext; r=asuth
https://hg.mozilla.org/integration/autoland/rev/4120c49b58c6
Cleanup: Remove nsTArrayView; r=xpcom-reviewers,barret

Comment 41

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/d556d3775e9e
https://hg.mozilla.org/mozilla-central/rev/ac92b01963ab
https://hg.mozilla.org/mozilla-central/rev/a07a748d5fd8
https://hg.mozilla.org/mozilla-central/rev/8be847627149
https://hg.mozilla.org/mozilla-central/rev/ce97d6f62b05
https://hg.mozilla.org/mozilla-central/rev/c59a40e54e30
https://hg.mozilla.org/mozilla-central/rev/a835a0f73714
https://hg.mozilla.org/mozilla-central/rev/b0c14823e829
https://hg.mozilla.org/mozilla-central/rev/492bd49620ae
https://hg.mozilla.org/mozilla-central/rev/66940c72c03b
https://hg.mozilla.org/mozilla-central/rev/367679be762e
https://hg.mozilla.org/mozilla-central/rev/cf65036c47de
https://hg.mozilla.org/mozilla-central/rev/afdf554510e5
https://hg.mozilla.org/mozilla-central/rev/14eb61468ec7
https://hg.mozilla.org/mozilla-central/rev/1428ff83cb43
https://hg.mozilla.org/mozilla-central/rev/dea6f4bb3494
https://hg.mozilla.org/mozilla-central/rev/adc701ccb87c
https://hg.mozilla.org/mozilla-central/rev/06cd5943a51c
https://hg.mozilla.org/mozilla-central/rev/26f520aa56a4
https://hg.mozilla.org/mozilla-central/rev/d81f6ead8f9c
https://hg.mozilla.org/mozilla-central/rev/151e92f813fc
https://hg.mozilla.org/mozilla-central/rev/23ea9963b359
https://hg.mozilla.org/mozilla-central/rev/3a1faab53f2b
https://hg.mozilla.org/mozilla-central/rev/bfe2e791de6e
https://hg.mozilla.org/mozilla-central/rev/4120c49b58c6