From ebda6bf0c526d3b6440b30a057ded26c9143097d Mon Sep 17 00:00:00 2001 From: Paul Davis Date: Wed, 16 Apr 2025 09:09:57 -0600 Subject: [PATCH] rework PBD::Signal emission code to avoid memory allocation We now use a stack allocator when making a copy of current connection state at the start of the signal emission process, and when collecting results from signal handlers in the case of a non-void return type. These changes also include a functionally neutral reworking of how the connection state copy is made and then used to check that a connection/handler is still valid mid-emission. Heap allocation will still happen if a signal has more than (currently) 512 connections. A little experimentation reveals that the maximum number of connections is typically nroutes+1, so 512 seems like a reasonable choice for this. --- libs/pbd/pbd/signals.h | 81 +++++++++++++++++++++++++++++++----------- 1 file changed, 61 insertions(+), 20 deletions(-) diff --git a/libs/pbd/pbd/signals.h b/libs/pbd/pbd/signals.h index 286c3d5cf1..81372ae521 100644 --- a/libs/pbd/pbd/signals.h +++ b/libs/pbd/pbd/signals.h @@ -40,6 +40,7 @@ #include "pbd/libpbd_visibility.h" #include "pbd/event_loop.h" +#include "pbd/stack_allocator.h" #ifndef NDEBUG #define DEBUG_PBD_SIGNAL_CONNECTIONS @@ -51,10 +52,15 @@ #include #endif + using namespace std::placeholders; namespace PBD { +#ifdef DEBUG_PBD_SIGNAL_CONNECTIONS +static std::size_t max_signal_subscribers; +#endif + class LIBPBD_API Connection; class LIBPBD_API ScopedConnection; @@ -427,40 +433,65 @@ template typename std::conditional_t, R, typename Combiner::result_type> SignalWithCombiner::operator() (A... a) { - /* First, take a copy of our list of slots as it is now */ #ifdef DEBUG_PBD_SIGNAL_EMISSION if (_debug_emission) { std::cerr << "------ Signal @ " << this << " emission process begins\n"; PBD::stacktrace (std::cerr, 19); } #endif - Slots s; + const std::size_t nslots = 512; + std::vector > s; + + /* First, make a copy of the current connection state for us to iterate + * over later (the connection state may be changed by a signal handler. + */ + { Glib::Threads::Mutex::Lock lm (_mutex); - s = _slots; + /* copy only the raw pointer, no need for a shared_ptr in this + * context, we only use the address as a lookup into the _slots + * container. Note: because of the use of a stack allocator, + * this is *unlikely* to cause any (heap) memory + * allocation. That will only happen if the number of + * connections to this signal exceeds the value of nslots + * defined above. As of April 2025, the maximum number of + * connections appears to be ntracks+1. + */ + for (auto const & [connection,functor] : _slots) { + s.push_back (connection.get()); + } } if constexpr (std::is_void_v) { - for (typename Slots::const_iterator i = s.begin(); i != s.end(); ++i) { + slot_function_type functor; - /* We may have just called a slot, and this may have resulted in - * disconnection of other slots from us. The list copy means that - * this won't cause any problems with invalidated iterators, but we - * must check to see if the slot we are about to call is still on the list. - */ + for (auto const & c : s) { + + /* We may have just called a slot, and this may have + * resulted in disconnection of other slots from us. + * The list copy means that this won't cause any + * problems with invalidated iterators, but we must + * check to see if the slot we are about to call is + * still on the list. + */ bool still_there = false; + { Glib::Threads::Mutex::Lock lm (_mutex); - still_there = _slots.find (i->first) != _slots.end (); + typename Slots::const_iterator f = std::find_if (_slots.begin(), _slots.end(), [&](typename Slots::value_type const & elem) { return elem.first.get() == c; }); + if (f != _slots.end()) { + functor = f->second; + still_there = true; + } } if (still_there) { #ifdef DEBUG_PBD_SIGNAL_EMISSION if (_debug_emission) { - std::cerr << "signal @ " << this << " calling slot for connection @ " << i->first << " of " << _slots.size() << std::endl; + std::cerr << "signal @ " << this << " calling slot for connection @ " << c << " of " << _slots.size() << std::endl; } #endif - (i->second)(a...); + functor (a...); } } @@ -472,9 +503,10 @@ SignalWithCombiner::operator() (A... a) return; } else { + std::list > r; + slot_function_type functor; - std::list r; - for (typename Slots::const_iterator i = s.begin(); i != s.end(); ++i) { + for (auto const & c : s) { /* We may have just called a slot, and this may have resulted in * disconnection of other slots from us. The list copy means that @@ -482,18 +514,23 @@ SignalWithCombiner::operator() (A... a) * must check to see if the slot we are about to call is still on the list. */ bool still_there = false; + { Glib::Threads::Mutex::Lock lm (_mutex); - still_there = _slots.find (i->first) != _slots.end (); - } + typename Slots::const_iterator f = std::find_if (_slots.begin(), _slots.end(), [&](typename Slots::value_type const & elem) { return elem.first.get() == c; }); + if (f != _slots.end()) { + functor = f->second; + still_there = true; + } + } if (still_there) { #ifdef DEBUG_PBD_SIGNAL_EMISSION if (_debug_emission) { - std::cerr << "signal @ " << this << " calling non-void slot for connection @ " << i->first << " of " << _slots.size() << std::endl; + std::cerr << "signal @ " << this << " calling non-void slot for connection @ " << c << " of " << _slots.size() << std::endl; } #endif - r.push_back ((i->second)(a...)); + r.push_back (functor (a...)); } #ifdef DEBUG_PBD_SIGNAL_EMISSION @@ -516,12 +553,16 @@ SignalWithCombiner::_connect (PBD::EventLoop::InvalidationRec std::shared_ptr c (new Connection (this, ir)); Glib::Threads::Mutex::Lock lm (_mutex); _slots[c] = f; - #ifdef DEBUG_PBD_SIGNAL_CONNECTIONS + +#ifdef DEBUG_PBD_SIGNAL_CONNECTIONS + if (_slots.size() > max_signal_subscribers) { + max_signal_subscribers = _slots.size(); + } if (_debug_connection) { std::cerr << "+++++++ CONNECT " << this << " via connection @ " << c << " size now " << _slots.size() << std::endl; stacktrace (std::cerr, 10); } - #endif +#endif return c; }