[GitHub] [activemq-artemis] franz1981 commented on a diff in pull request #4023: ARTEMIS-3771 - Rework destination handling for the OpenWire-protocol

[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