[RFC PATCH v1 03/12] apps: lc-compliance: Optimize `std::shared_ptr` usage

[Barnabás Pőcze]

Hi


2025. január 10., péntek 0:56 keltezéssel, Laurent Pinchart <laurent.pinchart at ideasonboard.com> írta:

> On Tue, Jan 07, 2025 at 05:30:37PM +0100, Jacopo Mondi wrote:
> > Hi
> >
> > On Fri, Dec 20, 2024 at 03:08:16PM +0000, Barnabás Pőcze wrote:
> > > Avoid unnecessary copies and try to move construct
> > > `std::shared_ptr` whenever possible.
> >
> > Doesn't copy-construct serve to increase the reference count ?
> > Don't you think it is intentional ?
> 
> I think the whole point of this patch is to avoid unnecessary increment
> and decrement of the reference count. See below.

Yes, indeed that is the goal, thanks for writing an explanation.
These changes were extracted from https://patchwork.libcamera.org/patch/20325/


Regards,
Barnabás Pőcze

> 
> > > Signed-off-by: Barnabás Pőcze <pobrn at protonmail.com>
> > > ---
> > >  src/apps/lc-compliance/helpers/capture.cpp | 8 ++++----
> > >  src/apps/lc-compliance/main.cpp            | 4 +---
> > >  2 files changed, 5 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp
> > > index 90c1530ba..d1dafb6cf 100644
> > > --- a/src/apps/lc-compliance/helpers/capture.cpp
> > > +++ b/src/apps/lc-compliance/helpers/capture.cpp
> > > @@ -12,8 +12,8 @@
> > >  using namespace libcamera;
> > >
> > >  Capture::Capture(std::shared_ptr<Camera> camera)
> > > -	: loop_(nullptr), camera_(camera),
> > > -	  allocator_(std::make_unique<FrameBufferAllocator>(camera))
> > > +	: loop_(nullptr), camera_(std::move(camera)),
> > > +	  allocator_(std::make_unique<FrameBufferAllocator>(camera_))
> > >  {
> > >  }
> > >
> > > @@ -72,7 +72,7 @@ void Capture::stop()
> > >  /* CaptureBalanced */
> > >
> > >  CaptureBalanced::CaptureBalanced(std::shared_ptr<Camera> camera)
> > > -	: Capture(camera)
> > > +	: Capture(std::move(camera))
> 
> For instance here. CaptureBalanced::CaptureBalanced() takes a
> std::shared_ptr<Camera> by value. The caller has therefore constructed a
> std::shared_ptr<Camera> instance to be passed to the function, which has
> incremented the reference count. When passing it to the
> Capture::Capture() function, a copy is made, incrementing the reference
> count again. When Capture::Capture() returns,
> CaptureBalanced::CaptureBalanced() finishes and returns too, so the copy
> made here is destroyed, decrementing the reference count. Control
> returns to the caller, which destroys the instance that was passed to
> CaptureBalanced::CaptureBalanced(), decrementing the reference count
> again. Total: +2, -2.
> 
> Using std::move(), the instance passed to
> CaptureBalanced::CaptureBalanced() (which has incremented the reference
> count as above) is moved to the argument of Capture::Capture(). The
> reference count is not incremented a second time. Then, when
> Capture::Capture() return, its argument is destroyed, decrementing the
> reference count. Finally, CaptureBalanced::CaptureBalanced() returns to
> its caller, which destroys the instance that was passed to
> CaptureBalanced::CaptureBalanced(). As the instance was moved, it is
> empty, so the reference count is not decremented. Total: +1, -1.
> 
> Using std::move() avoids an extra increment and decrement. This is
> completely transparent to the caller of
> CaptureBalanced::CaptureBalanced().
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> > >  {
> > >  }
> > >
> > > @@ -144,7 +144,7 @@ void CaptureBalanced::requestComplete(Request *request)
> > >  /* CaptureUnbalanced */
> > >
> > >  CaptureUnbalanced::CaptureUnbalanced(std::shared_ptr<Camera> camera)
> > > -	: Capture(camera)
> > > +	: Capture(std::move(camera))
> > >  {
> > >  }
> > >
> > > diff --git a/src/apps/lc-compliance/main.cpp b/src/apps/lc-compliance/main.cpp
> > > index 3f1d2a61b..98f2573d0 100644
> > > --- a/src/apps/lc-compliance/main.cpp
> > > +++ b/src/apps/lc-compliance/main.cpp
> > > @@ -50,8 +50,6 @@ static void listCameras(CameraManager *cm)
> > >
> > >  static int initCamera(CameraManager *cm, OptionsParser::Options options)
> > >  {
> > > -	std::shared_ptr<Camera> camera;
> > > -
> > >  	int ret = cm->start();
> > >  	if (ret) {
> > >  		std::cout << "Failed to start camera manager: "
> > > @@ -66,7 +64,7 @@ static int initCamera(CameraManager *cm, OptionsParser::Options options)
> > >  	}
> > >
> > >  	const std::string &cameraId = options[OptCamera];
> > > -	camera = cm->get(cameraId);
> > > +	std::shared_ptr<Camera> camera = cm->get(cameraId);
> > >  	if (!camera) {
> > >  		std::cout << "Camera " << cameraId << " not found, available cameras:" << std::endl;
> > >  		listCameras(cm);
> 
> --
> Regards,
> 
> Laurent Pinchart
>