1365483 - [meta] Remove the call to AbstractThread::GetCurrent() and specifiy an AbstractThread explicitly

Status: NEW

(Core :: XPCOM, enhancement, P3)

Description

[JW Wang [:jwwang] [:jw_wang]]

After fixing bug 1364821, AbstractThread::GetCurrent() will return non-null only when the caller is running in the context of an AbstractThread.

Examples are the constructor of Benchmark [1] which is called by [2] and not running in any AbstractThread context.

We should replace the calls with an AbstractThread instance specified explicitly.

[1] http://searchfox.org/mozilla-central/rev/9a7fbdee1d54f99cd548af95b81231d80e5f9ad1/dom/media/Benchmark.cpp#93
[2] http://searchfox.org/mozilla-central/rev/9a7fbdee1d54f99cd548af95b81231d80e5f9ad1/dom/media/gtest/TestMediaDataDecoder.cpp#58

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

So, I have work-in-progress code in bug 1293277 that uses AbstractThread::GetCurrent() a lot.  I need help figuring out how to remove it.

My question is, what is the correct way to handle this code now?  Do I have to do:

  AbstractThread* current = nullptr;
  if (NS_IsMainThread()) {
    current = AbstractThread::MainThread();
  } else {
    current = /* some other call? */
  }

Or do you intend a different way for this sort of thing?

If this is the code I need to write, I guess I don't understand why AbstractThread::GetCurrent() can't do this itself instead of duplicating many places.

Comment 2

[JW Wang [:jwwang] [:jw_wang]]

So does that mean the piece of the code could run both on the main thread and on some other thread? Can you give a more concrete example of that?

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

