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

[Laurent Pinchart]

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.

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