> On Dec. 13, 2013, 4:53 p.m., Gordon Sim wrote: > > /trunk/qpid/cpp/src/qpid/broker/Observers.h, line 55 > > <https://reviews.apache.org/r/16245/diff/1/?file=397491#file397491line55> > > > > For queue 'events' this would take a copy of all observers on each such > > event, whereas before this the observers would have been iterated over > > under a lock, right? > > > > This means that we copy the set for every enqueue and dequeue for > > example. Is there any concern that would affect performance? > > > > Could we instead have two versions of each, one where the lock is > > passed in, avoiding the need to copy? If I'm not mistaken, it is held in > > any case across the each() call for enqueue/dequeue.
What a good idea :) If you look further down you'll see there is a protected version of each() which does iterate under the lock, this is the one used by QueueObservers, so the locking behaviour should be identical. The copy version of each() is used by the other observers (BrokerObserver, ConnectionObserver) which was the case before this change. - Alan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/16245/#review30344 ----------------------------------------------------------- On Dec. 13, 2013, 3:16 p.m., Alan Conway wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/16245/ > ----------------------------------------------------------- > > (Updated Dec. 13, 2013, 3:16 p.m.) > > > Review request for qpid and Gordon Sim. > > > Repository: qpid > > > Description > ------- > > NO-JIRA: Refactor: clean up QueeuObservers. > > Refactor of queue observers to use the broker::Observers base class. > Simplifies Queue code > and makes it more consistent with other observers (BrokerObservers, > ConnectionObservers.) > > Modified Observers base class to allow identical locking behaviour to previous > impementation. > > > Diffs > ----- > > /trunk/qpid/cpp/src/qpid/broker/BrokerObservers.h 1550190 > /trunk/qpid/cpp/src/qpid/broker/ConnectionObservers.h 1550190 > /trunk/qpid/cpp/src/qpid/broker/Observers.h 1550190 > /trunk/qpid/cpp/src/qpid/broker/Queue.h 1550190 > /trunk/qpid/cpp/src/qpid/broker/Queue.cpp 1550190 > /trunk/qpid/cpp/src/qpid/broker/QueueFactory.cpp 1550190 > /trunk/qpid/cpp/src/qpid/broker/QueueFlowLimit.cpp 1550190 > /trunk/qpid/cpp/src/qpid/broker/QueueObservers.h PRE-CREATION > /trunk/qpid/cpp/src/qpid/broker/ThresholdAlerts.cpp 1550190 > /trunk/qpid/cpp/src/qpid/ha/QueueGuard.cpp 1550190 > /trunk/qpid/cpp/src/qpid/ha/QueueReplicator.cpp 1550190 > /trunk/qpid/cpp/src/qpid/ha/QueueSnapshots.h 1550190 > /trunk/qpid/cpp/src/qpid/ha/ReplicatingSubscription.cpp 1550190 > > Diff: https://reviews.apache.org/r/16245/diff/ > > > Testing > ------- > > Full ctest > > > Thanks, > > Alan Conway > >
