1769913 - Investigate better options for WorkerDebuggeeRunnables, in particular MessageEventRunnable

Status: RESOLVED FIXED

(Core :: DOM: Workers, task)

Description

[Jens Stutte [:jstutte]]

Currently we have two conflicting goals for WorkerDebuggeRunnables sent to the parent:

1. We need to be able to throttle them, in particular if the debugger is running
2. Events posted to the parent should be delivered even if the worker is dying

WorkerRunnable seems to be designed to always have a living WorkerPrivate even during Run on a non-worker thread. This design forced us to keep the worker alive in WorkerDebuggeeRunnable through a StrongWorkerRef until the event is actually run.

In case of a postMessage to the parent this should not be necessary, as the worker could well close immediately after having dispatched the message to the parent but before the parent actually run it. All the information needed to accept the message should be frozen at the moment of dispatch.

This opens the question if we could have a WorkerParentRunnable base class that accounts for the differences between runnables meant to run on the worker and not, ensuring that we expect its WorkerPrivate to be alive only during Dispatch but not during Run, freezing all needed information during the dispatch.

Comment 1

[Jens Stutte [:jstutte]]

Attachment: WIP: Bug 1769913: Use a weak worker reference for WorkerDebuggeeRunnable.

Some of the WorkerDebuggeeRunnables are dispatched to a throttled event queue on the main thread. They used to carry with them a StrongWorkerRef, which prevented the worker to be ever shut down while we wait for their processing, which might never happen.
We add now a ThreadSafeWeakWorkerRef and use it here to determine, if the runnable is still meaningful on execution by canceling it in case the worker died.
We believe that cancelling is sufficient to avoid its mWorkerPrivate to be touched after it has been freed.

Comment 2

[Jens Stutte [:jstutte]]

The patch here is just a proof of concept that canceling the runnable alone will lead to problems. And we might want to extract the ThreadSafeWeakWorkerRef changes into a separate patch.

Comment 3

[Jens Stutte [:jstutte]]

This will probably be part of a WorkerRunnable/WorkerDebuggeeRunnable refactoring we plan to do. :asuth, can you please tie the relevant bugs together?

Comment 4

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

When bug 1836700 (spun off from bug 1800659) removes the busy count, I think we can potentially get rid of the need for a special base class for the runnables.

That said, MessageEventRunnable by spec emits events on the Worker binding which means that it needs a tear-off threadsafe object that holds a reference to the non-threadsafe Worker binding so it can acquire a reference to the holder on the worker thread. dom/fs/TargetPtrHolder.h has an implementation of this right now but we probably need to put the work in to properly create a document version in XPCOM as proposed in bug 1805830. Right now the WorkerPrivate is serving that role (in addition to the busy count stuff).

Comment 5

[Eden Chuang[:edenchuang]]

Most of WorkerRunnables that are dispatched to the parent thread keep a raw pointer of WorkerPrivate just for the thread correctness checking. Therefore, for this kind of WorkerRunnable, keeping a WorkerPrivate raw pointer is not necessary if we can find an alternative way for thread correctness checking.
So we can say, these WorkerRunnables could be extracted from WorkerRunnable to a new base class WorkerParentThreadRunnable.
And just let the subclasses of WorkerParentThreadRunnable determine if it needs a WorkerPrivate raw pointer or not.

BusyCount modification runnable is a kind of WorkerRunnable on the parent thread that needs a WorkerPrivate raw pointer. However, bug 1836700 will remove the busy count mechanism in WorkerPrivate. We can base on the result of bug 1836700 to ease the implementation of this bug.

Comment 6

[Jens Stutte [:jstutte]]

Bug 1872913 contains an interesting case for such a shutdown hang as of comment 0.

It seems we post the CancelingOnParentRunnable (holding the strong worker ref that blocks our shutdown) to the worker's mMainThreadDebuggeeEventTarget but an incoming nsGlobalWindowInner::Suspend pauses that queue before it will ever be executed. I wonder if the CancelingOnParentRunnable should better be dispatched directly to the main thread queue? Or we need to drain/unpause the throttled queue on Cancel (but I fear that could always race with asynchronous events causing it to pause again) ?

Comment 7

[Eden Chuang[:edenchuang]]

Attachment: Bug 1769913 - P1 Make WorkerRunnable to be a base class for runnables on Worker thread and Worker's parent thread. r=#dom-worker-reviewers

Comment 8

[Eden Chuang[:edenchuang]]

Attachment: Bug 1769913 - P2 WorkerParentThreadRunnable for the runnables dispatched to worker's parent thread. r=#dom-worker-reviewers

Depends on D205178

Comment 9

[Eden Chuang[:edenchuang]]

Attachment: Bug 1769913 - P3 Remove WorkerRunnable::mWorkerPrivate. r=#dom-worker-reviewers

Depends on D205679

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/379cd5044768
P1 Make WorkerRunnable to be a base class for runnables on Worker thread and Worker's parent thread. r=dom-worker-reviewers,asuth
https://hg.mozilla.org/integration/autoland/rev/ffc69aa8f2c3
P2 WorkerParentThreadRunnable for the runnables dispatched to worker's parent thread. r=dom-worker-reviewers,asuth
https://hg.mozilla.org/integration/autoland/rev/7802e5f1bd13
P3 Remove WorkerRunnable::mWorkerPrivate. r=dom-worker-reviewers,asuth

Comment 11

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/379cd5044768
https://hg.mozilla.org/mozilla-central/rev/ffc69aa8f2c3
https://hg.mozilla.org/mozilla-central/rev/7802e5f1bd13

Comment 12

[Nicolas Chevobbe [:nchevobbe]]

Perfherder has detected a devtools performance change

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
25%	damp source-map-loader.getOriginalLocation.DAMP	macosx1015-64-shippable-qr	e10s fission stylo webrender-sw	144.19 -> 107.91
24%	damp source-map-loader.getOriginalLocation.DAMP	macosx1015-64-shippable-qr	e10s fission stylo webrender	147.16 -> 111.54
21%	damp source-map-loader.getOriginalLocation.DAMP	windows10-64-shippable-qr	e10s fission stylo webrender-sw	191.73 -> 151.49
21%	damp source-map-loader.getOriginalLocation.DAMP	windows10-64-shippable-qr	e10s fission stylo webrender	190.98 -> 151.81

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests.

If you need the profiling jobs you can trigger them yourself from treeherder job view or ask a sheriff to do that for you.

You can run these tests on try with ./mach try perf --alert 42345

For more information on performance sheriffing please see our FAQ.