feat(#1798) Performance fix for topic send

[jeanouii]

To be discussed

[cshannon]

@jeanouii - This is an interesting find, I didn't even notice that synchronized keyword on that method. Most of the synchronization happens internally on the various collections etc. and I looked and I didn't see synchronized use anywhere else on the topic object itself internally.

Did you look around the broker to see if anything happens to synchronize the topic externally? The destinations often get referred to more generic as RegionDestination plus there's various wrappers so saerching might be tricky. I will search too, I just am curious if anything else is synchronizing on the Topic's intrinsic lock that would now break if we switch the lock.

I guess there's always the risk of breaking existing plugins (if they were synchronizing for some reason on the topic) but we can't control any custom code so not much we could do in that scenario.

[jeanouii]

@cshannon thanks for the review Chris.
Good question — I did search through the codebase as much as I could

• RegionBroker uses synchronized(dest)
• AbstractRegion uses consumerChangeMutexMap
• The synchronized(consumers) blocks inside Topic are on the internal CopyOnWriteArrayList
• No BrokerFilter or wrapper synchronizes on the destination object

Queue.doMessageSend() is already NOT synchronized

I agree about the external plugin risk. There is nothing we can reallly do right?

[mattrpav]

@jeanouii do you have any benchmark numbers b/w the current and the proposed change?

I'd like to consider this for v6.3.0 (and 5.x backport)

[mattrpav]

dependency versions should be moved to top-level pom.xml

[mattrpav]

M3 benchmark output

Benchmark                                (numSubscribers)      (storeType)   Mode  Cnt      Score       Error  Units
TopicSendLockBenchmark.send_01_thread                   1  MEMORY_BUSYWAIT  thrpt    5   4506.850 ±    17.464  ops/s
TopicSendLockBenchmark.send_01_thread                   1          H2_JDBC  thrpt    5  13022.866 ± 13569.102  ops/s
TopicSendLockBenchmark.send_01_thread                   5  MEMORY_BUSYWAIT  thrpt    5   3613.160 ±   189.764  ops/s
TopicSendLockBenchmark.send_01_thread                   5          H2_JDBC  thrpt    5   3891.111 ±  1597.854  ops/s
TopicSendLockBenchmark.send_01_thread                  10  MEMORY_BUSYWAIT  thrpt    5   2539.337 ±   182.568  ops/s
TopicSendLockBenchmark.send_01_thread                  10          H2_JDBC  thrpt    5   2334.779 ±   432.767  ops/s
TopicSendLockBenchmark.send_02_threads                  1  MEMORY_BUSYWAIT  thrpt    5   4812.668 ±    17.245  ops/s
TopicSendLockBenchmark.send_02_threads                  1          H2_JDBC  thrpt    5  19597.978 ±  4331.420  ops/s
TopicSendLockBenchmark.send_02_threads                  5  MEMORY_BUSYWAIT  thrpt    5   4716.725 ±    17.291  ops/s
TopicSendLockBenchmark.send_02_threads                  5          H2_JDBC  thrpt    5   5222.872 ±   843.002  ops/s
TopicSendLockBenchmark.send_02_threads                 10  MEMORY_BUSYWAIT  thrpt    5   3044.563 ±   103.254  ops/s
TopicSendLockBenchmark.send_02_threads                 10          H2_JDBC  thrpt    5   2388.289 ±   814.576  ops/s
TopicSendLockBenchmark.send_100_threads                 1  MEMORY_BUSYWAIT  thrpt    5   4692.625 ±    60.077  ops/s
TopicSendLockBenchmark.send_100_threads                 1          H2_JDBC  thrpt    5  11999.306 ±  4193.370  ops/s
TopicSendLockBenchmark.send_100_threads                 5  MEMORY_BUSYWAIT  thrpt    5   4227.561 ±   217.221  ops/s
TopicSendLockBenchmark.send_100_threads                 5          H2_JDBC  thrpt    5   3205.261 ±   466.154  ops/s
TopicSendLockBenchmark.send_100_threads                10  MEMORY_BUSYWAIT  thrpt    5   2192.125 ±  1907.470  ops/s
TopicSendLockBenchmark.send_100_threads                10          H2_JDBC  thrpt    5   2196.829 ±   742.277  ops/s
TopicSendLockBenchmark.send_10_threads                  1  MEMORY_BUSYWAIT  thrpt    5   4762.876 ±   133.679  ops/s
TopicSendLockBenchmark.send_10_threads                  1          H2_JDBC  thrpt    5  21422.370 ±  8640.315  ops/s
TopicSendLockBenchmark.send_10_threads                  5  MEMORY_BUSYWAIT  thrpt    5   4744.431 ±    16.564  ops/s
TopicSendLockBenchmark.send_10_threads                  5          H2_JDBC  thrpt    5   6531.285 ±   759.708  ops/s
TopicSendLockBenchmark.send_10_threads                 10  MEMORY_BUSYWAIT  thrpt    5   3758.206 ±   530.746  ops/s
TopicSendLockBenchmark.send_10_threads                 10          H2_JDBC  thrpt    5   2716.978 ±   701.743  ops/s
TopicSendLockBenchmark.send_20_threads                  1  MEMORY_BUSYWAIT  thrpt    5   4710.561 ±   279.590  ops/s
TopicSendLockBenchmark.send_20_threads                  1          H2_JDBC  thrpt    5  19577.767 ±  6045.958  ops/s
TopicSendLockBenchmark.send_20_threads                  5  MEMORY_BUSYWAIT  thrpt    5   4714.381 ±    55.031  ops/s
TopicSendLockBenchmark.send_20_threads                  5          H2_JDBC  thrpt    5   5868.364 ±  1542.890  ops/s
TopicSendLockBenchmark.send_20_threads                 10  MEMORY_BUSYWAIT  thrpt    5   4030.784 ±   216.081  ops/s
TopicSendLockBenchmark.send_20_threads                 10          H2_JDBC  thrpt    5   2868.966 ±   192.023  ops/s
TopicSendLockBenchmark.send_50_threads                  1  MEMORY_BUSYWAIT  thrpt    5   4734.577 ±    14.308  ops/s
TopicSendLockBenchmark.send_50_threads                  1          H2_JDBC  thrpt    5  16562.868 ±  2194.198  ops/s
TopicSendLockBenchmark.send_50_threads                  5  MEMORY_BUSYWAIT  thrpt    5   4512.351 ±   329.818  ops/s
TopicSendLockBenchmark.send_50_threads                  5          H2_JDBC  thrpt    5   4507.221 ±  1500.869  ops/s
TopicSendLockBenchmark.send_50_threads                 10  MEMORY_BUSYWAIT  thrpt    5   2858.611 ±  1243.876  ops/s
TopicSendLockBenchmark.send_50_threads                 10          H2_JDBC  thrpt    5   2705.621 ±   381.384  ops/s

[jeanouii]

Thanks @mattrpav for the review and microbenchmark run. Is it with the PR or without?
We would both to compare within the same machine of course. So if you don't mind running the base line if this results are with the PR, that would be great.

[cshannon]

@jeanouii - I just wanted to add a comment that this hasn't been forgotten about and will still be targeted for 6.3.0 at least (and possible backport). We've just gotten hammered with all the AI generated reports the last few months (like every other open source project) so everyone's focus has been on that and 6.3.0 has gotten delayed.

[cshannon]

@jeanouii - can you rebase this with main?

[cshannon]

I'm doing some testing now but this looks pretty good.

This narrows the lock further and the previous intrinsic lock wasn't used anywhere else anyways. This is only on the doSend() method and not the top level send so Id on't see any risk of deadlocks or other weirdness with destination interceptors

Originally I was wondering if this would still work to guarantee ordering for async producers because they would not wait for a response. But I realized that every TransportConnection is going to process commands in order it receives them per connection, so the broker should never be calling the send method for two different messages from the same producer at the same time.

[cshannon]

Oops, I saw the comment from @mattrpav about moving the jm version to the top level which should be done first before merging

[cshannon]

I'll merge and do a follow on update for the versions

will fix in follow on

[cshannon]

@mattrpav - I removed 5.19.x as I think we should consider this a feature and we need to only be back porting fixes at this point to 5.19.x with the plan to EOL at some point