convert WrapRunnable* and supporting machinery to use variadic templates

RESOLVED FIXED in Firefox 41

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

(Depends on 1 bug, Blocks 1 bug)

unspecified
mozilla41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

Assignee

Description

4 years ago
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.
Assignee

Updated

4 years ago
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).
Assignee

Comment 3

4 years ago
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.
Assignee

Comment 5

4 years ago
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.
Assignee

Comment 6

4 years ago
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)
Assignee

Comment 8

4 years ago
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?
Assignee

Comment 10

4 years ago
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?
Assignee

Comment 12

4 years ago
(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.
Assignee

Comment 14

4 years ago
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.
Assignee

Comment 16

4 years ago
(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)
Assignee

Comment 20

4 years ago
Rebased and addressed review comments.  Carrying over r+.
Attachment #8610195 - Attachment is obsolete: true
Attachment #8610195 - Flags: review?(ekr)
Attachment #8621645 - Flags: review+
Assignee

Updated

4 years ago
Blocks: 1174274
Assignee

Comment 21

4 years ago
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: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Assignee

Updated

4 years ago
Assignee: nobody → nfroyd
Assignee

Updated

4 years ago
Blocks: 1175621
You need to log in before you can comment on or make changes to this bug.