rework request invalidation
authorRobin Gareus <robin@gareus.org>
Thu, 15 Dec 2016 05:11:20 +0000 (06:11 +0100)
committerRobin Gareus <robin@gareus.org>
Thu, 15 Dec 2016 05:11:30 +0000 (06:11 +0100)
This kills 2 birds with 1 stone: Removes the necessity of locks
and makes call_slot() realtime safe (req->invalidation->requests list
push_back). On object destruction, the invalidation-record (IR) itself is
invalidated.

Invalidated IRs are pushed onto a trash-pool and deleted in the event-loop
of the invalidated object (GUI thread) once all requests that reference it
have been processed.

One last detail remains: PBD::signal connect should reference the IR
and disconnect unreference it. This will guarantee that signal emission
will not reference the IR while the pool trash is dropped.

libs/pbd/event_loop.cc
libs/pbd/pbd/abstract_ui.cc
libs/pbd/pbd/event_loop.h

index aae0c21a08cf740d8f0a33f91fa47a8e3401bf2b..f72255dd7296b868c23e39016a7f6c7dd05f1e76 100644 (file)
@@ -88,18 +88,9 @@ EventLoop::invalidate_request (void* data)
 
        if (ir->event_loop) {
                DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("%1: EventLoop::invalidate_request %2\n", ir->event_loop, ir));
-               {
-                       Glib::Threads::Mutex::Lock lm (ir->event_loop->slot_invalidation_mutex());
-                       for (list<BaseRequestObject*>::iterator i = ir->requests.begin(); i != ir->requests.end(); ++i) {
-                               (*i)->invalidate ();
-                               (*i)->invalidation = 0;
-                       }
-               }
-               // This invalidation record may still be in-use in per-thread-request-ringbuffer.
-               // it cannot be deleted here,
+               Glib::Threads::Mutex::Lock lm (ir->event_loop->slot_invalidation_mutex());
+               ir->invalidate ();
                ir->event_loop->trash.push_back(ir);
-       } else {
-               DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("EventLoop::invalidate_request no event-loop for invalidation %1\n", ir));
        }
 
        return 0;
index db7ce38a6460771ba70072ceb630c8b87a3c484c..fcbf298020987bbb2ee017afea809f65d44ae163 100644 (file)
@@ -168,7 +168,6 @@ AbstractUI<RequestObject>::get_request (RequestType rt)
                DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("%1: allocated per-thread request of type %2, caller %3\n", event_loop_name(), rt, pthread_name()));
 
                vec.buf[0]->type = rt;
-               vec.buf[0]->validate ();
                return vec.buf[0];
        }
 
@@ -194,11 +193,30 @@ AbstractUI<RequestObject>::handle_ui_requests ()
        /* check all registered per-thread buffers first */
        Glib::Threads::Mutex::Lock rbml (request_buffer_map_lock);
 
