This is the start of a refactoring to convert most uses of nsIThread to nsIEventTarget. I'll add more details later, but I'm filing this to get some early feedback from Nathan.

This patch is fairly rough. I'm interested in feedback on the API itself rather than the implementation. I've started converting uses of NS_GetCurrentThread() to use this API, and I'd like to know about issues that would cause me to have to rewrite that patch, since it's a lot of work :-).

I worked on this for about an hour or two to get a sense of how well it would go. Basically everything worked as expected. Some remarks: - I converted a bunch of nsCOMPtr<nsIThread> fields to RefPtr<ThreadEventTarget>. I left the names of the fields alone (typically mThread, mOwnerThread, etc.). I could change the names to mTarget, mOwnerTarget, or whatever. Not sure what's clearer. - I left nsCOMPtr<nsIEventTarget> fields alone. - I tried to avoid #including ThreadEventTarget.h in nsThreadUtils.h. I'm now kind of regretting that because it forces lots of .cpp files to #include it themselves (including anyone calling the destructor of a class holding a RefPtr<ThreadEventTarget>). ThreadEventTarget.h is small, so it might just be easier to include it in nsThreadUtils.h or some other place that gets pulled in by everyone. - I left alone any callers that want to spin a nested event loop or do anything weird ([1] and [2] are the examples I remember). - Some places want to use both nsIThread and nsIEventTarget methods. Since I want to split these two classes apart eventually, I used two separate fields to store these. However, we could always just store the thread and then ask for its EventTarget() when needed. [1] http://searchfox.org/mozilla-central/rev/ae8c2e2354db652950fe0ec16983360c21857f2a/dom/base/Timeout.cpp#103 [2] http://searchfox.org/mozilla-central/rev/ae8c2e2354db652950fe0ec16983360c21857f2a/dom/indexedDB/ActorsParent.cpp#10431

