Closed Bug 1162026 Opened 6 years ago Closed 5 years ago

convert WrapRunnable* and supporting machinery to use variadic templates

Categories

(Core :: WebRTC, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed
Blocking Flags:
backlog parking-lot

People

(Reporter: froydnj, Assigned: froydnj)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(2 files, 3 obsolete files)

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.
Depends on: 1162029
MT and I discussed this and our current thinking is that instead we should convert uses of WrapRunnable to closures.
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).
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?
(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.
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.
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?
Flags: needinfo?(ekr)
If you want to submit a patch, I'll review it.
Flags: needinfo?(ekr)
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.
Attachment #8610185 - Flags: review?(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?
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.
Attachment #8610190 - Flags: review?(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?
(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.
Please. It seems silly to review the changes in position in the generator and generated code if that's just going away.
Single patch as requested.
Attachment #8610185 - Attachment is obsolete: true
Attachment #8610190 - Attachment is obsolete: true
Attachment #8610185 - Flags: review?(ekr)
Attachment #8610190 - Flags: review?(ekr)
Attachment #8610195 - Flags: review?(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.
(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.
Flags: needinfo?(ekr)
backlog: --- → parking-lot
Bug 1162026 - move WrapRunnable &co over to variadic templates; r=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.
froydnj: I reviewed this on ReviewBoard.  r=me with the changes in c18
Flags: needinfo?(ekr) → needinfo?(nfroyd)
Rebased and addressed review comments.  Carrying over r+.
Attachment #8610195 - Attachment is obsolete: true
Attachment #8610195 - Flags: review?(ekr)
Attachment #8621645 - Flags: review+
Blocks: 1174274
Attachment 8621645 [details] [diff] has the changes requested in comment 18.
Flags: needinfo?(nfroyd)
https://hg.mozilla.org/mozilla-central/rev/ca242ef328e1
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Assignee: nobody → nfroyd
Blocks: 1175621
You need to log in before you can comment on or make changes to this bug.