| Message ID | 20241220150759.709756-4-pobrn@protonmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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 ? > > Signed-off-by: Barnabás Pőcze <pobrn@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)) > { > } > > @@ -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); > -- > 2.47.1 > >
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@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@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);
Hi 2025. január 10., péntek 0:56 keltezéssel, Laurent Pinchart <laurent.pinchart@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@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@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 >
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)) { } @@ -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);
Avoid unnecessary copies and try to move construct `std::shared_ptr` whenever possible. Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> --- src/apps/lc-compliance/helpers/capture.cpp | 8 ++++---- src/apps/lc-compliance/main.cpp | 4 +--- 2 files changed, 5 insertions(+), 7 deletions(-)