You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by GitBox <gi...@apache.org> on 2022/04/08 13:23:09 UTC
[GitHub] [activemq-artemis] AntonRoskvist opened a new pull request, #4023: ARTEMIS-3771 - Rework destination handling for the OpenWire-protocol
AntonRoskvist opened a new pull request, #4023:
URL: https://github.com/apache/activemq-artemis/pull/4023
This gives a significant performance increase for my use case but I could not find a good way to demonstrate it through testing, so the added tests only focus on functionality
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] AntonRoskvist commented on pull request #4023: ARTEMIS-3771 - Rework destination handling for the OpenWire-protocol
Posted by GitBox <gi...@apache.org>.
AntonRoskvist commented on PR #4023:
URL: https://github.com/apache/activemq-artemis/pull/4023#issuecomment-1105525758
@clebertsuconic @franz1981
Restored the cache to previous behavior, this PR now primarily just gets rid of the binding-query which was the real performance sink anyway.
Don't really know how to write a meaningful test to demonstrate the benefit though, but the easiest way to "real-life" demonstrate it is by loading the broker with thousands of queues and have a single producer send messages in individual sessions (such as some frameworks unfortunately tend to do it).
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] franz1981 commented on a diff in pull request #4023: ARTEMIS-3771 - Rework destination handling for the OpenWire-protocol
Posted by GitBox <gi...@apache.org>.
franz1981 commented on code in PR #4023:
URL: https://github.com/apache/activemq-artemis/pull/4023#discussion_r846164417
##########
artemis-protocols/artemis-openwire-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/openwire/amq/AMQSession.java:
##########
@@ -196,26 +195,22 @@ public boolean updateDeliveryCountAfterCancel(ServerConsumer consumer, MessageRe
private boolean checkCachedExistingQueues(final SimpleString address,
final String physicalName,
final boolean isTemporary) throws Exception {
- String[] existingQueuesCache = this.existingQueuesCache;
//lazy allocation of the cache
if (existingQueuesCache == null) {
- //16 means 64 bytes with 32 bit references or 128 bytes with 64 bit references -> 1 or 2 cache lines with common archs
existingQueuesCache = new String[protocolManager.getOpenWireDestinationCacheSize()];
- assert (Integer.bitCount(existingQueuesCache.length) == 1) : "openWireDestinationCacheSize must be a power of 2";
- this.existingQueuesCache = existingQueuesCache;
}
- final int hashCode = physicalName.hashCode();
- //this.existingQueuesCache.length must be power of 2
- final int mask = existingQueuesCache.length - 1;
- final int index = hashCode & mask;
+
+ final int index = Math.floorMod(physicalName.hashCode(), existingQueuesCache.length);
Review Comment:
Modulus is a quite heavyweight operation, that's why the original code was using the mask trick using a power of two size: this would save impacting for the "common" use cases ie not many queues
##########
artemis-protocols/artemis-openwire-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/openwire/amq/AMQSession.java:
##########
@@ -91,7 +91,7 @@
private final CoreMessageObjectPools coreMessageObjectPools;
- private String[] existingQueuesCache;
+ static String[] existingQueuesCache;
Review Comment:
This is quite dangerous, making the cache to leak and outlive a broker restart (thinking about activation/deactivation on HA)
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] AntonRoskvist commented on pull request #4023: ARTEMIS-3771 - Rework destination handling for the OpenWire-protocol
Posted by GitBox <gi...@apache.org>.
AntonRoskvist commented on PR #4023:
URL: https://github.com/apache/activemq-artemis/pull/4023#issuecomment-1097615024
@franz1981
Hi,
Do you like these changes better? I opted to add the connection-level cache again since after some more testing I found that I was wrong. Both the bitwise operation and the modulus one produces the same level of cache distribution (and clashing). The distribution "issue" increase with array size... this effect is probably only news to me though.
I should mention that the biggest performance gain here is from removing the "BindingQueryResult", because that becomes a very heavy operation with a large amount of destinations.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] AntonRoskvist commented on a diff in pull request #4023: ARTEMIS-3771 - Rework destination handling for the OpenWire-protocol
Posted by GitBox <gi...@apache.org>.
AntonRoskvist commented on code in PR #4023:
URL: https://github.com/apache/activemq-artemis/pull/4023#discussion_r846216073
##########
artemis-protocols/artemis-openwire-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/openwire/amq/AMQSession.java:
##########
@@ -91,7 +91,7 @@
private final CoreMessageObjectPools coreMessageObjectPools;
- private String[] existingQueuesCache;
+ static String[] existingQueuesCache;
Review Comment:
I did consider placing this in the OpenWireProtocolManager instead but opted for this... Would you prefer I place it there and use setter/getter instead?
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] AntonRoskvist commented on a diff in pull request #4023: ARTEMIS-3771 - Rework destination handling for the OpenWire-protocol
Posted by GitBox <gi...@apache.org>.
AntonRoskvist commented on code in PR #4023:
URL: https://github.com/apache/activemq-artemis/pull/4023#discussion_r846228548
##########
artemis-protocols/artemis-openwire-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/openwire/amq/AMQSession.java:
##########
@@ -196,26 +195,22 @@ public boolean updateDeliveryCountAfterCancel(ServerConsumer consumer, MessageRe
private boolean checkCachedExistingQueues(final SimpleString address,
final String physicalName,
final boolean isTemporary) throws Exception {
- String[] existingQueuesCache = this.existingQueuesCache;
//lazy allocation of the cache
if (existingQueuesCache == null) {
- //16 means 64 bytes with 32 bit references or 128 bytes with 64 bit references -> 1 or 2 cache lines with common archs
existingQueuesCache = new String[protocolManager.getOpenWireDestinationCacheSize()];
- assert (Integer.bitCount(existingQueuesCache.length) == 1) : "openWireDestinationCacheSize must be a power of 2";
- this.existingQueuesCache = existingQueuesCache;
}
- final int hashCode = physicalName.hashCode();
- //this.existingQueuesCache.length must be power of 2
- final int mask = existingQueuesCache.length - 1;
- final int index = hashCode & mask;
+
+ final int index = Math.floorMod(physicalName.hashCode(), existingQueuesCache.length);
Review Comment:
Yes, but since now there will only be one such calculation per new queue, (plus new ones if the cache is full) I figure that might be cheaper overall anyway? I changed it because this method has a lower risk of creating identical hashes for different values (from my testing at least)...
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #4023: ARTEMIS-3771 - Rework destination handling for the OpenWire-protocol
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on PR #4023:
URL: https://github.com/apache/activemq-artemis/pull/4023#issuecomment-1105632651
@AntonRoskvist I was just curious.. thanks a lot...
probably I should have asked you in private though.. thanks
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #4023: ARTEMIS-3771 - Rework destination handling for the OpenWire-protocol
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on PR #4023:
URL: https://github.com/apache/activemq-artemis/pull/4023#issuecomment-1105742580
full pass on my CI.. merging it.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] AntonRoskvist commented on pull request #4023: ARTEMIS-3771 - Rework destination handling for the OpenWire-protocol
Posted by GitBox <gi...@apache.org>.
AntonRoskvist commented on PR #4023:
URL: https://github.com/apache/activemq-artemis/pull/4023#issuecomment-1104767472
@clebertsuconic thanks for the feedback
Okay... at this point I'm basically just moving the current "session-level" destination cache and making it shared, but the way this cache is implemented now makes collisions happen so often that I'm not really sure it's worth it to be honest. I'm mostly doing this for a use case I have with a very large number of queues and chances of a cache-clash is really high already at "just" some few hundreds of destinations.
If you prefer I change this (and I can do that for sure), do you think it makes sense to just store all destinations in something like a hashmap instead that just holds all destinations, or should I revert this particular part of the change to it's old behavior, i.e Session-level? Either way is fine for me.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #4023: ARTEMIS-3771 - Rework destination handling for the OpenWire-protocol
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on PR #4023:
URL: https://github.com/apache/activemq-artemis/pull/4023#issuecomment-1105600882
It seems you care about performance. Why OpenWire though?
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #4023: ARTEMIS-3771 - Rework destination handling for the OpenWire-protocol
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on PR #4023:
URL: https://github.com/apache/activemq-artemis/pull/4023#issuecomment-1105376081
this sort off cache should be at session Level I think
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] AntonRoskvist commented on pull request #4023: ARTEMIS-3771 - Rework destination handling for the OpenWire-protocol
Posted by GitBox <gi...@apache.org>.
AntonRoskvist commented on PR #4023:
URL: https://github.com/apache/activemq-artemis/pull/4023#issuecomment-1105629157
@clebertsuconic I'm running this on a pretty large scale (around 600 client applications or so in some environments, multiple environments interconnected globally) and all of which used to run against ActiveMQ 5. Most are supposed to get migrated over to Core but this is a pretty big undertaking so for some foreseeable future I have to support "backward" compatibility. I figure this is something I can do to make things run as smoothly as possible until such time that they can start to get migrated... and hopefully it might be helpful to someone else too.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #4023: ARTEMIS-3771 - Rework destination handling for the OpenWire-protocol
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on PR #4023:
URL: https://github.com/apache/activemq-artemis/pull/4023#issuecomment-1105640765
so, it seems we were doing two checks for the address/. queue, one was useless and not needed,, correct?
on that case can you rename the commit to something more accurate to the current state?
say:
```
ARTEMIS-3771 Avoid not needed lookup for address on OpenWire connections
```
I can of course use some Git-fu and do it myself... but if you do it yourself I guess you wouldn't have my trace as a contributor in your commit.
Also, if you could rebase (git pull --rebase upstream main) and git push -f?
I can then just run my private CI on top of your PR. (I have a parameterized build where I pass in GitHub address and branch name).
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic merged pull request #4023: ARTEMIS-3771 - Rework destination handling for the OpenWire-protocol
Posted by GitBox <gi...@apache.org>.
clebertsuconic merged PR #4023:
URL: https://github.com/apache/activemq-artemis/pull/4023
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #4023: ARTEMIS-3771 - Rework destination handling for the OpenWire-protocol
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on PR #4023:
URL: https://github.com/apache/activemq-artemis/pull/4023#issuecomment-1104523140
Let me try to understand...
you've created an Array on OpenWireprotocolManager that must be sized at the power of 2, you hash the queue and look for it on the ProtcolManager?
it would be better If you're implementing such cache it would be better to create some class... say.. name it as HashStringCache and have the logic encapsulated in there?
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] AntonRoskvist commented on pull request #4023: ARTEMIS-3771 - Rework destination handling for the OpenWire-protocol
Posted by GitBox <gi...@apache.org>.
AntonRoskvist commented on PR #4023:
URL: https://github.com/apache/activemq-artemis/pull/4023#issuecomment-1105661183
Of course, hope this does it. Otherwise let me know. Thanks @clebertsuconic
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org