You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@qpid.apache.org by cc...@apache.org on 2011/03/11 01:37:04 UTC
svn commit: r1080411 - in /qpid/trunk/qpid/cpp/src/qpid/broker:
TopicExchange.cpp TopicExchange.h
Author: cctrieloff
Date: Fri Mar 11 00:37:04 2011
New Revision: 1080411
URL: http://svn.apache.org/viewvc?rev=1080411&view=rev
Log:
QPID-3138 Topic exchange perf improvement
Modified:
qpid/trunk/qpid/cpp/src/qpid/broker/TopicExchange.cpp
qpid/trunk/qpid/cpp/src/qpid/broker/TopicExchange.h
Modified: qpid/trunk/qpid/cpp/src/qpid/broker/TopicExchange.cpp
URL: http://svn.apache.org/viewvc/qpid/trunk/qpid/cpp/src/qpid/broker/TopicExchange.cpp?rev=1080411&r1=1080410&r2=1080411&view=diff
==============================================================================
--- qpid/trunk/qpid/cpp/src/qpid/broker/TopicExchange.cpp (original)
+++ qpid/trunk/qpid/cpp/src/qpid/broker/TopicExchange.cpp Fri Mar 11 00:37:04 2011
@@ -221,6 +221,7 @@ TopicExchange::TopicExchange(const std::
bool TopicExchange::bind(Queue::shared_ptr queue, const string& routingKey, const FieldTable* args)
{
+ ClearCache cc(&cacheLock,&bindingCache); // clear the cache on function exit.
string fedOp(args ? args->getAsString(qpidFedOp) : fedOpBind);
string fedTags(args ? args->getAsString(qpidFedTags) : "");
string fedOrigin(args ? args->getAsString(qpidFedOrigin) : "");
@@ -288,6 +289,7 @@ bool TopicExchange::bind(Queue::shared_p
}
bool TopicExchange::unbind(Queue::shared_ptr queue, const string& constRoutingKey, const FieldTable* /*args*/){
+ ClearCache cc(&cacheLock,&bindingCache); // clear the cache on function exit.
RWlock::ScopedWlock l(lock);
string routingKey = normalize(constRoutingKey);
BindingKey* bk = bindingTree.getBindingKey(routingKey);
@@ -333,13 +335,24 @@ bool TopicExchange::isBound(Queue::share
void TopicExchange::route(Deliverable& msg, const string& routingKey, const FieldTable* /*args*/)
{
// Note: PERFORMANCE CRITICAL!!!
- BindingList b(new std::vector<boost::shared_ptr<qpid::broker::Exchange::Binding> >);
+ BindingList b;
+ std::map<std::string, BindingList>::iterator it;
+ { // only lock the cache for read
+ RWlock::ScopedRlock cl(cacheLock);
+ it = bindingCache.find(routingKey);
+ }
PreRoute pr(msg, this);
- BindingsFinderIter bindingsFinder(b);
+ if (it == bindingCache.end()) // no cache hit
{
RWlock::ScopedRlock l(lock);
+ b = BindingList(new std::vector<boost::shared_ptr<qpid::broker::Exchange::Binding> >);
+ BindingsFinderIter bindingsFinder(b);
bindingTree.iterateMatch(routingKey, bindingsFinder);
- }
+ RWlock::ScopedWlock cwl(cacheLock);
+ bindingCache[routingKey] = b; // update cache
+ }else {
+ b = it->second;
+ }
doRoute(msg, b);
}
Modified: qpid/trunk/qpid/cpp/src/qpid/broker/TopicExchange.h
URL: http://svn.apache.org/viewvc/qpid/trunk/qpid/cpp/src/qpid/broker/TopicExchange.h?rev=1080411&r1=1080410&r2=1080411&view=diff
==============================================================================
--- qpid/trunk/qpid/cpp/src/qpid/broker/TopicExchange.h (original)
+++ qpid/trunk/qpid/cpp/src/qpid/broker/TopicExchange.h Fri Mar 11 00:37:04 2011
@@ -135,7 +135,19 @@ class TopicExchange : public virtual Exc
BindingNode bindingTree;
unsigned long nBindings;
qpid::sys::RWlock lock; // protects bindingTree and nBindings
-
+ qpid::sys::RWlock cacheLock; // protects cache
+ std::map<std::string, BindingList> bindingCache; // cache of matched routes.
+ class ClearCache {
+ private:
+ qpid::sys::RWlock* cacheLock;
+ std::map<std::string, BindingList>* bindingCache;
+ public:
+ ClearCache(qpid::sys::RWlock* l, std::map<std::string, BindingList>* bc): cacheLock(l),bindingCache(bc) {};
+ ~ClearCache(){
+ qpid::sys::RWlock::ScopedWlock l(*cacheLock);
+ bindingCache->clear();
+ };
+ };
bool isBound(Queue::shared_ptr queue, const std::string& pattern);
class ReOriginIter;
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:commits-subscribe@qpid.apache.org
Re: svn commit: r1080411 - in /qpid/trunk/qpid/cpp/src/qpid/broker:
TopicExchange.cpp TopicExchange.h
Posted by Carl Trieloff <cc...@redhat.com>.
On 03/14/2011 01:12 PM, Alan Conway wrote:
>>
>
> In general end() is not thread safe or constant. If you are using
> plain pointers as iterators over a C array, then end() is a pointer to
> one place after the last element of the array and changes if the array
> size changes. Probably many std container iterator implementations do
> something similar to keep the arithmetic simple.
I no longer rely on that behaviour in the latest patch, and I stand
corrected :-)
Carl.
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org
Re: svn commit: r1080411 - in /qpid/trunk/qpid/cpp/src/qpid/broker:
TopicExchange.cpp TopicExchange.h
Posted by Alan Conway <ac...@redhat.com>.
On 03/14/2011 10:41 AM, Andrew Stitcher wrote:
> On Mon, 2011-03-14 at 10:12 -0400, Carl Trieloff wrote:
>> On 03/14/2011 10:07 AM, Gordon Sim wrote:
>>> Its the iterator that is the issue, not the smart pointer that it
>>> points to. You test (it == bindingCache.end()) outside the lock and
>>> that is not safe.
>>
>> FYI -- .end() function is independent of the validity of the iterator.
>> 'it' will either == the const of end, or ref the smart pointer which
>> is then safe, even if cleared. I maintain it it safe.
>
> It's not clear to me from a quick squint at the standard whether
> map::end is required to be a constant valid irrespective of further
> changes to a map in a different thread so I can't say for sure who is
> correct here. However changing the code to use a temporary bool to hold
> the value of it==bindCache.end() inside the mutex makes the code
> uncontroversial and the compiler will optimise it away if it truly is a
> constant (well if it's defined in the header).
>
In general end() is not thread safe or constant. If you are using plain pointers
as iterators over a C array, then end() is a pointer to one place after the last
element of the array and changes if the array size changes. Probably many std
container iterator implementations do something similar to keep the arithmetic
simple.
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org
RE: svn commit: r1080411 - in /qpid/trunk/qpid/cpp/src/qpid/broker: TopicExchange.cpp TopicExchange.h
Posted by Steve Huston <sh...@riverace.com>.
> -----Original Message-----
> From: Andrew Stitcher [mailto:astitcher@redhat.com]
> ...
> On Mon, 2011-03-14 at 10:12 -0400, Carl Trieloff wrote:
> > On 03/14/2011 10:07 AM, Gordon Sim wrote:
> > > Its the iterator that is the issue, not the smart pointer that it
> > > points to. You test (it == bindingCache.end()) outside
> the lock and
> > > that is not safe.
> >
> > FYI -- .end() function is independent of the validity of the
> > iterator.
> > 'it' will either == the const of end, or ref the smart pointer which
> > is then safe, even if cleared. I maintain it it safe.
>
> It's not clear to me from a quick squint at the standard
> whether map::end is required to be a constant valid
> irrespective of further changes to a map in a different
> thread so I can't say for sure who is correct here. However
> changing the code to use a temporary bool to hold the value
> of it==bindCache.end() inside the mutex makes the code
> uncontroversial and the compiler will optimise it away if it
> truly is a constant (well if it's defined in the header).
And any section of code which inspires such spirited debate amongst
three heavy hitters could probably benefit from some explanatory
comments and maybe some clarification so us mere mortals won't get
tripped up later :-)
-Steve
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org
Re: svn commit: r1080411 - in
/qpid/trunk/qpid/cpp/src/qpid/broker: TopicExchange.cpp TopicExchange.h
Posted by Andrew Stitcher <as...@redhat.com>.
On Mon, 2011-03-14 at 10:12 -0400, Carl Trieloff wrote:
> On 03/14/2011 10:07 AM, Gordon Sim wrote:
> > Its the iterator that is the issue, not the smart pointer that it
> > points to. You test (it == bindingCache.end()) outside the lock and
> > that is not safe.
>
> FYI -- .end() function is independent of the validity of the iterator.
> 'it' will either == the const of end, or ref the smart pointer which
> is then safe, even if cleared. I maintain it it safe.
It's not clear to me from a quick squint at the standard whether
map::end is required to be a constant valid irrespective of further
changes to a map in a different thread so I can't say for sure who is
correct here. However changing the code to use a temporary bool to hold
the value of it==bindCache.end() inside the mutex makes the code
uncontroversial and the compiler will optimise it away if it truly is a
constant (well if it's defined in the header).
Andrew
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org
Re: svn commit: r1080411 - in /qpid/trunk/qpid/cpp/src/qpid/broker:
TopicExchange.cpp TopicExchange.h
Posted by Gordon Sim <gs...@redhat.com>.
On 03/14/2011 02:12 PM, Carl Trieloff wrote:
> On 03/14/2011 10:07 AM, Gordon Sim wrote:
>> Its the iterator that is the issue, not the smart pointer that it
>> points to. You test (it == bindingCache.end()) outside the lock and
>> that is not safe.
>
> FYI -- .end() function is independent of the validity of the iterator.
> 'it' will either == the const of end, or ref the smart pointer which
> is then safe, even if cleared. I maintain it it safe.
I maintain it is not and offer the attached test as evidence of that.
This reliably crashes the broker after your change but not when that
change is reverted.
Re: svn commit: r1080411 - in /qpid/trunk/qpid/cpp/src/qpid/broker:
TopicExchange.cpp TopicExchange.h
Posted by Carl Trieloff <cc...@redhat.com>.
On 03/14/2011 10:07 AM, Gordon Sim wrote:
> Its the iterator that is the issue, not the smart pointer that it
> points to. You test (it == bindingCache.end()) outside the lock and
> that is not safe.
FYI -- .end() function is independent of the validity of the iterator.
'it' will either == the const of end, or ref the smart pointer which
is then safe, even if cleared. I maintain it it safe.
I'll set it to a temp variable to make it more readable, but as written
I believe it is thread safe.
Carl.
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org
Re: svn commit: r1080411 - in /qpid/trunk/qpid/cpp/src/qpid/broker:
TopicExchange.cpp TopicExchange.h
Posted by Gordon Sim <gs...@redhat.com>.
On 03/14/2011 02:07 PM, Gordon Sim wrote:
> On 03/14/2011 01:59 PM, Carl Trieloff wrote:
>> On 03/11/2011 06:14 AM, Gordon Sim wrote:
>>>
>>> ..the iterator may be invalidated by a subsequent update to the map
>>> once the lock is released. Testing against bindingCache.end() outside
>>> the lock is also not safe.
>>
>> I don't believe so, as it holds a smart pointer, the smart pointer will
>> hold the memory from .second, so
>> even if the map is released the memory will remain safe. This is the
>> whole point of the code, that you
>> can free the map under the current route. the smart pointer protects the
>> memory.
>
> Its the iterator that is the issue, not the smart pointer that it points
> to. You test (it == bindingCache.end()) outside the lock and that is not
> safe.
The whitespace is also a bit of a mess, so if you're making changes it
would be nice to correct that also.
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org
Re: svn commit: r1080411 - in /qpid/trunk/qpid/cpp/src/qpid/broker:
TopicExchange.cpp TopicExchange.h
Posted by Gordon Sim <gs...@redhat.com>.
On 03/14/2011 01:59 PM, Carl Trieloff wrote:
> On 03/11/2011 06:14 AM, Gordon Sim wrote:
>>
>> ..the iterator may be invalidated by a subsequent update to the map
>> once the lock is released. Testing against bindingCache.end() outside
>> the lock is also not safe.
>
> I don't believe so, as it holds a smart pointer, the smart pointer will
> hold the memory from .second, so
> even if the map is released the memory will remain safe. This is the
> whole point of the code, that you
> can free the map under the current route. the smart pointer protects the
> memory.
Its the iterator that is the issue, not the smart pointer that it points
to. You test (it == bindingCache.end()) outside the lock and that is not
safe.
> So the commit is thread safe.
>
> I can assign second to a local smart pointer under the lock to make it
> more readable, but the code is
> thread safe.
>
> Carl.
>
> ---------------------------------------------------------------------
> Apache Qpid - AMQP Messaging Implementation
> Project: http://qpid.apache.org
> Use/Interact: mailto:dev-subscribe@qpid.apache.org
>
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org
Re: svn commit: r1080411 - in /qpid/trunk/qpid/cpp/src/qpid/broker:
TopicExchange.cpp TopicExchange.h
Posted by Carl Trieloff <cc...@redhat.com>.
On 03/11/2011 06:14 AM, Gordon Sim wrote:
>
> ..the iterator may be invalidated by a subsequent update to the map
> once the lock is released. Testing against bindingCache.end() outside
> the lock is also not safe.
I don't believe so, as it holds a smart pointer, the smart pointer will
hold the memory from .second, so
even if the map is released the memory will remain safe. This is the
whole point of the code, that you
can free the map under the current route. the smart pointer protects the
memory.
So the commit is thread safe.
I can assign second to a local smart pointer under the lock to make it
more readable, but the code is
thread safe.
Carl.
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org
Re: svn commit: r1080411 - in /qpid/trunk/qpid/cpp/src/qpid/broker:
TopicExchange.cpp TopicExchange.h
Posted by Gordon Sim <gs...@redhat.com>.
On 03/11/2011 12:37 AM, cctrieloff@apache.org wrote:
> Author: cctrieloff
> Date: Fri Mar 11 00:37:04 2011
> New Revision: 1080411
>
> URL: http://svn.apache.org/viewvc?rev=1080411&view=rev
> Log:
> QPID-3138 Topic exchange perf improvement
>
> Modified:
> qpid/trunk/qpid/cpp/src/qpid/broker/TopicExchange.cpp
> qpid/trunk/qpid/cpp/src/qpid/broker/TopicExchange.h
This commit causes a test failure:
> 0% 10 20 30 40 50 60 70 80 90 100%
> |----|----|----|----|----|----|----|----|----|----|
> Running 318 test cases...
> *********************../../../src/tests/ExchangeTest.cpp(282): error in "testIVEOption": check 1u == queue3->getMessageCount() failed [1 != 0]
> ******************************
>
> *** 1 failure detected in test suite "Master Test Suite"
> FAIL: unit_test
I believe it is also unsafe. In the route method...
> Modified: qpid/trunk/qpid/cpp/src/qpid/broker/TopicExchange.cpp
> URL: http://svn.apache.org/viewvc/qpid/trunk/qpid/cpp/src/qpid/broker/TopicExchange.cpp?rev=1080411&r1=1080410&r2=1080411&view=diff
> ==============================================================================
> --- qpid/trunk/qpid/cpp/src/qpid/broker/TopicExchange.cpp (original)
> +++ qpid/trunk/qpid/cpp/src/qpid/broker/TopicExchange.cpp Fri Mar 11 00:37:04 2011
> @@ -221,6 +221,7 @@ TopicExchange::TopicExchange(const std::
>
> bool TopicExchange::bind(Queue::shared_ptr queue, const string& routingKey, const FieldTable* args)
> {
> + ClearCache cc(&cacheLock,&bindingCache); // clear the cache on function exit.
> string fedOp(args ? args->getAsString(qpidFedOp) : fedOpBind);
> string fedTags(args ? args->getAsString(qpidFedTags) : "");
> string fedOrigin(args ? args->getAsString(qpidFedOrigin) : "");
> @@ -288,6 +289,7 @@ bool TopicExchange::bind(Queue::shared_p
> }
>
> bool TopicExchange::unbind(Queue::shared_ptr queue, const string& constRoutingKey, const FieldTable* /*args*/){
> + ClearCache cc(&cacheLock,&bindingCache); // clear the cache on function exit.
> RWlock::ScopedWlock l(lock);
> string routingKey = normalize(constRoutingKey);
> BindingKey* bk = bindingTree.getBindingKey(routingKey);
> @@ -333,13 +335,24 @@ bool TopicExchange::isBound(Queue::share
> void TopicExchange::route(Deliverable& msg, const string& routingKey, const FieldTable* /*args*/)
> {
> // Note: PERFORMANCE CRITICAL!!!
> - BindingList b(new std::vector<boost::shared_ptr<qpid::broker::Exchange::Binding> >);
> + BindingList b;
> + std::map<std::string, BindingList>::iterator it;
> + { // only lock the cache for read
> + RWlock::ScopedRlock cl(cacheLock);
> + it = bindingCache.find(routingKey);
> + }
> PreRoute pr(msg, this);
> - BindingsFinderIter bindingsFinder(b);
> + if (it == bindingCache.end()) // no cache hit
..the iterator may be invalidated by a subsequent update to the map once
the lock is released. Testing against bindingCache.end() outside the
lock is also not safe.
> {
> RWlock::ScopedRlock l(lock);
> + b = BindingList(new std::vector<boost::shared_ptr<qpid::broker::Exchange::Binding> >);
> + BindingsFinderIter bindingsFinder(b);
> bindingTree.iterateMatch(routingKey, bindingsFinder);
> - }
> + RWlock::ScopedWlock cwl(cacheLock);
> + bindingCache[routingKey] = b; // update cache
> + }else {
> + b = it->second;
> + }
> doRoute(msg, b);
> }
>
>
> Modified: qpid/trunk/qpid/cpp/src/qpid/broker/TopicExchange.h
> URL: http://svn.apache.org/viewvc/qpid/trunk/qpid/cpp/src/qpid/broker/TopicExchange.h?rev=1080411&r1=1080410&r2=1080411&view=diff
> ==============================================================================
> --- qpid/trunk/qpid/cpp/src/qpid/broker/TopicExchange.h (original)
> +++ qpid/trunk/qpid/cpp/src/qpid/broker/TopicExchange.h Fri Mar 11 00:37:04 2011
> @@ -135,7 +135,19 @@ class TopicExchange : public virtual Exc
> BindingNode bindingTree;
> unsigned long nBindings;
> qpid::sys::RWlock lock; // protects bindingTree and nBindings
> -
> + qpid::sys::RWlock cacheLock; // protects cache
> + std::map<std::string, BindingList> bindingCache; // cache of matched routes.
> + class ClearCache {
> + private:
> + qpid::sys::RWlock* cacheLock;
> + std::map<std::string, BindingList>* bindingCache;
> + public:
> + ClearCache(qpid::sys::RWlock* l, std::map<std::string, BindingList>* bc): cacheLock(l),bindingCache(bc) {};
> + ~ClearCache(){
> + qpid::sys::RWlock::ScopedWlock l(*cacheLock);
> + bindingCache->clear();
> + };
> + };
> bool isBound(Queue::shared_ptr queue, const std::string& pattern);
>
> class ReOriginIter;
>
>
>
> ---------------------------------------------------------------------
> Apache Qpid - AMQP Messaging Implementation
> Project: http://qpid.apache.org
> Use/Interact: mailto:commits-subscribe@qpid.apache.org
>
>
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org