[jira] [Commented] (AMQ-6226) Eliminate AbstractRegion.destinationsLock

["Graeme Moss (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/AMQ-6226?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16916680#comment-16916680 ] 

Graeme Moss commented on AMQ-6226:
----------------------------------

I agree that this is a significant performance issue.  In particular, in the vast majority of cases we see that there is a single thread holding this "destinationsLock" whilst checking whether existing subscriptions apply to the new destination, e.g.
{code:none}
"ActiveMQ NIO Worker 230" #726 daemon prio=5 os_prio=0 tid=0x00007fb70c13b000 nid=0x231d runnable [0x00007fb482be9000]
   java.lang.Thread.State: RUNNABLE
        at org.apache.activemq.broker.region.AbstractSubscription.matches(AbstractSubscription.java:123)
        at org.apache.activemq.broker.region.AbstractRegion.addSubscriptionsForDestination(AbstractRegion.java:241)
        at org.apache.activemq.broker.region.TopicRegion.addSubscriptionsForDestination(TopicRegion.java:255)
        at org.apache.activemq.broker.region.AbstractRegion.addDestination(AbstractRegion.java:162)
        at org.apache.activemq.broker.region.RegionBroker.addDestination(RegionBroker.java:339)
        at org.apache.activemq.broker.BrokerFilter.addDestination(BrokerFilter.java:174)
        at org.apache.activemq.advisory.AdvisoryBroker.addDestination(AdvisoryBroker.java:242)
        at org.apache.activemq.broker.BrokerFilter.addDestination(BrokerFilter.java:174)
        at org.apache.activemq.broker.BrokerFilter.addDestination(BrokerFilter.java:174)
        at org.apache.activemq.broker.BrokerFilter.addDestination(BrokerFilter.java:174)
        at org.apache.activemq.broker.BrokerFilter.addDestination(BrokerFilter.java:174)
        at org.apache.activemq.broker.BrokerFilter.addDestination(BrokerFilter.java:174)
        at org.apache.activemq.broker.BrokerFilter.addDestination(BrokerFilter.java:174)
        at org.apache.activemq.broker.region.AbstractRegion.lookup(AbstractRegion.java:555)
        at org.apache.activemq.broker.region.AbstractRegion.addConsumer(AbstractRegion.java:342)
        at org.apache.activemq.broker.region.TopicRegion.addConsumer(TopicRegion.java:188)
        at org.apache.activemq.broker.region.RegionBroker.addConsumer(RegionBroker.java:418)
        at org.apache.activemq.broker.jmx.ManagedRegionBroker.addConsumer(ManagedRegionBroker.java:240)
        at org.apache.activemq.broker.BrokerFilter.addConsumer(BrokerFilter.java:104)
 ...
{code}
whilst there are 100+ threads waiting on this lock to be released.


> Eliminate AbstractRegion.destinationsLock
> -----------------------------------------
>
>                 Key: AMQ-6226
>                 URL: https://issues.apache.org/jira/browse/AMQ-6226
>             Project: ActiveMQ
>          Issue Type: Bug
>    Affects Versions: 5.2.0
>            Reporter: Tim Bain
>            Priority: Major
>              Labels: performance
>
> This mailing list thread (http://activemq.2283324.n4.nabble.com/ActiveMQ-with-KahaDB-as-persistent-store-becomes-very-slow-almost-unresponsive-after-creating-large-s-td4709985.html) described awful performance problems when creating large numbers of destinations in parallel, and provided thread dumps that pointed to lots of threads contending for AbstractRegion.destinationsLock.
> AbstractRegion has lots of places where we do something like this:
> 455        destinationsLock.readLock().lock();
> 456        try {
> 457            dest = destinations.get(destination);
> 458        } finally {
> 459            destinationsLock.readLock().unlock();
> 460        }
> destinations is a ConcurrentHashMap, so it's already thread-safe.  Why are we using a single external lock around a ConcurrentHashMap that would be concurrent if we weren't making it single-locked???
> I suspect that the primary goal is to make sure that addDestination() and receiveDestination() don't create or remove the same destination twice, but there are other ways to do that (for example, having the map be ConcurrentHashMap<ActiveMQDestination, AtomicReference<Destination>> and putting the AtomicReference into the map immediately and then later populating the reference once the object is constructed).
> We need to eliminate this singleton lock and use the thread-safe ConcurrentHashMap's own locking, possibly coupled with thread-safe objects (AtomicReference, etc.) and/or explicit per-key locking, to implement the same algorithm in a way that allows reasonable parallelism under heavy load.
> NOTE: This might also help eliminate AMQ-5901.



--
This message was sent by Atlassian Jira
(v8.3.2#803003)