1382922 - Refactor XPCOM thread event loop

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Our event loop has been getting more complicated lately due to the prioritization work for Quantum DOM. Making it support DOM workers has also made it more complicated. This bug will try to break it apart somewhat and make the pieces easier to understand.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: Remove thread parameter from onDispatchedEvent

This parameter isn't used by any implementation of onDispatchedEvent,
and keeping the parameter makes later refactorings in this bug more difficult.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: Use separate mLock field for workers

The fact that WorkerThread uses the mLock field from nsThread is a
pretty big hole in the nsThread abstraction. I don't see any reason
for the re-use, either. This locks protects a few fields inside
WorkerThread, but none of them seem in any way related to things
done by nsThread. This patch uses a separate lock for WorkerThreads.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: Refactor event queue to allow multiple implementations

This patch refactors the nsThread event queue to clean it up and to make it easier to restructure. The fundamental concepts are as follows:
    
Each nsThread will have a pointer to a refcounted SynchronizedEventQueue. A SynchronizedEQ takes care of doing the locking and condition variable work when posting and popping events. For the actual storage of events, it delegates to an EventQueue data structure. It keeps a UniquePtr to the EventQueue that it uses for storage.
    
Both SynchronizedEQ and EventQueue are abstract classes. There is only one concrete implementation of SynchronizedEQ in this patch, which is called ThreadEventQueue. ThreadEventQueue uses locks and condition variables to post and pop events the same way nsThread does. It also encapsulates the functionality that DOM workers need to implement their special event loops (PushEventQueue and PopEventQueue). In later Quantum DOM work, I plan to have another SynchronizedEQ implementation for the main thread, called SchedulerEventQueue. It will have special code for the cooperatively scheduling threads in Quantum DOM.
    
There are two concrete implementations of EventQueue in this patch: SimpleEventQueue and PrioritizedEventQueue. SimpleEventQueue is basically just an nsEventQueue. (Eventually, I plan to replace nsEventQueue with SimpleEventQueue since nsEventQueue is still used by ThrottledEventQueue even after my patches.) One difference is that SimpleEventQueue tries to abstract the queue implementation (which is done via a linked list of arrays) into a more generic Queue class. The other EventQueue implementation is PrioritizedEventQueue, which uses multiple queues for different event priorities.
    
The final major piece here is ThreadEventTarget, which splits some of the code for posting events out of nsThread. Eventually, my plan is for multiple cooperatively scheduled nsThreads to be able to share a ThreadEventTarget. In this patch, though, each nsThread has its own ThreadEventTarget. The class's purpose is just to collect some related code together.
    
One final note: I tried to avoid virtual dispatch overhead as much as possible. Calls to SynchronizedEQ methods do use virtual dispatch, since I plan to use different implementations for different threads with Quantum DOM. But all the calls to EventQueue methods should be non-virtual. Although the methods are declared virtual, all the classes used are final and the concrete classes involved should all be known through templatization.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Also, bug 1351148 will force me to change the implementation of PrioritizedEventQueue somewhat, but I don't think the changes should be significant.

Comment 5

[Eric Rahm [:erahm]]

Comment on attachment 8888625 [details] [diff] [review]
Remove thread parameter from onDispatchedEvent

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

::: xpcom/threads/nsIThreadInternal.idl
@@ -105,2 @@
>     */
> -  void onDispatchedEvent(in nsIThreadInternal thread);

I'm just going to assume that given this is called nsIThreadInternal we don't expect it to be used / implemented in JS-land.

Comment 6

[Eric Rahm [:erahm]]

Comment on attachment 8888627 [details] [diff] [review]
Refactor event queue to allow multiple implementations

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

I didn't get super far into the overall design, mostly just focused on the code. The logic seems right, we could discuss the naming of the the classes (there are a ton of Queues now) and the inter-dependencies between NestedSink, NestSinkItem, and ThreadEventQueue are worth talking about too.

And of course it would be great to see some tests.

