[libcamera-devel] [PATCH] libcamera: object: Avoid argument copies in invokeMethod()

[Umang Jain]

Hi Laurent,

On 11/19/21 4:00 PM, Laurent Pinchart wrote:
> Hi Umang,
>
> On Fri, Nov 19, 2021 at 11:59:43AM +0530, Umang Jain wrote:
>> On 11/19/21 5:15 AM, Laurent Pinchart wrote:
>>> Template argument deduction results in the lvalue and lvalue reference
>>> arguments to the invokeMethod() function causing deduction of the Args
>>> template type to a non-reference type. This results in the argument
>>> being passed by value and copied.
>>
>> I think this is a documented behavior (and by documented, I mean
>> intentional...)
>>
>>            *
>>            * Arguments \a args passed by value or reference are copied,
>> while pointers
>>            * are passed untouched. The caller shall ensure that any
>> pointer argument
>>            * remains valid until the method is invoked.
>>
>> Should the documented behavior be tweaked as well?
> There's still a copy, when passing the arguments to ->activate() and
> storing them in the pack. This patch only avoids one extra copy.


Ah okay, I got confused with the copy itself is happening the 
invokeMethod() stage (hideous). Indeed we do avoid one extra copy now 
after I read the invokeMethod() implementation.

>
>>> Fix this by using a cv-unqualified rvalue reference parameter type. The
>>> type is then deduced to an lvalue reference when the argument is an
>> So does this mean, references can be passed from one thread to another
>> in ::invokeMethod() ? Instead of copy-by-value behavior as before? Are
>> there any caveats for doing so?
> The references can now be passed to invokeMethod(), but still not to the
> next layer. We still need copies for cross-thread calls, at least when
> they're asynchronous.


Yes, the documentation is intact, sorry for the noise.

I wonder why we can't send references across threads? But for this patch,

Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>

>
>>> lvalue or lvalue reference, due to a combination of the special template
>>> argument deduction rule for rvalue reference parameter types:
>>>
>>>     If P is an rvalue reference to a cv-unqualified template parameter
>>>     (so-called forwarding reference), and the corresponding function call
>>>     argument is an lvalue, the type lvalue reference to A is used in place
>>>     of A for deduction.
>>>
>>>     (https://en.cppreference.com/w/cpp/language/template_argument_deduction)
>>>
>>> and the reference collapsing rule
>>> (https://en.cppreference.com/w/cpp/language/reference#Reference_collapsing).
>> As long as we are OK with passing references(lvalue or rvalue) across
>> threads (that's my understanding of use-case of ::invokeMethod()) the
>> reference collapsing done here makes sense.
>>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>>> ---
>>>    include/libcamera/base/object.h | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/include/libcamera/base/object.h b/include/libcamera/base/object.h
>>> index 5c385ab4b140..ae2a275caf68 100644
>>> --- a/include/libcamera/base/object.h
>>> +++ b/include/libcamera/base/object.h
>>> @@ -34,7 +34,7 @@ public:
>>>    	template<typename T, typename R, typename... FuncArgs, typename... Args,
>>>    		 typename std::enable_if_t<std::is_base_of<Object, T>::value> * = nullptr>
>>>    	R invokeMethod(R (T::*func)(FuncArgs...), ConnectionType type,
>>> -		       Args... args)
>>> +		       Args&&... args)
>>>    	{
>>>    		T *obj = static_cast<T *>(this);
>>>    		auto *method = new BoundMethodMember<T, R, FuncArgs...>(obj, this, func, type);
>>>
>>> base-commit: d9a2a1f703273a28b001dee40fddd378bba7a1b6