| Message ID | 20200114201452.14078-1-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Commit | 38dd90307ab2b0d25a0a233eae04455f769153b4 |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, Thanks for your patch. On 2020-01-14 22:14:52 +0200, 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. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > 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 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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. 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. 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. No change is expected in the output code generated by this update. > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> With or without any update to the commit message, this change is fine. Reviewed-by: Kieran Bingham <kieran.bingham@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); >
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@ideasonboard.com> > > With or without any update to the commit message, this change is fine. > > Reviewed-by: Kieran Bingham <kieran.bingham@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);
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);
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. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/libcamera/pipeline/uvcvideo.cpp | 4 +--- src/libcamera/pipeline/vimc.cpp | 4 +--- 2 files changed, 2 insertions(+), 6 deletions(-)