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

[Kieran Bingham]

Quoting Laurent Pinchart (2021-11-19 11:50:36)
> Hi Umang,
> 
> On Fri, Nov 19, 2021 at 05:15:02PM +0530, Umang Jain wrote:
> > On 11/19/21 4:00 PM, Laurent Pinchart wrote:
> > > 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,
> 
> We can, but it's dangerous. For asynchronous calls, the caller would
> need to guarantee that the reference stays valid until the asynchronous
> call finishes. It's possible in some cases, but would very likely be a
> source of hard to debug issues.
> 
> For synchronous calls, we could store references in the parameters pack.
> That's a possible optimization candidate for later.
> 
> > Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>

Reviewed-by: Kieran Bingham <kieran.bingham 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
> 
> -- 
> Regards,
> 
> Laurent Pinchart