[libcamera-devel,v1] libcamera: framebuffer_allocator: Avoid double map lookup

17900 diff mbox series

Use `try_emplace()` on the map instead of `count()`
and `operator[]` to avoid walking the tree twice.

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

--
2.38.1

Hi Barnabás,

Thank you for the patch.

On Sat, Nov 26, 2022 at 11:42:27PM +0000, Barnabás Pőcze via libcamera-devel wrote:
> Use `try_emplace()` on the map instead of `count()`
> and `operator[]` to avoid walking the tree twice.
> 
> Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
> ---
>  src/libcamera/framebuffer_allocator.cpp | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp
> index 4df27cac..17c5db49 100644
> --- a/src/libcamera/framebuffer_allocator.cpp
> +++ b/src/libcamera/framebuffer_allocator.cpp
> @@ -88,12 +88,14 @@ FrameBufferAllocator::~FrameBufferAllocator()
>   */
>  int FrameBufferAllocator::allocate(Stream *stream)
>  {
> -	if (buffers_.count(stream)) {
> +	const auto &[ it, inserted ] = buffers_.try_emplace(stream);

No spaces within [ ... ]

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

No need to resubmit if you agree with this change, I'll fix when
applying.

> +
> +	if (!inserted) {
>  		LOG(Allocator, Error) << "Buffers already allocated for stream";
>  		return -EBUSY;
>  	}
> 
> -	int ret = camera_->exportFrameBuffers(stream, &buffers_[stream]);
> +	int ret = camera_->exportFrameBuffers(stream, &it->second);
>  	if (ret == -EINVAL)
>  		LOG(Allocator, Error)
>  			<< "Stream is not part of " << camera_->id()

Quoting Laurent Pinchart via libcamera-devel (2022-11-28 06:37:56)
> Hi Barnabás,
> 
> Thank you for the patch.
> 
> On Sat, Nov 26, 2022 at 11:42:27PM +0000, Barnabás Pőcze via libcamera-devel wrote:
> > Use `try_emplace()` on the map instead of `count()`
> > and `operator[]` to avoid walking the tree twice.
> > 
> > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
> > ---
> >  src/libcamera/framebuffer_allocator.cpp | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp
> > index 4df27cac..17c5db49 100644
> > --- a/src/libcamera/framebuffer_allocator.cpp
> > +++ b/src/libcamera/framebuffer_allocator.cpp
> > @@ -88,12 +88,14 @@ FrameBufferAllocator::~FrameBufferAllocator()
> >   */
> >  int FrameBufferAllocator::allocate(Stream *stream)
> >  {
> > -     if (buffers_.count(stream)) {
> > +     const auto &[ it, inserted ] = buffers_.try_emplace(stream);
> 
> No spaces within [ ... ]
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

LGTM too.


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

> 
> No need to resubmit if you agree with this change, I'll fix when
> applying.
> 
> > +
> > +     if (!inserted) {
> >               LOG(Allocator, Error) << "Buffers already allocated for stream";
> >               return -EBUSY;
> >       }
> > 
> > -     int ret = camera_->exportFrameBuffers(stream, &buffers_[stream]);
> > +     int ret = camera_->exportFrameBuffers(stream, &it->second);
> >       if (ret == -EINVAL)
> >               LOG(Allocator, Error)
> >                       << "Stream is not part of " << camera_->id()
> 
> -- 
> Regards,
> 
> Laurent Pinchart

2022. november 28., hétfő 7:37 keltezéssel, Laurent Pinchart írta:


> Hi Barnabás,
> 
> Thank you for the patch.
> 
> On Sat, Nov 26, 2022 at 11:42:27PM +0000, Barnabás Pőcze via libcamera-devel wrote:
> 
> > Use `try_emplace()` on the map instead of `count()`
> > and `operator[]` to avoid walking the tree twice.
> > 
> > Signed-off-by: Barnabás Pőcze pobrn@protonmail.com
> > ---
> > src/libcamera/framebuffer_allocator.cpp | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp
> > index 4df27cac..17c5db49 100644
> > --- a/src/libcamera/framebuffer_allocator.cpp
> > +++ b/src/libcamera/framebuffer_allocator.cpp
> > @@ -88,12 +88,14 @@ FrameBufferAllocator::~FrameBufferAllocator()
> > */
> > int FrameBufferAllocator::allocate(Stream *stream)
> > {
> > - if (buffers_.count(stream)) {
> > + const auto &[ it, inserted ] = buffers_.try_emplace(stream);
> 
> 
> No spaces within [ ... ]
> 
> Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
> 
> 
> No need to resubmit if you agree with this change, I'll fix when
> applying.

Yes, please, fix it when applying.


Regards,
Barnabás Pőcze

> 
> > +
> > + if (!inserted) {
> > LOG(Allocator, Error) << "Buffers already allocated for stream";
> > return -EBUSY;
> > }
> > 
> > - int ret = camera_->exportFrameBuffers(stream, &buffers_[stream]);
> > + int ret = camera_->exportFrameBuffers(stream, &it->second);
> > if (ret == -EINVAL)
> > LOG(Allocator, Error)
> > << "Stream is not part of " << camera_->id()
> 
> 
> --
> Regards,
> 
> Laurent Pinchart