1366072 - Allow MozPromise to take an nsIEventTarget as well as an AbstractThread

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch

I think it's unfortunate how a lot of code ends up using AbstractThread just because it wants to use MozPromise. Reducing the usage of AbstractThread will make problems like bug 1365483 easier to solve.

The easiest way to do this would be to change MozPromise to use only nsIEventTargets and then to make AbstractThread implement the nsIEventTarget interface. However, I'm guessing that there would be objections to that (not sure). Otherwise I'd expect we would have done this a while ago. However, if you'd prefer that I take this approach, I'd be happy to do that.

Anyway, this patch allows MozPromise to take either an AbstractThread or an nsIEventTarget.

Comment 1

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

I can't remember the reason why requiring AbstractThread. Maybe Bobby can recall the old memory.

Comment 2

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

Comment on attachment 8869233 [details] [diff] [review]
patch

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

::: xpcom/threads/MozPromise.h
@@ +93,5 @@
> +  }
> +
> +  void Dispatch(already_AddRefed<nsIRunnable> aRunnable)
> +  {
> +    if (mAbstractThread) {

I would expect to use template specialization to save the value switch in the runtime. Also it would be easier to be extended to accept MessageLoop in the future.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to JW Wang [:jwwang] [:jw_wang] from comment #2)
> I would expect to use template specialization to save the value switch in
> the runtime. Also it would be easier to be extended to accept MessageLoop in
> the future.

I think you would have to make MozPromise be templated as well for that to work. Is that what you want?

Comment 4

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

No. It's the sub-classes of ThenValueBase that need to be templated.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I talked to Bobby on IRC about this. He doesn't want to allow nsIEventTarget to be passed to MozPromise because it makes assumptions that would be broken by a thread pool (not sure what those are, exactly).

So we need to create a new nsIEventTarget-like interface that's only implemented by non-thread pools. Bobby would like to call this nsIAbstractThread, since it's sort of like a thread (single-threaded) but not necessarily 1:1 with an OS thread. I would prefer a name like nsISingleThreadedEventTarget. That's more in keeping with Gecko's naming. Also, calling something a thread implies to me that it actually processes events. This interface is just for dispatching. Nathan, can you make a call here?

Comment 6

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

http://searchfox.org/mozilla-central/source/ipc/glue/MessageLoopAbstractThreadWrapper.h

We have use of MozPromise in IPC where it has to create a wrapper around a MessageLoop in order to play with MozPromise which is not ideal to me for it introduce dependency on XPCOM to IPC. I would prefer to use template specialization to allow more 'thread types' to cope with MozPromise without introducing a direct dependency on some specific thread type to MozPromise.

Comment 7

[Bobby Holley (:bholley)]

OK. In that case I'm ok with the template specialization stuff I guess.

Comment 8

[Nathan Froyd [:froydnj]]

(In reply to Bill McCloskey (:billm) from comment #5)
> I talked to Bobby on IRC about this. He doesn't want to allow nsIEventTarget
> to be passed to MozPromise because it makes assumptions that would be broken
> by a thread pool (not sure what those are, exactly).

I suppose, by the same token, that we'd not want to add a method to nsIEventTarget like isSingleThreaded/isFixedThread and assert that in MozPromise, because we'd lose out on type guarantees?

> Nathan, can you make a call here?

I prefer nsISingleThreadedEventTarget, presumably inheriting from nsIEventTarget, and it's not going to have any additional methods, right?  Are we going to rewrite NS_GetCurrentThread and friends to return nsISingleThreadedEventTarget, so that people using MozPromise don't have to go through some awkward do_QueryInterface dance?

Comment 9

[Bobby Holley (:bholley)]

I would assume that nsISingleThreadedEventTarget would inherit from nsIEventTarget, and nsIThread would inherit from that.

FWIW, I still think nsISingleThreadedEventTarget is a suboptimal name, because TaskQueues would implement this and TaskQueues are not single-threaded per se, they just ensure that two events don't run at the same time.

How about at least nsISerialEventTarget or nsISequentialEventTarget?

Comment 10

[Nathan Froyd [:froydnj]]

That's a good point.  nsISerialEventTarget seems good.  Bill?

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Yes, I like nsISerialEventTarget.

Comment 12

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I posted a patch for nsISerialEventTarget in bug 1361164. If that looks okay then I'll use it in this bug.

I haven't written the patch yet, but I assume we'll have something like this:

template<class T>
class MozPromiseTarget {};

template<>
class MozPromiseTarget<nsISerialEventTarget*>
{
  nsISerialEventTarget* mTarget;
  // Define a Dispatch and IsOnCurrentThread method
};

template<>
class MozPromiseTarget<MessageLoop*>
{
  MessageLoop* mLoop;
  // Define a Dispatch and IsOnCurrentThread method
};

Then ThenValueBase would take a MozPromiseTarget<T>. Is that roughly what you had in mind, JW?

Comment 13

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

// Add a comment to enforce all specialization must guarantee events are run sequentially.
template<class T>
class MozPromiseTarget {};

template<>
class MozPromiseTarget<nsISerialEventTarget>
{
  // Store a nsCOMPtr<> or RefPtr<> so we don't have to worry about dangling pointers.
  nsCOMPtr<nsISerialEventTarget> mTarget;
};

template<>
class MozPromiseTarget<MessageLoop>
{
  // Alas, we have to store a raw pointer.
  MessageLoop* mLoop;
};

MozPromiseTarget<A> and MozPromiseTarget<B> are different types and can't be stored in ThenValueBase::mResponseTarget. You can either create a base class from which MozPromiseTarget<A> and MozPromiseTarget<B> will inherit or add a template parameter to FunctionThenValue/MethodThenValue and move mResponseTarget down the class hierarchy. The former is simpler but the later is more efficient which involves fewer virtual function calls.

Comment 14

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch, v2

I spent some time on this today and this patch is what I came up with.

1. I don't really see how we can avoid virtual dispatch overhead. A MozPromise keeps an array of ThenValueBases (mThenValues) and it needs to call Dispatch on them. Assuming we don't want to template MozPromise, there will have to be some sort of runtime decision about which kind of target we're using. I don't think the overhead of virtual dispatch is a problem (dispatching to threads already uses a lot of virtual dispatch), but you seem to be worried about it.

2. I did make a base class for MozPromiseTarget<T> as you suggested. However, I didn't want to have to dynamically allocate it since that could be expensive. And we can't store it in ThenValueBase since the size is not known. So I stored it in the different ThenValue classes. Unfortunately, that leads to some duplication.

3. In the Then() methods, we can't make the target parameter be a templated type. If I do that, then template type deduction won't work in cases like this:
  RefPtr<AbstractThread> thread = ...;
  ...->Then(thread, ...);
Even through |thread| has a conversion operator to AbstractThread*, type deduction won't figure that out. Consequently, I ended up using the horrible macro that you see in the patch.

4. Some users of MozPromise aren't able to #include "base/message_loop.h" (it's not exported in the normal way). I think we can fix that by forward declaring MessageLoop in MozPromise.h and then implementing MozPromiseTarget<MessageLoop>::IsCurrentThreadIn and MozPromiseTarget<MessageLoop>::Dispatch in a .cpp file that can use "base/message_loop.h".

This needs some tests and comments, but I wanted to get your feedback before I move any further.

Comment 15

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

Comment on attachment 8875925 [details] [diff] [review]
patch, v2

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

::: xpcom/threads/MozPromise.h
@@ +86,5 @@
> +};
> +
> +template<typename T>
> +class MozPromiseTarget
> +{};

No need to define the class body here since it will not compile anyway if T doesn't deduce to any of the specialization below.

@@ +105,5 @@
> +  {
> +    return mEventTarget->IsOnCurrentThread();
> +  }
> +
> +  void Dispatch(already_AddRefed<Runnable> aRunnable) override

Any reason to take Runnable instead of nsIRunnable?

@@ +135,5 @@
> +  }
> +};
> +
> +template<>
> +class MozPromiseTarget<MessageLoop> : public MozPromiseTargetBase

