-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9698/#review17243
-----------------------------------------------------------
I would remove the bindingLock from Exchange and put it in each subclass.
Exchange is more an interface than an implementation. Its possible that
future bindings might use different locking strategies as they do now.
This change doesn't itself address atomic binding. I think for that you need
virtual Exchange::rebind(old-info, new-info) = 0
virtual FooExchange::rebind(old, new) { wlock{ unbind(old); bind(new);} }
The lock scopes will have to change. The rlock in route() has to cover the call
to doRoute, otherwise you can't do atomic rebind. You could get this sequence:
thread1 route(): rlock{ lookup binding, find x}; doRoute(x,
lost!!)
thread2 rebind(): wlock{ unbind(x); rebind(y)}
Also we have to refactor the existing bind/unbind to have unlocked versions that
can be called by rebind under a single lock.
So the lock scopes will grow quite a bit. This may not be a disaster since we're
now using RW locks so routes can proceed in parallel. We only pay when we change
the bindings. We definitely need before/after performance tests though:
1. throughput/latency of many senders to 1 exchange
2. 1. + rapidly changing bindings concurrently.
Typo in DirectExchange::route, it uses a wlock
Once we have the RW locks we don't need CopyOnWrite array, so we should take it
out.
The approach in principle seems sound, so its worth a try.
We just need to be careful to measure the performance impact.
- Alan Conway
On March 1, 2013, 3:30 p.m., Chug Rolke wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9698/
> -----------------------------------------------------------
>
> (Updated March 1, 2013, 3:30 p.m.)
>
>
> Review request for qpid, Andrew Stitcher, Alan Conway, Gordon Sim, and Ted
> Ross.
>
>
> Description
> -------
>
> * Move Topic exchange RWlock up into the parent exchange class and call it
> 'bindingLock'.
>
> * In Topic exchange take RWlock out of the derived class and use the parent's
> lock.
> * In Direct and Headers exchanges change the semantics of the current lock to
> use the Topic exchange pattern.
> * In FanOut exchange add locks at the same places as in the other exchanges
> except only use Rlock. FanOut receives the locking it needs from the
> CopyOnWriteArray and does not need anything else. By adding only Rlocks
> threads will never block in the normal case. However, the Rlocks are the
> hooks that the new application will need to freeze the exchange when that app
> takes out the Wlock.
>
>
> This addresses bug QPID-4616.
> https://issues.apache.org/jira/browse/QPID-4616
>
>
> Diffs
> -----
>
> trunk/qpid/cpp/src/qpid/broker/DirectExchange.h 1451602
> trunk/qpid/cpp/src/qpid/broker/DirectExchange.cpp 1451602
> trunk/qpid/cpp/src/qpid/broker/Exchange.h 1451602
> trunk/qpid/cpp/src/qpid/broker/FanOutExchange.cpp 1451602
> trunk/qpid/cpp/src/qpid/broker/HeadersExchange.h 1451602
> trunk/qpid/cpp/src/qpid/broker/HeadersExchange.cpp 1451602
> trunk/qpid/cpp/src/qpid/broker/TopicExchange.h 1451602
> trunk/qpid/cpp/src/qpid/broker/TopicExchange.cpp 1451602
>
> Diff: https://reviews.apache.org/r/9698/diff/
>
>
> Testing
> -------
>
> Passes normal self tests
>
>
> Thanks,
>
> Chug Rolke
>
>
