Closed Bug 1206598 Opened 6 years ago Closed 6 years ago

Use universal reference to reduce redundant copy when using InvokeAsync in MozPromise.

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: JamesCheng, Assigned: JamesCheng)

Details

Attachments

(1 file)

When using 
template<typename PromiseType, typename ThisType, typename ...ArgTypes>
static nsRefPtr<PromiseType>
InvokeAsync(AbstractThread* aTarget, ThisType* aThisVal, const char* aCallerName,
            nsRefPtr<PromiseType>(ThisType::*aMethod)(ArgTypes...), ArgTypes... aArgs)

The ArgTypes... will copy/move all the args once due to the ArgTypes is bound by the member function pointer arguments type.

We should make member function pointer prototype independent from args type.

Hi BHolley,

Could you please give me some feedback if this modification is reasonable?

Thank you.
Attachment #8663523 - Flags: feedback?(bobbyholley)
Comment on attachment 8663523 [details] [diff] [review]
Use-universal-reference-to-reduce-the-redundant-copy.patch

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

Bouncing the C++ template trickery to Nathan.
Attachment #8663523 - Flags: feedback?(bobbyholley) → review?(nfroyd)
Comment on attachment 8663523 [details] [diff] [review]
Use-universal-reference-to-reduce-the-redundant-copy.patch

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

I think this does the right thing (have you checked via assembly inspection or printf debugging?), though I'm wondering if MethodCall's constructor gets in the way here.  Tests for MethodCall and InvokeAsync here that check they do the minimum amount of copying would be splendid, but they're not something to hold the patch on.
Attachment #8663523 - Flags: review?(nfroyd) → review+
Hi Nathan,

Thanks for reviewing.

Yes, I tested.

In addition, the original code will have potential issues.

1. For Policy, Forward<T>(...) should use in perfect forwarding case(with universal reference).

2. Please see the sample code below for example. If the function prototype have reference type, the args type Args(should be Foo) deduction will be ambiguous against the member function pointer's Args(should be Foo&), this can also reproduce in gecko code with reference type.
http://ideone.com/tmFGsK

Thus, compiling will got error. So should use this patch to fix this issue i think.

Thank you.
Flags: needinfo?(nfroyd)
Keywords: checkin-needed
(In reply to James Cheng[:JamesCheng] from comment #3)
> In addition, the original code will have potential issues.
> 
> 1. For Policy, Forward<T>(...) should use in perfect forwarding case(with
> universal reference).
> 
> 2. Please see the sample code below for example. If the function prototype
> have reference type, the args type Args(should be Foo) deduction will be
> ambiguous against the member function pointer's Args(should be Foo&), this
> can also reproduce in gecko code with reference type.
> http://ideone.com/tmFGsK
> 
> Thus, compiling will got error. So should use this patch to fix this issue i
> think.

Fair enough.  But doesn't MethodCall's constructor have the same issues as your sample code, i.e. using forwarding without universal references?

Actually, since we're packaging up code to run on a different thread, don't we perhaps *want* copying to happen somewhere, so we're not inadvertently sharing data across threads?
Flags: needinfo?(nfroyd) → needinfo?(jacheng)
MethodCall(MethodType aMethod, ThisType* aThisVal, ArgTypes... aArgs)
, mArgs(Forward<ArgTypes>(aArgs)...)

(a)Forward<ArgTypes> will cast to ArgTypes&& which means call by value and move to mArgs.
(b)Forward<ArgTypes&> will cast to ArgTypes&&& and collapse to ArgTypes& which means call by l-ref and pass l-ref to mArgs.
(c)Forward<ArgTypes&&> will cast to  ArgTypes&&&& and collapse to ArgTypes&& which means call by r-ref and pass r-ref to mArgs.

Sorry that I used the "policy" word, I just want to express that |Forward| usually used with T&& :)

In this case, it is OK since no matter how Forward casts, the tuple will store a new object.

This bug just try to make the pointer args and actual args type independent and 
when the the function prototype composed of reference, the compiling will never get failed.

I didn't dig into MozPromise implementation so I am not capable of modifying other parts related to the logic :(

There still have a potential issue that |InvokeAsync| cannot accept the const member function since
nsRefPtr<PromiseType>(ThisType::*aMethod)(ArgTypes...) cannot accept const member function unless we have a nsRefPtr<PromiseType>(ThisType::*aMethod)(ArgTypes...) const version of |InvokeAsync|.

Thanks~
Flags: needinfo?(jacheng)
https://hg.mozilla.org/mozilla-central/rev/96e9bda29751
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.