[
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)