Comment on attachment 8863520 [details] [diff] [review] API patch Review of attachment 8863520 [details] [diff] [review]: ----------------------------------------------------------------- I think this is headed in the right direction, though I'm not at all clear about how much ThreadEventTarget is buying us. Maybe we should just try to do something like: class ThreadEventTarget : public nsIEventTarget { public: bool IsOnCurrentThread() { bool v; DebugOnly<nsresult> rv = this->IsOnCurrentThread(&v); MOZ_ALWAYS_SUCCEEDS(rv); return v; } }; and then have nsThread inherit from that? We get a more ergonomic IsOnCurrentThread(), we don't pay the double virtual function dispatch, and we've inserted a structure that we can refine later if performance profiles show that we're having problems. ::: xpcom/threads/ThreadEventTarget.h @@ +31,5 @@ > + NS_IMETHOD QueryInterface(REFNSIID aIID, void** aInstancePtr) override; > + > + NS_IMETHOD IsOnCurrentThread(bool *aRv) override > + { > + *aRv = GetCurrentVirtualThread() == mVirtualThread; Nit: this method should be calling the no-arg IsOnCurrentThread. @@ +35,5 @@ > + *aRv = GetCurrentVirtualThread() == mVirtualThread; > + return NS_OK; > + } > + > + NS_IMETHOD Dispatch(already_AddRefed<nsIRunnable> aEvent, uint32_t aFlags) override I am not super-excited about making these calls go through two layers of virtual function dispatch: one for this class, and one for mTarget. Is the intent to (eventually) make mTarget a straight nsThread that has non-virtual Dispatch et al methods, so we eliminate one layer of virtual function dispatch? I know we had talked about this intermediate class to make some things not require virtual calls/be more ergonomic, but I don't know if we're quite winning with the current design. @@ +57,5 @@ > + } > + > + void SetVirtualThread(PRThread* aVirtualThread) > + { > + mVirtualThread = aVirtualThread; WDYT about trying to hide this API as much as possible outside of xpcom/threads/ to start with? We can make it more public if we need to, but I'd rather this not be generally exposed. ::: xpcom/threads/nsThreadManager.cpp @@ +394,5 @@ > +SetCurrentVirtualThread(PRThread* aVirtualThread) > +{ > + MOZ_ASSERT(aVirtualThread); > + MOZ_ASSERT(!sTLSVirtualThread.get()); > + sTLSVirtualThread.set(aVirtualThread); Storing the current virtual thread in TLS means that we have to execute the SetCurrentVirtualThread() call on the thread itself, right? Would we be any better off storing the virtual thread in some non-TLS datastructure (e.g. the nsIThread itself)? I guess we won't be calling it that many times, so maybe it doesn't matter *too* much... ::: xpcom/threads/nsThreadUtils.h @@ +1365,5 @@ > +ThreadEventTarget* > +GetCurrentEventTarget(); > + > +ThreadEventTarget* > +GetMainEventTarget(); For avoidance of doubt, these would all need documentation of some sort before landing.

Also, are we really winning if we tell people, "hey, instead of calling NS_GetCurrentThread to get the current thread, you should really be using this event target abstraction...so you should call GetCurrentEventTarget to get...a ThreadEventTarget*!" That does not sound like the most convincing pitch to me. I guess the plan is that eventually multiple threads will return the same ThreadEventTarget* ?

Comment on attachment 8863524 [details] [diff] [review] initial refactoring Review of attachment 8863524 [details] [diff] [review]: ----------------------------------------------------------------- I think this is OK... * I think we will want to change the field names, but that can be left for later. * I don't know what the right solution for the ThreadEventTarget.h issue is. On the one hand, having to add all the #includes is annoying...but adding something to nsThreadUtils.h and simply letting people cargo-cult the header in doesn't feel right either. * I do think we'll want to move to just storing the nsIThread if appropriate, and using the EventTarget() method, rather than storing two fields. ::: dom/base/ImageEncoder.cpp @@ +215,5 @@ > } else { > mEncodingCompleteEvent->SetMembers(imgData, imgSize, mType); > } > rv = mEncodingCompleteEvent->GetCreationThread()-> > + Dispatch(do_AddRef(mEncodingCompleteEvent), nsIThread::DISPATCH_NORMAL); These sorts of changes shouldn't be necessary, since nsIEventTarget has a Dispatch(nsIRunnable*, uint32_t) overload, no?

Oh, one other thing: do we want to specify the virtual thread/physical thread thing as PRThread* or nsThread*? I had assumed from earlier discussions that they would be specified as nsThread* for some reason, but that's not the direction this patch is going.

Thanks for the quick feedback. (In reply to Nathan Froyd [:froydnj] from comment #3) > Comment on attachment 8863520 [details] [diff] [review] > API patch > > Review of attachment 8863520 [details] [diff] [review]: > ----------------------------------------------------------------- > > I think this is headed in the right direction, though I'm not at all clear > about how much ThreadEventTarget is buying us. Maybe we should just try to > do something like: > > [snip] > > and then have nsThread inherit from that? We get a more ergonomic > IsOnCurrentThread(), we don't pay the double virtual function dispatch, and > we've inserted a structure that we can refine later if performance profiles > show that we're having problems. Well, the purpose of the change is to have only one EventTargetThread for the main thread, and to have the same one be shared by the cooperative threads. The inheritance approach would make that kinda pointless. > I am not super-excited about making these calls go through two layers of > virtual function dispatch: one for this class, and one for mTarget. Is the > intent to (eventually) make mTarget a straight nsThread that has non-virtual > Dispatch et al methods, so we eliminate one layer of virtual function > dispatch? I know we had talked about this intermediate class to make some > things not require virtual calls/be more ergonomic, but I don't know if > we're quite winning with the current design. I don't think there will be two layers of virtual dispatch. ThreadEventTarget is declared final, so threadEventTarget->Foo() should not be a virtual call. The call through mTarget will be virtual, but that's how it is now. > WDYT about trying to hide this API as much as possible outside of > xpcom/threads/ to start with? We can make it more public if we need to, but > I'd rather this not be generally exposed. Yeah, originally I made it private and tried to do some friend class stuff, but it was kind of a pain. I think it would be fine to do that though. > Storing the current virtual thread in TLS means that we have to execute the > SetCurrentVirtualThread() call on the thread itself, right? Would we be any > better off storing the virtual thread in some non-TLS datastructure (e.g. > the nsIThread itself)? I guess we won't be calling it that many times, so > maybe it doesn't matter *too* much... I guess you're worried about going through two TLS calls in the case when we don't have a virtual thread set? Yeah, I think we'll have to figure out a better way to handle that. > For avoidance of doubt, these would all need documentation of some sort > before landing. Of course. (In reply to Nathan Froyd [:froydnj] from comment #6) > Oh, one other thing: do we want to specify the virtual thread/physical > thread thing as PRThread* or nsThread*? I had assumed from earlier > discussions that they would be specified as nsThread* for some reason, but > that's not the direction this patch is going. I'm still not sure what we would do either when a process has no thread manager or when the thread manager has already shut down.

I re-titled the bug to cover only the API changes. I'll file other bugs to cover the conversion. The patch incorporates a few changes we discussed. I've moved the code to nsThreadUtils.h, added some comments, and made SetVirtualThread private.

Oh, I also changed the name to GetCurrentThreadEventTarget. There can be a lot of event targets in play, so I felt it would be better to be specific that this returns a ThreadEventTarget.

I just realized why I needed to use do_AddRef all over the place in the second patch. C++ has a rule that a non-virtual function in a base class will be hidden by a virtual function with the same name in an overriding class. So the Dispatch function defined in nsIEventTarget that takes an nsIRunnable* will be hidden by the virtual Dispatch function in ThreadEventTarget that takes an already_AddRefed<nsIRunnable>. I fixed this by copying the Dispatch(nsIRunnable*) implementation to ThreadEventTarget.

I filed bug 1365096 to cover the conversion process.

This ended up leaking. There's a cycle between ThreadEventTarget and nsThread. I thought I could clear it in nsThread::ShutdownComplete. However, that never gets called for threads that are not started through nsThreadManager (either the main thread or some other callers that must use PR_CreateThread). I fixed this by changing the TLS destructor thing to clear the cycle. The only change is to ReleaseObject in nsThreadManager.cpp.

Comment on attachment 8868322 [details] [diff] [review] patch, v3 Review of attachment 8868322 [details] [diff] [review]: ----------------------------------------------------------------- Thanks. ::: xpcom/threads/nsThread.h @@ +74,5 @@ > { > mEventObservers.Clear(); > } > > + void ClearEventTarget() We should really only make this callable from the thread manager. Surprisingly, nsThreadManager is not already a friend of nsThread, though. Maybe that'd be a good thing for a followup bug.

Ugh, sorry to keep making you review this patch. There was an intermittent leak involving LazyIdleThread that I found today. The problem is that a LazyIdleThread that's set up with AutomaticShutdown doesn't necessarily ever get any notification that it's dead. It just kinda shuts down when the last reference to it goes away. That fouls up my cycle breaking mechanism. To fix this, I decided to make the reference from ThreadEventTarget to nsThread/LazyIdleThread be weak. We'll clear the field in the nsThread/LazyIdleThread destructor. For LazyIdleThread, this means that someone who thinks they can create a LazyIdleThread and then just keep a ThreadEventTarget reference to it will be surprised. The thread can die and a call to Dispatch will fail. I guess I don't feel too bad about this, but maybe you have stronger feelings. For nsThread, this isn't really an issue. You need to call Shutdown on the actual nsThread anyway, so there will always be someone keeping the thread itself alive in order to do that.

Can you get to this soon, Nathan? The first couple conversion patches are ready to land now.

Comment on attachment 8868869 [details] [diff] [review] patch v4 I had a couple ideas for this over the weekend, so I'm going to cancel review and work on a new patch on Monday. Specifically: - I think it would make sense to have a separate method from IsOnCurrentThread called DispatchesToCurrentThread, for two reasons. First, this method could be infallible without breaking existing callers. Second, the semantics of IsOnCurrentThread aren't that clear. Some people interpret it to mean "Are we running events from this event target on the current thread right now?" Other people interpret it to mean "Do events from this event target ever run on this thread?" For non-thread event targets, this distinction matters. I would like DispatchesToCurrentThread specifically to mean the latter thing, which is more useful for thread safety assertions. - If we do this, then we might as well put this code inside nsIEventTarget as Olli suggested. - In bug 1366072, Bobby wants to have an nsIEventTarget sub-class for single-threaded event targets (i.e., not thread pools). I might as well add that in this bug. - Then I think it would make sense for GetCurrentThreadEventTarget to return an nsIEventTarget and to have something like GetCurrentSingleThreadedEventTarget that asserts that our current thread has a single-threaded event target and then returns that. This latter one could be used for passing an event target to promises. We could also use it when storing an event target for thread safety assertions, since we probably don't want to use a thread pool event target for such assertions.

This patch adds a new DispatchesToCurrentThread method to nsIEventTarget. I tried to make the difference in semantics from IsOnCurrentThread clear in the comments. Otherwise, this does pretty much what Olli was asking for in IRC the other day: to put the assertion checking code in nsIEventTarget so that we don't have to create a new class. I think it works fairly well.

This adds the interface that we discussed in bug 1366072.

This adds some getter methods, similar to the previous approach. The reason I didn't just make everything return nsISerialEventTarget is because I'd like the EventTarget() method for threads in a thread pool to return the thread pool's event target eventually. That should probably happen in a separate bug though. GetCurrentThreadSerialEventTarget is a bit of a mouthful, but it seems to fit the bill.

Comment on attachment 8870202 [details] [diff] [review] Add DispatchesToCurrentThread method to nsIEventTarget Review of attachment 8870202 [details] [diff] [review]: ----------------------------------------------------------------- WFM. I'm a little concerned that we're introducing fine-grained differences between things (dispatches to vs. is on, and serial vs. non-serial in future patches); we'll have to see how this plays out. ::: xpcom/threads/nsIEventTarget.idl @@ +58,5 @@ > + * and false otherwise. In the case of an nsIEventTarget for a thread pool, it > + * should return true on all threads in the pool. In the case of a non-thread > + * nsIEventTarget such as ThrottledEventQueue, it should return true on the > + * thread where events are expected to be processed, even if no events from > + * the queue are actually being processed right now. Maybe provide a concrete example of a situation where you would care about the distinction between DispatchesToCurrentThread vs. IsOnCurrentThread? I'm sure one could be imagined from the documentation comments, but it might take a fair bit of thought if you're not thinking about all this stuff 24/7. @@ +78,5 @@ > + > +public: > + %} > + > + [noscript,notxpcom] boolean dispatchesToCurrentThreadInternal(); The publicness of this method is concerning, but I guess there's no good way around that? We should have a comment about this arrangement here. ::: xpcom/threads/nsThread.cpp @@ +893,5 @@ > +bool > +nsThread::DispatchesToCurrentThreadInternal() > +{ > + // Rely on mVirtualThread being correct. > + return false; Should we just MOZ_CRASH() here, expecting that this will never be called?

Comment on attachment 8870203 [details] [diff] [review] add nsISerialEventTarget and make common event targets implement it Review of attachment 8870203 [details] [diff] [review]: ----------------------------------------------------------------- Include some tests for QI'ing various things to nsISerialEventTarget, and for not QI'ing various things to nsISerialEventTarget (e.g. thread pools), please? ::: dom/workers/WorkerPrivate.cpp @@ +7006,5 @@ > NS_IMPL_RELEASE(WorkerPrivateParent<Derived>::EventTarget) > > template <class Derived> > NS_INTERFACE_MAP_BEGIN(WorkerPrivateParent<Derived>::EventTarget) > + NS_INTERFACE_MAP_ENTRY(nsISerialEventTarget) Shouldn't all the implementations be QI'able to nsIEventTarget as well? ::: xpcom/threads/SchedulerGroup.cpp @@ +130,5 @@ > } > > } // namespace > > +NS_IMPL_ISUPPORTS(SchedulerEventTarget, SchedulerEventTarget, nsISerialEventTarget) Likewise with QI here, shouldn't we be including nsIEventTarget? ::: xpcom/threads/TaskQueue.cpp @@ +63,5 @@ > > NS_DECL_THREADSAFE_ISUPPORTS > }; > > +NS_IMPL_ISUPPORTS(TaskQueue::EventTargetWrapper, nsISerialEventTarget) Here too. ::: xpcom/threads/ThrottledEventQueue.cpp @@ +410,5 @@ > }; > > NS_IMPL_ISUPPORTS(ThrottledEventQueue::Inner, nsIObserver); > > +NS_IMPL_ISUPPORTS(ThrottledEventQueue, ThrottledEventQueue, nsISerialEventTarget); And here. ::: xpcom/threads/nsISerialEventTarget.idl @@ +12,5 @@ > + * to execute serially. That is, two different runnables dispatched to the > + * target should never be allowed to execute simultaneously. So, for example, a > + * thread pool should not implement nsISerialEventTarget. However, one can > + * "convert" a thread pool into an nsISerialEventTarget by putting a > + * ThrottledEventQueue in front of it. The documentation on ThrottledEventQueue confuses me on this point: // In addition, ThrottledEventQueue currently dispatches its next // runnable to the base target *before* running the current event. This language makes it sound like you could execute multiple runnables simultaneously...but I think that's not the case? Also, this language: // So, if you are targeting a thread pool you probably want a TaskQueue. // If you are targeting a single thread or other non-concurrent event // target, you probably want a ThrottledEventQueue. That sounds like putting a ThrottledEventQueue in front of a thread pool would not be so great? ::: xpcom/threads/nsIThread.idl @@ +24,5 @@ > * > * See nsIThreadManager for the API used to create and locate threads. > */ > [builtinclass, scriptable, uuid(5801d193-29d1-4964-a6b7-70eb697ddf2b)] > +interface nsIThread : nsISerialEventTarget nsThread's QI implementation should QI to nsISerialEventTarget, yes?

Comment on attachment 8870204 [details] [diff] [review] add some getters for event targets Review of attachment 8870204 [details] [diff] [review]: ----------------------------------------------------------------- Echo'ing concerns about fine-grained differences here, but let's try it and see how it goes. ::: xpcom/threads/nsThreadUtils.cpp @@ +497,5 @@ > + if (NS_FAILED(rv)) { > + return nullptr; > + } > + > + return thread->SerialEventTarget(); I don't see the thread pool-related assertions here that are mentioned in the doc comments for the serial event target methods. Which part of the code is wrong?

(In reply to Nathan Froyd [:froydnj] from comment #22) > I don't see the thread pool-related assertions here that are mentioned in > the doc comments for the serial event target methods. Which part of the > code is wrong? The assertions will only be needed when we make the EventTarget getter for nsThreads tied to an nsThreadPool return the nsThreadPool as their event target. I haven't done that yet since it's sort of a separate issue.

Comment on attachment 8871091 [details] [diff] [review] add nsISerialEventTarget and make common event targets implement it, v2 Review of attachment 8871091 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/tests/gtest/TestEventTargetQI.cpp @@ +16,5 @@ > +#include "gtest/gtest.h" > + > +using namespace mozilla; > + > +TEST(TestEventTargetQI, ThreadPool) These are great, thank you! @@ +32,5 @@ > +} > + > +TEST(TestEventTargetQI, SharedThreadPool) > +{ > + nsCOMPtr<nsIThreadPool> thing = SharedThreadPool::Get(NS_LITERAL_CSTRING("TestPool"), 1); Do we need to shut this down to avoid leaks? @@ +69,5 @@ > +} > + > +TEST(TestEventTargetQI, LazyIdleThread) > +{ > + nsCOMPtr<nsIThread> thing = new LazyIdleThread(0, NS_LITERAL_CSTRING("TestThread")); Do we need to shut this down to avoid leaks? ::: xpcom/threads/ThrottledEventQueue.cpp @@ +412,5 @@ > NS_IMPL_ISUPPORTS(ThrottledEventQueue::Inner, nsIObserver); > > +NS_IMPL_ISUPPORTS(ThrottledEventQueue, > + ThrottledEventQueue, > + nsIEventTarget); Nit: I guess this formatting change is unnecessary?

In bug 1368358, Olli eliminated the main reason for having an IsOnCurrentTarget/DispatchesToCurrentThread distinction. So this patch just adds an infallible, non-virtual IsOnCurrentThread method to nsIEventTarget.

Comment on attachment 8873583 [details] [diff] [review] Add infallible, non-virtual IsOnCurrentThread method to nsIEventTarget Review of attachment 8873583 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/threads/nsIEventTarget.idl @@ +64,5 @@ > + * When called on an nsISerialEventTarget, IsOnCurrentThread can be > + * used to ensure that no other thread has "ownership" of the event target. As > + * such, it's useful for asserting that an object is only used on a particular > + * thread. However, IsOnCurrentThread can't guarantee that the current > + * event has been dispatched through a particular event target. What is this "However..." here for? It doesn't really seem to follow from the previous sentences on ownership. @@ +88,5 @@ > + PRThread* mVirtualThread; > + > + nsIEventTarget() : mVirtualThread(nullptr) {} > + %} > + // This method is protected. Maybe "Note that this method is protected. We define it through IDL, rather than in a %{C++ block, to ensure that the correct method indices are recorded for XPConnect purposes."?

Pushed by wmccloskey@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a26040f4d439 Add infallible IsOnCurrentThread to nsIEventTarget (r=froydnj) https://hg.mozilla.org/integration/mozilla-inbound/rev/5dae479fd477 Add nsISerialEventTarget (r=froydnj) https://hg.mozilla.org/integration/mozilla-inbound/rev/a2e617b58eba Add Get{Current,Main}ThreadEventTarget getters to replace NS_Get{Current,Main}Thread (r=froydnj)

https://hg.mozilla.org/mozilla-central/rev/a26040f4d439 https://hg.mozilla.org/mozilla-central/rev/5dae479fd477 https://hg.mozilla.org/mozilla-central/rev/a2e617b58eba