1450059 - Have a singleton thread for low-priority work

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Markus Stange [:mstange]]

In bug 1444430 I have some work that I would like to run on a separate thread at some point in the future. The work is low priority, doesn't need to be done soon, but should be done eventually.

Rather than launching my own thread, I'd like to use a central thread for this so that other consumers with similar requirements can use it, too, because each extra thread comes with some overhead.

I'm envisioning something like:
static nsresult
GenericWorkerThread::RunEventually(already_AddRefed<nsIRunnable>&& aEvent,
                                   bool aDiscardOnShutdown)

Nika tells me that the background hang reporter has a similar use case and currently uses its own thread.

What would be a good place for this? xpcom/threads/GenericWorkerThread.h maybe?
Do we have something like this already?

Bug 703948 has a similar feature request, but for a pool of threads instead of just one.

Comment 1

[Nathan Froyd [:froydnj]]

A couple of thoughts:

1) We don't have something like this already, at least not formally.  I think some people use the stream transport service as a sort of "run this when you have the time" thread, but I don't think there's widespread acknowledgement that this is a good idea.
2) This is a better idea than everybody spinning up their own private thread, and possibly forgetting to shut said thread down.
3) This imaginary thread would need to be started fairly early on in startup.
4) I think it should be run out of nsThreadManager.
5) We should try to convert various places to use this new facility.  I have some ideas on who should start using this.

Comment 2

[Josh Aas]

Attachment: Concept Impl v1.0

This is not ready to go, I am not even sure if it works yet. I'd like to get feedback on the approach before I spend any more time on it.

Comment 3

[Nathan Froyd [:froydnj]]

Comment on attachment 8987382 [details] [diff] [review]
Concept Impl v1.0

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

I think this is pretty reasonable.  Canceling review as you have indicated this is not ready to land.

::: xpcom/threads/nsIThreadManager.idl
@@ +12,4 @@
>  interface nsIRunnable;
>  interface nsIThread;
>  
> +[scriptable, function, uuid(E8564A08-6484-11E8-A15E-8FCBE0FC7600)]

