ServiceWorker event dispatch should not go through the main thread of the content process
Categories
(Core :: DOM: Service Workers, enhancement, P2)
Tracking
()
People
(Reporter: asuth, Assigned: edenchuang)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
Attachments
(6 files)
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review |
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.)
Reporter | ||
Updated•3 years ago
|
Reporter | ||
Comment 1•3 years ago
•
|
||
: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.
Assignee | ||
Comment 2•9 months ago
|
||
Updated•9 months ago
|
Assignee | ||
Comment 3•9 months ago
|
||
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
Assignee | ||
Comment 4•8 months ago
|
||
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
Assignee | ||
Comment 5•8 months ago
|
||
Depends on D198005
Assignee | ||
Comment 6•8 months ago
|
||
Depends on D198022
Assignee | ||
Comment 7•8 months ago
|
||
Depends on D198029
Comment 9•3 months ago
|
||
Backed out for causing servicewoker related perma failures
Backout link: https://hg.mozilla.org/integration/autoland/rev/f75273742b59bde984e44be59057c880e946ddb3
Assignee | ||
Updated•3 months ago
|
Description
•