[libcamera-devel] [PATCH 2/2] libcamera: base: signal: Prevent excessive slot usage

[David Plowman]

Hi Kieran

Also for this one:

Tested-by: David Plowman <david.plowman at raspberrypi.com>

Thanks!
David

On Fri, 7 Jan 2022 at 15:11, Kieran Bingham
<kieran.bingham at ideasonboard.com> wrote:
>
> Quoting Jacopo Mondi (2022-01-07 15:01:32)
> > Hi Kieran,
> >
> > On Fri, Jan 07, 2022 at 12:55:06PM +0000, Kieran Bingham wrote:
> > > Slots are not expected to be connected to lots of signals. Our normal
> >
> > Isn't it the other way around ?
> > "Signals are not expected to be connected to a lots of slots" :)
>
> I ... er ... um ... probably? maybe ;-)
>
> >
> > > use case is a one to one mapping with a signal expected to call one
> > > slot.
> > >
> > > While we support multiple slots, excessive use is likely the result of a
> > > bug indicating a failure to disconnect slots or repeated connection of a
> > > slot which can lead to memory leaks and poor performance when deleting
> > > the Object.
> > >
> > > Assert that a maximum of 8 slots are connected which will catch any bug
> > > and still allow some multiple slot usage. If this limit is ever met it
> > > can be extended and considered at that time.
> >
> > I wish we had WARN_ONCE().
>
> We could add that, but in this instance, I really want the assertion to
> break things, otherwise for instance in CTS it woudl just keep running -
> and woudl require someone to manually go through the logs.
>
> Maybe it would be noisy enough to get noticed ... but maybe not...
>
>
> >
> > >
> > > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > > ---
> > >
> > > Note that this patch causes the CTS tests to fail as they hit this
> > > assertion.
> > >
> > > It is not yet investigated if we really need more slots or if the
> > > assertion has already caught a slot-leak, but posting for early review
> > > anyway.
>
> Of course the fact that it breaks CTS currently means this patch can't
> go in until it's resolved anyway.
>
>
>
> > >
> > >  src/libcamera/base/signal.cpp | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > >
> > > diff --git a/src/libcamera/base/signal.cpp b/src/libcamera/base/signal.cpp
> > > index 9df45d079a42..7e6a23cc909d 100644
> > > --- a/src/libcamera/base/signal.cpp
> > > +++ b/src/libcamera/base/signal.cpp
> > > @@ -7,6 +7,7 @@
> > >
> > >  #include <libcamera/base/signal.h>
> > >
> > > +#include <libcamera/base/log.h>
> > >  #include <libcamera/base/mutex.h>
> > >
> > >  /**
> > > @@ -35,6 +36,13 @@ void SignalBase::connect(BoundMethodBase *slot)
> > >       if (object)
> > >               object->connect(this);
> > >       slots_.push_back(slot);
> > > +
> > > +     /*
> > > +      * Slots are not expected to be used to connect many items. If we exceed
> > > +      * a reasonable amount, presume that there is a bug and fail fast on
> > > +      * debug builds.
> > > +      */
> > > +     ASSERT(slots_.size() < 8);
> >
> > That's indeed a useful safety check when developing. I think it's
> > worth having it in.
>
> Thanks, I agree of course (once we find/fix the associated CTS bug)
>
> >
> > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> >
> > Thanks
> >   j
> >
> > >  }
> > >
> > >  void SignalBase::disconnect(Object *object)
> > > --
> > > 2.32.0
> > >