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

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[James Cheng[:JamesCheng]]

Attachment: Use-universal-reference-to-reduce-the-redundant-copy.patch

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.

Comment 1

[Bobby Holley (:bholley)]

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.

Comment 2

[Nathan Froyd [:froydnj]]

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.

Comment 3

[James Cheng[:JamesCheng]]

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.

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/96e9bda29751

Comment 5

[Nathan Froyd [:froydnj]]

(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?

Comment 6

[James Cheng[:JamesCheng]]

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~

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/96e9bda29751