Closed
Bug 1211903
Opened 9 years ago
Closed 8 years ago
WorkerDebugger should live on the main thread.
Categories
(Core :: DOM: Workers, defect)
Core
DOM: Workers
Tracking
()
RESOLVED
FIXED
mozilla46
Tracking | Status | |
---|---|---|
firefox46 | --- | fixed |
People
(Reporter: ejpbruel, Assigned: ejpbruel)
References
Details
Attachments
(1 file, 2 obsolete files)
27.10 KB,
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
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•9 years ago
|
||
See comment 1 for details on this patch.
Attachment #8670254 -
Flags: review?(khuey)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → ejpbruel
Assignee | ||
Comment 2•9 years ago
|
||
I should add that this patch applies on top of the patches in bug 1178726.
Depends on: 1178726
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•9 years ago
|
||
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)
Assignee | ||
Comment 5•9 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•9 years ago
|
||
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 10•8 years ago
|
||
sorry backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=18892076&repo=mozilla-inbound
Flags: needinfo?(ejpbruel)
Comment 11•8 years ago
|
||
Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/d51bf9597ea6
Assignee | ||
Comment 12•8 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•8 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 15•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c498ef6aac98
Status: NEW → RESOLVED
Closed: 8 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.
Description
•