1162026 - convert WrapRunnable* and supporting machinery to use variadic templates

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Nathan Froyd [:froydnj]]

This will make a number of things easier, get rid of some grotty generated code, and it will also enable reuse of some of the NS_RunnableMethod* machinery.

Comment 1

[Eric Rescorla (:ekr)]

MT and I discussed this and our current thinking is that instead we should convert uses of WrapRunnable to closures.

Comment 2

[Eric Rescorla (:ekr)]

To fill this in a little bit, we could provide a dispatch variant which took a nullary function object and then just pass in a closure, dispensing with WrapRunnable (and the other things in our code base that wrap functions of various kinds in runnables).

Comment 3

[Nathan Froyd [:froydnj]]

By "dispatch variant" in comment 2, I'm assuming you mean a Dispatch() overload on nsIEventTarget?

One of the nice things about WrapRunnable* is that it can cooperate with things like RUN_ON_THREAD to automatically get the flags to Dispatch() correct, e.g. the variants that return values.  Did you have a plan for handling that in the closure-ified world?

Comment 4

[Eric Rescorla (:ekr)]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #3)
> By "dispatch variant" in comment 2, I'm assuming you mean a Dispatch()
> overload on nsIEventTarget?

Yes.


> One of the nice things about WrapRunnable* is that it can cooperate with
> things like RUN_ON_THREAD to automatically get the flags to Dispatch()
> correct, e.g. the variants that return values.  Did you have a plan for
> handling that in the closure-ified world?

Was thinking we could write a clang extension to see if you had bound
pointers to objects in local scope.

Comment 5

[Nathan Froyd [:froydnj]]

