[libcamera-devel] [PATCH 2/2] libcamera: controls: Don't over-optimize ControlValue layout

[Laurent Pinchart]

Hi Kieran,

On Fri, Mar 20, 2020 at 12:57:38PM +0000, Kieran Bingham wrote:
> On 20/03/2020 00:55, Laurent Pinchart wrote:
> > The ControlValue class size should be minimized to save space, as it can
> > be instantiated in large numbers. The current implementation does so by
> > specifying several members as bitfields, but does so too aggressively,
> > resulting in fields being packed in an inefficient to access way on some
> > platforms. For instance, on 32-bit x86, the numElements_ field is offset
> > by 7 bits in a 32-bit word. This additionally causes a static assert
> > that checks the size of the class to fail.
> > 
> > Relax the constraints on the isArray_ and numElements_ fields to avoid
> > inefficient access, and to ensure that the class size is identical
> > across all platforms. This will anyway need to be revisited when
> > stabilizing the ABI, so add a \todo comment as a reminder.
> > 
> > Fixes: 1fa4b43402a0 ("libcamera: controls: Support array controls in ControlValue")
> > Reported-by: Jan Engelhardt <jengelh at inai.de>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  include/libcamera/controls.h | 4 ++--
> >  src/libcamera/controls.cpp   | 1 +
> >  2 files changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > index 2fca975f7512..40a29189ba01 100644
> > --- a/include/libcamera/controls.h
> > +++ b/include/libcamera/controls.h
> > @@ -158,8 +158,8 @@ public:
> >  
> >  private:
> >  	ControlType type_ : 8;
> > -	bool isArray_ : 1;
> > -	std::size_t numElements_ : 16;
> > +	bool isArray_;
> > +	std::size_t numElements_ : 32;
> 
> Out of interest, is using "std::size_t : 32;" here any better than using
> int32_t?

size_t is the type used in the C++ standard library to store sizes, and
it makes sense to use it through the API I think. It's 64-bit long on 64
bit platforms, which would be overkill here :-)

(and it's an unsigned type, so it would be uint32_t)

> >  	union {
> >  		uint64_t value_;
> >  		void *storage_;
> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> > index 4615e71d7e3c..3c910d38f05d 100644
> > --- a/src/libcamera/controls.cpp
> > +++ b/src/libcamera/controls.cpp
> > @@ -83,6 +83,7 @@ static constexpr size_t ControlValueSize[] = {
> >   * \brief Abstract type representing the value of a control
> >   */
> >  
> > +/** \todo Revisit the ControlValue layout when stabilizing the ABI */
> 
> Revisit how? I assume if the ControlValue size changes the assert will
> fire anyway so it's somewhat self documenting at that point ?

I mean that, when stabilizing the ABI, we'll have to look at the
ControlValue layout and decide how to pack it efficiently. As long as
the ControlValue implementation is still in flux it would be premature
optimization I think.

> Not objecting to the comment/todo - but curious as to what (in X months)
> the reader will have to interpret that message as, to determine 'what'
> needs to be done to satisfy this todo.
> 
> Anyway, nothing to block:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> >  static_assert(sizeof(ControlValue) == 16, "Invalid size of ControlValue class");
> >  
> >  /**

-- 
Regards,

Laurent Pinchart