::: xpcom/threads/EventQueue.h
@@ +15,5 @@
> +namespace mozilla {
> +
> +enum class EventPriority
> +{
> +  Normal = 0,

It's a little weird that Normal is listed before high.

@@ +34,5 @@
> +class EventQueue
> +{
> +public:
> +  virtual void PutEvent(already_AddRefed<nsIRunnable>&& aEvent,
> +                        EventPriority aPriority,

Can you add a doc here explaining how aPriority is intended to be used? It's not super obvious that in PrioritizedEventQueue it's ignored for nsIRunnablePriority classes.

@@ +36,5 @@
> +public:
> +  virtual void PutEvent(already_AddRefed<nsIRunnable>&& aEvent,
> +                        EventPriority aPriority,
> +                        MutexAutoLock& aProofOfLock) = 0;
> +  virtual already_AddRefed<nsIRunnable> GetEvent(EventPriority* aPriority,

Can you add docs to this, it's not clear why aPriority is a pointer.

::: xpcom/threads/PrioritizedEventQueue.cpp
@@ +36,5 @@
> +    uint32_t prio = nsIRunnablePriority::PRIORITY_NORMAL;
> +    runnablePrio->GetPriority(&prio);
> +    if (prio == nsIRunnablePriority::PRIORITY_HIGH) {
> +      priority = EventPriority::High;
> +    }

So it seems like we got rid of handling of PRIORITY_INPUT. Is that intentional? (I'm assuming this corresponds to nsChainedEventQueue).

@@ +110,5 @@
> +  // we're running an idle runnable in ProcessNextEvent.
> +  *mNextIdleDeadline = TimeStamp();
> +#endif
> +
> +  bool canUseHighPrio = mProcessHighPriorityQueue

This needs a comment, it seems wrong at first glance, but I guess it's part of some sort of "don't starve the lower priority queues" logic.

@@ +113,5 @@
> +
> +  bool canUseHighPrio = mProcessHighPriorityQueue
> +                     || !mNormalQueue->HasPendingEvent(aProofOfLock);
> +
> +  if (mHighQueue->HasPendingEvent(aProofOfLock) && canUseHighPrio) {

This test seems out of order.

@@ +125,5 @@
> +  }
> +
> +  if (mNormalQueue->HasPendingEvent(aProofOfLock)) {
> +    nsCOMPtr<nsIRunnable> event = mNormalQueue->GetEvent(aPriority, aProofOfLock);
> +    mProcessHighPriorityQueue = mHighQueue->HasPendingEvent(aProofOfLock);

This logic doesn't feel right. We seem to prioritize the normal queue over the high priority queue.

::: xpcom/threads/PrioritizedEventQueue.h
@@ +41,5 @@
> +
> +public:
> +  PrioritizedEventQueue(UniquePtr<InnerQueueT> aHighQueue,
> +                        UniquePtr<InnerQueueT> aNormalQueue,
> +                        UniquePtr<InnerQueueT> aIdleQueue,

It seems silly to pass these in, is there a case where we'd call this w/o brand new instances? It might be nice to hide it as an implementation detail in case we change the number of priorities etc.

@@ +42,5 @@
> +public:
> +  PrioritizedEventQueue(UniquePtr<InnerQueueT> aHighQueue,
> +                        UniquePtr<InnerQueueT> aNormalQueue,
> +                        UniquePtr<InnerQueueT> aIdleQueue,
> +                        nsIIdlePeriod* aIdlePeriod);

Should this be an already_AddRefed? Or is it optional?

@@ +55,5 @@
> +  // When checking the idle deadline, we need to drop whatever mutex protects
> +  // this queue. This method allows that mutex to be stored so that we can drop
> +  // it and reacquire it when checking the idle deadline. The mutex must live at
> +  // least as long as the queue.
> +  void SetMutexRef(Mutex& aMutex) { mMutex = &aMutex; }

This is all a bit sketchy. I don't have anything constructive to say beyond that though :( At least the comment makes it pretty clear what's going on.

@@ +96,5 @@
> +  // event from GetIdleEvent() to ensure that HasPendingEvents() never lies.
> +  bool mHasPendingEventsPromisedIdleEvent = false;
> +};
> +
> +extern template class PrioritizedEventQueue<SimpleEventQueue>;

Oh look I learned something new :)

::: xpcom/threads/Queue.h
@@ +9,5 @@
> +
> +namespace mozilla {
> +
> +template<class T>
> +class Queue

So this is a trimmed down and generalized nsEventQueue. Can you add some comments? Also do you think it would be useful to have it under xpcom/ds or is it really only applicable to thread stuff?

@@ +16,5 @@
> +  Queue() {}
> +
> +  ~Queue()
> +  {
> +    MOZ_ASSERT(IsEmpty());

I don't love that we leak everything in release...

@@ +19,5 @@
> +  {
> +    MOZ_ASSERT(IsEmpty());
> +
> +    if (mHead) {
> +      FreePage(mHead);

Lets just call |free| directly and not pretend this does anything else.

@@ +71,5 @@
> +      FreePage(dead);
> +      mOffsetHead = 0;
> +    }
> +
> +    return result;

Does this mean we're going to have more addref traffic for nsCOMPtrs? Or maybe we're counting on the compiler to be smart.

@@ +74,5 @@
> +
> +    return result;
> +  }
> +
> +  T& FirstElement()

We might want const and non-const versions of this.

@@ +83,5 @@
> +    MOZ_ASSERT_IF(mHead == mTail, mOffsetHead <= mOffsetTail);
> +    return mHead->mEvents[mOffsetHead];
> +  }
> +
> +  T& LastElement()

Ditto.

@@ +129,5 @@
> +
> +private:
> +  enum
> +  {
> +    ITEMS_PER_PAGE = 255

We might need to rethink this number, before we were just holding pointers. Now we're holding a T. It should probably be some sort of calculation based on sizeof(T). I guess the assert below helps.

@@ +145,5 @@
> +                "sizeof(Page) should be a power of two to avoid heap slop.");
> +
> +  static Page* NewPage()
> +  {
> +    return static_cast<Page*>(moz_xcalloc(1, sizeof(Page)));

I don't think we really need to calloc here. Wdyt of just using |new Page| and initialize mNext (or add a ctor).

::: xpcom/threads/SimpleEventQueue.h
@@ +18,5 @@
> +  SimpleEventQueue();
> +
> +  void PutEvent(already_AddRefed<nsIRunnable>&& aEvent,
> +                EventPriority aPriority,
> +                MutexAutoLock& aProofOfLock) override;

These should be final right?

::: xpcom/threads/ThreadEventQueue.cpp
@@ +38,5 @@
> +
> +private:
> +  friend class ThreadEventQueue;
> +
> +  SimpleEventQueue* mQueue;

Can you annotate this as non-owning?

@@ +97,5 @@
> +
> +    // Make sure to grab the observer before dropping the lock, otherwise the
> +    // event that we just placed into the queue could run and eventually delete
> +    // this nsThread before the calling thread is scheduled again. We would then
> +    // crash while trying to access a dead nsThread.

Nice comment.

@@ +117,5 @@
> +  MutexAutoLock lock(mLock);
> +
> +  nsCOMPtr<nsIRunnable> event;
> +  for (;;) {
> +    // We always get events from the topmost queue when there are nested queues.

Maybe move this comment to the else.

@@ +197,5 @@
> +
> +  // Move events from the old queue to the new one.
> +  nsCOMPtr<nsIRunnable> event;
> +  EventPriority prio;
> +  while ((event = item.mQueue->GetEvent(&prio, lock))) {

It might be worth adding an EventQueue::Append(EventQueue&) to avoid all this put/get traffic.

::: xpcom/threads/ThreadEventQueue.h
@@ +27,5 @@
> +public:
> +  explicit ThreadEventQueue(UniquePtr<InnerQueueT> aQueue);
> +
> +  bool PutEvent(already_AddRefed<nsIRunnable>&& aEvent,
> +                EventPriority aPriority) override;

nit: these should all be final

::: xpcom/threads/nsThread.cpp
@@ +405,5 @@
>    }
>  
>    // Wait for and process startup event
>    nsCOMPtr<nsIRunnable> event;
>    {

This doesn't need a scope anymore.

@@ +407,5 @@
>    // Wait for and process startup event
>    nsCOMPtr<nsIRunnable> event;
>    {
> +    event = self->mEvents->GetEvent(true, nullptr);
> +    MOZ_ASSERT(event);

Is it just not possible to fail anymore?

@@ +542,5 @@
>  #ifdef MOZ_CANARY
>  int sCanaryOutputFD = -1;
>  #endif
>  
> +nsThread::nsThread(SynchronizedEventQueue* aQueue,

This could probably be a NotNull (which is used throughout this file).

@@ +601,5 @@
>    // ThreadFunc will wait for this event to be run before it tries to access
>    // mThread.  By delaying insertion of this event into the queue, we ensure
>    // that mThread is set properly.
>    {
> +    mEvents->PutEvent(do_AddRef(startup), EventPriority::Normal); // retain a reference

I guess we got rid of the `nsIRunnable*` interface, is that worth retaining?

@@ +946,5 @@
>      noJSAPI.emplace();
>      mScriptObserver->BeforeProcessTask(reallyWait);
>    }
>  
> +  nsCOMPtr<nsIThreadObserver> obs = mEvents->GetObserverLockFree();

Why is it okay to do this without locking?

::: xpcom/threads/nsThreadManager.cpp
@@ +104,5 @@
> +                                                  MakeUnique<SimpleEventQueue>(),
> +                                                  idlePeriod);
> +
> +  // Save a copy temporarily so we can set some state on it.
> +  MainThreadQueueT* prioritizedCopy = prioritized.get();

This isn't really a copy...maybe change the wording?

@@ +112,4 @@
>    // Setup "main" thread
> +  mMainThread = new nsThread(queue, nsThread::MAIN_THREAD, 0);
> +
> +  prioritizedCopy->SetMutexRef(queue->MutexRef());

It seems like the `ThreadEventQueue` ctor could do this. Or is it optional?

@@ +112,5 @@
>    // Setup "main" thread
> +  mMainThread = new nsThread(queue, nsThread::MAIN_THREAD, 0);
> +
> +  prioritizedCopy->SetMutexRef(queue->MutexRef());
> +  prioritizedCopy->SetNextIdleDeadlineRef(mMainThread->NextIdleDeadlineRef());

Same deal, but the `nsThread` ctor.

::: xpcom/threads/nsThreadManager.h
@@ +15,5 @@
>  class nsIRunnable;
>  
> +namespace mozilla {
> +class SynchronizedEventQueue;
> +}

Why is this here? Is it to avoid declaring elsewhere?

Comment 7

[Eric Rahm [:erahm]]

Comment on attachment 8888627 [details] [diff] [review]
Refactor event queue to allow multiple implementations

Nathan I think this is at least worth you taking a look at the design as you're pretty familiar with nsThread and friends. Maybe just focus on the commit message.

Comment 8

[Nathan Froyd [:froydnj]]

Comment on attachment 8888627 [details] [diff] [review]
Refactor event queue to allow multiple implementations

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

I tried to add some higher-level comments below.  I had to draw a picture to understand all the different relationships between *EventQueue in this patch...maybe we should put some things in mozilla::detail:: to permit a little more naming freedom?  I know we haven't been huge fans of using "Abstract" in names, but maybe s/EventQueue/AbstractEventQueue/, s/SimpleEventQueue/EventQueue/ would help too?

I do like ripping a lot of logic out of nsThread, though; that class was getting a bit unwieldly...

::: xpcom/threads/Queue.h
@@ +9,5 @@
> +
> +namespace mozilla {
> +
> +template<class T>
> +class Queue

It would be super, super nice if the confusion between this and nsEventQueue went away.  This patch is already adding enough things with "Queue" or "EventQueue" in their names that we don't need two classes that are very nearly copies of one another.

::: xpcom/threads/SynchronizedEventQueue.h
@@ +21,5 @@
> +// SynchronizedEventQueue are split between ThreadTargetSink (which contains
> +// methods for posting events) and SynchronizedEventQueue (which contains
> +// methods for getting events). This split allows event targets (specifically
> +// ThreadEventTarget) to use a narrow interface, since they only need to post
> +// events.

There's a fair amount of overlap between the SynchronizedEventQueue interface and the EventQueue interface, though I guess SynchronizedEventQueue expects implementations to do their own locking.  Do you think it's really that much of a win to put the locking requirement in this intermediate interface, rather than just figuring out better, shareable interfaces between SynchronizedEventQueue, ThreadTargetSink, and EventQueue, and requiring clients of SynchronizedEventQueue to do their own locking?

@@ +37,5 @@
> +  virtual bool PutEvent(already_AddRefed<nsIRunnable>&& aEvent,
> +                        EventPriority aPriority) = 0;
> +
> +  // After this method is called, no more events can be posted.
> +  virtual void Disconnect(MutexAutoLock& aProofOfLock) {}

A defaulted, do-nothing implementation here seems very peculiar.  Shouldn't this be a pure virtual function?

@@ +61,5 @@
> +  // called on the thread that processes events. GetObserver can be called from
> +  // any thread. GetObserverLockFree can be used from the thread that processes
> +  // events.
> +  virtual already_AddRefed<nsIThreadObserver> GetObserver() = 0;
> +  virtual already_AddRefed<nsIThreadObserver> GetObserverLockFree() = 0;

In the profiler, I think the naming convention that has been adopted is "Racy", to not only suggest speed, but also danger.  Maybe that's not appropriate here, but instead `GetObserverFromSelf` or something to emphasize that we can only call this when we're on the processing thread?

::: xpcom/threads/ThreadEventQueue.h
@@ +18,5 @@
> +
> +namespace mozilla {
> +
> +template<class InnerQueueT>
> +class ThreadEventQueue final : public SynchronizedEventQueue

Some documentation on this class, including what are expected/valid values for `InnerQueueT` would be good.

Is the idea behind making this a templated class that virtual dispatch for InnerQueueT should be able to be optimized away by the compiler?

::: xpcom/threads/moz.build
@@ +49,5 @@
>      'Monitor.h',
>      'MozPromise.h',
>      'Mutex.h',
> +    'PrioritizedEventQueue.h',
> +    'Queue.h',

I'm not so sure Queue.h should be exported; people might get the idea that they should start using it more generally, and it's really not designed to be a general-purpose queue.  I guess worker code needs to see it via some chain of includes, though, doesn't it?  Ugh.  Remind me again why nsThread's constructor needs to know what sort of queue it's using? :)  If we didn't need that, many of the things added in this patch could just become private implementation details of our threading implementation...

::: xpcom/threads/nsThread.h
@@ +138,4 @@
>    struct nsThreadShutdownContext* ShutdownInternal(bool aSync);
>  
> +  RefPtr<mozilla::SynchronizedEventQueue> mEvents;
> +  RefPtr<mozilla::ThreadEventTarget> mEventTarget;

I am not super-excited about mEvents requiring virtual dispatch.  LTO at the moment might be able to make the virtual dispatch go away, but it will be harder to make the virtual dispatch go away when we implement QDOM scheduling.  I don't think there's a really good way around this, though...

::: xpcom/threads/nsThreadManager.cpp
@@ +112,2 @@
>    // Setup "main" thread
> +  mMainThread = new nsThread(queue, nsThread::MAIN_THREAD, 0);

Can you comment on why you chose to pass the queue in, rather than letting nsThread decide for itself what the queue implementation should be based on whether it's the main thread or not?  I thought I remember you mentioning this is some past discussion, but I can't find it in this bug.

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #6)
> ::: xpcom/threads/PrioritizedEventQueue.cpp
> @@ +36,5 @@
> > +    uint32_t prio = nsIRunnablePriority::PRIORITY_NORMAL;
> > +    runnablePrio->GetPriority(&prio);
> > +    if (prio == nsIRunnablePriority::PRIORITY_HIGH) {
> > +      priority = EventPriority::High;
> > +    }
> 
> So it seems like we got rid of handling of PRIORITY_INPUT. Is that
> intentional? (I'm assuming this corresponds to nsChainedEventQueue).

I'm still debating when to rebase over bug 1351148, which is what added PRIORITY_INPUT. That bug landed and then got backed out today.

> @@ +125,5 @@
> > +  }
> > +
> > +  if (mNormalQueue->HasPendingEvent(aProofOfLock)) {
> > +    nsCOMPtr<nsIRunnable> event = mNormalQueue->GetEvent(aPriority, aProofOfLock);
> > +    mProcessHighPriorityQueue = mHighQueue->HasPendingEvent(aProofOfLock);
> 
> This logic doesn't feel right. We seem to prioritize the normal queue over
> the high priority queue.

This logic is pre-existing. It is weird that it's called "high priority" when it sort of has a lower priority than normal priority stuff. The naming will at least get fixed up with bug 1351148.

> ::: xpcom/threads/PrioritizedEventQueue.h
> @@ +41,5 @@
> > +
> > +public:
> > +  PrioritizedEventQueue(UniquePtr<InnerQueueT> aHighQueue,
> > +                        UniquePtr<InnerQueueT> aNormalQueue,
> > +                        UniquePtr<InnerQueueT> aIdleQueue,
> 
> It seems silly to pass these in, is there a case where we'd call this w/o
> brand new instances? It might be nice to hide it as an implementation detail
> in case we change the number of priorities etc.

I'd like to have the ability to pass parameters to the constructors of the different sub-queues (although we don't use that right now).

> @@ +55,5 @@
> > +  // When checking the idle deadline, we need to drop whatever mutex protects
> > +  // this queue. This method allows that mutex to be stored so that we can drop
> > +  // it and reacquire it when checking the idle deadline. The mutex must live at
> > +  // least as long as the queue.
> > +  void SetMutexRef(Mutex& aMutex) { mMutex = &aMutex; }
> 
> This is all a bit sketchy. I don't have anything constructive to say beyond
> that though :( At least the comment makes it pretty clear what's going on.

Dropping the mutex definitely sucks (and is pre-existing). I tried to fix this, but I couldn't figure out a good way to do it.

> ::: xpcom/threads/Queue.h
> @@ +9,5 @@
> > +
> > +namespace mozilla {
> > +
> > +template<class T>
> > +class Queue
> 
> So this is a trimmed down and generalized nsEventQueue. Can you add some
> comments? Also do you think it would be useful to have it under xpcom/ds or
> is it really only applicable to thread stuff?

Probably not. The fact that calloc is used to allocate pages (which I think is needed for efficiency) makes it not very general-purpose.

> @@ +71,5 @@
> > +      FreePage(dead);
> > +      mOffsetHead = 0;
> > +    }
> > +
> > +    return result;
> 
> Does this mean we're going to have more addref traffic for nsCOMPtrs? Or
> maybe we're counting on the compiler to be smart.

Even if the compiler doesn't do the return value optimization here, it should still use a move constructor, which will avoid refcounting overhead.

> @@ +145,5 @@
> > +                "sizeof(Page) should be a power of two to avoid heap slop.");
> > +
> > +  static Page* NewPage()
> > +  {
> > +    return static_cast<Page*>(moz_xcalloc(1, sizeof(Page)));
> 
> I don't think we really need to calloc here. Wdyt of just using |new Page|
> and initialize mNext (or add a ctor).

I suspect it will be slower. The nsCOMPtr constructor does a bit of work. It's possible that the compiler will realize that 255 calls to it can be turned into whatever SIMD stuff calloc does, but I kinda doubt it.

> @@ +197,5 @@
> > +
> > +  // Move events from the old queue to the new one.
> > +  nsCOMPtr<nsIRunnable> event;
> > +  EventPriority prio;
> > +  while ((event = item.mQueue->GetEvent(&prio, lock))) {
> 
> It might be worth adding an EventQueue::Append(EventQueue&) to avoid all
> this put/get traffic.

This code rarely runs. It's only used when DOM workers spin nested event loops. I'd rather not add complexity to the EventQueue interface for this. It's something we'd like to remove eventually.

> @@ +407,5 @@
> >    // Wait for and process startup event
> >    nsCOMPtr<nsIRunnable> event;
> >    {
> > +    event = self->mEvents->GetEvent(true, nullptr);
> > +    MOZ_ASSERT(event);
> 
> Is it just not possible to fail anymore?

Correct. As long as aMayWait is true, we should not fail.

> @@ +542,5 @@
> >  #ifdef MOZ_CANARY
> >  int sCanaryOutputFD = -1;
> >  #endif
> >  
> > +nsThread::nsThread(SynchronizedEventQueue* aQueue,
> 
> This could probably be a NotNull (which is used throughout this file).

I did this, but it's pretty ugly (lots of wrap calls). I think the main problem is that using NotNull doesn't allow any implicit conversions. That is, NotNull<S*> cannot be turned into NotNull<T*> even if S* turns into T*. The same thing happens with NotNull<RefPtr<S>> and NotNull<RefPtr<T>>. I'm definitely not a fan of this thing.

> @@ +601,5 @@
> >    // ThreadFunc will wait for this event to be run before it tries to access
> >    // mThread.  By delaying insertion of this event into the queue, we ensure
> >    // that mThread is set properly.
> >    {
> > +    mEvents->PutEvent(do_AddRef(startup), EventPriority::Normal); // retain a reference
> 
> I guess we got rid of the `nsIRunnable*` interface, is that worth retaining?

I don't think so. It's rarely used.

> @@ +946,5 @@
> >      noJSAPI.emplace();
> >      mScriptObserver->BeforeProcessTask(reallyWait);
> >    }
> >  
> > +  nsCOMPtr<nsIThreadObserver> obs = mEvents->GetObserverLockFree();
> 
> Why is it okay to do this without locking?

I added a comment.

> @@ +112,4 @@
> >    // Setup "main" thread
> > +  mMainThread = new nsThread(queue, nsThread::MAIN_THREAD, 0);
> > +
> > +  prioritizedCopy->SetMutexRef(queue->MutexRef());
> 
> It seems like the `ThreadEventQueue` ctor could do this. Or is it optional?

ThreadEventQueue is generic. It doesn't know anything about PrioritizedEventQueues.

> @@ +112,5 @@
> >    // Setup "main" thread
> > +  mMainThread = new nsThread(queue, nsThread::MAIN_THREAD, 0);
> > +
> > +  prioritizedCopy->SetMutexRef(queue->MutexRef());
> > +  prioritizedCopy->SetNextIdleDeadlineRef(mMainThread->NextIdleDeadlineRef());
> 
> Same deal, but the `nsThread` ctor.

Same deal. nsThread doesn't know that it's being passed a PrioritizedEventQueue.

Comment 10

[Nathan Froyd [:froydnj]]

(In reply to Bill McCloskey (:billm) from comment #9)
> (In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment
> > @@ +542,5 @@
> > >  #ifdef MOZ_CANARY
> > >  int sCanaryOutputFD = -1;
> > >  #endif
> > >  
> > > +nsThread::nsThread(SynchronizedEventQueue* aQueue,
> > 
> > This could probably be a NotNull (which is used throughout this file).
> 
> I did this, but it's pretty ugly (lots of wrap calls). I think the main
> problem is that using NotNull doesn't allow any implicit conversions. That
> is, NotNull<S*> cannot be turned into NotNull<T*> even if S* turns into T*.
> The same thing happens with NotNull<RefPtr<S>> and NotNull<RefPtr<T>>. I'm
> definitely not a fan of this thing.

We should fix the conversion thing; all of our other smart pointers permit such conversions.

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Nathan Froyd [:froydnj] from comment #8)
> Comment on attachment 8888627 [details] [diff] [review]
> Refactor event queue to allow multiple implementations
> 
> Review of attachment 8888627 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I tried to add some higher-level comments below.  I had to draw a picture to
> understand all the different relationships between *EventQueue in this
> patch...maybe we should put some things in mozilla::detail:: to permit a
> little more naming freedom?  I know we haven't been huge fans of using
> "Abstract" in names, but maybe s/EventQueue/AbstractEventQueue/,
> s/SimpleEventQueue/EventQueue/ would help too?

Done.

> ::: xpcom/threads/Queue.h
> @@ +9,5 @@
> > +
> > +namespace mozilla {
> > +
> > +template<class T>
> > +class Queue
> 
> It would be super, super nice if the confusion between this and nsEventQueue
> went away.  This patch is already adding enough things with "Queue" or
> "EventQueue" in their names that we don't need two classes that are very
> nearly copies of one another.

OK, done. I replaced uses of nsEventQueue with EventQueue (which was SimpleEventQueue in the previous patch).

> ::: xpcom/threads/SynchronizedEventQueue.h
> @@ +21,5 @@
> > +// SynchronizedEventQueue are split between ThreadTargetSink (which contains
> > +// methods for posting events) and SynchronizedEventQueue (which contains
> > +// methods for getting events). This split allows event targets (specifically
> > +// ThreadEventTarget) to use a narrow interface, since they only need to post
> > +// events.
> 
> There's a fair amount of overlap between the SynchronizedEventQueue
> interface and the EventQueue interface, though I guess
> SynchronizedEventQueue expects implementations to do their own locking.  Do
> you think it's really that much of a win to put the locking requirement in
> this intermediate interface, rather than just figuring out better, shareable
> interfaces between SynchronizedEventQueue, ThreadTargetSink, and EventQueue,
> and requiring clients of SynchronizedEventQueue to do their own locking?

The reason I split locking apart from the actual queue implementation is that I want the cooperative thread scheduling code to be able to share the queue data structures (including the prioritization and such) with our existing code, but have its own synchronization. (Cooperative scheduling requires a bunch of extra synchronization logic.)

Additionally, I don't want nsThread to know anything about locking since it should be able to use either cooperative scheduling or normal monitor-style synchronization. Putting that logic in different concrete SynchronizedEventQueue implementations serves that purpose (at the cost of virtual dispatch overhead).

It's possible there's a better way to do this. I don't really understand what you mean about "shareable interfaces". Having the clients handle synchronization would be bad, though.

> ::: xpcom/threads/ThreadEventQueue.h
> @@ +18,5 @@
> > +
> > +namespace mozilla {
> > +
> > +template<class InnerQueueT>
> > +class ThreadEventQueue final : public SynchronizedEventQueue
> 
> Is the idea behind making this a templated class that virtual dispatch for
> InnerQueueT should be able to be optimized away by the compiler?

Yes, exactly.

> ::: xpcom/threads/moz.build
> @@ +49,5 @@
> >      'Monitor.h',
> >      'MozPromise.h',
> >      'Mutex.h',
> > +    'PrioritizedEventQueue.h',
> > +    'Queue.h',
> 
> I'm not so sure Queue.h should be exported; people might get the idea that
> they should start using it more generally, and it's really not designed to
> be a general-purpose queue.  I guess worker code needs to see it via some
> chain of includes, though, doesn't it?  Ugh.  Remind me again why nsThread's
> constructor needs to know what sort of queue it's using? :)  If we didn't
> need that, many of the things added in this patch could just become private
> implementation details of our threading implementation...

I mostly managed to hide the includes. The worker thing is not a problem because dom/workers already has /xpcom/threads in its LOCAL_INCLUDES. The problem I ran into is that, once I started using EventQueue in nsThreadPool, I had to expose EventQueue.h to everything. And EventQueue.h needs to #include Queue.h. A solution here would be for nsThreadPool to have a UniquePtr to its queue. I can do that if you want.

I was able to un-export PrioritizedEventQueue.h and ThreadEventTarget.h.

> ::: xpcom/threads/nsThread.h
> @@ +138,4 @@
> >    struct nsThreadShutdownContext* ShutdownInternal(bool aSync);
> >  
> > +  RefPtr<mozilla::SynchronizedEventQueue> mEvents;
> > +  RefPtr<mozilla::ThreadEventTarget> mEventTarget;
> 
> I am not super-excited about mEvents requiring virtual dispatch.  LTO at the
> moment might be able to make the virtual dispatch go away, but it will be
> harder to make the virtual dispatch go away when we implement QDOM
> scheduling.  I don't think there's a really good way around this, though...

Yeah, this is the cost this patch imposes. I thought about some sort of weird thing where we use if conditions to select which implementation to use rather than virtual dispatch. Then the cost would be a branch mispredict rather than an indirect jump. But I don't think it's worth it. Virtual dispatch isn't *that* expensive. Remember that the refcounting of nsIRunnable already forces some virtual calls here.

> ::: xpcom/threads/nsThreadManager.cpp
> @@ +112,2 @@
> >    // Setup "main" thread
> > +  mMainThread = new nsThread(queue, nsThread::MAIN_THREAD, 0);
> 
> Can you comment on why you chose to pass the queue in, rather than letting
> nsThread decide for itself what the queue implementation should be based on
> whether it's the main thread or not?  I thought I remember you mentioning
> this is some past discussion, but I can't find it in this bug.

In bug 1350432, I need all the cooperatively scheduled threads to share the same event queue. I'm not sure how to do that besides passing it in.

Comment 12

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: refactoring patch

I moved the other patches to bug 1385413 and landed them.

Comment 13

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch v3

Here's a new version that simplifies Queue.h a bit.

Comment 14

[Eric Rahm [:erahm]]

Comment on attachment 8893107 [details] [diff] [review]
patch v3

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

Thanks, looks good to me. It would still be nice to add some tests, but I guess this is keeping the status quo from before.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/36ef70762b74
Refactor event queue to allow multiple implementations (r=erahm)

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d2ae762f65a
Fix build bustage on a CLOSED TREE

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/583b21cb9210
Fix test bustage on a CLOSED TREE

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/36ef70762b74
https://hg.mozilla.org/mozilla-central/rev/4d2ae762f65a
https://hg.mozilla.org/mozilla-central/rev/583b21cb9210