Works with no apparent problems. i've pushed a Try of just this patch alone; later I'll push one with the timer patch that uses this. Happy to fiddle with some of the details. Longer-term we might consider making the PrioritizedEventQueue less tightly tied to the list of queues, so we have have finer-grained queuing and add them without lots of work. Chrome has something like 70+ queues IIRC

Comment on attachment 9038562 [details] [diff] [review] Add a DeferredTimers queue ahead of the normal Idle EventQueue >@@ -131,24 +133,27 @@ interface nsIThread : nsISerialEventTarg > > /** > * Dispatch an event to the thread's idle queue. This function may be called > * from any thread, and it may be called re-entrantly. This talks about idle queue, but the method is now generic. > * > * @param event > * The alreadyAddRefed<> event to dispatch. > * NOTE that the event will be leaked if it fails to dispatch. >+ * @param queue >+ * Which event priority queue this should be added to > * > * @throws NS_ERROR_INVALID_ARG > * Indicates that event is null. > * @throws NS_ERROR_UNEXPECTED > * Indicates that the thread is shutting down and has finished processing > * events, so this event would never run and has not been dispatched. > */ >- [noscript] void idleDispatch(in alreadyAddRefed_nsIRunnable event); >+ [noscript] void dispatchToQueue(in alreadyAddRefed_nsIRunnable event, >+ in EventPriority queue); > nsresult NS_IdleDispatchToThread(already_AddRefed<nsIRunnable>&& aEvent, >- nsIThread* aThread) { >+ nsIThread* aThread, EventPriority aQueue) { This is really weird. Idle is idle, not some other queue. > nsresult NS_IdleDispatchToCurrentThread( >- already_AddRefed<nsIRunnable>&& aEvent) { >- return NS_IdleDispatchToThread(std::move(aEvent), NS_GetCurrentThread()); >+ already_AddRefed<nsIRunnable>&& aEvent, EventPriority aQueue) { >+ return NS_IdleDispatchToThread(std::move(aEvent), NS_GetCurrentThread(), aQueue); > } Same here. These APIs lets one to dispatch high priority event using NS_Idle* methods But otherwise, treating DeferredTimers queue similarly, but just a bit higher priority as idle queue looks good.

Comment on attachment 9038562 [details] [diff] [review] Add a DeferredTimers queue ahead of the normal Idle EventQueue Review of attachment 9038562 [details] [diff] [review]: ----------------------------------------------------------------- f+ for me. I'm really hoping I'm wrong about the requirement for IdleRunnable wrapping, below, but I suspect I'm not. ::: xpcom/threads/nsThreadUtils.cpp @@ +289,5 @@ > } > > nsresult NS_IdleDispatchToCurrentThread( > + already_AddRefed<nsIRunnable>&& aEvent, EventPriority aQueue) { > + return NS_IdleDispatchToThread(std::move(aEvent), NS_GetCurrentThread(), aQueue); I agree with smaug here: it seems silly to have `NS_IdleDispatchTo*Thread` take an explicit queue. I don't really relish the idea of adding *another* set of NS_ dispatch functions, but I think we should; and these *IdleDispatch* variants can just call into those (or we can rewrite all the IdleDispatch callsites). Please add a separate function for this queue-specific functionality. Or I guess we could make it go through SchedulerGroup et al and then the particular TaskCategory that gets used could get sent to the particular EventPriority? @@ +360,5 @@ > }; > > extern nsresult NS_IdleDispatchToThread(already_AddRefed<nsIRunnable>&& aEvent, > + uint32_t aTimeout, nsIThread* aThread, > + EventPriority aQueue) { Everything said above goes doubly so here; what does it even mean to idle dispatch to a non-idle queue with a timeout? Oh, I see, the DOM patch depends on the deferred timer runnables going through idle dispatch so that idle dispatch wraps them up in IdleRunnable. Ugh. I guess we need that so we only defer the timers for so long, right, and we can't just have them be normal runnables in a lower-priority queue? Ugh. OK, then, we need a separate set of functions and then we need to have a *third*, refactored, specific-to-.cpp set of functions that NS_IdleDispatchToThread and the new set of functions can build off of?

This renames the API methods to avoid 'NS_Idle*' for things that can touch any queue.

Comment on attachment 9038978 [details] [diff] [review] Add a DeferredTimers queue ahead of the normal Idle EventQueue Review of attachment 9038978 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for not explaining the naming thing below more clearly. ::: xpcom/threads/MainThreadQueue.h @@ +25,5 @@ > auto queue = MakeUnique<MainThreadQueueT>( > MakeUnique<InnerQueueT>(EventQueuePriority::High), > MakeUnique<InnerQueueT>(EventQueuePriority::Input), > MakeUnique<InnerQueueT>(EventQueuePriority::Normal), > + MakeUnique<InnerQueueT>(EventQueuePriority::DeferredTimers), We should--as a followup--make it so PrioritizedEventQueue implicitly constructs its sub-queues in the constructor, and doesn't get them passed as arguments, so people can stop adding a new queue here. Instead, we can just iterate over the EventQueuePriority values somehow in PrioritizedEventQueue(). ::: xpcom/threads/nsIThread.idl @@ +133,4 @@ > > /** > * Dispatch an event to the thread's idle queue. This function may be called > * from any thread, and it may be called re-entrantly. This comment no longer reflects the actual behavior of the method. I think we should also encourage people to *not* use this function directly, but to use the higher-level NS_* functions. ::: xpcom/threads/nsThreadUtils.h @@ +125,5 @@ > * If the thread is shutting down. > */ > extern nsresult NS_IdleDispatchToCurrentThread( > + already_AddRefed<nsIRunnable>&& aEvent, > + mozilla::EventQueuePriority aQueue = mozilla::EventQueuePriority::Idle); This parameter is not documented here or in all the similar changes below. The documentation stating that the event is dispatched to the idle queue of the thread is no longer accurate, here or in the changes below This change and similar changes is different from what I thought smaug and I were both requesting; looking back, I don't think I was explicit enough, which is my fault. I was expecting: // nsThreadUtils.h // Unchanged, no aQueue parameter. NS_IdleDispatchToCurrentThread(...) // ...and existing variants. // New declarations, no aQueue parameter. NS_DeferredTimerDispatchToCurrentThread(...) // ...and other necessary variants. // nsThreadUtils.cpp // No timeout version can dispatch straight into the Idle queue. NS_IdleDispatchToCurrentThread(...) { ... } // No timeout version can dispatch straight into the DeferredTimer queue. NS_DeferredTimerDispatchToCurrentThread(...) { ... } static nsresult DispatchToThreadQueueWithTimeout(...) { // Wrap things up in IdleRunnableWrapper and dispatch to the appropriate queue. } // Timeout-using variants NS_IdleDispatchToThread(...) { return DispatchToThreadQueueWithTimeout(..., EventPriority::Idle); } NS_DeferredTimerDispatchToThread(...) return DispatchToThreadQueueWithTimeout(..., EventPriority::DeferredTimer); } So that the caller never directly passes EventPriority, but leaves that to the particular named function. I realize this makes the API surface huge (and I think we do want to do something about that...but not yet), but my sense is that a larger API surface here is preferable to enabling people to pass in unconstrained EventPriority values, at least for the moment.

Comment on attachment 9039024 [details] [diff] [review] Rename NS_IdleDispatch* functions since they take queue identifiers Review of attachment 9039024 [details] [diff] [review]: ----------------------------------------------------------------- OK, thinking about this some more, the event queue for non-timeout dispatches seems reasonable, so let's keep that. I would say that we should have separate functions for dispatching to the idle queue or the deferred timer queue with a timeout, but then that seems kind of asymmetric, and not in a good way. We'll see how it works with the assert, I guess. I'm not super-thrilled about adding constant loads to every single callsite, but that's life. r=me for this and part 2. ::: xpcom/threads/nsThreadUtils.h @@ +170,5 @@ > + already_AddRefed<nsIRunnable>&& aEvent, uint32_t aTimeout, > + mozilla::EventQueuePriority aQueue); > + > +/** > + * Dispatch the given event to the idle queue of a thread. This comment is not correct ("an" idle queue?) @@ +188,3 @@ > > /** > * Dispatch the given event to the idle queue of a thread. This comment is not correct. @@ +197,2 @@ > * @param aTimeout The time in milliseconds until the event should be > * moved from the idle queue to the regular queue, if it hasn't been This is no longer "the" idle queue.

Pushed by rjesup@wgate.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/0f5c896960f5 Rename EventPriority to EventQueuePriority to avoid name conflict with MacOS r=froyd https://hg.mozilla.org/integration/mozilla-inbound/rev/65cf08e33fe2 Add a DeferredTimers queue ahead of the normal Idle EventQueue r=froyd