+       /* clean up any dead invalidation records (object was deleted) */
+       trash.sort();
+       trash.unique();
+       for (std::list<InvalidationRecord*>::const_iterator r = trash.begin(); r != trash.end();) {
+               if (!(*r)->in_use ()) {
+                       assert (!(*r)->valid ());
+                       DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("%1 drop invalidation trash %2\n", event_loop_name(), *r));
+                       delete *r;
+                       r = trash.erase (r);
+               } else {
+                       ++r;
+               }
+       }
+#ifndef NDEBUG
+       if (trash.size() > 0) {
+               DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("%1 items in trash: %2\n", event_loop_name(), trash.size()));
+       }
+#endif
+
        DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("%1 check %2 request buffers for requests\n", event_loop_name(), request_buffers.size()));
 
        for (i = request_buffers.begin(); i != request_buffers.end(); ++i) {
 
-               while (true) {
+               while (!(*i).second->dead) {
 
                        /* we must process requests 1 by 1 because
                         * the request may run a recursive main
@@ -213,57 +231,38 @@ AbstractUI<RequestObject>::handle_ui_requests ()
                        i->second->get_read_vector (&vec);
 
                        DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("%1 reading requests from RB[%2] @ %5, requests = %3 + %4\n",
-                                                                            event_loop_name(), std::distance (request_buffers.begin(), i), vec.len[0], vec.len[1], i->second));
+                                               event_loop_name(), std::distance (request_buffers.begin(), i), vec.len[0], vec.len[1], i->second));
 
                        if (vec.len[0] == 0) {
                                break;
                        } else {
-                               bool alive = true;
-                               if (vec.buf[0]->valid ()) {
-                                       /* We first need to remove the event from the list.
-                                        * If the event results in object destruction, PBD::EventLoop::invalidate_request
-                                        * will delete the invalidation record (aka buf[0]), so we cannot use it after calling do_request
-                                        */
-                                       DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("%1: remove request %2 from its invalidation list %3\n", event_loop_name(), vec.buf[0], vec.buf[0]->invalidation));
-                                       if (vec.buf[0]->invalidation) {
-                                               alive = std::find (trash.begin(), trash.end(), vec.buf[0]->invalidation) == trash.end();
-                                               DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("%1: removing invalidation record for that request\n", event_loop_name()));
-                                               if (vec.buf[0]->invalidation->event_loop && vec.buf[0]->invalidation->event_loop != this) {
-                                                       vec.buf[0]->invalidation->event_loop->slot_invalidation_mutex().lock ();
-                                               }
-                                               vec.buf[0]->invalidation->requests.remove (vec.buf[0]);
-                                               if (vec.buf[0]->invalidation->event_loop && vec.buf[0]->invalidation->event_loop != this) {
-                                                       vec.buf[0]->invalidation->event_loop->slot_invalidation_mutex().unlock ();
-                                               }
-                                       } else {
-                                               DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("%1: no invalidation record for that request\n", event_loop_name()));
-                                       }
+                               if (vec.buf[0]->invalidation && !vec.buf[0]->invalidation->valid ()) {
+                                       DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("%1: skipping invalidated request\n", event_loop_name()));
+                                       rbml.release ();
+                               } else {
 
                                        DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("%1: valid request, unlocking before calling\n", event_loop_name()));
                                        rbml.release ();
 
-                                       if (alive) {
-                                               DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("%1: valid request, calling ::do_request()\n", event_loop_name()));
-                                               do_request (vec.buf[0]);
-                                       } else {
-                                               DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("%1: skipping invalidated request\n", event_loop_name()));
-                                       }
-
-                                       /* if the request was CallSlot, then we need to ensure that we reset the functor in the request, in case it
-                                        * held a shared_ptr<>. Failure to do so can lead to dangling references to objects passed to PBD::Signals.
-                                        *
-                                        * Note that this method (::handle_ui_requests()) is by definition called from the event loop thread, so
-                                        * caller_is_self() is true, which means that the execution of the functor has definitely happened after
-                                        * do_request() returns and we no longer need the functor for any reason.
-                                        */
-
-                                       if (vec.buf[0]->type == CallSlot) {
-                                               vec.buf[0]->the_slot = 0;
-                                       }
-
-                                       rbml.acquire ();
-                               } else {
-                                       DEBUG_TRACE (PBD::DEBUG::AbstractUI, "invalid request, ignoring\n");
+                                       DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("%1: valid request, calling ::do_request()\n", event_loop_name()));
+                                       do_request (vec.buf[0]);
+                               }
+
+                               /* if the request was CallSlot, then we need to ensure that we reset the functor in the request, in case it
+                                * held a shared_ptr<>. Failure to do so can lead to dangling references to objects passed to PBD::Signals.
+                                *
+                                * Note that this method (::handle_ui_requests()) is by definition called from the event loop thread, so
+                                * caller_is_self() is true, which means that the execution of the functor has definitely happened after
+                                * do_request() returns and we no longer need the functor for any reason.
+                                */
+
+                               if (vec.buf[0]->type == CallSlot) {
+                                       vec.buf[0]->the_slot = 0;
+                               }
+
+                               rbml.acquire ();
+                               if (vec.buf[0]->invalidation) {
+                                       vec.buf[0]->invalidation->unref ();
                                }
                                i->second->increment_read_ptr (1);
                        }
