Closed Bug 1145354 Opened 7 years ago Closed 7 years ago

Need a simple shared-xpcom-thread with lazy-instantiation and idle shutdown (LazySingletonThread)

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: jesup, Assigned: jesup)

References

Details

Attachments

(5 files, 2 obsolete files)

LazyIdleThread is too lazy for a number of use-cases; it shuts down threads if not used (even if users still exist and may want to send events to it, such as IPC).  Some of the on-creating-thread requirements (for Dispatch(), etc) are problematic for things like IPC as well.

We could use a LazySingletonThread which is created lazily, is shared, can be set to shut down if idle "enough" when no active users (references) still exist.

An example use would be for IPC in media/mtransport (instead of sending all IPC to MainThread).
Comment on attachment 8580277 [details] [diff] [review]
add LazySingletonThread (wip)

I don't believe this will pass review yet, but let's get the ball rolling
Attachment #8580277 - Flags: review?(nfroyd)
Attachment #8580277 - Flags: feedback?(khuey)
Attached patch add LazySingletonThread (obsolete) — Splinter Review
Changed API to use explicit AddUse/ReleaseUse counting because refcounts are swizzled by things like the shutdown even observers, etc.  Made Enable/DisableIdleTimeout() threadsafe to enable that.  Verified that thread shuts down when unused in webrtc, and starts up again as needed.
Attachment #8580277 - Attachment is obsolete: true
Attachment #8580277 - Flags: review?(nfroyd)
Attachment #8580277 - Flags: feedback?(khuey)
Attachment #8580733 - Attachment description: add LazySingletonThread (wip) → add LazySingletonThread
Attachment #8580733 - Flags: review?(nfroyd)
Attachment #8580733 - Flags: feedback?(khuey)
Comment on attachment 8580277 [details] [diff] [review]
add LazySingletonThread (wip)

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

I haven't looked through the logic, these are just some things I noticed while skimming...and I see you have a new patch up, so I might as well get these out of the way.

