WorkerDebugger should live on the main thread.

RESOLVED FIXED in Firefox 46

Status

()

Core
DOM: Workers
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: ejpbruel, Assigned: ejpbruel)

Tracking

(Blocks: 1 bug)

unspecified
mozilla46
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

2 years ago
Currently, the WorkerDebugger for a WorkerPrivate is created on the parent thread in which the WorkerPrivate was created, used on both the main thread and the worker thread, and owned by both the main thread and the worker thread (debugger runnables that originate on the worker thread hold an owning reference to the WorkerDebugger). With this in mind, we decided to make WorkerDebugger thread-safe.

Needless to say, the above situation is somewhat messy, to say the least. Since the lifetime of a WorkerDebugger corresponds to its registration/deregistration on the WorkerDebuggerManager, there is actually never a need for the worker thread to hold a strong reference to it: since the WorkerDebuggerManager holds a strong reference to the WorkerDebugger, as long as the worker has access to it, the WorkerDebugger is guaranteed to be alive.

This patch refactors the WorkerDebugger to a non-thread-safe class that is both created on and owned by the main thread. Only the main thread and the worker thread ever use it. The parent thread is never involved. The idea is that instead of creating the WorkerDebugger on the parent thread and registering it with the WorkerDebuggerManager on the main thread, we make the WorkerDebuggerManager responsible for both steps.

This leads to the following observations:

1. Although the WorkerDebugger is now created and owned by the   
   WorkerDebuggerManager, the WorkerPrivate still needs access to it. The
   WorkerDebuggerManager therefore calls SetDebugger() on the WorkerPrivate
   to give it a weak reference to the debugger. This reference is nulled out
   when the debugger is unregistered.

2. The worker should not start running until after its debugger has been set.
   To accomplish this, the parent thread needs to block until registration is
   complete. Note that is incurs a slight performance penalty compared to the
   previous solution, where the parent thread only had to wait if we had any
   listeners installed.

   I expect this performance penalty will be acceptable, however, because the
   parent thread only actually ever blocks for non-top level workers (for top-
   level-workers, the parent thread is the main thread, so it can call the
   WorkerDebuggerManager directly). Moreover, the worker needs to wait for the
   main thread to load its main script anyway, which I expect to dominate
   startup time by far.

3. Because the WorkerDebugger was created on the parent thread, we used a
   condition variable on the WorkerDebugger to make the parent thread block until
   registration was complete. Now that the WorkerDebugger is created by the
   WorkerDebuggerManager, the parent thread no longer has access to it. However,
   we can simply use the condition variable on WorkerPrivate for the same purpose.
   This allows us to remove the condition variable from WorkerDebugger completely.

4. In most cases, the mWorkerPrivate pointer in WorkerDebugger is used from the
   main thread. In the few cases where it is not, this is done from methods that
   are no longer reachable when the WorkerDebugger is unregistered. Since
   unregistering the WorkerDebugger is a blocking operation that also closes
   the WorkerDebugger, and closing the WorkerDebugger nulls out its
   mWorkerPrivate pointer, there is no need to protected this pointer with a
   mutex.

5. Similarly, the mListeners array on WorkerDebugger is only ever accessed on the
   main thread, either when the user adds/removes a listener to/from it, or when
   the listeners need to be fired. In either case, the mListeners array does not
   have to be mutex protected.

6. Due to 3, 4, and 5, we can get rid of the mutex in WorkerDebugger.

7. Similarly, because all members of WorkerDebuggerManager are only ever accessed
   from the main thread, we can get rid of the mutex in WorkerDebuggerManager.

The end result is a significant simplification of the code:

1. We removed one condition variables, and two mutexes.
2. The WorkerDebugger is now only accessed by the main thread and the worker 
   thread, not the parent thread.
3. Because WorkerDebugger is only owned by the main thread, it no longer needs
   to be thread-safe.
