1672493 - ServiceWorker event dispatch should not go through the main thread of the content process

Status: NEW

(Core :: DOM: Service Workers, enhancement, P2)

Description

[Andrew Sutherland [:asuth] (he/him)]

As expressed in bug 1672491, ServiceWorker "ops" including fetch ops are bounced off the main thread of the content process from the RemoteWorkerService "Worker Launcher" thread due to invariants about the main thread needing to be the owner of any top-level worker. This is hypothesized to be less than awesome for performance and the most likely point of contention with other browser activity and should be addressed.

Note that in theory this impacts SharedWorkers too, but the reality is that SharedWorkers' API surface is exposed via MessageChannel and so all steady-state interactions with a SharedWorker don't contend with the main thread. (However, binding to an existing SharedWorker will involve the main thread.)

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

:edenchuang and :jesup and I just had a quick talk on this and another potential means of taking the main thread out of latency was identified.

PRemoteWorker direct to Worker thread for non-lifecycle operations

• Currently PRemoteWorker is used to connect the RemoteWorkerManager in the IPDL Background thread to the RemoteWorkerService in the Worker Launcher thread on a content process. As operations are received in the Worker Launcher thread, they are dispatched by way of the main thread to the worker thread proper.
  • This provides a homogenous data path for the lifecycle operations of spawning the worker and terminating the worker as well as the non-lifecyle operations like "fetch" events, "message" events, etc.
• Instead, the PRemoteWorker connections could be established for non-lifecycle events directly to the worker, allowing "fetch" and "message" events to be dispatched directly to the worker.
• This would mean that lifecycle events would need to be split into their own protocol. The existing PRemoteWorkerService could be used for this purpose. This would cover both spawning and termination.
  • Arguably there are advantages to having an actor whose lifecycle corresponds to the length of the worker (versus passing identifiers back and forth). So it might make sense to create a PRemoteWorkerParent which could be managed by PRemoteWorkerService for additional clarity. (I'm not 100% clear on why PRemoteWorker wasn't already managed by PRemoteWorkerService at this moment.)

The main logistical complication is that non-lifecycle operations could be directly sent over PRemoteWorker as soon as it was constructed as part of the lifecycle action of spawning the ServiceWorker. But under this new split-protocol scheme, the operations would not be able to be sent until PRemoteWorker has registered itself with the parent process (and this would potentially require a new rendezvous via identifier mechanism unless IPC enhancements related to endpoints allow the worker spawning to pass an endpoint down over PRemoteWorkerService to be used to provide a pre-named/pre-allocated RemoteWorkerParent to be established from a different thread in the content process and thereby via a different PBackground parent/child pair).

Benefits

• This would allow us to side-step bug 1672491 as a blocking dependency. Until bug 1672491 is fixed, there would of course potentially be some amount of main-thread contention experienced when spawning the ServiceWorker, but the steady state operation would not involve the main thread.
• Thread hopping elimination:
  • Relative to the current state of the tree: this eliminates the hop from the worker launcher thread to the main thread and then to the worker.
  • Relative to the proposal from comment 0: this eliminates the hop from the worker launcher thread to the worker. This is less a performance win, as the worker launcher thread should never be contended, but would represent a meaningful simplification in implementation and conceptual complexity as all non-lifecycle events would only be dealing with a single thread.

Comment 2

[Eden Chuang[:edenchuang]]

Attachment: Bug 1672493 - P1 Let PRemoteWorkerService manage PRemoteWorker. r=asuth

Comment 3

[Eden Chuang[:edenchuang]]

Attachment: Bug 1672493 - P2 Create IPC PRemoteWorkerNonLifeCycleOpController. r=asuth

In this patch, IPC PRemoteWorkerNonLifeCycleOpController is created for non-life cycle operations of SharedWorker/ServiceWorker.

PRemoteWorkerNonLifeCycleOpController is the IPC between the content process worker thread and the parent process background thread.
This IPC helps to build a direct communication instead of go through worker launcher thread and the main thread.

When a worker gets into Running status, PRemoteWorkerNonLifeCycleOpControllerChild/Parent is created from content process,
and PRemoteWorkerNonLifeCycleOpControllerParent would binds to the coresponding RemoteWorkerController by re-using RemoteWorkerManager to find the the target RemoteWorkerServiceParent.
After connection is built, RemoteWorkerController can send non-life cycle related operations by PRemoteWorkerNonLifeCycleOpController and send life cycle related operations by PRemoteWorker.

Depends on D196946

Comment 4

[Eden Chuang[:edenchuang]]

Attachment: Bug 1672493 - P3 Move RemoteWorker::Op and SharedWorkerOp out from RemoteWorkerChild. r=asuth

To reuse the code of RemoteWorker::State and RemoteWorker::Op in RemoteWorkerNonLifeCycleOpControllerChild, this patch isolates these reused codes into a new file RemoteWorkerOp.h. And also extract RemoteWorker::SharedWorkerOp into SharedWorkerOp.h.cpp

Depends on D197563

Comment 5

[Eden Chuang[:edenchuang]]

Attachment: Bug 1672493 - P4 Move ShareWorker Non-life cycle related operations from RemoteWorker to RemoteWorkerNonLifeCycleOpController. r=asuth

Depends on D198005

Comment 6

[Eden Chuang[:edenchuang]]

Attachment: Bug 1672493 - P5 Move ServiceWorker Non-life cycle related operations from RemoteWorker to RemoteWorkerNonLifeCycleController, excepts FetchEventOp. r=asuth

Depends on D198022

Comment 7

[Eden Chuang[:edenchuang]]

Attachment: Bug 1672493 - P6 Manage PFetchEventOpProxy by PRemoteWorkerNonLifeCycleOpController. r=asuth

Depends on D198029

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e80cf5cf2fe5
P1 Let PRemoteWorkerService manage PRemoteWorker. r=asuth
https://hg.mozilla.org/integration/autoland/rev/e05276c441ef
P2 Create IPC PRemoteWorkerNonLifeCycleOpController. r=asuth
https://hg.mozilla.org/integration/autoland/rev/4f1ee5c1e441
P3 Move RemoteWorker::Op and SharedWorkerOp out from RemoteWorkerChild. r=asuth
https://hg.mozilla.org/integration/autoland/rev/b4e568bfb9c7
P4 Move ShareWorker Non-life cycle related operations from RemoteWorker to RemoteWorkerNonLifeCycleOpController. r=asuth
https://hg.mozilla.org/integration/autoland/rev/a38e64bdda2c
P5 Move ServiceWorker Non-life cycle related operations from RemoteWorker to RemoteWorkerNonLifeCycleController, excepts FetchEventOp. r=asuth

Comment 9

[Sandor Molnar[:smolnar]]

Backed out for causing servicewoker related perma failures

Backout link: https://hg.mozilla.org/integration/autoland/rev/f75273742b59bde984e44be59057c880e946ddb3

Push with failures

Failure log -> TEST-UNEXPECTED-FAIL | dom/serviceworkers/test/test_devtools_track_serviceworker_time.html | assertion count 1 is more than expected 0 assertions

Failure log -> TEST-UNEXPECTED-FAIL | /html/cross-origin-embedder-policy/reflection-credentialless.tentative.https.any.serviceworker.html | assertion count 1 is more than expected 0 assertions

Failure log -> TEST-UNEXPECTED-FAIL | dom/serviceworkers/test/test_serviceworkerregistrationinfo.xhtml | assertion count 1 is more than expected 0 assertions