@@ -273,13 +272,17 @@ AbstractUI<RequestObject>::handle_ui_requests ()
        assert (rbml.locked ());
        for (i = request_buffers.begin(); i != request_buffers.end(); ) {
                if ((*i).second->dead) {
-                       DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("%1/%2 deleting dead per-thread request buffer for %3 @ %4\n",
-                                                                            event_loop_name(), pthread_name(), i->second));
+                       DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("%1/%2 deleting dead per-thread request buffer for %3 @ %4 (%5 requests)\n", event_loop_name(), pthread_name(), i->second, (*i).second->read_space()));
                        RequestBufferMapIterator tmp = i;
                        ++tmp;
                        /* remove it from the EventLoop static map of all request buffers */
                        EventLoop::remove_request_buffer_from_map ((*i).second);
-                       /* delete it */
+                       /* delete it
+                        *
+                        * Deleting the ringbuffer destroys all RequestObjects
+                        * and thereby drops any InvalidationRecord references of
+                        * requests that have not been processed.
+                        */
                        delete (*i).second;
                        /* remove it from this thread's list of request buffers */
                        request_buffers.erase (i);
@@ -301,37 +304,7 @@ AbstractUI<RequestObject>::handle_ui_requests ()
                 * the request as "done" before we start.
                 */
 
-               if (req->invalidation) {
-                       DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("%1/%2 remove request %3 from its invalidation list %4\n", event_loop_name(), pthread_name(), req, req->invalidation));
-
-                       Glib::Threads::Mutex::Lock li (req->invalidation->event_loop->slot_invalidation_mutex(), Glib::Threads::NOT_LOCK);
-                       if (req->invalidation->event_loop && req->invalidation->event_loop != this) {
-                               li.acquire ();
-                       }
-
-                       /* after this call, if the object referenced by the
-                        * invalidation record is deleted, it will no longer
-                        * try to mark the request as invalid.
-                        */
-
-                       req->invalidation->requests.remove (req);
-
-                       /* req->invalidation->requests is identical to
-                        * EventLoop::invalidate_request()'s ir->requests.
-                        *
-                        * However EventLoop::invalidate_request() cannot
-                        * run in parallel to this, because sigc slots are always
-                        * called in the same thread as signal emission (this event-loop).
-                        * So we do not need to expose "request_invalidation_lock",
-                        * and only provide a safeguard for modifications to the
-                        * container itself.
-                        */
-                       if (req->invalidation->event_loop && req->invalidation->event_loop != this) {
-                               li.release ();
-                       }
-               }
-
-               if (!req->valid ()) {
+               if (req->invalidation && !req->invalidation->valid()) {
                        DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("%1/%2 handling invalid heap request, type %3, deleting\n", event_loop_name(), pthread_name(), req->type));
                        delete req;
                        continue;
@@ -374,15 +347,6 @@ AbstractUI<RequestObject>::handle_ui_requests ()
                rbml.acquire();
        }
 
-       /* clean up any dead invalidation records (object was deleted) */
-       trash.sort();
-       trash.unique();
-       for (std::list<InvalidationRecord*>::const_iterator r = trash.begin(); r != trash.end(); ++r) {
-               DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("%1 drop invalidation trash %2\n", event_loop_name(), *r));
-               delete *r;
-       }
-       trash.clear ();
-
        rbml.release ();
 }
 
@@ -395,6 +359,7 @@ AbstractUI<RequestObject>::send_request (RequestObject *req)
         */
 
        if (base_instance() == 0) {
+               delete req;
                return; /* XXX is this the right thing to do ? */
        }
 
@@ -422,13 +387,13 @@ AbstractUI<RequestObject>::send_request (RequestObject *req)
                RequestBuffer* rbuf = per_thread_request_buffer.get ();
 
                if (rbuf != 0) {
-                       DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("%1/%2 send per-thread request type %3 using ringbuffer @ %4\n", event_loop_name(), pthread_name(), req->type, rbuf));
+                       DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("%1/%2 send per-thread request type %3 using ringbuffer @ %4 IR: %5\n", event_loop_name(), pthread_name(), req->type, rbuf, req->invalidation));
                        rbuf->increment_write_ptr (1);
                } else {
                        /* no per-thread buffer, so just use a list with a lock so that it remains
-                          single-reader/single-writer semantics
-                       */
-                       DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("%1/%2 send heap request type %3\n", event_loop_name(), pthread_name(), req->type));
+                        * single-reader/single-writer semantics
+                        */
+                       DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("%1/%2 send heap request type %3 IR %4\n", event_loop_name(), pthread_name(), req->type, req->invalidation));
                        Glib::Threads::Mutex::Lock lm (request_buffer_map_lock);
                        request_list.push_back (req);
                }
