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

[Laurent Pinchart]

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.

> > 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.

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

-- 
Regards,

Laurent Pinchart