::: xpcom/threads/LazyIdleThread.cpp
@@ +80,5 @@
> +  nsIThread* currentThread = NS_GetCurrentThread();
> +  if (currentThread) {
> +    nsCOMPtr<nsISupports> current(do_QueryInterface(currentThread));
> +    nsCOMPtr<nsISupports> test(do_QueryInterface(mOwningThread));
> +    return current == test;

Isn't this just a complicated way of saying:

  mOwningThead->IsOnCurrentThread();

?  (Minus some XPCOM goo.)  I guess this way matches the check for ASSERT_OWNING_THREAD earlier in the file...

@@ +92,5 @@
> +  MOZ_ASSERT(NS_IsMainThread(), "Wrong thread!");
> +
> +  nsresult rv;
> +  nsCOMPtr<nsIObserverService> obs =
> +    do_GetService(NS_OBSERVERSERVICE_CONTRACTID, &rv);

mozilla::services::GetObserverService(), please.

::: xpcom/threads/LazyIdleThread.h
@@ +59,5 @@
>    LazyIdleThread(uint32_t aIdleTimeoutMS,
>                   const nsCSubstring& aName,
>                   ShutdownMethod aShutdownMethod = AutomaticShutdown,
> +                 nsIObserver* aIdleObserver = nullptr,
> +                 bool aShutdownWhenUnused = false);

Since we can use C++11 delegating constructors ([class.base.init]p6ff), let's make a protected constructor:

LazyIdleThread(uint32_t, const nsCSubstring& ShutdownMethod, nsIObserver*, bool)

and have this public one call that with the last bool parameter false, rather than exposing this extra parameter people shouldn't really use.
Attachment #8580277 - Attachment is obsolete: false
Attachment #8580277 - Attachment is obsolete: true
One thing I should note: part of the reason for needing this class is that the base LazyIdleThread has limitations due to it's design; in particular that the nsIEventTarget exposed is *not* what is returned by NS_GetCurrentThread()

NS_GetCurrentThread() is used in many places to grab a target to pass events to (such as in IPC).  If this didn't wrap a separate nsThread object that actually runs (and instead was (or acted like) a single nsThread object), then much of the reason for LazySingletonThread would go away.  Some or all of the owning-thread-requirements for access to the variables would also go away or be simplified such that the semantics could more closely match normal nsThread semantics (such as being able to Dispatch() to it from any thread).

I'm not saying it would be easy to build a more unified LazyIdleTHread on top of the existing structure; likely aspects of LazyIdleThread would need to migrate to nsThread or additional hooks get added.  And there may still be need/reason for a (simpler) LazySingletonThread for other reasons.  I think any push to "fix" LazyIdleThread's design should be a separate longer-term bug unless someone has a great revelation on how that could be done right now.
Comment on attachment 8580733 [details] [diff] [review]
add LazySingletonThread

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

I'm still not completely clear on the differences between LazySingletonThread and LazyIdleThread; LazySingletonThread.h should put some effort into describing that.  Comment 5, especially, is confusing to me, as it seems to assert that we can't use LazyIdleThread because of conflicts with NS_GetCurrentThread(), but this class doesn't seem to do much different than LazyIdleThread in that regard.  Unless the OnDispatchedEvent override addresses part of that?

I do think that comment 5 is pointing in the right direction in saying that Lazy{Singleton,Idle}Thread should really somehow be built into nsThread, so we're not building an abstraction (Lazy*Thread) on top of an abstraction (nsThread) on top of an abstraction (PRThread) on top of OS threads...but now is probably not the time to try to untangle all that, either.

I didn't carefully verify the locking behavior for member variables.

::: xpcom/threads/LazyIdleThread.cpp
@@ +183,5 @@
> +  {
> +    MutexAutoLock lock(mMutex);
> +
> +    MOZ_ASSERT(mIdleTimeoutOnlyWhenUnused ||
> +               (mIdleTimeoutEnabled ? !mPendingEventCount : mPendingEventCount==1), "Shouldn't have events yet!");

Once you have the enum change in for s/mIdleTimeoutOnlyWhenUnused/mIdleTimeoutBehavior/, this assert and others can be made more readable:

MOZ_ASSERT_IF(mIdleTimeoutBehavior == ..., ....);

@@ +193,4 @@
>  
>    nsresult rv;
>  
>    if (mShutdownMethod == AutomaticShutdown && NS_IsMainThread()) {

This block shouldn't be necessary now that you're doing this work in the constructor, right?

@@ +318,3 @@
>  
>    if (mThread) {
>      if (mShutdownMethod == AutomaticShutdown && NS_IsMainThread()) {

This block shouldn't be necessary now that you're doing this work in the constructor, right?

@@ +637,5 @@
> +    NS_WARNING("Failed to remove observer!");
> +  }
> +
> +  // XXX Would it be better to grab the Mutex, set mOwningThread to nullptr,
> +  // and then just call Shutdown()?

Assuming we can get away with it, I think that would be a better solution, yes.

::: xpcom/threads/LazyIdleThread.h
@@ +228,5 @@
>    /**
> +   * Whether or not the idle timeout is used only when refcount is 1 (LazySingleton)
> +   * or when no events are currently pending (LazyIdle)
> +   */
> +  bool mIdleTimeoutOnlyWhenUnused;

I think this (along with the constructor argument) would be clearer if it was something like:

enum class TimeoutBehavior {
  SingletonReference,
  NoEventsPending,
};

TimeoutBehavior mIdleTimeoutBehavior.

It is not immediately obvious to me from the comment which boolean corresponds to which behavior.  This change would make a number of asserts etc. easier to read.

::: xpcom/threads/LazySingletonThread.h
@@ +14,5 @@
> + * This class provides a basic event target that creates its thread lazily and
> + * destroys its thread when there are no references after an optional
> + * period of inactivity. It may be created on any thread.  Once an additional
> + * ref (beyond 1) is added, the wrapped thread will not be destroyed until
> + * the ref is dropped, even if idle, unless Shutdown() is called.

I found talking about "references" here confusing, given that AddUse/ReleaseUse is really the mechanism that's being described here, and "references"/"refs" would typically refer to normal refcounts of a refcounted object.  This paragraph needs a rewrite.

The code sample may want to mention AddUse/ReleaseUse; comment should say something about why you would care.

@@ +58,5 @@
> +    : LazyIdleThread(aIdleTimeoutMS, aName, aShutdownMethod, nullptr, true)
> +  {}
> +
> +  // Don't require same-thread for Dispatch/PutEvent
> +  NS_IMETHODIMP OnDispatchedEvent(nsIThreadInternal* /*aThread */) {

Probably deserves an |override| annotation?

@@ +66,5 @@
> +  /*
> +   * Keep track of how many instances are using a LazySingletonThread.  When no one
> +   * is using it, enable idle shutdown.
> +   */
> +  MozExternalRefCountType AddUse()

So, really, these just serve as optimization hints to tell the object whether the underlying thread can be shutdown?

I'm not totally thrilled with these, but I'm not sure that the alternative(s) are any better...
Attachment #8580733 - Flags: review?(nfroyd) → feedback+
interdiffs for ease in reviewing (the one thing that's better about ReviewBoard...)
Attachment #8580733 - Attachment is obsolete: true
Attachment #8580733 - Flags: feedback?(khuey)
Attachment #8582000 - Flags: review?(nfroyd)
Attachment #8582000 - Flags: feedback?(khuey)
Comment on attachment 8582000 [details] [diff] [review]
add LazySingletonThread

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

Apologies for the comments here; I should have started asking some of these questions yesterday.

::: xpcom/threads/LazyIdleThread.cpp
@@ +105,5 @@
> +  MOZ_ASSERT(NS_IsMainThread(), "Wrong thread!");
> +
> +  nsresult rv;
> +  nsCOMPtr<nsIObserverService> obs =
> +    do_GetService(NS_OBSERVERSERVICE_CONTRACTID, &rv);

mozilla::services::GetObserverService();

And then you'll probably want to make |rv| a |DebugOnly<nsresult>|.

@@ +121,5 @@
> +  MOZ_ASSERT(NS_IsMainThread(), "Wrong thread!");
> +
> +  nsresult rv;
> +  nsCOMPtr<nsIObserverService> obs =
> +    do_GetService(NS_OBSERVERSERVICE_CONTRACTID, &rv);

Same comment applies here.

@@ +181,3 @@
>    if (mThread) {
>      nsCOMPtr<nsIRunnable> runnable(new nsRunnable());
> +    if (NS_FAILED(mThread->Dispatch(runnable, NS_DISPATCH_NORMAL))) {

Why is this change correct?  In particular, we are now skipping PreDispatch() (and the associated incrementing of mPendingEventCount) and the checks for whether we need to shove things on the runnable queue.

@@ +212,5 @@
> +  {
> +    MutexAutoLock lock(mMutex);
> +
> +    MOZ_ASSERT(mIdleTimeoutBehavior == SingletonReference ||
> +               (mIdleTimeoutEnabled ? !mPendingEventCount : mPendingEventCount==1), "Shouldn't have events yet!");

Why the two changes here:

1) mIdleTimeoutBehavior == SingletonReference completely skips the second part, which doesn't look right--we should still be able to say something about mPendingEventCount even in that case;
2) The assert changes even for the default behavior of mIdleTimeoutBehavior == NoEventsPending, which I do not understand.

@@ +219,5 @@
> +    MOZ_ASSERT(!mThreadIsShuttingDown, "Should have cleared that!");
> +  }
> +#endif
> +
> +  RegisterShutdown(true);

In the previous version of the patch, I thought you were trying to simplify things by registering a shutdown observer in the constructor only; the register/unregister pairs then become superfluous.  But you're explicitly keeping both the constructor and the register/unregister pairs in this version.  Why is that?

@@ +294,5 @@
>  
> +    shouldSchedule = !mIdleNotificationCount &&
> +                     (!mPendingEventCount ||
> +                      (mIdleTimeoutBehavior == SingletonReference &&
> +                       mIdleTimeoutEnabled));

Why this change?

@@ +378,5 @@
>      {
>        MutexAutoLock lock(mMutex);
>  
> +      MOZ_ASSERT(mIdleTimeoutBehavior == SingletonReference ||
> +                 (mIdleTimeoutEnabled ? !mPendingEventCount : mPendingEventCount==1), "Huh?!");

Same questions here as in EnsureThread();

@@ +551,5 @@
>    {
>      MutexAutoLock lock(mMutex);
>  
> +    if (mIdleTimeoutBehavior == SingletonReference) {
> +      if (!mIdleTimeoutEnabled || mIdleNotificationCount || mShutdown) {

Why do we not care about mPendingEventCount in the case of LazySingletonThread?

@@ +595,5 @@
>      MutexAutoLock lock(mMutex);
>  
>      if (aEventWasProcessed) {
> +      MOZ_ASSERT(mIdleTimeoutBehavior == SingletonReference || mPendingEventCount,
> +                 "Mismatched calls to observer methods!");

Why do we not care about the value of mPendingEventCount in the LazySingletonThread case?

@@ +605,5 @@
>        return NS_OK;
>      }
>  
> +    shouldNotifyIdle = !mPendingEventCount ||
> +                       (mIdleTimeoutBehavior == SingletonReference && mIdleTimeoutEnabled);

Why are we scheduling the timer again in the middle of the time running when we have a LazySingletonThread?

::: xpcom/threads/LazySingletonThread.h
@@ +29,5 @@
> + *     : mThread(GetIOThread())  // nsIEventTarget
> + *   {
> + *     // Mark that we're using the shared thread and need it to stick around.
> + *     // (We use a local var and call GetIOThread() again to avoid lots of pain.)
> + *     LazySingletonThread* thread = static_cast<LazySingletonThread*>(GetIOThread());

I don't understand the "lots of pain" comment; GetIOThread() returns a LazySingletonThread* pointer already...and mThread->AddUse() would work just fine...

@@ +49,5 @@
> + *     mThread->Dispatch(...); // or whatever might use a thread
> + *   }
> + *
> + * private:
> + *   static LazySingletonThread* GetThread() {

Given your example ctor/dtor code, I guess this supposed to be called GetIOThread()?
Attachment #8582000 - Flags: review?(nfroyd) → feedback+
Can you expand on the use case more?  You essentially want to be able to "pin" a LazyIdleThread to prevent it from shutting down while you're doing something important, right?  This patch seems like a lot more effort than that should require.
Flags: needinfo?(rjesup)
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #10)
> Can you expand on the use case more?  You essentially want to be able to
> "pin" a LazyIdleThread to prevent it from shutting down while you're doing
> something important, right?  This patch seems like a lot more effort than
> that should require.

You'd think so, but there's some more to it than just pinning (which DisableIdleShutdown() would have done, if layered with some equivalent to AddUse()/ReleaseUse() to control when - effectively one part of what we've done here).

This also relaxes the restriction that only the "owner" thread (which created it) can send it events (required for IPC use), and also makes it more usable from non-mainthread "owners" (i.e. the xpcom-shutdown support).

And, when used with anything that caches NS_GetCurrentThread as an event target (like IPC), the mPendingEventCount will be horribly incorrect, because Dispatches are bypassing the LazyIdle wrapper.

As I said, the "right" solution (to that part of this problem) is to get rid of the two-part LazyIdle, but that's even more work.
Flags: needinfo?(rjesup)
> @@ +181,3 @@
> >    if (mThread) {
> >      nsCOMPtr<nsIRunnable> runnable(new nsRunnable());
> > +    if (NS_FAILED(mThread->Dispatch(runnable, NS_DISPATCH_NORMAL))) {
> 
> Why is this change correct?  In particular, we are now skipping
> PreDispatch() (and the associated incrementing of mPendingEventCount) and
> the checks for whether we need to shove things on the runnable queue.

This is a point; I may have broken base LazyIdle behavior here.  I'll try reverting it and see if this change is still needed (it may not be)

> 
> @@ +212,5 @@
> > +  {
> > +    MutexAutoLock lock(mMutex);
> > +
> > +    MOZ_ASSERT(mIdleTimeoutBehavior == SingletonReference ||
> > +               (mIdleTimeoutEnabled ? !mPendingEventCount : mPendingEventCount==1), "Shouldn't have events yet!");
> 
> Why the two changes here:
> 
> 1) mIdleTimeoutBehavior == SingletonReference completely skips the second
> part, which doesn't look right--we should still be able to say something
> about mPendingEventCount even in that case;

We can't - things that call Dispatch() on NS_GetCurrentThread bypass the LazyIdle Dispatch(), so mPendingEventCount becomes horribly unbalanced.

> 2) The assert changes even for the default behavior of mIdleTimeoutBehavior
> == NoEventsPending, which I do not understand.
> 
> @@ +219,5 @@
> > +    MOZ_ASSERT(!mThreadIsShuttingDown, "Should have cleared that!");
> > +  }
> > +#endif
> > +
> > +  RegisterShutdown(true);
> 
> In the previous version of the patch, I thought you were trying to simplify
> things by registering a shutdown observer in the constructor only; the
> register/unregister pairs then become superfluous.  But you're explicitly
> keeping both the constructor and the register/unregister pairs in this
> version.  Why is that?

We don't need it in the constructor; removed that.

> 
> @@ +294,5 @@
> >  
> > +    shouldSchedule = !mIdleNotificationCount &&
> > +                     (!mPendingEventCount ||
> > +                      (mIdleTimeoutBehavior == SingletonReference &&
> > +                       mIdleTimeoutEnabled));
> 
> Why this change?

Because we can't trust mPendingEventCount for Singletons
> 
> @@ +605,5 @@
> >        return NS_OK;
> >      }
> >  
> > +    shouldNotifyIdle = !mPendingEventCount ||
> > +                       (mIdleTimeoutBehavior == SingletonReference && mIdleTimeoutEnabled);
> 
> Why are we scheduling the timer again in the middle of the time running when
> we have a LazySingletonThread?

For Singletons, the timer should be active if mIdleTimeoutEnabled is true (i.e. for singletons, it's all about timeout enabled or not).

> ::: xpcom/threads/LazySingletonThread.h
> @@ +29,5 @@
> > + *     : mThread(GetIOThread())  // nsIEventTarget
> > + *   {
> > + *     // Mark that we're using the shared thread and need it to stick around.
> > + *     // (We use a local var and call GetIOThread() again to avoid lots of pain.)
> > + *     LazySingletonThread* thread = static_cast<LazySingletonThread*>(GetIOThread());
> 
> I don't understand the "lots of pain" comment; GetIOThread() returns a
> LazySingletonThread* pointer already...and mThread->AddUse() would work just
> fine...

I fought with C++ for a while before giving in trying to get it to let me cast it (and mThread should be an nsIEventTarget).  I'll revise this to match how the final r+'d patch for nr_socket_prsock.h/cpp ended up.

> @@ +49,5 @@
> > + *     mThread->Dispatch(...); // or whatever might use a thread
> > + *   }
> > + *
> > + * private:
> > + *   static LazySingletonThread* GetThread() {
> 
> Given your example ctor/dtor code, I guess this supposed to be called
> GetIOThread()?

yes
Alternate patch to drop trying to specialize LazyIdleThread.  Reduced complexity for short-term use until we can make LazyIdleThread do 'the right thing' and avoid needing this specialization in the first place.  Note: does not shut down the wrapped thread when idle (at least not yet, that could be a follow-on).  Much easier to follow locking and thread-safety requirements by untangling from LazyIdle's event-counting and OwningThread assertions, and no chance of breaking LazyIdle in the process
Attachment #8584188 - Flags: review?(nfroyd)
Comment on attachment 8584188 [details] [diff] [review]
add LazySingletonThread (no longer inheriting from LazyIdleThread)

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

I have a lot more confidence in saying this is correct.  Thanks for splitting it out on its own.

Ideally we can make it really lazy in the next iteration, rather than a glorified wrapper for nsIThread.

::: xpcom/threads/LazySingletonThread.cpp
@@ +15,5 @@
> +{}
> +
> +LazySingletonThread::~LazySingletonThread()
> +{
> +  Shutdown();

Here or in Shutdown, we need to assert that the use count is 0.

@@ +19,5 @@
> +  Shutdown();
> +}
> +
> +void
> +LazySingletonThread::RegisterShutdown(bool aIsActive)

Please don't abuse boolean parameters like this.  Just move the (cut-and-pasted) logic into RegisterShutdownObserver() and UnRegisterShutdownObserver.  Bonus points if you eliminate cut-and-paste via templates that take pointer-to-member-function arguments.

@@ +49,5 @@
> +  MOZ_ASSERT(NS_IsMainThread(), "Wrong thread!");
> +
> +  nsresult rv;
> +  nsCOMPtr<nsIObserverService> obs =
> +    do_GetService(NS_OBSERVERSERVICE_CONTRACTID, &rv);

services::GetObserverService().

@@ +65,5 @@
> +  MOZ_ASSERT(NS_IsMainThread(), "Wrong thread!");
> +
> +  nsresult rv;
> +  nsCOMPtr<nsIObserverService> obs =
> +    do_GetService(NS_OBSERVERSERVICE_CONTRACTID, &rv);

services::GetObserverService().

@@ +86,5 @@
> +  if (mThread) {
> +    return NS_OK;
> +  }
> +
> +  RegisterShutdown(true);

I still think moving this to the constructor, rather than doing it on every Ensure/Shutdown is better.  (I realize there's only one Ensure/Shutdown at the moment.)  But whatever.

@@ +271,5 @@
> +
> +NS_IMETHODIMP
> +LazySingletonThread::AfterProcessNextEvent(nsIThreadInternal* /* aThread */,
> +                                           uint32_t /* aRecursionDepth */,
> +                                           bool aEventWasProcessed)

|bool /* aEventWasProcessed */| ?

::: xpcom/threads/LazySingletonThread.h
@@ +24,5 @@
> +namespace mozilla {
> +/**
> + * This class provides a basic event target that creates its thread lazily and
> + * destroys its thread when there are no users after an optional
> + * period of inactivity. It may be created on any thread.  Once an additional

Add "Users register their intent to use this thread via the AddUse() function." prior to "Once an..."

@@ +28,5 @@
> + * period of inactivity. It may be created on any thread.  Once an additional
> + * user (beyond 1) is added, the wrapped thread will not be destroyed until
> + * the user calls ReleaseUse(), even if idle, unless Shutdown() is called.
> + *
> + * Note that unlike LazyIdleThread, LazySingletonThread can be used for threads

I think I might say, "Because of this use tracking, LazySingletonThread..." to tie this paragraph to the previous paragraph, and to make it slightly more obvious why we have the {Add,Release}Use API.

@@ +41,5 @@
> + *     : mThread(GetIOThread())  // nsIEventTarget
> + *   {
> + *     // Mark that we're using the shared thread and need it to stick around.
> + *     // (We use a local var and call GetIOThread() again to avoid lots of pain.)
> + *     LazySingletonThread* thread = static_cast<LazySingletonThread*>(GetIOThread());

Types and/or comments still need updating in your example.

@@ +98,5 @@
> +   * invokable from any thread
> +   */
> +  void RegisterShutdown(bool aIsActive); // from any thread
> +  void RegisterShutdownObserver();       // MainThread-only
> +  void UnRegisterShutdownObserver();     // MainThread-only

I don't think these need to be public members.

@@ +102,5 @@
> +  void UnRegisterShutdownObserver();     // MainThread-only
> +
> +  /*
> +   * Keep track of how many instances are using a LazySingletonThread.  When no one
> +   * is using it, enable idle shutdown.

Add "Please note that as we do not currently shutdown the idle thread, calls to AddUse and ReleaseUse are for documentation purposes only." to the end of this comment, please.

@@ +165,5 @@
> +   * Name of the thread (accessed from both threads, read-only)
> +   */
> +  nsCString mName;
> +};
> +}

Please add a "// namespace mozilla" after this brace.
Attachment #8584188 - Flags: review?(nfroyd) → review+
Comment on attachment 8584188 [details] [diff] [review]
add LazySingletonThread (no longer inheriting from LazyIdleThread)

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

Of the three words in the name of this class, only one of them is true.  Without support for the laziness, there's really no purpose in using this instead of nsThread.

More importantly, the "laziness" in LazyIdleThread is time based.  In that case, we want the thread to shut down (and stop taking up virtual address space, a tid, etc) if we haven't used it for a while.  If you want to do IPC on this thread you need to disable the idle timer (which LazyIdleThread already provides the ability to do).

As you point out, LazyIdleThread does not accept event dispatch from arbitrary threads.  But that is a difficult problem to solve because of the nsITimer usage, which would apply here too if LazySingletonThread were in fact lazy.

I think the easiest way to proceed here is to use a regular nsThread, and to have your top-level actor, when destroyed, tell the owning thread/main thread to schedule a timer to just down this thread if it's no longer in use.  Would that not solve this issue?
Attachment #8584188 - Flags: review-
Comment on attachment 8608254 [details] [diff] [review]
alternative SingletonThreadHolder for media/mtransport

Replace LazySingletonThread with SingletonThreadHolder; a simpler (much) instance of a ref-counted thread-wrapper.  Would be folded into Bug 1109338 "Part 4: Use LazySingletonThread for mtransport IPC IO" to replace references to LazySingletonThread.

Simply has a usecount for a singleton thread, and when it goes to 0, shuts it down, and restarts a new thread if there's a new use.  Based on a few bits from the previous LazySingleton patch.

I could wrap all the uses of mParentThread in #ifdef DEBUG, but I didn't.

Green Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=534d0c6bd0fb
Attachment #8608254 - Flags: review?(docfaraday)
Comment on attachment 8608254 [details] [diff] [review]
alternative SingletonThreadHolder for media/mtransport

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

A bunch of small stuff/nits.

::: media/mtransport/nr_socket_prsock.cpp
@@ +119,5 @@
>  
>  // Implement the nsISupports ref counting
>  namespace mozilla {
>  
> +#if defined(MOZILLA_INTERNAL_API) && !defined(MOZILLA_XPCOMRT_API)

I got the impression that we were switching from MOZILLA_INTERNAL_API to !MOZILLA_XPCOMRT_API.

@@ +120,5 @@
>  // Implement the nsISupports ref counting
>  namespace mozilla {
>  
> +#if defined(MOZILLA_INTERNAL_API) && !defined(MOZILLA_XPCOMRT_API)
> +class SingletonThreadHolder final

Let's disable the copy c'tor and assignment operator.

@@ +147,5 @@
> +    return mThread;
> +  }
> +
> +  /*
> +   * Keep track of how many instances are using a SingletonThreadHolder.  When no one

Wrap to 80 here.

@@ +153,5 @@
> +   */
> +  MozExternalRefCountType AddUse()
> +  {
> +    MOZ_ASSERT(mParentThread == NS_GetCurrentThread());
> +    MOZ_ASSERT(int32_t(mUseCount) >= 0, "illegal refcnt");

This will never fail since mUseCount is an unsigned type (AFAICT).

@@ +161,5 @@
> +      nsresult rv = NS_NewThread(getter_AddRefs(mThread));
> +      MOZ_RELEASE_ASSERT(NS_SUCCEEDED(rv) && mThread,
> +                         "Should successfully create mtransport I/O thread");
> +      NS_SetThreadName(mThread, mName);
> +      r_log(LOG_GENERIC,LOG_DEBUG,"Created wrapped SingletonThread %p", (void*)mThread.get());

Wrap to 80 here. Also, I didn't think the cast was necessary here. Same comment in ReleaseUse.

@@ +163,5 @@
> +                         "Should successfully create mtransport I/O thread");
> +      NS_SetThreadName(mThread, mName);
> +      r_log(LOG_GENERIC,LOG_DEBUG,"Created wrapped SingletonThread %p", (void*)mThread.get());
> +    }
> +    r_log(LOG_GENERIC,LOG_DEBUG,"AddUse: %u", (uint32_t) count);

Casting to unsigned is more foolproof here, but this will almost certainly work on any platform that matters to us. Same comment in ReleaseUse.

@@ +164,5 @@
> +      NS_SetThreadName(mThread, mName);
> +      r_log(LOG_GENERIC,LOG_DEBUG,"Created wrapped SingletonThread %p", (void*)mThread.get());
> +    }
> +    r_log(LOG_GENERIC,LOG_DEBUG,"AddUse: %u", (uint32_t) count);
> +    return (nsrefcnt) count;

No need to cast here.

@@ +171,5 @@
> +  MozExternalRefCountType ReleaseUse()
> +  {
> +    MOZ_ASSERT(mParentThread == NS_GetCurrentThread());
> +    nsrefcnt count = --mUseCount;
> +    MOZ_ASSERT(int32_t(mUseCount) >= 0, "illegal refcnt");

Pretty sure this will never fail since mUseCount is an unsigned type. We need to 0 check before the decrement. Also, maybe we should RELEASE_ASSERT this.

@@ +192,5 @@
> +};
> +
> +static StaticRefPtr<SingletonThreadHolder> sThread;
> +
> +static void ClearSingleton()

The name makes it sound like the singleton is being cleared now.

@@ +1234,1 @@
>    nsRefPtr<nsIUDPSocketChild> socket_child_ref = already_AddRefed<nsIUDPSocketChild>(aChild);

Line wrapping needed here.
Attachment #8608254 - Flags: review?(docfaraday)
On a second look, I guess casting back to int32_t will cause the overflow to be converted into a negative integer, provided mUseCount is a 32-bit signed.
Regarding MOZILLA_EXTERNAL_API, there seems to be MOZILLA_EXTERNAL_LINKAGE now too...
> > +#if defined(MOZILLA_INTERNAL_API) && !defined(MOZILLA_XPCOMRT_API)
> 
> I got the impression that we were switching from MOZILLA_INTERNAL_API to
> !MOZILLA_XPCOMRT_API.

Yeah, if I try that it blows up building some of the tests/etc (nsString internal APIs not available in external code, etc).  With the above it works.

> > +    r_log(LOG_GENERIC,LOG_DEBUG,"AddUse: %u", (uint32_t) count);
> 
> Casting to unsigned is more foolproof here, but this will almost certainly
> work on any platform that matters to us. Same comment in ReleaseUse.

unsigned long, and %lu.
Attached patch interdiffsSplinter Review
Attachment #8608884 - Flags: review?(docfaraday)
Comment on attachment 8608884 [details] [diff] [review]
interdiffs

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

I did some poking around, and it seems that bug 1101651 replaced almost every instance (in the webrtc code) of MOZILLA_INTERNAL_API with !MOZILLA_EXTERNAL_LINKAGE. It wasn't _every_ instance though. Were some just missed? Do you know what might be going on here?
Attachment #8608884 - Flags: review?(docfaraday)
Comment on attachment 8608884 [details] [diff] [review]
interdiffs

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

Ok, given that this MOZILLA_INTERNAL_API vs MOZILLA_EXTERNAL_LINKAGE thing doesn't seem to be consistent anyway, let's just finish this and try to sort that mess out in another ticket.
Attachment #8608884 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/47751071d2c8
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
See Also: → 1329449
You need to log in before you can comment on or make changes to this bug.