MessageLoop is not part of XPCOM where MozPromise resides. I would prefer move this to a header under IPC.

@@ +583,5 @@
>        // Invoke the resolve or reject method.
>        DoResolveOrRejectInternal(aValue);
>      }
>  
> +    detail::MozPromiseTargetBase* mResponseTarget;

Document this raw pointer is guaranteed to be valid by the sub-class.

@@ +1086,5 @@
>    }
>  
> +THEN_METHODS(AbstractThread)
> +THEN_METHODS(nsISerialEventTarget)
> +THEN_METHODS(MessageLoop)

I don't want to introduce the dependency on MessageLoop here.

We can either require the caller to pass a raw pointer like:
RefPtr<AbstractThread> thread;
promise->Then(thread.get(), ...);

or specialize Then() like:
Then(TargetType*, ...)
Then(RefPtr<TargetType>&, ...)
Then(nsCOMPtr<argetType>&, ...)
to allow the caller to pass a smart pointer.

I would prefer the former we have less code to write and it is just a bit overhead (.get()) to the caller.

Comment 16

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I thought about this some more over dinner, and now I'm thinking it would probably be cleaner to avoid templates. We could:
- make MozPromise take an nsISerialEventTarget
- make AbstractThread inherit from nsISerialEventTarget
- provide an adapter so that messageLoop->EventTarget() would return an nsISerialEventTarget for a MessageLoop.