(In reply to JW Wang [:jwwang] [:jw_wang] from comment #2)
> So does that mean the piece of the code could run both on the main thread
> and on some other thread? Can you give a more concrete example of that?

Its an asynchronous DOM API.  We have many DOM APIs that are exposed on both window and worker.  So I get some call from the bindings layer without any indication what thread I am on.  I have to use TLS via GetCurrentThread or NS_IsMainThread() to know which.

My current thought is that if the thread TLS functions go away I'll have to write helpers to somehow extract the thread for the nsIGlobalObject.  Currently, though, most code does the opposite by doing:

  if (NS_IsMainThread()) {
    // I'm on a window
  } else  {
    // this asserts if called on main thread...
    WorkerPrivate* wp = GetCurrentThreadWorkerPrivate();
  }

I'm not sure we have enough information in the globals currently to reach back to a "current thread" object.

If GetCurrentThread and friends go away, though, I think we will need to add:

  nsIThread*
  NS_GetThreadFromGlobal(nsIGlobalObject* aGlobal);

  AbstractThread*
  AbstractThread::GetForGlobal(nsIGlobalObject* aGlobal);

I think a lot of code is going to need this if we can't use TLS to get the current thread.

Of course, these helpers don't help with things that work with threads without a global.  For example, its going to add complexity to the code (i.e. maintenance cost) to replace stuff like this:

https://dxr.mozilla.org/mozilla-central/source/dom/cache/Manager.cpp#653

Comment 4

[JW Wang [:jwwang] [:jw_wang]]

We want to fix the semantics (ambiguity, see bug 1364821) of AbstractThread::GetCurrent() which should return the instance the caller is running in. For a task dispatched via NS_DispatchToMainThread(), AbstractThread::GetCurrent() should return null since it is not running in any AbstractThread. Then it won't be ambiguous when there are multiple AbstractThreads wrapping around the main thread.

So AbstractThread::GetCurrent() won't help you after fixing bug 1364821 since your code doesn't run in any AbstractThread. If the only need for AbstractThread::GetCurrent() is for working with MozPromise in your case, we can change MozPromise to accept something like ThreadEventTarget (see Bug 1361164) so GetCurrentThreadEventTarget() is available to you.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

(In reply to JW Wang [:jwwang] [:jw_wang] from comment #4)
> We want to fix the semantics (ambiguity, see bug 1364821) of
> AbstractThread::GetCurrent() which should return the instance the caller is
> running in. For a task dispatched via NS_DispatchToMainThread(),
> AbstractThread::GetCurrent() should return null since it is not running in
> any AbstractThread. Then it won't be ambiguous when there are multiple
> AbstractThreads wrapping around the main thread.

Doesn't this make it extremely difficult to integrate code using MozPromise with code not using MozPromise?  One uses AbstractThread and the other uses nsIThread.  How is this supposed to work?

Comment 6

[JW Wang [:jwwang] [:jw_wang]]

It is already the case we have several 'thread types' like MessageLoop, nsIThread, and AbstractThread which is especially true for users of IPDL actors.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

(In reply to JW Wang [:jwwang] [:jw_wang] from comment #6)
> It is already the case we have several 'thread types' like MessageLoop,
> nsIThread, and AbstractThread which is especially true for users of IPDL
> actors.

We only have MessageLoop because we imported chromium IPC, right?  And we are pretty vigilant in not letting much code reference it.

I don't think AbstractThread needed to be created.  That code could have been built on nsIEventTarget, but a design decision was made to fork a new thread type.

By the way, I was making AbstractThread::GetCurrent() work on threads like Workers by explicitly having those threads setup an AbstractThread:

https://dxr.mozilla.org/mozilla-central/source/dom/workers/RuntimeService.cpp#2866

I don't see why we can't do this on other threads that want to use AbstractThread::GetCurrent().

Comment 8

[JW Wang [:jwwang] [:jw_wang]]

(In reply to Ben Kelly [reviewing, but slowly][:bkelly] from comment #7)
> We only have MessageLoop because we imported chromium IPC, right?  And we
> are pretty vigilant in not letting much code reference it.
IIUC, code under gfx uses both MessageLoop and nsIThread.


> I don't think AbstractThread needed to be created.  That code could have
> been built on nsIEventTarget, but a design decision was made to fork a new
> thread type.
There is a good reason not to use nsIEventTarget. As you said, nsIEventTarget is a 'queue' instead of a 'thread' and it doesn't guarantee tasks are executed in the same order they are dispatched (e.g. nsIThreadPool) which is however an important requirement of our media code.

How about nsIThread? Our media code used to use it as the decoding threads and found out the performance went down significantly when there were many (say 100) videos playing at the same time. So cpearce devised a class to distribute tasks among a pool with limited number of threads to reduce the overhead of context switch and also guarantee the order of task execution required by our media code. The class was called MediaTaskQueue and later renamed to TaskQueue and moved to xpcom.

Then we had bholley to overhaul the media code and improved the async programming model which at that time was lock-based and deadlock prone. bholley devised MozPromse and state-mirroring to replace the lock-based synchronization in the media code. To use MozPromise and state-mirroring on both the main thread and decoding threads, we need a new thread type which guarantees the order of execution (an media code requirement) and is more generic than nsIThread (which is tied to a physical thread). This new thread type also needs to support tail dispatching for state-mirroring to work. That's how AbstractThread came to be from which we derive TaskQueue and XPCOMThreadWrapper to wrap around the main thread and enable use of MozPromise and state-mirroring on the main thread.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

(In reply to JW Wang [:jwwang] [:jw_wang] from comment #8)
> There is a good reason not to use nsIEventTarget. As you said,
> nsIEventTarget is a 'queue' instead of a 'thread' and it doesn't guarantee
> tasks are executed in the same order they are dispatched (e.g.
> nsIThreadPool) which is however an important requirement of our media code.

This could have been achieved with a subclass or mixin nsIOrderedEventTarget.  If you get an nsIEventTarget you could then QI to see if it provides the guarantee you need.  If it doesn't, then you wrap it in the TaskQueue.

No need to invent a parallel threading API to the existing API for this feature, IMO.

But anyway, AbstractThread exists.  My question right now is how can I write my code using MozPromise if you proceed with the changes you are talking about here.  It appears to make it a lot harder to me and I still don't really understand why its even necessary.

Comment 10

[JW Wang [:jwwang] [:jw_wang]]

(In reply to Ben Kelly [reviewing, but slowly][:bkelly] from comment #7)
> By the way, I was making AbstractThread::GetCurrent() work on threads like
> Workers by explicitly having those threads setup an AbstractThread:
> 
> https://dxr.mozilla.org/mozilla-central/source/dom/workers/RuntimeService.
> cpp#2866

This is the exact ambiguity bug 1364821 wants to fix. Consider the following code:

  SomeClass1()
    // We don't need tail dispatching for this class.
    : mAbstractThread(AbstractThread::CreateXPCOMThreadWrapper(NS_GetCurrentThread(), false))
    { }

  SomeClass2()
    // We need tail dispatching for this class.
    : mAbstractThread(AbstractThread::CreateXPCOMThreadWrapper(NS_GetCurrentThread(), true))
    { }

  void SomeClass1::SomeFunction()
  {
    // Is it gonna return SomeClass1::mAbstractThread or SomeClass2::mAbstractThread?
    // Is tail dispatching enabled?
    // Is it safe to call SyncRunnable::DispatchToThread(AbstractThread*, ...)?
    AbstractThread::GetCurrent();
  }

  void SomeClass2::SomeFunction()
  {
    // Is tail dispatching enabled?
    // Can we use state-mirroring?
    AbstractThread::GetCurrent();
  }

Note the ambiguity comes from the special setup of sCurrentThreadTLS at http://searchfox.org/mozilla-central/rev/f55349994fdac101d121b11dac769f3f17fbec4b/xpcom/threads/AbstractThread.cpp#300 which is what bug 1364821 will remove.

Comment 11

[JW Wang [:jwwang] [:jw_wang]]

(In reply to Ben Kelly [reviewing, but slowly][:bkelly] from comment #9)
> But anyway, AbstractThread exists.  My question right now is how can I write
> my code using MozPromise if you proceed with the changes you are talking
> about here.  It appears to make it a lot harder to me and I still don't
> really understand why its even necessary.

As https://dxr.mozilla.org/mozilla-central/source/dom/workers/RuntimeService.cpp#2866 shows, store an AbstractThread as a member in the constructor and all other member functions should use mAbstractThread instead of GetCurrent(). Then your class will be able to work on both the main thread and work threads depending on where (NS_GetCurrentThread()) it is initialized.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

(In reply to JW Wang [:jwwang] [:jw_wang] from comment #11)
> As
> https://dxr.mozilla.org/mozilla-central/source/dom/workers/RuntimeService.
> cpp#2866 shows, store an AbstractThread as a member in the constructor and
> all other member functions should use mAbstractThread instead of
> GetCurrent(). Then your class will be able to work on both the main thread
> and work threads depending on where (NS_GetCurrentThread()) it is
> initialized.

Where does my constructor get that from?  I'm getting called from content script js.  I don't have an AbstractThread handy there.  And my code can be called from main thread window code or web worker thread code.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

(In reply to JW Wang [:jwwang] [:jw_wang] from comment #10)
> This is the exact ambiguity bug 1364821 wants to fix. Consider the following
> code:
> 
>   SomeClass1()
>     // We don't need tail dispatching for this class.
>     :
> mAbstractThread(AbstractThread::
> CreateXPCOMThreadWrapper(NS_GetCurrentThread(), false))
>     { }
> 
>   SomeClass2()
>     // We need tail dispatching for this class.
>     :
> mAbstractThread(AbstractThread::
> CreateXPCOMThreadWrapper(NS_GetCurrentThread(), true))
>     { }
> 
>   void SomeClass1::SomeFunction()
>   {
>     // Is it gonna return SomeClass1::mAbstractThread or
> SomeClass2::mAbstractThread?
>     // Is tail dispatching enabled?
>     // Is it safe to call SyncRunnable::DispatchToThread(AbstractThread*,
> ...)?
>     AbstractThread::GetCurrent();
>   }
> 
>   void SomeClass2::SomeFunction()
>   {
>     // Is tail dispatching enabled?
>     // Can we use state-mirroring?
>     AbstractThread::GetCurrent();
>   }

IMO the bug here is with the AbstractThread base class having variant behavior based on construction arguments.  Whichever version is the more primitive behavior should be the base class and the other a specialized type with a different TLS getter.  Or a different decorator pattern used to add the enhanced behavior.  Admittedly I don't know all your use cases, but it seems wrong to have a variant constructor on a type meant to represent a TLS singleton like a thread.

Comment 14

[JW Wang [:jwwang] [:jw_wang]]

How about adding a new helper function:

RefPtr<AbstractThread>
AbstractThread::GetCurrentnsIThreadWrapper()
{
  return AbstractThread::CreateXPCOMThreadWrapper(NS_GetCurrentThread(), false);
}

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Hmm, I guess I had the impression NS_GetCurrentThread() was on the chopping block as well.  Its hard to keep up with all the threading changes recently.

I can probably do this in my constructor yes.  This should be ok since the objects will be relatively long lived.  It would be nice not to require allocation overhead in all cases, though.

Comment 16

[JW Wang [:jwwang] [:jw_wang]]

How about:

AbstractThread*
AbstractThread::GetCurrentnsIThreadWrapper()
{
  if (someTLS.get() == nullptr) {
    someTLS.set(AbstractThread::CreateXPCOMThreadWrapper(NS_GetCurrentThread(), false));
  }
  return someTLS.get();
}

to reduce allocation overhead by using cache and only pay the price when needed by lazy initialization.

Comment 17

[JW Wang [:jwwang] [:jw_wang]]

(In reply to Ben Kelly [reviewing, but slowly][:bkelly] from comment #13)
> IMO the bug here is with the AbstractThread base class having variant
> behavior based on construction arguments.  Whichever version is the more
> primitive behavior should be the base class and the other a specialized type
> with a different TLS getter.  Or a different decorator pattern used to add
> the enhanced behavior.  Admittedly I don't know all your use cases, but it
> seems wrong to have a variant constructor on a type meant to represent a TLS
> singleton like a thread.

I thought about that too. By decoupling tail dispatcher from AbstractThread, all AbstractThreads are effectively the same if they wrap the same nsIThread and thus removing the ambiguity. However, this requires much more effort than bug 1364821 and I haven't figured out the way to do it.

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Another way to solve my problem would be to enhance MozPromise to accept nsIEventTarget.