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

["Christopher L. Shannon (JIRA)" <ji...@apache.org>]

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

Christopher L. Shannon commented on AMQ-6226:
---------------------------------------------

This is worth looking at but would need some investigation to see what side effects there might be.  I know the lock is used for iterating and also to synchronize across the destinationMap (which is not a concurrent hash map).  I am also not sure how much removing that lock will help with contention without truly understanding the cause of the slowness.  While there are threads are waiting on that lock, if you remove that lock you could just push the contention down stream.  For example, if the reason that lots of threads are waiting on the destinations lock is because some threads are being slow to write to disk then it's possible removing this lock wouldn't help much because those threads would now just be waiting on the locks in the message store instead.

> 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
>              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
(v6.3.4#6332)