My real motivation is that there are some calls that look like:

  RefPtr<T> p = ...;
  RUN_ON_THREAD(..., WrapRunnable(..., p.forget());

and currently, RefPtr::forget returns a TemporaryRef, which is happy to act like a RefPtr in the context of being a type of a class member.

Bug 1161627, however, removes TemporaryRef, and makes RefPtr::forget return an already_AddRefed, which does not work in the same way for this: we should have a class member of type nsRefPtr or similar instead of already_AddRefed.

Comment 6

[Nathan Froyd [:froydnj]]

Since converting everything to lambdas + clang analysis extension seems a bit overmuch for just wanting to get rid of a lot of duplicated code and classes, can we just do the latter here and then do the lambda conversion in some other bug?

Comment 7

[Eric Rescorla (:ekr)]

If you want to submit a patch, I'll review it.

Comment 8

[Nathan Froyd [:froydnj]]

Attachment: part 1 - move result argument for WrapRunnable*Ret out of tail position

We do this to prepare the way for using variadic templates for
WrapRunnable*, which will make things significantly simpler, and enable
code-sharing between WrapRunnable and NS_NewRunnableMethodWithArgs.

I'm sure there's probably template magic that would enable us to say that in:

WrapRunnableNMRet(FunType f, Args... args)

the last argument of |args| is considered special, as the return value pointer,
and would avoid this mass rewrite.  I don't think it's particularly clear, and
the error messages for getting it wrong would be confusing at best.

Comment 9

[Eric Rescorla (:ekr)]

Yeah, I'm pretty sad about this change, since it seems pretty unidiomatic. Can you sketch out where you're eventually going and why this helps?

Comment 10

[Nathan Froyd [:froydnj]]

Attachment: part 2 - move WrapRunnable &co over to variadic templates

Hooray for deleting lots of code.  I didn't do the unification with
nsRunnableMethodArguments in this bug because there's some impedance mismatch
between WrapRunnable*'s implicitly-typed members in the runnable and
nsRunnableMethod*'s explicitly-typed members.  I haven't figured out what to do
there yet.

Comment 11

[Eric Rescorla (:ekr)]

(In reply to Eric Rescorla (:ekr) from comment #9)
> Yeah, I'm pretty sad about this change, since it seems pretty unidiomatic.
> Can you sketch out where you're eventually going and why this helps?

Ah, I didn't realize part 2 was coming right away. Is there a reason for this
part 1/part 2 structure. How about I just review the merged patch?

Comment 12

[Nathan Froyd [:froydnj]]

(In reply to Eric Rescorla (:ekr) from comment #11)
> (In reply to Eric Rescorla (:ekr) from comment #9)
> > Yeah, I'm pretty sad about this change, since it seems pretty unidiomatic.
> > Can you sketch out where you're eventually going and why this helps?
> 
> Ah, I didn't realize part 2 was coming right away. Is there a reason for this
> part 1/part 2 structure. How about I just review the merged patch?

*shrug* I thought things would be easier to review if the changes were split up.  I can post a merged patch if you like.

Comment 13

[Eric Rescorla (:ekr)]

Please. It seems silly to review the changes in position in the generator and generated code if that's just going away.

Comment 14

[Nathan Froyd [:froydnj]]

Attachment: move WrapRunnable &co over to variadic templates

Single patch as requested.

Comment 15

[Eric Rescorla (:ekr)]

Thanks. This will probably take me a little bit. I'm on travel today and in meetings all of tomorrow, but I'll try to get to it this week.

Comment 16

[Nathan Froyd [:froydnj]]

(In reply to Eric Rescorla (:ekr) from comment #15)
> Thanks. This will probably take me a little bit. I'm on travel today and in
> meetings all of tomorrow, but I'll try to get to it this week.

Two-week review ping.

Comment 17

[Eric Rescorla (:ekr)]

Attachment: MozReview Request: Bug 1162026 - move WrapRunnable &co over to variadic templates; r=ekr

Bug 1162026 - move WrapRunnable &co over to variadic templates; r=ekr

Comment 18

[Eric Rescorla (:ekr)]

https://reviewboard.mozilla.org/r/10879/#review9519

I rebased this patch to make it work on mozilla-inbound. You will also need to rebase, presumably.

Otherwise LGTM with the changes below.

::: media/webrtc/signaling/test/mediaconduit_unittests.cpp:535
(Diff revision 1)
> -                                            WrapRunnableNMRet(
> +                                            WrapRunnableNMRet(&mAudioSession, 

Fix trailing whitespace.

::: media/webrtc/signaling/test/mediaconduit_unittests.cpp:541
(Diff revision 1)
> -                                            WrapRunnableNMRet(
> +                                            WrapRunnableNMRet(&mAudioSession2, 

Fix trailing whitespace throughout.

::: media/webrtc/signaling/test/signaling_unittests.cpp:1126
(Diff revision 1)
> -      gMainThread, WrapRunnableRet(
> +      gMainThread, WrapRunnableRet(&info, 

Trailing whitespace

::: media/webrtc/signaling/test/signaling_unittests.cpp:1173
(Diff revision 1)
> -      gMainThread, WrapRunnableRet(
> +      gMainThread, WrapRunnableRet(&info, 

Trailing whitespace.

::: media/webrtc/signaling/test/signaling_unittests.cpp:1506
(Diff revision 1)
> -        gMainThread, WrapRunnableRet(
> +        gMainThread, WrapRunnableRet(&streamInfo, 
>            pc->media(), &PeerConnectionMedia::GetLocalStreamByIndex,
> -          stream, &streamInfo));
> +          stream));
>      } else {
>        mozilla::SyncRunnable::DispatchToThread(
> -        gMainThread, WrapRunnableRet(
> +        gMainThread, WrapRunnableRet(&streamInfo, 
>            pc->media(), &PeerConnectionMedia::GetRemoteStreamByIndex,
> -          stream, &streamInfo));
> +          stream));

Trailing whitespace.

::: media/mtransport/runnable_utils.h:92
(Diff revision 1)
> +template<>

Add a comment about this specialization.

Comment 19

[Eric Rescorla (:ekr)]

froydnj: I reviewed this on ReviewBoard.  r=me with the changes in c18

Comment 20

[Nathan Froyd [:froydnj]]

Attachment: move WrapRunnable &co over to variadic templates

Rebased and addressed review comments.  Carrying over r+.

Comment 21

[Nathan Froyd [:froydnj]]

Attachment 8621645 [details] [diff] has the changes requested in comment 18.

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ca242ef328e1

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ca242ef328e1