4. Because WorkerDebugger does no longer need to be thread-safe, we can use WebIDL
   bindings with it (something I'd like to do in the future).

The only downside to this refactor is the minor performance penalty mentioned in in 2, which I think is outweighed by the simplifications above.
(Assignee)

Comment 1

2 years ago
Created attachment 8670254 [details] [diff] [review]
WorkerDebugger should live on the main thread.

See comment 1 for details on this patch.
Attachment #8670254 - Flags: review?(khuey)
(Assignee)

Updated

2 years ago
Assignee: nobody → ejpbruel
(Assignee)

Comment 2

2 years ago
I should add that this patch applies on top of the patches in bug 1178726.
Depends on: 1178726
(Assignee)

Updated

2 years ago
Blocks: 1212333
(Assignee)

Updated

2 years ago
Blocks: 1212344
Comment on attachment 8670254 [details] [diff] [review]
WorkerDebugger should live on the main thread.

Review of attachment 8670254 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think that slowing down the non-debugging case by waiting on the main thread is acceptable.  We went through a lot of trouble in bug 757133 to get this right, why would we undo this now?  "Simpler code" is of little benefit when the code is already written.
Attachment #8670254 - Flags: review?(khuey) → review-
(Assignee)

Comment 4

2 years ago
Created attachment 8673730 [details] [diff] [review]
WorkerDebugger should live on the main thread.

I do think there is value in simplifying the code, if for no other reason than to make it more maintainable. That said, I agree that simplifying the code is not valuable enough that it should come at the price of slowing down the non-debugging case.

I've written a new patch that changes WorkerDebugger to live on the main thread, without slowing down the non-debugging case. I had to reintroduce one of the mutexes on WorkerDebuggerManager for this purpose, but the resulting patch is still a significant simplification.
Attachment #8670254 - Attachment is obsolete: true
Attachment #8673730 - Flags: review?(khuey)

Updated

2 years ago
Blocks: 1214248
(Assignee)

Comment 5

2 years ago
Friendly review ping. Let me know if you still have objections to the patch in its current form.
Comment on attachment 8673730 [details] [diff] [review]
WorkerDebugger should live on the main thread.

Review of attachment 8673730 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/WorkerDebuggerManager.cpp
@@ +178,3 @@
>  
>    if (NS_IsMainThread()) {
> +    RegisterDebuggerMainThread(aWorkerPrivate, true);

Why are we always passing true for hasListeners here?

@@ +181,5 @@
>    } else {
> +    bool hasListeners = false;
> +    {
> +      MutexAutoLock lock(mMutex);
> + 

nit: extra whitespace

::: dom/workers/WorkerPrivate.cpp
@@ +3445,5 @@
>  }
>  
>  #endif
>  
> +class PostDebuggerMessageRunnable final : public nsIRunnable

You could just inherit from nsRunnable

@@ +3458,5 @@
> +    mMessage(aMessage)
> +  {
> +  }
> +
> +  NS_DECL_THREADSAFE_ISUPPORTS

then you don't need this

::: dom/workers/WorkerPrivate.h
@@ +979,5 @@
>    static void
>    OverrideLoadInfoLoadGroup(WorkerLoadInfo& aLoadInfo);
>  
> +  void
> +  SetRegistered(bool aIsRegistered)

These need names that mention debugger somewhere ... registered could mean anything.

@@ +992,5 @@
> +    mCondVar.Notify();
> +  }
> +
> +  void
> +  WaitForRegistered(bool aIsRegistered)

and here.

@@ +994,5 @@
> +
> +  void
> +  WaitForRegistered(bool aIsRegistered)
> +  {
> +    AssertIsOnParentThread();

MOZ_ASSERT(!NS_IsMainThread) too, to prevent deadlock.

@@ +998,5 @@
> +    AssertIsOnParentThread();
> +
> +    MutexAutoLock lock(mMutex);
> +
> +    while (mRegistered != aIsRegistered) {

and mRegistered too
Attachment #8673730 - Flags: review?(khuey)
(Assignee)

Comment 7

2 years ago
Created attachment 8680052 [details] [diff] [review]
WorkerDebugger should live on the main thread.

Thanks for the review! I created a new patch with your comments addressed. I added some comments to clarify the parts you had questions about.
Attachment #8673730 - Attachment is obsolete: true
Attachment #8680052 - Flags: review?(khuey)
Comment on attachment 8680052 [details] [diff] [review]
WorkerDebugger should live on the main thread.

Review of attachment 8680052 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

::: dom/workers/WorkerPrivate.h
@@ +994,5 @@
> +
> +  void
> +  WaitForDebuggerRegistered(bool aDebuggerRegistered)
> +  {
> +    AssertIsOnParentThread();

MOZ_ASSERT(!NS_IsMainThread()); too please, to make sure we can't deadlock.
Attachment #8680052 - Flags: review?(khuey) → review+

Comment 9

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/9441286007c4
sorry backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=18892076&repo=mozilla-inbound
Flags: needinfo?(ejpbruel)

Comment 11

2 years ago
Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d51bf9597ea6
(Assignee)

Comment 12

2 years ago
The failing assert is actually faulty and should not be there.

Try push for the patch with that issue addressed:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc3f7e708e56
Flags: needinfo?(ejpbruel)
(Assignee)

Comment 13

2 years ago
I had to add in a check to make sure we don't try to unregister the debugger if it was never succesfully registered.

Here's a new try push with that issue addressed:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=62c393252508

Comment 14

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c498ef6aac98

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c498ef6aac98
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.