[v2,1/3] libcamera: framebuffer_allocator: Move from argument in constructor

19653 diff mbox series

The single argument, of type `std::shared_ptr<Camera>`,
is passed by value, so it can simply be moved from in order to
avoid calling the copy constructor.

Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
---
 src/libcamera/framebuffer_allocator.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Quoting Barnabás Pőcze (2024-03-10 14:30:33)
> The single argument, of type `std::shared_ptr<Camera>`,
> is passed by value, so it can simply be moved from in order to
> avoid calling the copy constructor.
> 


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
> ---
>  src/libcamera/framebuffer_allocator.cpp | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp
> index 94389735..8cf45ab2 100644
> --- a/src/libcamera/framebuffer_allocator.cpp
> +++ b/src/libcamera/framebuffer_allocator.cpp
> @@ -59,7 +59,7 @@ LOG_DEFINE_CATEGORY(Allocator)
>   * \param[in] camera The camera
>   */
>  FrameBufferAllocator::FrameBufferAllocator(std::shared_ptr<Camera> camera)
> -       : camera_(camera)
> +       : camera_(std::move(camera))
>  {
>  }
>  
> -- 
> 2.44.0
> 
>

Hi Barnabás

On Sun, Mar 10, 2024 at 02:30:33PM +0000, Barnabás Pőcze wrote:
> The single argument, of type `std::shared_ptr<Camera>`,
> is passed by value, so it can simply be moved from in order to
> avoid calling the copy constructor.
>
> Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
> ---
>  src/libcamera/framebuffer_allocator.cpp | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp
> index 94389735..8cf45ab2 100644
> --- a/src/libcamera/framebuffer_allocator.cpp
> +++ b/src/libcamera/framebuffer_allocator.cpp
> @@ -59,7 +59,7 @@ LOG_DEFINE_CATEGORY(Allocator)
>   * \param[in] camera The camera
>   */
>  FrameBufferAllocator::FrameBufferAllocator(std::shared_ptr<Camera> camera)
> -	: camera_(camera)
> +	: camera_(std::move(camera))

However, if the caller of the FrameBufferAllocator constructor uses
move() when calling the constructur, this will not increase the
shared_ptr<> reference count and this could lead to a path where the
object held by the shared_ptr<> (the camera in this case) is released
before the FrameBufferAllocator instance referencing it ?

Do yuo think it's an issue ? Applications shouldn't

        FrameBufferAllocator(std::move(camera))

but is there anything preventing them from doing so ?

>  {
>  }
>
> --
> 2.44.0
>
>

Hi


2024. március 11., hétfő 18:01 keltezéssel, Jacopo Mondi <jacopo.mondi@ideasonboard.com> írta:

> Hi Barnabás
> 
> On Sun, Mar 10, 2024 at 02:30:33PM +0000, Barnabás Pőcze wrote:
> > The single argument, of type `std::shared_ptr<Camera>`,
> > is passed by value, so it can simply be moved from in order to
> > avoid calling the copy constructor.
> >
> > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
> > ---
> >  src/libcamera/framebuffer_allocator.cpp | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp
> > index 94389735..8cf45ab2 100644
> > --- a/src/libcamera/framebuffer_allocator.cpp
> > +++ b/src/libcamera/framebuffer_allocator.cpp
> > @@ -59,7 +59,7 @@ LOG_DEFINE_CATEGORY(Allocator)
> >   * \param[in] camera The camera
> >   */
> >  FrameBufferAllocator::FrameBufferAllocator(std::shared_ptr<Camera> camera)
> > -	: camera_(camera)
> > +	: camera_(std::move(camera))
> 
> However, if the caller of the FrameBufferAllocator constructor uses
> move() when calling the constructur, this will not increase the
> shared_ptr<> reference count and this could lead to a path where the
> object held by the shared_ptr<> (the camera in this case) is released
> before the FrameBufferAllocator instance referencing it ?

That is not possible. The constructor's argument holds a reference to the camera
object preventing it from being destroyed.


> 
> Do yuo think it's an issue ? Applications shouldn't
> 
>         FrameBufferAllocator(std::move(camera))
> 
> but is there anything preventing them from doing so ?

The observable behaviour for the caller does not really change after the proposed change.
If someone uses `FrameBufferAllocator(std::move(camera))`, then that will construct
a temporary with the shared_ptr's move constructor, so `camera` will point to nullptr
afterwards, regardless of what is done inside the constructor. This is what happens
now and what will happen after this change.

Previously if someone did

  FrameBufferAllocator { ... }

then there would be
  * two copy constructor invocations, or
  * one move constructor and one copy constructor invocation.