Why are we revving this UUID and not the UUID of nsIThreadManager?  ('m not even sure we have to rev UUIDs anymore, given our addon situation.)

::: xpcom/threads/nsThreadManager.cpp
@@ +292,5 @@
>  {
>    MOZ_ASSERT(NS_IsMainThread(), "shutdown not called from main thread");
>  
> +  // Shut down the low priority thread pool before shutting down the
> +  // thread manager in general.

Why do we want to do this, rather than putting this check underneath the setting of mInitialized?  Perhaps the rationale is that we should be holding mLowPriorityThreadMutex for the entirety of this function?

@@ +687,5 @@
>  
> +NS_IMETHODIMP
> +nsThreadManager::DispatchToLowPriorityThread(nsIRunnable *aEvent)
> +{
> +  MutexAutoLock lock(mLowPriorityThreadMutex);

Do we want to check for !mInitialized here, and fail if so?

@@ +704,5 @@
> +    // Leave threads alive for up to 5 minutes
> +    rv = pool->SetIdleThreadTimeout(300000);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    pool.swap(mLowPriorityThreadPool);

We want to call SetThreadStackSize(nsIThreadManager::kThreadPoolStackSize) somewhere in here.

::: xpcom/threads/nsThreadUtils.cpp
@@ +328,5 @@
> +nsresult
> +NS_DispatchToLowPriorityThread(already_AddRefed<nsIRunnable>&& aEvent)
> +{
> +  nsCOMPtr<nsIRunnable> event(aEvent);
> +  return nsThreadManager::get().nsThreadManager::DispatchToLowPriorityThread(event);

Can't this just be .DispatchToLowPriorityThread(event), instead of qualifying it?

Can we have the nsThreadManager interface just accept already_AddRefed, so we're not doing extra refcounting here?  (I'm not completely sure whether we even want to expose this interface to JS in the first place; presumably something dispatched from JS is going to run JS, and we're not going to be on the right thread at that point...)

::: xpcom/threads/nsThreadUtils.h
@@ +142,5 @@
> + * @returns NS_ERROR_UNEXPECTED
> + *   If the thread is shutting down.
> + */
> +extern nsresult
> +NS_DispatchToLowPriorityThread(already_AddRefed<nsIRunnable>&& aEvent);

We may also want to have something like:

extern nsIEventTarget*
NS_LowPriorityEventTarget();

at some point.

Comment 4

[Eric Rahm [:erahm]]

Josh, are you still interested in working on this or should we hand it off to someone else?

Comment 5

[Josh Aas]

I'm interested in finishing the job but I won't be able to do it this week. I can commit to working on it next week though. If that isn't fast enough then someone else can take it. My apologies for the delay.

Comment 6

[Josh Aas]

Attachment: Concept Impl v1.1

Comment 7

[Josh Aas]

I posted an updated patch. I think we should convert at least one other consumer before we land this, and have confirmation that this actually works for the new consumer.

(In reply to Nathan Froyd [:froydnj] from comment #3)
> ::: xpcom/threads/nsIThreadManager.idl
> > +[scriptable, function, uuid(E8564A08-6484-11E8-A15E-8FCBE0FC7600)]
> 
> Why are we revving this UUID and not the UUID of nsIThreadManager?  ('m not
> even sure we have to rev UUIDs anymore, given our addon situation.)

I just got rid of the UUID rev. It was the wrong one and it doesn't seem necessary anyway.

> ::: xpcom/threads/nsThreadManager.cpp
> @@ +292,5 @@
> >  {
> >    MOZ_ASSERT(NS_IsMainThread(), "shutdown not called from main thread");
> >  
> > +  // Shut down the low priority thread pool before shutting down the
> > +  // thread manager in general.
> 
> Why do we want to do this, rather than putting this check underneath the
> setting of mInitialized?  Perhaps the rationale is that we should be holding
> mLowPriorityThreadMutex for the entirety of this function?

That was my rationale, but I think you're right that it should happen after the thread manager is marked uninitialized.

> @@ +687,5 @@
> >  
> > +NS_IMETHODIMP
> > +nsThreadManager::DispatchToLowPriorityThread(nsIRunnable *aEvent)
> > +{
> > +  MutexAutoLock lock(mLowPriorityThreadMutex);
> 
> Do we want to check for !mInitialized here, and fail if so?

Seems reasonable, done.

> @@ +704,5 @@
> > +    // Leave threads alive for up to 5 minutes
> > +    rv = pool->SetIdleThreadTimeout(300000);
> > +    NS_ENSURE_SUCCESS(rv, rv);
> > +
> > +    pool.swap(mLowPriorityThreadPool);
> 
> We want to call SetThreadStackSize(nsIThreadManager::kThreadPoolStackSize)
> somewhere in here.

I added this, but why isn't that the default for thread pools? This is a bizarre thing for anyone to think of doing explicitly when creating a thread pool.

> ::: xpcom/threads/nsThreadUtils.cpp
> @@ +328,5 @@
> > +nsresult
> > +NS_DispatchToLowPriorityThread(already_AddRefed<nsIRunnable>&& aEvent)
> > +{
> > +  nsCOMPtr<nsIRunnable> event(aEvent);
> > +  return nsThreadManager::get().nsThreadManager::DispatchToLowPriorityThread(event);
> 
> Can't this just be .DispatchToLowPriorityThread(event), instead of
> qualifying it?

Fixed

> Can we have the nsThreadManager interface just accept already_AddRefed, so
> we're not doing extra refcounting here?  (I'm not completely sure whether we
> even want to expose this interface to JS in the first place; presumably
> something dispatched from JS is going to run JS, and we're not going to be
> on the right thread at that point...)

I did it this way to match all of the other dispatch methods. Inconsistency seems like something to avoid here.

Comment 8

[Nathan Froyd [:froydnj]]

(In reply to Josh Aas from comment #7)
> > @@ +704,5 @@
> > > +    // Leave threads alive for up to 5 minutes
> > > +    rv = pool->SetIdleThreadTimeout(300000);
> > > +    NS_ENSURE_SUCCESS(rv, rv);
> > > +
> > > +    pool.swap(mLowPriorityThreadPool);
> > 
> > We want to call SetThreadStackSize(nsIThreadManager::kThreadPoolStackSize)
> > somewhere in here.
> 
> I added this, but why isn't that the default for thread pools? This is a
> bizarre thing for anyone to think of doing explicitly when creating a thread
> pool.

I added kThreadPoolStackSize, but didn't make it the default, because I wasn't sure how many things would break and wanted to convert them incrementally.  I think we're probably in a position where all the thread pools either are already using it or explicitly setting their own stack size, so we could make it the default now.

> > Can we have the nsThreadManager interface just accept already_AddRefed, so
> > we're not doing extra refcounting here?  (I'm not completely sure whether we
> > even want to expose this interface to JS in the first place; presumably
> > something dispatched from JS is going to run JS, and we're not going to be
> > on the right thread at that point...)
> 
> I did it this way to match all of the other dispatch methods. Inconsistency
> seems like something to avoid here.

The dispatch methods in nsIThreadManager?  Or dispatch methods generally (e.g. nsIEventTarget)?

I think we can aim for consistency here, but we need to ensure this method is not exposed to script.  If it were, script could then do:

  Services.dm.dispatchToLowPriorityThread(function() { ... });

which is probably going to break things when the closure is called, because it's manipulating JS objects off the main thread.

We can add already_AddRefed interfaces in a followup or something.

Comment 9

[Nathan Froyd [:froydnj]]

Comment on attachment 8996000 [details] [diff] [review]
Concept Impl v1.1

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

r=me with the [noscript] bit added and some discussion about what the name of this new-fangled thing should be.

::: xpcom/threads/nsIThreadManager.idl
@@ +171,5 @@
> +   *
> +   * @param event
> +   *   The event to dispatch.
> +   */
> +  void dispatchToLowPriorityThread(in nsIRunnable runnable);

As a user, this name suggests to me that my stuff isn't going to get executed in a timely fashion.  Maybe we want to say dispatchToCommonWorkThread or dispatchToWorkerPool or something, and revise the docs?  I'd want to avoid people saying "oh, I started up my own thread because my runnables are important" or something like that.  WDYT?

This also needs to be [noscript] as discussed in the bug.

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:jaas, could you have a look please?

Comment 11

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

Should this still land?

Comment 12

[Eric Rahm [:erahm]]

Josh picked this up. I believe there was a minor nit to address and some bikeshedding to be done.

Comment 13

[Josh Aas]

I don't think this should be landed yet, it needs another look over with Nathan's comments in mind. I don't know when I'll get to it but it's on my list if nobody else beats me to it.

Comment 14

[Nathan Froyd [:froydnj]]

Attachment: Bug 1450059 - part 1 - add a NS_DispatchToBackgroundThread function; r=kriswright

We have a number of people starting up singleton threads for the sole
purpose of running a single runnable on them. These consumers often
leave the thread running until some point close to shutdown, or they
never shut it down at all. Let's add a helper function to do the thing
they actually want to do, and then we can modify the implementation of
that function as necessary as we merge singleton threads (and even
thread pools) together.

Comment 15

[Nathan Froyd [:froydnj]]

Attachment: Bug 1450059 - part 2 - move ProfilerScreenshots over to the background thread; r=gregtatum

Depends on D46997

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2a96da82da40
part 1 - add a NS_DispatchToBackgroundThread function; r=KrisWright
https://hg.mozilla.org/integration/autoland/rev/8860f157dcb2
part 2 - move ProfilerScreenshots over to the background thread; r=gregtatum

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Hey emalysz, once this patch lands in mozilla-central, we might want to migrate nsSystemInfo from its internal lazy thread to this shared background thread.

Comment 18

[Nathan Froyd [:froydnj]]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #17)

Hey emalysz, once this patch lands in mozilla-central, we might want to migrate nsSystemInfo from its internal lazy thread to this shared background thread.

One thing to note is that, at the moment, there's no provision for "these tasks are cpu-intensive (and probably short)" vs. "these tasks do a bunch of I/O and tie up a thread for a long time". Those patches are coming.

Comment 19

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/2a96da82da40
https://hg.mozilla.org/mozilla-central/rev/8860f157dcb2