cshannon commented on PR #1659: URL: https://github.com/apache/activemq/pull/1659#issuecomment-3894219649
> Looks good. Synchronized is good enough and it's a perf killer only on vm transport. Other transports would marshall/unmarshall and would use a different instance. On vm, it creates contentions but that's way better than messing up with the data. Using RentrantReadWriteLock would not improve the things because all methods basically mutate under the cover. Agreed, synchronized here is fine to use and as you pointed out we don't gain anything from a ReadWrite lock as we need a write lock for basically all methods. I wasn't around for the original implementation of classes but it looked like the goal was to avoid synchronization as messages were meant to be copied if used across threads. Unfortunately that pattern breaks with the VM transport as we see due to the nature of dispatching to multiple consumers. I don't think we see this issue with network bridges because there is only one consumer for the VM transports in bridges (the broker itself). The performance impact for single threaded applications using synchronized that have no contention is pretty negligible and even with contention as we can see correctness is more important. I'd rather the application take a couple extra ms to dispatch all the copies of the message than to receive a null body. I was looking more at [AMQ-5857](https://issues.apache.org/jira/browse/AMQ-5857) and the discussion to try and remember why we didn't add synchronized back then when trying to clear memory to avoid storing the data twice and OOM errors and i assume we just didn't have good enough tests to show the real issue. Plus, it was like 10 years ago so I was now to ActiveMQ :) I haven't had time to fully review this yet but will either tomorrow or Monday and will post more feedback. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected] For further information, visit: https://activemq.apache.org/contact