depending on the type of `...` above.

The first one when constructing the temporary for the argument,
and the second one when constructing the member variable from the argument.

With the proposed change, there will be at most one copy constructor call
when constructing a FrameBufferAllocator.


> 
> >  {
> >  }
> >
> > --
> > 2.44.0
> >
> >


Regards,
Barnabás Pőcze

Hi Barnabás

On Mon, Mar 11, 2024 at 05:42:55PM +0000, Barnabás Pőcze wrote:
> Hi
>
>
> 2024. március 11., hétfő 18:01 keltezéssel, Jacopo Mondi <jacopo.mondi@ideasonboard.com> írta:
>
> > Hi Barnabás
> >
> > On Sun, Mar 10, 2024 at 02:30:33PM +0000, Barnabás Pőcze wrote:
> > > The single argument, of type `std::shared_ptr<Camera>`,
> > > is passed by value, so it can simply be moved from in order to
> > > avoid calling the copy constructor.
> > >
> > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
> > > ---
> > >  src/libcamera/framebuffer_allocator.cpp | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp
> > > index 94389735..8cf45ab2 100644
> > > --- a/src/libcamera/framebuffer_allocator.cpp
> > > +++ b/src/libcamera/framebuffer_allocator.cpp
> > > @@ -59,7 +59,7 @@ LOG_DEFINE_CATEGORY(Allocator)
> > >   * \param[in] camera The camera
> > >   */
> > >  FrameBufferAllocator::FrameBufferAllocator(std::shared_ptr<Camera> camera)
> > > -	: camera_(camera)
> > > +	: camera_(std::move(camera))
> >
> > However, if the caller of the FrameBufferAllocator constructor uses
> > move() when calling the constructur, this will not increase the
> > shared_ptr<> reference count and this could lead to a path where the
> > object held by the shared_ptr<> (the camera in this case) is released
> > before the FrameBufferAllocator instance referencing it ?
>
> That is not possible. The constructor's argument holds a reference to the camera
> object preventing it from being destroyed.
>
>
> >
> > Do yuo think it's an issue ? Applications shouldn't
> >
> >         FrameBufferAllocator(std::move(camera))
> >
> > but is there anything preventing them from doing so ?
>
> The observable behaviour for the caller does not really change after the proposed change.
> If someone uses `FrameBufferAllocator(std::move(camera))`, then that will construct
> a temporary with the shared_ptr's move constructor, so `camera` will point to nullptr
> afterwards, regardless of what is done inside the constructor. This is what happens
> now and what will happen after this change.

I think the key point is also that std::move() of a shared_ptr<>
effectively resets it, so it's not something anyone would do, which
makes my argument moot

>
> Previously if someone did
>
>   FrameBufferAllocator { ... }
>
> then there would be
>   * two copy constructor invocations, or
>   * one move constructor and one copy constructor invocation.
> depending on the type of `...` above.
>
> The first one when constructing the temporary for the argument,
> and the second one when constructing the member variable from the argument.
>
> With the proposed change, there will be at most one copy constructor call
> when constructing a FrameBufferAllocator.
>

For reference, this is the code generated by goldbot for the two use
cases with the following code snippet:

-------------------------------------------------------------------------------
#include <memory>

using namespace std;

class klass
{
public:
    klass(shared_ptr<int> i)
        : i_(i)
        // : i_(move(i))
    {
    }

private:
    shared_ptr<int> i_;
};

int main()
{
    shared_ptr<int> i = std::make_shared<int>(5);

    klass k(i);

    return 0;
}
-------------------------------------------------------------------------------


* move)
	mov     rbx, QWORD PTR [rbp-24]
        mov     rax, QWORD PTR [rbp-32]
        mov     rdi, rax
        call    std::remove_reference<std::shared_ptr<int>&>::type&& std::move<std::shared_ptr<int>&>(std::shared_ptr<int>&)
        mov     rsi, rax
        mov     rdi, rbx
        call    std::shared_ptr<int>::shared_ptr(std::shared_ptr<int>&&) [complete object constructor]


* non move)
        mov     rax, QWORD PTR [rbp-8]
        mov     rdx, QWORD PTR [rbp-16]
        mov     rsi, rdx
        mov     rdi, rax
        call    std::shared_ptr<int>::shared_ptr(std::shared_ptr<int> const&) [complete object constructor]

I hoped for the compiler to be smart enough to avoid calling the copy
constructor for both the temporary object and the class member
initialization, but that doesn't seem the case, so

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
  j

>
> >
> > >  {
> > >  }
> > >
> > > --
> > > 2.44.0
> > >
> > >
>
>
> Regards,
> Barnabás Pőcze