Allow MozPromise to take an nsIEventTarget as well as an AbstractThread

RESOLVED FIXED in Firefox 56

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: billm, Assigned: billm)

Tracking

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(7 attachments, 3 obsolete attachments)

5.45 KB, patch
jwwang
: review+
Details | Diff | Splinter Review
5.65 KB, patch
jwwang
: review+
Details | Diff | Splinter Review
22.46 KB, patch
jwwang
: review+
Details | Diff | Splinter Review
8.15 KB, patch
kanru
: review+
Details | Diff | Splinter Review
6.10 KB, patch
kanru
: review+
Details | Diff | Splinter Review
7.96 KB, patch
kanru
: review+
Details | Diff | Splinter Review
30.52 KB, patch
jwwang
: review+
Details | Diff | Splinter Review
Posted patch patch (obsolete) — Splinter Review
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.
Attachment #8869233 - Flags: review?(jwwang)
I can't remember the reason why requiring AbstractThread. Maybe Bobby can recall the old memory.
Flags: needinfo?(bobbyholley)
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.
(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?
No. It's the sub-classes of ThenValueBase that need to be templated.
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?
Flags: needinfo?(bobbyholley) → needinfo?(nfroyd)
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.
OK. In that case I'm ok with the template specialization stuff I guess.
(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?
Flags: needinfo?(nfroyd)
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?
That's a good point.  nsISerialEventTarget seems good.  Bill?
Flags: needinfo?(wmccloskey)
Yes, I like nsISerialEventTarget.
Flags: needinfo?(wmccloskey)
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?
Flags: needinfo?(jwwang)
// 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.
Flags: needinfo?(jwwang)
Posted patch patch, v2 (obsolete) — Splinter Review
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.
Attachment #8869233 - Attachment is obsolete: true
Attachment #8869233 - Flags: review?(jwwang)
Attachment #8875925 - Flags: feedback?(jwwang)
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.
Attachment #8875925 - Flags: feedback?(jwwang) → feedback+
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?
Flags: needinfo?(jwwang)
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.
Flags: needinfo?(jwwang) → needinfo?(kanru)
(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?
Flags: needinfo?(kanru)
I worked on this today and it's much nicer than the template version.
This patch just removes a field that seems unnecessary. It's not needed for this bug, but it makes things a little simpler.
Attachment #8875925 - Attachment is obsolete: true
Attachment #8876310 - Flags: review?(jwwang)
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.
Attachment #8876313 - Flags: review?(jwwang)
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.
Attachment #8876315 - Flags: review?(jwwang)
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.
Attachment #8876318 - Flags: review?(kchen)
This patch makes it easy to get an nsISerialEventTarget for any MessageLoop--even the ones where there isn't a corresponding nsIThread.
Attachment #8876320 - Flags: review?(kchen)
Posted patch Tests (obsolete) — Splinter Review
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?
Attachment #8876322 - Flags: review?(kchen)
Attachment #8876322 - Flags: review?(jwwang)
I don't think we need this anymore.
Attachment #8876323 - Flags: review?(kchen)
(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?
(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.
(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 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.
Attachment #8876313 - Flags: review?(jwwang) → review+
Attachment #8876310 - Flags: review?(jwwang) → review+
Attachment #8876315 - Flags: review?(jwwang) → review+
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.
Attachment #8876322 - Flags: review?(jwwang) → review-
Attachment #8876318 - Flags: review?(kchen) → review+
Attachment #8876320 - Flags: review?(kchen) → review+
Attachment #8876322 - Flags: review?(kchen) → review+
Attachment #8876323 - Flags: review?(kchen) → review+
Posted patch tests v2Splinter Review
I think spinning the event loop should work here.
Attachment #8876322 - Attachment is obsolete: true
Attachment #8876904 - Flags: review?(jwwang)
(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?
Attachment #8876904 - Flags: review?(jwwang) → review+
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)
(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

2 years ago
Pushed by wmccloskey@mozilla.com:
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)
Depends on: 1373603
You need to log in before you can comment on or make changes to this bug.