1522150 - Add a DeferredTimers queue that executes before the normal Idle queue

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Randell Jesup [:jesup] (needinfo me)]

We want to prioritize deferred setTimeout/setInterval timers (bug 1270059) ahead of "normal" idle tasks, and to do that the simplest method is to add another queue to PrioritizedEventQueue.cpp/etc

Overall it's straightforward, though a few bits of care are needed as there are now 2 "idle" queues to be concerned with.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Add a DeferredTimers queue ahead of the normal Idle EventQueue

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 2

[Olli Pettay [:smaug][[e-mail]]]

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 3

[Olli Pettay [:smaug][[e-mail]]]

Or if we want to think DeferredTimers as a variant of idle queue, perhaps separate enum which can be passed to NS_Idle* methods. The enum would only have Idle and DeferredTimers, defaulting to Idle.

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

The naming is now a bit wonky. I was going to go through and rename, but OTOH the default if you don't add the param is to dispatch to the Idle queue. I figure if someone (froyd) has a strong preference on how the NS_FooToBar stuff should look, I'll just make it do that.

I could just add NS_DeferredTimerDispatchToCurrentThread() instead of all the other changes, for example, or an independent "which idle queue" bool/enum - but I'm not sure that's a great solution.

I was trying to think ahead, realizing that chrome found reasons to have something crazy like 70+ queues.

Bas said: Maybe we'll want less than 70, but I feel we'll almost certainly discover we'll find more.

Comment 5

[Nathan Froyd [:froydnj]]

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?

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Rename EventPriority to EventQueuePriority to avoid name conflict with MacOS

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Add a DeferredTimers queue ahead of the normal Idle EventQueue

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Rename NS_IdleDispatch* functions since they take queue identifiers

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

Comment 9

[(no longer active)]

(In reply to Randell Jesup [:jesup] from comment #8)

Created attachment 9039024 [details] [diff] [review]
Rename NS_IdleDispatch* functions since they take queue identifiers

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

FWIW I just added a couple more of these calls in bug 1522370 and bug 1522630.

Comment 10

[Nathan Froyd [:froydnj]]

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 11

[Nathan Froyd [:froydnj]]

I meant to r- part 2, whoops.

(In reply to Nathan Froyd [:froydnj] from comment #10)

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.

Oh, I see that part 3 actually does a renaming, which makes this a little more palatable, and also complains about using queues we don't expect. Hmm...

Comment 12

[Nathan Froyd [:froydnj]]

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.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
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

Comment 14

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/0f5c896960f5
https://hg.mozilla.org/mozilla-central/rev/65cf08e33fe2