[libcamera-devel] [PATCH] libcamera: Remove std::piecewise_construct where not necessary

[Laurent Pinchart]

Hi Kieran,

On Thu, Jan 16, 2020 at 12:41:05PM +0000, Kieran Bingham wrote:
> Hi Laurent,
> 
> I find the commit message quite terse here, and it doesn't help me in
> understanding the meaning behind why the piecewise construct was (or
> should) be used, or why it's being removed.
> 
> Perhaps that's not quite the purpose of the commit message of course
> (it's not supposed to teach us C++), but I feel like this is one point
> where a good -easy-to-understand message would be useful. Particularly
> if it helps us prevent this mistake in the future.

I could have written a 3-pages message to explain rvalue references,
perfect forwarding, piewcewise construct and emplace, but I indeed
thought it wasn't the purpose of the commit message :-)

> Now that we've gone through the purpose of the piecewise_constructor,
> I've tried to rewrite your commit message below to try to clarify my
> understanding.

Thank you. If not useful for me, I think it can be useful for others
(which was the point of you rewriting this as I understand it).

> If you find any of the content more pleasing, or easier to read, feel
> free to take it in any form.
> 
> On 14/01/2020 20:14, Laurent Pinchart wrote:
> > When emplacing an element in a std::map, std::piecewise_construct is
> > required to forward the parameters to the constructors of the key and
> > mapped type, respectively. However, when the caller already has an
> > instance of the mapped type, forwarding it to the mapped type copy
> > constructor isn't really required, as the copy constructor would be
> > called anyway. Drop std::piecewise_construct in those cases. This does
> > not incur any performance cost.
> 
> When emplacing a control on to the ControlInfoMap with an ID and a
> Range, each of the parameters are objects and are directly attributed to
> the construction of the key value pairs of the ControlInfoMap.
> 
> The use of std::piecewise_construct permits, for example the range to be
> specified using a non-default constructor (such as {0, 100}) directly in
> the emplace method, where the parameter values would be forwarded to the
> constructor.
> 
> In the UVC Video and the VIMC pipeline handlers, this has been overused
> as the parameters to the emplace method are objects themselves, and the
> emplace can use the copy constructors to create the new ControlInfoMap
> entry.
> 
> Simplify the code and improve readability by utilising the default
> emplace method for the map which takes a single argument for each of the
> two elements.

I understand what you're saying, but I'm honestly not sure it's clearer
I'm afraid. I think we're both confusing, but in different ways :-)

How about this combined version ?

When inserting an element with emplace(), the element is constructed
in-place with the parameters to the emplace() method being forwarded to
the constructor of the element. For std::map containers, the element is
an std::pair<const Key, T>. The constructors of std::pair<T1, T2> fall
into three categories:

(1) Default, copy and move constructors (and related versions)
(2) Constructors that take lvalue or rvalue references to T1 and T2
(3) A forwarding constructor that forwards parameters to the
    constructors of T1 and T2, called 

The first category isn't useful in most cases for std::map::emplace(),
as the caller usually doesn't have an existing std::pair<const Key, T>
for the element to be inserted.

The constructor from the third category is useful to avoid constructing
intermediate Key or T instances when the caller doesn't have them
available. This constructor takes two std::tuple arguments that contain
the arguments for the Key and T constructors, respectively. Due to
template deduction rules, usage of such a constructor couldn't be
deduced by the compiler automatically in all cases, so the constructor
takes a first argument of type std::piecewise_construct_t that lets the
caller force the usage ot the forwarding constructor (also known for
this reason as the piecewise constructor). The caller uses a construct
such as

	map.emplace(std::piecewise_construct,
		    std::forward_as_tuple(args_for_Key, ...),
		    std::forward_as_tuple(args_for_T, ...));

This syntax is a bit heavy, but is required to construct Key and T
in-place from arguments to their non-default constructor (it is also the
only std::pair non-default constructor that can be used for non-copyable
non-movable types).

When the caller of std::map::emplace() already has references to a Key
and a T, they can be passed to the std::pair piecewise constructor, and
this will create std::tuple instance to wrap the Key and T references
arguments to ultimately pass them to the Key and T copy constructors.

	map.emplace(std::piecewise_construct,
		    std::forward_as_tuple(Key_value),
		    std::forward_as_tuple(T_value));

While this mechanism works, it's unnecessary complex. A constructor of
std::pair that takes references to Key and T can be used without any
performance penalty, as it will also call the copy constructor of Key
and T. In this case we can use a simpler constructor of std::pair, and
thus a simpler call of std::map::emplace.

	map.emplace(Key_value, T_value);

We have a couple occurrences of this above misuse of piecewise
construction. Simplify them, which simplifies the code and reduces the
generated code size.

> No change is expected in the output code generated by this update.

This is actually not true. We're calling different constructors of
std::pair<>, so different code paths are expected. I've tested this, and
the code size has actually gone down :-)

> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> With or without any update to the commit message, this change is fine.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> > ---
> >  src/libcamera/pipeline/uvcvideo.cpp | 4 +---
> >  src/libcamera/pipeline/vimc.cpp     | 4 +---
> >  2 files changed, 2 insertions(+), 6 deletions(-)
> > 
> > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> > index 83093676ec73..29afb121aa46 100644
> > --- a/src/libcamera/pipeline/uvcvideo.cpp
> > +++ b/src/libcamera/pipeline/uvcvideo.cpp
> > @@ -365,9 +365,7 @@ int UVCCameraData::init(MediaEntity *entity)
> >  			continue;
> >  		}
> >  
> > -		ctrls.emplace(std::piecewise_construct,
> > -			      std::forward_as_tuple(id),
> > -			      std::forward_as_tuple(range));
> > +		ctrls.emplace(id, range);
> >  	}
> >  
> >  	controlInfo_ = std::move(ctrls);
> > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> > index c99560a45cfa..b1054d307ea2 100644
> > --- a/src/libcamera/pipeline/vimc.cpp
> > +++ b/src/libcamera/pipeline/vimc.cpp
> > @@ -448,9 +448,7 @@ int VimcCameraData::init(MediaDevice *media)
> >  			continue;
> >  		}
> >  
> > -		ctrls.emplace(std::piecewise_construct,
> > -			      std::forward_as_tuple(id),
> > -			      std::forward_as_tuple(range));
> > +		ctrls.emplace(id, range);
> >  	}
> >  
> >  	controlInfo_ = std::move(ctrls);

-- 
Regards,

Laurent Pinchart