| Message ID | 20211118234546.9848-1-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Commit | 0eacde623bb08504c71e18713188a7cfaf4e8e80 |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, Thank you for the patch 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? > > 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? > 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@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
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@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
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@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@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
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@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@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
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@ideasonboard.com> Reviewed-by: Kieran Bingham <kieran.bingham@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@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
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);
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. 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 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). Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- include/libcamera/base/object.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) base-commit: d9a2a1f703273a28b001dee40fddd378bba7a1b6