What do you think about doing this instead?

Comment 17

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

I am fine with that. We should have IPC guys to comment on |messageLoop->EventTarget()|. It is just a bit confusing to me the inter-dependency between nsIThread and MessageLoop.

Comment 18

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to JW Wang [:jwwang] [:jw_wang] from comment #17)
> I am fine with that. We should have IPC guys to comment on
> |messageLoop->EventTarget()|. It is just a bit confusing to me the
> inter-dependency between nsIThread and MessageLoop.

(Bill is the ultimate IPC guy when it comes to IPC questions ;-)

The dependency between nsIThread and MessageLoop is already a mess. My only concern would be for the MessageLoops not backed by a nsIThread.

I'm not sure how popular is the nsIMessageLoop interface. Maybe we can use that instead?

Comment 19

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I worked on this today and it's much nicer than the template version.

Comment 20

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: Remove useless ThenCommand::mResponseThread field

This patch just removes a field that seems unnecessary. It's not needed for this bug, but it makes things a little simpler.

Comment 21

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: Make AbstractThread inherit from nsISerialEventTarget

This patch adds a little refcounting overhead. We might be able to avoid that with a lot of ugliness (defining AddRef and Release as inline virtual final methods). I don't know how important that is.

Comment 22

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: Change promises to take nsISerialEventTarget instead of AbstractThread

This is the main patch. I had to add a bunch of #includes to random files since MozPromise.h no longer pulls in AbstractThread.h.

Comment 23

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: Use nsIRunnable instead of Runnable in MessageLoop

I don't know why MessageLoop::PostTask takes a mozilla::Runnable instead of nsIRunnable. Maybe it was some effort to de-COM-taminate this code. The problem is that I would like to pass an nsIRunnable to PostTask in the next patch. So this patch makes PostTask take an nsIRunnable.

Comment 24

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: Expose an event target for MessageLoop

This patch makes it easy to get an nsISerialEventTarget for any MessageLoop--even the ones where there isn't a corresponding nsIThread.

Comment 25

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: Tests

I moved the MozPromise test to xpcom from dom/media and I added tests for XPCOM event targets and MessageLoop. JW, please review this part.

I also changed the IPC promise test to use a MessageLoop event target. Kan-Ru, can you review this part?

Comment 26

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: Remove MessageLoopAbstractThreadWrapper

I don't think we need this anymore.

Comment 27

[Bobby Holley (:bholley)]