@@ -450,9 +415,23 @@ AbstractUI<RequestObject>::call_slot (InvalidationRecord* invalidation, const bo
                return;
        }
 
+       if (invalidation) {
+               Glib::Threads::Mutex::Lock lm (request_buffer_map_lock); //  -- remove this once signal connect/disconnect uses ir->un/ref()
+               if (!invalidation->valid()) {
+                       DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("%1/%2 ignoring call-slot using functor @ %3, dead invalidation %4\n", event_loop_name(), pthread_name(), &f, invalidation));
+                       return;
+               }
+               invalidation->ref ();
+               assert (invalidation->event_loop == this);
+               invalidation->event_loop = this; // XXX is this needed,  PBD::signal::connect sets it
+       }
+
        RequestObject *req = get_request (BaseUI::CallSlot);
 
        if (req == 0) {
+               if (invalidation) {
+                       invalidation->unref ();
+               }
                return;
        }
 
@@ -472,24 +451,6 @@ AbstractUI<RequestObject>::call_slot (InvalidationRecord* invalidation, const bo
 
        req->invalidation = invalidation;
 
-       if (invalidation) {
-               /* We're about to modify a std::list<> container, this must not
-                * happen concurrently with other container modifications
-                * (invalidation->requests.remove in handle_ui_requests).
-                *
-                * Let's take a dedicated lock that is less contended than
-                * request_buffer_map_lock.
-                *
-                * This works because object's d'tor disconnect signals
-                * (member variable) before the sigc::trackable's destroy_notify
-                * runs. So there is no race between signal-emissions/call_slot()
-                * and adding new requests after the object has been invalidated.
-                */
-               Glib::Threads::Mutex::Lock lm (request_buffer_map_lock);
-               invalidation->requests.push_back (req);
-               invalidation->event_loop = this;
-       }
-
        send_request (req);
 }
 
index e84832ebcba71fae5022250d8e23819a35cb03d8..09265c13d85b3ded97cf4e9658d35183e250b255 100644 (file)
@@ -58,25 +58,33 @@ public:
        struct InvalidationRecord {
                std::list<BaseRequestObject*> requests;
                PBD::EventLoop* event_loop;
+               gint _valid;
+               gint _ref;
                const char* file;
                int line;
 
-               InvalidationRecord() : event_loop (0) {}
+               InvalidationRecord() : event_loop (0), _valid (1), _ref (0) {}
+               void invalidate () { g_atomic_int_set (&_valid, 0); }
+               bool valid () { return g_atomic_int_get (&_valid) == 1; }
+
+               void ref ()    { g_atomic_int_inc (&_ref); }
+               void unref ()  { g_atomic_int_dec_and_test (&_ref); }
+               bool in_use () { return g_atomic_int_get (&_ref) > 0; }
        };
 
        static void* invalidate_request (void* data);
 
        struct BaseRequestObject {
                RequestType             type;
-               gint                    _valid;
                InvalidationRecord*     invalidation;
                boost::function<void()> the_slot;
 
-               BaseRequestObject() : _valid (0), invalidation (0) {}
-
-               void validate () { g_atomic_int_set (&_valid, 1); }
-               void invalidate () { g_atomic_int_set (&_valid, 0); }
-               bool valid () { return g_atomic_int_get (&_valid) == 1; }
+               BaseRequestObject() : invalidation (0) {}
+               ~BaseRequestObject() {
+                       if (invalidation) {
+                               invalidation->unref ();
+                       }
+               }
        };
 
        virtual void call_slot (InvalidationRecord*, const boost::function<void()>&) = 0;