| Message ID | 20220107125506.1477795-3-kieran.bingham@ideasonboard.com |
|---|---|
| State | Superseded |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series |
|
| Related | show |
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" :) > 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(). > > Signed-off-by: Kieran Bingham <kieran.bingham@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. > > 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. Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > } > > void SignalBase::disconnect(Object *object) > -- > 2.32.0 >
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@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@jmondi.org> > > Thanks > j > > > } > > > > void SignalBase::disconnect(Object *object) > > -- > > 2.32.0 > >
Hi Kieran Also for this one: Tested-by: David Plowman <david.plowman@raspberrypi.com> Thanks! David On Fri, 7 Jan 2022 at 15:11, Kieran Bingham <kieran.bingham@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@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@jmondi.org> > > > > Thanks > > j > > > > > } > > > > > > void SignalBase::disconnect(Object *object) > > > -- > > > 2.32.0 > > >
Hi Kieran, Thank you for the patch. On Fri, Jan 07, 2022 at 03:11:23PM +0000, Kieran Bingham wrote: > Quoting Jacopo Mondi (2022-01-07 15:01:32) > > 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@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) You know how much I dislike such arbitrary limits :-) One option would be to check if an identical connection already exists. Qt has a Qt::UniqueConnection flag (https://doc.qt.io/qt-6/qt.html#ConnectionType-enum) for this purpose, we could make it the default and add a non-unique connection flag (or just skip it for now until we have use cases for multiple connections of the same signal to the same slot). The advantage is that we could then catch the very first invalid connection attempt, possibly also catching more bugs that would create extraneous connections but not go above the limit. One issue, however, is that comparing connections may not be trivial when the slot is a lambda function, as the closure class created by lambda function has no equality comparison operator (the problem would be the same for any functor without an equality comparison operator). I think erroneous connections to lambda slots is exactly what you were trying to catch in this patch series. It could be possible to hack around this, by passing __builtin_FILE() and __builtin_LINE() as value for default arguments to the connect() function (considering that two lambda slots are equal if they're defined on the same source line), but that's not very pretty, nor very exact: for (auto object : objects) mySignal.connect([object]() { object->slot(); }); would consider the slots to be identical. It could be argued that, in this case, the caller should pass the non-unique connection flag though. > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > } > > > > > > void SignalBase::disconnect(Object *object)
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); } void SignalBase::disconnect(Object *object)
Slots are not expected to be connected to lots of signals. Our normal 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. Signed-off-by: Kieran Bingham <kieran.bingham@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. src/libcamera/base/signal.cpp | 8 ++++++++ 1 file changed, 8 insertions(+)