(In reply to Bill McCloskey (:billm) from comment #16)
> I thought about this some more over dinner, and now I'm thinking it would
> probably be cleaner to avoid templates. We could:
> - make MozPromise take an nsISerialEventTarget
> - make AbstractThread inherit from nsISerialEventTarget
> - provide an adapter so that messageLoop->EventTarget() would return an
> nsISerialEventTarget for a MessageLoop.
> 
> What do you think about doing this instead?

Just so that I understand - is this more or less the solution I was pushing for before, or is there a difference?

Comment 28

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #27)
> (In reply to Bill McCloskey (:billm) from comment #16)
> > I thought about this some more over dinner, and now I'm thinking it would
> > probably be cleaner to avoid templates. We could:
> > - make MozPromise take an nsISerialEventTarget
> > - make AbstractThread inherit from nsISerialEventTarget
> > - provide an adapter so that messageLoop->EventTarget() would return an
> > nsISerialEventTarget for a MessageLoop.
> > 
> > What do you think about doing this instead?
> 
> Just so that I understand - is this more or less the solution I was pushing
> for before, or is there a difference?

It's what we talked about. JW wanted to use templates to handle MessageLoop, but that turned out to be pretty ugly. This new way requires a bit more work when using a MessageLoop (you have to pass in MessageLoop::current()->SerialEventTarget() rather than MessageLoop::current()) but I think that's a small price to pay for cleaner code otherwise.

Comment 29

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

(In reply to Bill McCloskey (:billm) from comment #28)
> It's what we talked about. JW wanted to use templates to handle MessageLoop,
> but that turned out to be pretty ugly. This new way requires a bit more work
> when using a MessageLoop (you have to pass in
> MessageLoop::current()->SerialEventTarget() rather than
> MessageLoop::current()) but I think that's a small price to pay for cleaner
> code otherwise.

The original idea is not to introduce dependency on XPCOM to MessageLoop. Therefore I prefer template specialization to allow more thread types for MozPromise user instead of having to create a sub-class of AbstractThread. This also helps fix bug 1364821 by using less AbstractThread.

However, seeing MessageLoop already depends on nsIThead, I am fine with the nsISerialEventTarget approach which also reduces the use of ambiguous AbstractThread::GetCurrent().

Comment 30

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

Comment on attachment 8876313 [details] [diff] [review]
Make AbstractThread inherit from nsISerialEventTarget

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

::: xpcom/threads/AbstractThread.cpp
@@ +217,5 @@
>  
> +NS_IMPL_ISUPPORTS(AbstractThread, nsIEventTarget, nsISerialEventTarget)
> +
> +NS_IMETHODIMP_(bool)
> +AbstractThread::IsOnCurrentThreadInfallible()

IsOnCurrentThreadInfallible is a verbose name. Can we use overloading for both IsOnCurrentThread() and IsOnCurrentThread(bool*)?

@@ +232,5 @@
> +
> +NS_IMETHODIMP
> +AbstractThread::DispatchFromScript(nsIRunnable* aEvent, uint32_t aFlags)
> +{
> +  nsCOMPtr<nsIRunnable> event(aEvent);

I am surprised we don't have do_AddRef() for an XPCOM pointer.

@@ +239,5 @@
> +
> +NS_IMETHODIMP
> +AbstractThread::Dispatch(already_AddRefed<nsIRunnable> aEvent, uint32_t aFlags)
> +{
> +  Dispatch(Move(aEvent), DontAssertDispatchSuccess, NormalDispatch);

Ideally we should pass AssertDispatchSuccess since |return NS_OK| below assumes Dispatch() will succeed. However, MozPromise assumes Dispatch() could fail and passes DontAssertDispatchSuccess instead. I don't have a good idea how to reconcile such a conflict in the semantics.

Comment 31

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

Comment on attachment 8876322 [details] [diff] [review]
Tests

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

::: xpcom/tests/gtest/TestMozPromise.cpp
@@ +408,5 @@
> +      queue->BeginShutdown();
> +    });
> +}
> +
> +TEST(MozPromise, XPCOMEventTarget)

The resolve callback will run after |TEST(MozPromise, XPCOMEventTarget)| returns which I don't think is OK for gtest.

Comment 32

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: tests v2

I think spinning the event loop should work here.

Comment 33

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to JW Wang [:jwwang] [:jw_wang] from comment #30)
> Comment on attachment 8876313 [details] [diff] [review]
> Make AbstractThread inherit from nsISerialEventTarget
> 
> Review of attachment 8876313 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: xpcom/threads/AbstractThread.cpp
> @@ +217,5 @@
> >  
> > +NS_IMPL_ISUPPORTS(AbstractThread, nsIEventTarget, nsISerialEventTarget)
> > +
> > +NS_IMETHODIMP_(bool)
> > +AbstractThread::IsOnCurrentThreadInfallible()
> 
> IsOnCurrentThreadInfallible is a verbose name. Can we use overloading for
> both IsOnCurrentThread() and IsOnCurrentThread(bool*)?

I'm not sure what you mean. Can you please explain more?

Comment 34

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

Comment on attachment 8876313 [details] [diff] [review]
Make AbstractThread inherit from nsISerialEventTarget

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

::: xpcom/threads/AbstractThread.cpp
@@ +217,5 @@
>  
> +NS_IMPL_ISUPPORTS(AbstractThread, nsIEventTarget, nsISerialEventTarget)
> +
> +NS_IMETHODIMP_(bool)
> +AbstractThread::IsOnCurrentThreadInfallible()

Instead of having IsOnCurrentThreadInfallible() and IsOnCurrentThread(bool*), can we have IsOnCurrentThread() and IsOnCurrentThread(bool*)? (2 overloads vs 2 names)

Comment 35

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to JW Wang [:jwwang] [:jw_wang] from comment #34)
> Comment on attachment 8876313 [details] [diff] [review]
> Make AbstractThread inherit from nsISerialEventTarget
> 
> Review of attachment 8876313 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: xpcom/threads/AbstractThread.cpp
> @@ +217,5 @@
> >  
> > +NS_IMPL_ISUPPORTS(AbstractThread, nsIEventTarget, nsISerialEventTarget)
> > +
> > +NS_IMETHODIMP_(bool)
> > +AbstractThread::IsOnCurrentThreadInfallible()
> 
> Instead of having IsOnCurrentThreadInfallible() and
> IsOnCurrentThread(bool*), can we have IsOnCurrentThread() and
> IsOnCurrentThread(bool*)? (2 overloads vs 2 names)

Sort of. IsOnCurrentThread() is a non-virtual fast path for the nsIThread case. If the event target isn't an nsIThread, then it calls IsOnCurrentThreadInfallible. So callers can use IsOnCurrentThread(), but implementors need to declare IsOnCurrentThreadInfallible().

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c012fb80a72b
Remove useless ThenCommand::mResponseThread field (r=jwwang)
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b7ca9164832
Make AbstractThread inherit from nsISerialEventTarget (r=jwwang)
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9fd6a30931c
Change promises to take nsISerialEventTarget instead of AbstractThread (r=jwwang)
https://hg.mozilla.org/integration/mozilla-inbound/rev/a78d74080490
Use nsIRunnable instead of Runnable in MessageLoop (r=kanru)
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce8656869342
Expose an event target for MessageLoop (r=kanru)
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d1e7a8da310
MozPromise tests (r=jwwang)
https://hg.mozilla.org/integration/mozilla-inbound/rev/651cebf4986c
Remove MessageLoopAbstractThreadWrapper (r=kanru)

Comment 37

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c012fb80a72b
https://hg.mozilla.org/mozilla-central/rev/8b7ca9164832
https://hg.mozilla.org/mozilla-central/rev/c9fd6a30931c
https://hg.mozilla.org/mozilla-central/rev/a78d74080490
https://hg.mozilla.org/mozilla-central/rev/ce8656869342
https://hg.mozilla.org/mozilla-central/rev/1d1e7a8da310
https://hg.mozilla.org/mozilla-central/rev/651cebf4986c