You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@activemq.apache.org by franz1981 <gi...@git.apache.org> on 2018/11/11 11:08:16 UTC
[GitHub] activemq-artemis pull request #2427: ARTEMIS-2170 Optimized CoreMessage's ch...
GitHub user franz1981 opened a pull request:
https://github.com/apache/activemq-artemis/pull/2427
ARTEMIS-2170 Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
It includes 2 optimizations related to CoreMessage hot paths operations: checkProperties and cleanupInternalProperties.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/franz1981/activemq-artemis ARTEMIS-2170
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/activemq-artemis/pull/2427.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2427
----
commit f4f1e125603f6c510541395a102df68bb7dae66e
Author: Francesco Nigro <ni...@...>
Date: 2018-11-11T09:44:29Z
ARTEMIS-2170 Optimized CoreMessage's cleanupInternalProperties method
The cleanup is now performed into TypedProperties both
for performance reasons (avoilock/unlock many times)
and consistency, given that the operation is now atomic.
commit 8b6e9a417c39ef6b043b7b11c504254e6721d10b
Author: Francesco Nigro <ni...@...>
Date: 2018-11-11T10:32:49Z
ARTEMIS-2170 Optimized CoreMessage's checkProperties method
Any checkProperties();<usage of this.properties> pattern has been
replaced by an atomic checkProperties().<usage of returned properties>
to help both performance and consistency.
----
---
[GitHub] activemq-artemis pull request #2427: ARTEMIS-2170 Optimized CoreMessage's ch...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2427#discussion_r234113042
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/message/impl/CoreMessage.java ---
@@ -52,6 +52,11 @@
* consumers */
public class CoreMessage extends RefCountMessage implements ICoreMessage {
+ // We use properties to establish routing context on clustering.
+ // However if the client resends the message after receiving, it needs to be removed
+ private static final Predicate<SimpleString> INTERNAL_PROPERTY_NAMES_CLEANUP_FILTER =
--- End diff --
@clebertsuconic Do you know of any other way (faster) to know if a `CoreMessage` it is seen for the first time ie not a resent message?
---
[GitHub] activemq-artemis pull request #2427: ARTEMIS-2170 Optimized CoreMessage's ch...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2427#discussion_r233491147
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/message/impl/CoreMessage.java ---
@@ -946,119 +994,103 @@ public Integer getIntProperty(final String key) throws ActiveMQPropertyConversio
@Override
public CoreMessage putLongProperty(final SimpleString key, final long value) {
messageChanged();
- checkProperties();
- properties.putLongProperty(key, value);
+ checkProperties().putLongProperty(key, value);
return this;
}
@Override
public CoreMessage putLongProperty(final String key, final long value) {
messageChanged();
- checkProperties();
- properties.putLongProperty(SimpleString.toSimpleString(key, getPropertyKeysPool()), value);
+ checkProperties().putLongProperty(SimpleString.toSimpleString(key, getPropertyKeysPool()), value);
return this;
}
@Override
public Long getLongProperty(final SimpleString key) throws ActiveMQPropertyConversionException {
- checkProperties();
- return properties.getLongProperty(key);
+ return checkProperties().getLongProperty(key);
}
@Override
public Long getLongProperty(final String key) throws ActiveMQPropertyConversionException {
- checkProperties();
return getLongProperty(SimpleString.toSimpleString(key));
}
@Override
public CoreMessage putFloatProperty(final SimpleString key, final float value) {
messageChanged();
- checkProperties();
- properties.putFloatProperty(key, value);
+ checkProperties().putFloatProperty(key, value);
return this;
}
@Override
public CoreMessage putFloatProperty(final String key, final float value) {
messageChanged();
- checkProperties();
- properties.putFloatProperty(SimpleString.toSimpleString(key, getPropertyKeysPool()), value);
+ checkProperties().putFloatProperty(SimpleString.toSimpleString(key, getPropertyKeysPool()), value);
return this;
}
@Override
public CoreMessage putDoubleProperty(final SimpleString key, final double value) {
messageChanged();
- checkProperties();
- properties.putDoubleProperty(key, value);
+ checkProperties().putDoubleProperty(key, value);
return this;
}
@Override
public CoreMessage putDoubleProperty(final String key, final double value) {
messageChanged();
- checkProperties();
- properties.putDoubleProperty(SimpleString.toSimpleString(key, getPropertyKeysPool()), value);
+ checkProperties().putDoubleProperty(SimpleString.toSimpleString(key, getPropertyKeysPool()), value);
return this;
}
@Override
public Double getDoubleProperty(final SimpleString key) throws ActiveMQPropertyConversionException {
- checkProperties();
- return properties.getDoubleProperty(key);
+ return checkProperties().getDoubleProperty(key);
}
@Override
public Double getDoubleProperty(final String key) throws ActiveMQPropertyConversionException {
- checkProperties();
return getDoubleProperty(SimpleString.toSimpleString(key));
}
@Override
public CoreMessage putStringProperty(final SimpleString key, final SimpleString value) {
messageChanged();
- checkProperties();
- properties.putSimpleStringProperty(key, value);
+ checkProperties().putSimpleStringProperty(key, value);
return this;
}
@Override
public CoreMessage putStringProperty(final SimpleString key, final String value) {
messageChanged();
- checkProperties();
- properties.putSimpleStringProperty(key, SimpleString.toSimpleString(value, getPropertyValuesPool()));
+ checkProperties().putSimpleStringProperty(key, SimpleString.toSimpleString(value, getPropertyValuesPool()));
return this;
}
@Override
public CoreMessage putStringProperty(final String key, final String value) {
messageChanged();
- checkProperties();
- properties.putSimpleStringProperty(SimpleString.toSimpleString(key, getPropertyKeysPool()), SimpleString.toSimpleString(value, getPropertyValuesPool()));
+ checkProperties().putSimpleStringProperty(SimpleString.toSimpleString(key, getPropertyKeysPool()), SimpleString.toSimpleString(value, getPropertyValuesPool()));
--- End diff --
delegate to
public CoreMessage putStringProperty(final SimpleString key, final String value) {
---
[GitHub] activemq-artemis pull request #2427: ARTEMIS-2170 Optimized CoreMessage's ch...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2427#discussion_r234112554
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/message/impl/CoreMessage.java ---
@@ -52,6 +52,62 @@
* consumers */
public class CoreMessage extends RefCountMessage implements ICoreMessage {
+ private static final class CoreTypedProperties extends TypedProperties {
+
+ // We use properties to establish routing context on clustering.
+ // However if the client resends the message after receiving, it needs to be removed
+ private static final Predicate<SimpleString> INTERNAL_PROPERTY_NAMES_CLEANUP_FILTER =
+ name -> (name.startsWith(Message.HDR_ROUTE_TO_IDS) && !name.equals(Message.HDR_ROUTE_TO_IDS)) ||
+ (name.startsWith(Message.HDR_ROUTE_TO_ACK_IDS) && !name.equals(Message.HDR_ROUTE_TO_ACK_IDS));
+ private static final SimpleString AMQ_PROPNAME = new SimpleString("_AMQ_");
+ private boolean internalProperties;
+
+ CoreTypedProperties() {
+ super();
+ internalProperties = false;
+ }
+
+ private CoreTypedProperties(TypedProperties other) {
+ super(other);
+ if (other instanceof CoreTypedProperties) {
+ internalProperties = ((CoreTypedProperties) other).internalProperties;
+ } else {
+ internalProperties = other.containsProperty(name -> name.startsWith(AMQ_PROPNAME));
+ }
+ }
+
+ @Override
+ protected synchronized void doPutValue(SimpleString key, TypedProperties.PropertyValue value) {
+ if (!internalProperties && key.startsWith(AMQ_PROPNAME)) {
+ internalProperties = true;
+ }
+ super.doPutValue(key, value);
+ }
+
+ public synchronized boolean cleanupInternalProperties() {
+ if (!internalProperties) {
+ return false;
+ }
+ return removeProperty(INTERNAL_PROPERTY_NAMES_CLEANUP_FILTER);
+ }
+
+ public static boolean cleanupInternalProperties(TypedProperties typedProperties) {
+ if (typedProperties == null) {
+ return false;
+ }
+ if (typedProperties instanceof CoreTypedProperties) {
+ return ((CoreTypedProperties) typedProperties).cleanupInternalProperties();
+ }
+ return typedProperties.removeProperty(INTERNAL_PROPERTY_NAMES_CLEANUP_FILTER);
+ }
+
+ public static CoreTypedProperties copyOf(TypedProperties typedProperties) {
+ synchronized (typedProperties) {
--- End diff --
Nice: I have removed subclass of `TypedProperties` and cleaned up the code: it seems much better now. The only thing that bother me is that I would like to avoid at all to perform that `cleanupInternalProperties`, given that most of time it won't find anything; i just need to understand what's its purpose and how to know in advance when do it is a waste of time.
---
[GitHub] activemq-artemis pull request #2427: ARTEMIS-2170 Optimized CoreMessage's ch...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2427#discussion_r232758121
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/collections/TypedProperties.java ---
@@ -318,6 +320,33 @@ public synchronized boolean containsProperty(final SimpleString key) {
}
}
+ public synchronized boolean cleanupInternalProperties(Predicate<SimpleString> propertyNamePredicate) {
+ if (!internalProperties) {
--- End diff --
I have pushed a new version of the change that's using a private subclass of 'TypedProperties' where needed to allow atomic updates of the `internalProperties` flag: I'm not very about about the factory method on 'CoreMessage' to allow the client messages to not use that subclass: this is an additional optimisation given that the original code where always using 'TypedProperties' that where chacking on each added property if it was an 'internal" one.
---
[GitHub] activemq-artemis pull request #2427: ARTEMIS-2170 Optimized CoreMessage's ch...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2427#discussion_r232654687
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/collections/TypedProperties.java ---
@@ -318,6 +320,33 @@ public synchronized boolean containsProperty(final SimpleString key) {
}
}
+ public synchronized boolean cleanupInternalProperties(Predicate<SimpleString> propertyNamePredicate) {
+ if (!internalProperties) {
--- End diff --
Whats your branch? Ill have a better look later tonight or tomorrow and send you some ideas if you want.
---
[GitHub] activemq-artemis pull request #2427: ARTEMIS-2170 Optimized CoreMessage's ch...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2427#discussion_r232558150
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/collections/TypedProperties.java ---
@@ -318,6 +320,33 @@ public synchronized boolean containsProperty(final SimpleString key) {
}
}
+ public synchronized boolean cleanupInternalProperties(Predicate<SimpleString> propertyNamePredicate) {
+ if (!internalProperties) {
--- End diff --
It may have them from legacy someone put it there, but if you were designing a collections class, you'd design it in a fashion so it focussed just on the logic it needs to have so you ensure its good.
If anything that field, the flag and the method check hasInternalProperties really should move upto CoreMessage, as its only used there.
---
[GitHub] activemq-artemis pull request #2427: ARTEMIS-2170 Optimized CoreMessage's ch...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2427#discussion_r234527890
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/message/impl/CoreMessage.java ---
@@ -52,6 +52,62 @@
* consumers */
public class CoreMessage extends RefCountMessage implements ICoreMessage {
+ private static final class CoreTypedProperties extends TypedProperties {
+
+ // We use properties to establish routing context on clustering.
+ // However if the client resends the message after receiving, it needs to be removed
+ private static final Predicate<SimpleString> INTERNAL_PROPERTY_NAMES_CLEANUP_FILTER =
+ name -> (name.startsWith(Message.HDR_ROUTE_TO_IDS) && !name.equals(Message.HDR_ROUTE_TO_IDS)) ||
+ (name.startsWith(Message.HDR_ROUTE_TO_ACK_IDS) && !name.equals(Message.HDR_ROUTE_TO_ACK_IDS));
+ private static final SimpleString AMQ_PROPNAME = new SimpleString("_AMQ_");
+ private boolean internalProperties;
+
+ CoreTypedProperties() {
+ super();
+ internalProperties = false;
+ }
+
+ private CoreTypedProperties(TypedProperties other) {
+ super(other);
+ if (other instanceof CoreTypedProperties) {
+ internalProperties = ((CoreTypedProperties) other).internalProperties;
+ } else {
+ internalProperties = other.containsProperty(name -> name.startsWith(AMQ_PROPNAME));
+ }
+ }
+
+ @Override
+ protected synchronized void doPutValue(SimpleString key, TypedProperties.PropertyValue value) {
+ if (!internalProperties && key.startsWith(AMQ_PROPNAME)) {
+ internalProperties = true;
+ }
+ super.doPutValue(key, value);
+ }
+
+ public synchronized boolean cleanupInternalProperties() {
+ if (!internalProperties) {
+ return false;
+ }
+ return removeProperty(INTERNAL_PROPERTY_NAMES_CLEANUP_FILTER);
+ }
+
+ public static boolean cleanupInternalProperties(TypedProperties typedProperties) {
+ if (typedProperties == null) {
+ return false;
+ }
+ if (typedProperties instanceof CoreTypedProperties) {
+ return ((CoreTypedProperties) typedProperties).cleanupInternalProperties();
+ }
+ return typedProperties.removeProperty(INTERNAL_PROPERTY_NAMES_CLEANUP_FILTER);
+ }
+
+ public static CoreTypedProperties copyOf(TypedProperties typedProperties) {
+ synchronized (typedProperties) {
--- End diff --
+1 agreed this would need to be resolved as currently thats why the flag exist to avoid the iterating over the properties.
---
[GitHub] activemq-artemis pull request #2427: ARTEMIS-2170 Optimized CoreMessage's ch...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2427#discussion_r233512044
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/message/impl/CoreMessage.java ---
@@ -564,34 +604,59 @@ public CoreMessage setUserID(UUID userID) {
/**
* I am keeping this synchronized as the decode of the Properties is lazy
*/
- protected TypedProperties checkProperties() {
+ protected final TypedProperties checkProperties() {
try {
+ TypedProperties properties = this.properties;
if (properties == null) {
- synchronized (this) {
- if (properties == null) {
- TypedProperties properties = new TypedProperties();
- if (buffer != null && propertiesLocation >= 0) {
- final ByteBuf byteBuf = buffer.duplicate().readerIndex(propertiesLocation);
- properties.decode(byteBuf, coreMessageObjectPools == null ? null : coreMessageObjectPools.getPropertiesDecoderPools());
- }
- this.properties = properties;
- }
+ properties = getOrInitializeTypedProperties();
+ }
+ return properties;
+ } catch (Throwable e) {
+ throw onCheckPropertiesError(e);
+ }
+ }
+
+ private TypedProperties getOrInitializeTypedProperties() {
--- End diff --
i would rename "checkProperties" to "getProperties", and i would call this initalizeTypedProperties without the return.
---
[GitHub] activemq-artemis pull request #2427: ARTEMIS-2170 Optimized CoreMessage's ch...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2427#discussion_r236646355
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/message/impl/CoreMessage.java ---
@@ -578,34 +567,41 @@ public CoreMessage setUserID(UUID userID) {
/**
* I am keeping this synchronized as the decode of the Properties is lazy
*/
- protected TypedProperties checkProperties() {
+ protected final TypedProperties getOrCreateProperties() {
--- End diff --
Yes, just pushed, but I haven't had the chance yet to ask @mtaylor or @clebertsuconic on how to remove the cleanup internal properties!
---
[GitHub] activemq-artemis pull request #2427: ARTEMIS-2170 Optimized CoreMessage's ch...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2427#discussion_r232491039
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/message/impl/CoreMessage.java ---
@@ -803,52 +802,45 @@ public CoreMessage setDurable(boolean durable) {
@Override
public CoreMessage putBooleanProperty(final String key, final boolean value) {
messageChanged();
- checkProperties();
- properties.putBooleanProperty(SimpleString.toSimpleString(key, getPropertyKeysPool()), value);
+ checkProperties().putBooleanProperty(SimpleString.toSimpleString(key, getPropertyKeysPool()), value);
return this;
}
@Override
public CoreMessage putBooleanProperty(final SimpleString key, final boolean value) {
messageChanged();
- checkProperties();
- properties.putBooleanProperty(key, value);
+ checkProperties().putBooleanProperty(key, value);
return this;
}
@Override
public Boolean getBooleanProperty(final SimpleString key) throws ActiveMQPropertyConversionException {
- checkProperties();
- return properties.getBooleanProperty(key);
+ return checkProperties().getBooleanProperty(key);
}
@Override
public Boolean getBooleanProperty(final String key) throws ActiveMQPropertyConversionException {
- checkProperties();
- return properties.getBooleanProperty(SimpleString.toSimpleString(key, getPropertyKeysPool()));
+ return checkProperties().getBooleanProperty(SimpleString.toSimpleString(key, getPropertyKeysPool()));
--- End diff --
Delegate to simplestring method
---
[GitHub] activemq-artemis pull request #2427: ARTEMIS-2170 Optimized CoreMessage's ch...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2427#discussion_r234909113
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/message/impl/CoreMessage.java ---
@@ -52,6 +52,11 @@
* consumers */
public class CoreMessage extends RefCountMessage implements ICoreMessage {
+ // We use properties to establish routing context on clustering.
+ // However if the client resends the message after receiving, it needs to be removed
+ private static final Predicate<SimpleString> INTERNAL_PROPERTY_NAMES_CLEANUP_FILTER =
--- End diff --
@franz1981 in RemoteQueueBindingImpl.addRouteContextToMessage these are added as extraByteProperties, and used in ClusterConnectionBridge maybe a simpler solution is to hold extraByteProperties in a separate TypeProperties like in AMQPMessage, then its easy to strip these extra internal properties (no need to even iterate) / not let it leak to clients, just as in AMQPMessage.
---
[GitHub] activemq-artemis pull request #2427: ARTEMIS-2170 Optimized CoreMessage's ch...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2427#discussion_r233493524
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/message/impl/CoreMessage.java ---
@@ -52,6 +52,62 @@
* consumers */
public class CoreMessage extends RefCountMessage implements ICoreMessage {
+ private static final class CoreTypedProperties extends TypedProperties {
+
+ // We use properties to establish routing context on clustering.
+ // However if the client resends the message after receiving, it needs to be removed
+ private static final Predicate<SimpleString> INTERNAL_PROPERTY_NAMES_CLEANUP_FILTER =
+ name -> (name.startsWith(Message.HDR_ROUTE_TO_IDS) && !name.equals(Message.HDR_ROUTE_TO_IDS)) ||
+ (name.startsWith(Message.HDR_ROUTE_TO_ACK_IDS) && !name.equals(Message.HDR_ROUTE_TO_ACK_IDS));
+ private static final SimpleString AMQ_PROPNAME = new SimpleString("_AMQ_");
+ private boolean internalProperties;
+
+ CoreTypedProperties() {
+ super();
+ internalProperties = false;
+ }
+
+ private CoreTypedProperties(TypedProperties other) {
+ super(other);
+ if (other instanceof CoreTypedProperties) {
+ internalProperties = ((CoreTypedProperties) other).internalProperties;
+ } else {
+ internalProperties = other.containsProperty(name -> name.startsWith(AMQ_PROPNAME));
+ }
+ }
+
+ @Override
+ protected synchronized void doPutValue(SimpleString key, TypedProperties.PropertyValue value) {
+ if (!internalProperties && key.startsWith(AMQ_PROPNAME)) {
+ internalProperties = true;
+ }
+ super.doPutValue(key, value);
+ }
+
+ public synchronized boolean cleanupInternalProperties() {
--- End diff --
again is this really public, should this only be invokable by CoreMessage
---
[GitHub] activemq-artemis pull request #2427: ARTEMIS-2170 Optimized CoreMessage's ch...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2427#discussion_r233496401
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/collections/TypedProperties.java ---
@@ -318,6 +320,33 @@ public synchronized boolean containsProperty(final SimpleString key) {
}
}
+ public synchronized boolean cleanupInternalProperties(Predicate<SimpleString> propertyNamePredicate) {
+ if (!internalProperties) {
--- End diff --
tbh id rather we do one refactor. so +1 for me.
---
[GitHub] activemq-artemis pull request #2427: ARTEMIS-2170 Optimized CoreMessage's ch...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2427#discussion_r233491363
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/message/impl/CoreMessage.java ---
@@ -1070,26 +1102,22 @@ public CoreMessage putObjectProperty(final String key, final Object value) throw
@Override
public Short getShortProperty(final SimpleString key) throws ActiveMQPropertyConversionException {
- checkProperties();
- return properties.getShortProperty(key);
+ return checkProperties().getShortProperty(key);
}
@Override
public Short getShortProperty(final String key) throws ActiveMQPropertyConversionException {
- checkProperties();
- return properties.getShortProperty(SimpleString.toSimpleString(key, getPropertyKeysPool()));
+ return checkProperties().getShortProperty(SimpleString.toSimpleString(key, getPropertyKeysPool()));
--- End diff --
delegate to
public Short getShortProperty(final SimpleString key) throws ActiveMQPropertyConversionException {
---
[GitHub] activemq-artemis pull request #2427: ARTEMIS-2170 Optimized CoreMessage's ch...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2427#discussion_r233493239
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/message/impl/CoreMessage.java ---
@@ -52,6 +52,62 @@
* consumers */
public class CoreMessage extends RefCountMessage implements ICoreMessage {
+ private static final class CoreTypedProperties extends TypedProperties {
+
+ // We use properties to establish routing context on clustering.
+ // However if the client resends the message after receiving, it needs to be removed
+ private static final Predicate<SimpleString> INTERNAL_PROPERTY_NAMES_CLEANUP_FILTER =
+ name -> (name.startsWith(Message.HDR_ROUTE_TO_IDS) && !name.equals(Message.HDR_ROUTE_TO_IDS)) ||
+ (name.startsWith(Message.HDR_ROUTE_TO_ACK_IDS) && !name.equals(Message.HDR_ROUTE_TO_ACK_IDS));
+ private static final SimpleString AMQ_PROPNAME = new SimpleString("_AMQ_");
+ private boolean internalProperties;
+
+ CoreTypedProperties() {
+ super();
+ internalProperties = false;
+ }
+
+ private CoreTypedProperties(TypedProperties other) {
+ super(other);
+ if (other instanceof CoreTypedProperties) {
+ internalProperties = ((CoreTypedProperties) other).internalProperties;
+ } else {
+ internalProperties = other.containsProperty(name -> name.startsWith(AMQ_PROPNAME));
+ }
+ }
+
+ @Override
+ protected synchronized void doPutValue(SimpleString key, TypedProperties.PropertyValue value) {
+ if (!internalProperties && key.startsWith(AMQ_PROPNAME)) {
+ internalProperties = true;
+ }
+ super.doPutValue(key, value);
+ }
+
+ public synchronized boolean cleanupInternalProperties() {
+ if (!internalProperties) {
+ return false;
+ }
+ return removeProperty(INTERNAL_PROPERTY_NAMES_CLEANUP_FILTER);
+ }
+
+ public static boolean cleanupInternalProperties(TypedProperties typedProperties) {
--- End diff --
is this really public?
---
[GitHub] activemq-artemis pull request #2427: ARTEMIS-2170 Optimized CoreMessage's ch...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2427#discussion_r234622321
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/message/impl/CoreMessage.java ---
@@ -803,139 +799,122 @@ public CoreMessage setDurable(boolean durable) {
@Override
public CoreMessage putBooleanProperty(final String key, final boolean value) {
messageChanged();
- checkProperties();
- properties.putBooleanProperty(SimpleString.toSimpleString(key, getPropertyKeysPool()), value);
+ getOrCreateProperties().putBooleanProperty(key(key), value);
return this;
}
@Override
public CoreMessage putBooleanProperty(final SimpleString key, final boolean value) {
messageChanged();
- checkProperties();
- properties.putBooleanProperty(key, value);
+ getOrCreateProperties().putBooleanProperty(key, value);
return this;
}
@Override
public Boolean getBooleanProperty(final SimpleString key) throws ActiveMQPropertyConversionException {
- checkProperties();
- return properties.getBooleanProperty(key);
+ return getOrCreateProperties().getBooleanProperty(key);
}
@Override
public Boolean getBooleanProperty(final String key) throws ActiveMQPropertyConversionException {
- checkProperties();
- return properties.getBooleanProperty(SimpleString.toSimpleString(key, getPropertyKeysPool()));
+ return getOrCreateProperties().getBooleanProperty(key(key));
--- End diff --
can be delegated as: getBooleanProperty(key(key))
---
[GitHub] activemq-artemis pull request #2427: ARTEMIS-2170 Optimized CoreMessage's ch...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2427#discussion_r234697388
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/message/impl/CoreMessage.java ---
@@ -578,34 +567,41 @@ public CoreMessage setUserID(UUID userID) {
/**
* I am keeping this synchronized as the decode of the Properties is lazy
*/
- protected TypedProperties checkProperties() {
+ protected final TypedProperties getOrCreateProperties() {
--- End diff --
Just name this getProperties. The name otherwise is too confusing with your extracted method
---
[GitHub] activemq-artemis pull request #2427: ARTEMIS-2170 Optimized CoreMessage's ch...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2427#discussion_r234622417
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/message/impl/CoreMessage.java ---
@@ -803,139 +799,122 @@ public CoreMessage setDurable(boolean durable) {
@Override
public CoreMessage putBooleanProperty(final String key, final boolean value) {
messageChanged();
- checkProperties();
- properties.putBooleanProperty(SimpleString.toSimpleString(key, getPropertyKeysPool()), value);
+ getOrCreateProperties().putBooleanProperty(key(key), value);
return this;
}
@Override
public CoreMessage putBooleanProperty(final SimpleString key, final boolean value) {
messageChanged();
- checkProperties();
- properties.putBooleanProperty(key, value);
+ getOrCreateProperties().putBooleanProperty(key, value);
return this;
}
@Override
public Boolean getBooleanProperty(final SimpleString key) throws ActiveMQPropertyConversionException {
- checkProperties();
- return properties.getBooleanProperty(key);
+ return getOrCreateProperties().getBooleanProperty(key);
}
@Override
public Boolean getBooleanProperty(final String key) throws ActiveMQPropertyConversionException {
- checkProperties();
- return properties.getBooleanProperty(SimpleString.toSimpleString(key, getPropertyKeysPool()));
+ return getOrCreateProperties().getBooleanProperty(key(key));
}
@Override
public CoreMessage putByteProperty(final SimpleString key, final byte value) {
messageChanged();
- checkProperties();
- properties.putByteProperty(key, value);
+ getOrCreateProperties().putByteProperty(key, value);
return this;
}
@Override
public CoreMessage putByteProperty(final String key, final byte value) {
messageChanged();
- checkProperties();
- properties.putByteProperty(SimpleString.toSimpleString(key, getPropertyKeysPool()), value);
+ getOrCreateProperties().putByteProperty(key(key), value);
--- End diff --
can be delegated as: putByteProperty(key(key), value)
---
[GitHub] activemq-artemis pull request #2427: ARTEMIS-2170 Optimized CoreMessage's ch...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2427#discussion_r232498771
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/collections/TypedProperties.java ---
@@ -318,6 +320,33 @@ public synchronized boolean containsProperty(final SimpleString key) {
}
}
+ public synchronized boolean cleanupInternalProperties(Predicate<SimpleString> propertyNamePredicate) {
+ if (!internalProperties) {
--- End diff --
The biggest improvement here is exactly due to the fact that cleaning up properties atomically is a duty of who own the data ie TypedProperties.
About the message particulars I do believe that `TypedProperty` already contains some of them:
- internal property name prefix ie `TypedProperties.AMQ_PROPNAME`
- internal properties presence checks ie `TypedProperties::hasInternalProperties`
I have used the `Predicate` to allow decoupling, but I understand your point here: we can move it outer (as it is, algoritmically), but I'm not fully sure that it will bring the same benefit (eg enter/exit synchronize TypedProperty once).
Just to give some reason of this change: with this PR it is producing less then 1/4 of the garbage while having more then twice the throughput.
---
[GitHub] activemq-artemis pull request #2427: ARTEMIS-2170 Optimized CoreMessage's ch...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2427#discussion_r233520960
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/message/impl/CoreMessage.java ---
@@ -52,6 +52,62 @@
* consumers */
public class CoreMessage extends RefCountMessage implements ICoreMessage {
+ private static final class CoreTypedProperties extends TypedProperties {
+
+ // We use properties to establish routing context on clustering.
+ // However if the client resends the message after receiving, it needs to be removed
+ private static final Predicate<SimpleString> INTERNAL_PROPERTY_NAMES_CLEANUP_FILTER =
+ name -> (name.startsWith(Message.HDR_ROUTE_TO_IDS) && !name.equals(Message.HDR_ROUTE_TO_IDS)) ||
+ (name.startsWith(Message.HDR_ROUTE_TO_ACK_IDS) && !name.equals(Message.HDR_ROUTE_TO_ACK_IDS));
+ private static final SimpleString AMQ_PROPNAME = new SimpleString("_AMQ_");
+ private boolean internalProperties;
+
+ CoreTypedProperties() {
+ super();
+ internalProperties = false;
+ }
+
+ private CoreTypedProperties(TypedProperties other) {
+ super(other);
+ if (other instanceof CoreTypedProperties) {
+ internalProperties = ((CoreTypedProperties) other).internalProperties;
--- End diff --
I have used a static factory on this same class to do it :)
---
[GitHub] activemq-artemis pull request #2427: ARTEMIS-2170 Optimized CoreMessage's ch...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2427#discussion_r233496010
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/message/impl/CoreMessage.java ---
@@ -564,34 +604,59 @@ public CoreMessage setUserID(UUID userID) {
/**
* I am keeping this synchronized as the decode of the Properties is lazy
*/
- protected TypedProperties checkProperties() {
+ protected final TypedProperties checkProperties() {
try {
+ TypedProperties properties = this.properties;
if (properties == null) {
- synchronized (this) {
- if (properties == null) {
- TypedProperties properties = new TypedProperties();
- if (buffer != null && propertiesLocation >= 0) {
- final ByteBuf byteBuf = buffer.duplicate().readerIndex(propertiesLocation);
- properties.decode(byteBuf, coreMessageObjectPools == null ? null : coreMessageObjectPools.getPropertiesDecoderPools());
- }
- this.properties = properties;
- }
+ properties = getOrInitializeTypedProperties();
+ }
+ return properties;
+ } catch (Throwable e) {
+ throw onCheckPropertiesError(e);
+ }
+ }
+
+ private TypedProperties getOrInitializeTypedProperties() {
+ synchronized (this) {
--- End diff --
if youre separating this to its own method, which is nice +1, theres an extra trick which is to change from sync block to sync method, which will be marginally faster.
---
[GitHub] activemq-artemis pull request #2427: ARTEMIS-2170 Optimized CoreMessage's ch...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2427#discussion_r234767548
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/message/impl/CoreMessage.java ---
@@ -52,6 +52,11 @@
* consumers */
public class CoreMessage extends RefCountMessage implements ICoreMessage {
+ // We use properties to establish routing context on clustering.
+ // However if the client resends the message after receiving, it needs to be removed
+ private static final Predicate<SimpleString> INTERNAL_PROPERTY_NAMES_CLEANUP_FILTER =
--- End diff --
@franz1981 not in top of my mind. I would first check if properties = null, or empty... ? or add a parsed boolean? but i would need to debug to remember the best way.
---
[GitHub] activemq-artemis pull request #2427: ARTEMIS-2170 Optimized CoreMessage's ch...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2427#discussion_r234533807
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/message/impl/CoreMessage.java ---
@@ -52,6 +52,62 @@
* consumers */
public class CoreMessage extends RefCountMessage implements ICoreMessage {
+ private static final class CoreTypedProperties extends TypedProperties {
+
+ // We use properties to establish routing context on clustering.
+ // However if the client resends the message after receiving, it needs to be removed
+ private static final Predicate<SimpleString> INTERNAL_PROPERTY_NAMES_CLEANUP_FILTER =
+ name -> (name.startsWith(Message.HDR_ROUTE_TO_IDS) && !name.equals(Message.HDR_ROUTE_TO_IDS)) ||
+ (name.startsWith(Message.HDR_ROUTE_TO_ACK_IDS) && !name.equals(Message.HDR_ROUTE_TO_ACK_IDS));
+ private static final SimpleString AMQ_PROPNAME = new SimpleString("_AMQ_");
+ private boolean internalProperties;
+
+ CoreTypedProperties() {
+ super();
+ internalProperties = false;
+ }
+
+ private CoreTypedProperties(TypedProperties other) {
+ super(other);
+ if (other instanceof CoreTypedProperties) {
+ internalProperties = ((CoreTypedProperties) other).internalProperties;
+ } else {
+ internalProperties = other.containsProperty(name -> name.startsWith(AMQ_PROPNAME));
+ }
+ }
+
+ @Override
+ protected synchronized void doPutValue(SimpleString key, TypedProperties.PropertyValue value) {
+ if (!internalProperties && key.startsWith(AMQ_PROPNAME)) {
+ internalProperties = true;
+ }
+ super.doPutValue(key, value);
+ }
+
+ public synchronized boolean cleanupInternalProperties() {
+ if (!internalProperties) {
+ return false;
+ }
+ return removeProperty(INTERNAL_PROPERTY_NAMES_CLEANUP_FILTER);
+ }
+
+ public static boolean cleanupInternalProperties(TypedProperties typedProperties) {
+ if (typedProperties == null) {
+ return false;
+ }
+ if (typedProperties instanceof CoreTypedProperties) {
+ return ((CoreTypedProperties) typedProperties).cleanupInternalProperties();
+ }
+ return typedProperties.removeProperty(INTERNAL_PROPERTY_NAMES_CLEANUP_FILTER);
+ }
+
+ public static CoreTypedProperties copyOf(TypedProperties typedProperties) {
+ synchronized (typedProperties) {
--- End diff --
Yep, I was expecting it to be, but reality was that even with that flag it would fail and will iterate through the properties: the flag was only checking if *any* internal properties is being put, while the `cleanupInternlProperties` aim to cleanup just specific internal ones.
---
[GitHub] activemq-artemis pull request #2427: ARTEMIS-2170 Optimized CoreMessage's ch...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2427#discussion_r232490920
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/collections/TypedProperties.java ---
@@ -318,6 +320,33 @@ public synchronized boolean containsProperty(final SimpleString key) {
}
}
+ public synchronized boolean cleanupInternalProperties(Predicate<SimpleString> propertyNamePredicate) {
+ if (!internalProperties) {
--- End diff --
-1 moving this to TypedProperties, as typedproperties is generic and used not just for message. It shouldnt have message particulars. Would you be against moving this back to coremessage? Or to messageutils
---
[GitHub] activemq-artemis pull request #2427: ARTEMIS-2170 Optimized CoreMessage's ch...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2427#discussion_r233491698
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/message/impl/CoreMessage.java ---
@@ -1133,8 +1158,7 @@ public Object removeProperty(final SimpleString key) {
@Override
public Object removeProperty(final String key) {
messageChanged();
- checkProperties();
- Object oldValue = properties.removeProperty(SimpleString.toSimpleString(key, getPropertyKeysPool()));
+ Object oldValue = checkProperties().removeProperty(SimpleString.toSimpleString(key, getPropertyKeysPool()));
--- End diff --
delegate to
public Object removeProperty(final SimpleString key) {
---
[GitHub] activemq-artemis pull request #2427: ARTEMIS-2170 Optimized CoreMessage's ch...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2427#discussion_r232560925
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/collections/TypedProperties.java ---
@@ -318,6 +320,33 @@ public synchronized boolean containsProperty(final SimpleString key) {
}
}
+ public synchronized boolean cleanupInternalProperties(Predicate<SimpleString> propertyNamePredicate) {
+ if (!internalProperties) {
--- End diff --
I agree: in that case I would move the other `Message`ish things outside `TypedProperties`.
I was hoping for a less impactfull change TBH, but you really got a point about this. Let me check if I can perform a surgical removal to make the code simpler and cleaner :+1:
---
[GitHub] activemq-artemis pull request #2427: ARTEMIS-2170 Optimized CoreMessage's ch...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2427#discussion_r233490193
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/message/impl/CoreMessage.java ---
@@ -564,34 +604,59 @@ public CoreMessage setUserID(UUID userID) {
/**
* I am keeping this synchronized as the decode of the Properties is lazy
*/
- protected TypedProperties checkProperties() {
+ protected final TypedProperties checkProperties() {
--- End diff --
could this be renamed getProperties as it seems much like a getter method.
---
[GitHub] activemq-artemis pull request #2427: ARTEMIS-2170 Optimized CoreMessage's ch...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2427#discussion_r234941860
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/message/impl/CoreMessage.java ---
@@ -52,6 +52,11 @@
* consumers */
public class CoreMessage extends RefCountMessage implements ICoreMessage {
+ // We use properties to establish routing context on clustering.
+ // However if the client resends the message after receiving, it needs to be removed
+ private static final Predicate<SimpleString> INTERNAL_PROPERTY_NAMES_CLEANUP_FILTER =
--- End diff --
Good catch!
I'm looking from the tablet so I hope to have searched all the interesting points, but it seems to me that HDR_ROUTE_TO_ACK_IDS is not needed anymore? Or am I missing anything?
`Message.HDR_ROUTE_TO_IDS.concat(bridgeName);` is using HDR_ROUTE_TO_IDS in a way that it will match the `CoreMessage::cleanupInternalProperties` cases, but for HDR_ROUTE_TO_ACK_IDS I can't see any points doing anything similar: just using the entire property name ie it won't match the `CoreMessage::cleanupInternalProperties` checks.
---
[GitHub] activemq-artemis pull request #2427: ARTEMIS-2170 Optimized CoreMessage's ch...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2427#discussion_r233526079
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/message/impl/CoreMessage.java ---
@@ -52,6 +52,62 @@
* consumers */
public class CoreMessage extends RefCountMessage implements ICoreMessage {
+ private static final class CoreTypedProperties extends TypedProperties {
+
+ // We use properties to establish routing context on clustering.
+ // However if the client resends the message after receiving, it needs to be removed
+ private static final Predicate<SimpleString> INTERNAL_PROPERTY_NAMES_CLEANUP_FILTER =
+ name -> (name.startsWith(Message.HDR_ROUTE_TO_IDS) && !name.equals(Message.HDR_ROUTE_TO_IDS)) ||
+ (name.startsWith(Message.HDR_ROUTE_TO_ACK_IDS) && !name.equals(Message.HDR_ROUTE_TO_ACK_IDS));
+ private static final SimpleString AMQ_PROPNAME = new SimpleString("_AMQ_");
+ private boolean internalProperties;
+
+ CoreTypedProperties() {
+ super();
+ internalProperties = false;
+ }
+
+ private CoreTypedProperties(TypedProperties other) {
+ super(other);
+ if (other instanceof CoreTypedProperties) {
+ internalProperties = ((CoreTypedProperties) other).internalProperties;
+ } else {
+ internalProperties = other.containsProperty(name -> name.startsWith(AMQ_PROPNAME));
+ }
+ }
+
+ @Override
+ protected synchronized void doPutValue(SimpleString key, TypedProperties.PropertyValue value) {
+ if (!internalProperties && key.startsWith(AMQ_PROPNAME)) {
+ internalProperties = true;
+ }
+ super.doPutValue(key, value);
+ }
+
+ public synchronized boolean cleanupInternalProperties() {
+ if (!internalProperties) {
+ return false;
+ }
+ return removeProperty(INTERNAL_PROPERTY_NAMES_CLEANUP_FILTER);
+ }
+
+ public static boolean cleanupInternalProperties(TypedProperties typedProperties) {
+ if (typedProperties == null) {
+ return false;
+ }
+ if (typedProperties instanceof CoreTypedProperties) {
+ return ((CoreTypedProperties) typedProperties).cleanupInternalProperties();
+ }
+ return typedProperties.removeProperty(INTERNAL_PROPERTY_NAMES_CLEANUP_FILTER);
+ }
+
+ public static CoreTypedProperties copyOf(TypedProperties typedProperties) {
+ synchronized (typedProperties) {
--- End diff --
You can't put a synchronized before the call to `super(other)`, that's why I have used a factory: if I will drop `CoreTypedProperties` all of this will disappear right?
---
[GitHub] activemq-artemis issue #2427: ARTEMIS-2170 Optimized CoreMessage's checkProp...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:
https://github.com/apache/activemq-artemis/pull/2427
I need to run a full CI on this one: in the meantime please take time to review it, given that is changing parts that have been recently cause of several concurrency bugs.
My change shouldn't introduce any semantic changes, but is addressing the concurrent changes on TypedProperties in a more optimized and consistent manner :+1:
---
[GitHub] activemq-artemis pull request #2427: ARTEMIS-2170 Optimized CoreMessage's ch...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2427#discussion_r233491975
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/message/impl/CoreMessage.java ---
@@ -1143,20 +1167,17 @@ public Object removeProperty(final String key) {
@Override
public boolean containsProperty(final SimpleString key) {
- checkProperties();
- return properties.containsProperty(key);
+ return checkProperties().containsProperty(key);
}
@Override
public boolean containsProperty(final String key) {
- checkProperties();
- return properties.containsProperty(SimpleString.toSimpleString(key, getPropertyKeysPool()));
+ return checkProperties().containsProperty(SimpleString.toSimpleString(key, getPropertyKeysPool()));
--- End diff --
delegate to:
public boolean containsProperty(final SimpleString key) {
Just repeat this review comment on all methods taking a String key ;) to delegate to the SimpleString equiv.
---
[GitHub] activemq-artemis pull request #2427: ARTEMIS-2170 Optimized CoreMessage's ch...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2427#discussion_r232590511
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/collections/TypedProperties.java ---
@@ -318,6 +320,33 @@ public synchronized boolean containsProperty(final SimpleString key) {
}
}
+ public synchronized boolean cleanupInternalProperties(Predicate<SimpleString> propertyNamePredicate) {
+ if (!internalProperties) {
--- End diff --
@michaelandrepearce I'm working on `CoreMessage` to inject into it both `internalProperties` and the cleanupLogic, but TBH the concurrent behaviour is not trivial: it has to be consistent with `TypedProperties` content assuming that the volatile reference of it into `CoreMessage` could change: I'm not sure it would be that easy to change it
---
[GitHub] activemq-artemis pull request #2427: ARTEMIS-2170 Optimized CoreMessage's ch...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2427#discussion_r235089654
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/message/impl/CoreMessage.java ---
@@ -52,6 +52,11 @@
* consumers */
public class CoreMessage extends RefCountMessage implements ICoreMessage {
+ // We use properties to establish routing context on clustering.
+ // However if the client resends the message after receiving, it needs to be removed
+ private static final Predicate<SimpleString> INTERNAL_PROPERTY_NAMES_CLEANUP_FILTER =
--- End diff --
You'd be better asking people like martyn or clebert to confirm. But the looks of it there seems some streamlining to be done.
---
[GitHub] activemq-artemis issue #2427: ARTEMIS-2170 Optimized CoreMessage's checkProp...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:
https://github.com/apache/activemq-artemis/pull/2427
@michaelandrepearce I think that this PR is in a good shape: ATM we are near holidays, but I hope the new year it will be merged :+1:
---
[GitHub] activemq-artemis pull request #2427: ARTEMIS-2170 Optimized CoreMessage's ch...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2427#discussion_r233520221
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/message/impl/CoreMessage.java ---
@@ -52,6 +52,62 @@
* consumers */
public class CoreMessage extends RefCountMessage implements ICoreMessage {
+ private static final class CoreTypedProperties extends TypedProperties {
+
+ // We use properties to establish routing context on clustering.
+ // However if the client resends the message after receiving, it needs to be removed
+ private static final Predicate<SimpleString> INTERNAL_PROPERTY_NAMES_CLEANUP_FILTER =
+ name -> (name.startsWith(Message.HDR_ROUTE_TO_IDS) && !name.equals(Message.HDR_ROUTE_TO_IDS)) ||
+ (name.startsWith(Message.HDR_ROUTE_TO_ACK_IDS) && !name.equals(Message.HDR_ROUTE_TO_ACK_IDS));
+ private static final SimpleString AMQ_PROPNAME = new SimpleString("_AMQ_");
+ private boolean internalProperties;
+
+ CoreTypedProperties() {
+ super();
+ internalProperties = false;
+ }
+
+ private CoreTypedProperties(TypedProperties other) {
+ super(other);
+ if (other instanceof CoreTypedProperties) {
+ internalProperties = ((CoreTypedProperties) other).internalProperties;
--- End diff --
CoreTypedProperties is not sync safe due to direct access to ((CoreTypedProperties) other).internalProperties;
Best solution is to create a private syncronized get method to access internalProperties from the other variable. This then means sync bloc in copyOf method is not needed and this constructor becomes sync safe.
---
[GitHub] activemq-artemis pull request #2427: ARTEMIS-2170 Optimized CoreMessage's ch...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2427#discussion_r232491068
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/message/impl/CoreMessage.java ---
@@ -887,55 +876,48 @@ public CoreMessage putBytesProperty(final String key, final byte[] value) {
@Override
public CoreMessage putCharProperty(SimpleString key, char value) {
messageChanged();
- checkProperties();
- properties.putCharProperty(key, value);
+ checkProperties().putCharProperty(key, value);
return this;
}
@Override
public CoreMessage putCharProperty(String key, char value) {
messageChanged();
- checkProperties();
- properties.putCharProperty(SimpleString.toSimpleString(key, getPropertyKeysPool()), value);
+ checkProperties().putCharProperty(SimpleString.toSimpleString(key, getPropertyKeysPool()), value);
--- End diff --
Delegate to simplestring method variant. Ditto for all cases on this
---
[GitHub] activemq-artemis pull request #2427: ARTEMIS-2170 Optimized CoreMessage's ch...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2427#discussion_r234622813
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/message/impl/CoreMessage.java ---
@@ -803,139 +799,122 @@ public CoreMessage setDurable(boolean durable) {
@Override
public CoreMessage putBooleanProperty(final String key, final boolean value) {
messageChanged();
- checkProperties();
- properties.putBooleanProperty(SimpleString.toSimpleString(key, getPropertyKeysPool()), value);
+ getOrCreateProperties().putBooleanProperty(key(key), value);
return this;
}
@Override
public CoreMessage putBooleanProperty(final SimpleString key, final boolean value) {
messageChanged();
- checkProperties();
- properties.putBooleanProperty(key, value);
+ getOrCreateProperties().putBooleanProperty(key, value);
return this;
}
@Override
public Boolean getBooleanProperty(final SimpleString key) throws ActiveMQPropertyConversionException {
- checkProperties();
- return properties.getBooleanProperty(key);
+ return getOrCreateProperties().getBooleanProperty(key);
}
@Override
public Boolean getBooleanProperty(final String key) throws ActiveMQPropertyConversionException {
- checkProperties();
- return properties.getBooleanProperty(SimpleString.toSimpleString(key, getPropertyKeysPool()));
+ return getOrCreateProperties().getBooleanProperty(key(key));
}
@Override
public CoreMessage putByteProperty(final SimpleString key, final byte value) {
messageChanged();
- checkProperties();
- properties.putByteProperty(key, value);
+ getOrCreateProperties().putByteProperty(key, value);
return this;
}
@Override
public CoreMessage putByteProperty(final String key, final byte value) {
messageChanged();
- checkProperties();
- properties.putByteProperty(SimpleString.toSimpleString(key, getPropertyKeysPool()), value);
+ getOrCreateProperties().putByteProperty(key(key), value);
return this;
}
@Override
public Byte getByteProperty(final SimpleString key) throws ActiveMQPropertyConversionException {
- checkProperties();
- return properties.getByteProperty(key);
+ return getOrCreateProperties().getByteProperty(key);
}
@Override
public Byte getByteProperty(final String key) throws ActiveMQPropertyConversionException {
- return getByteProperty(SimpleString.toSimpleString(key, getPropertyKeysPool()));
+ return getByteProperty(key(key));
}
@Override
public CoreMessage putBytesProperty(final SimpleString key, final byte[] value) {
messageChanged();
- checkProperties();
- properties.putBytesProperty(key, value);
+ getOrCreateProperties().putBytesProperty(key, value);
return this;
}
@Override
public CoreMessage putBytesProperty(final String key, final byte[] value) {
messageChanged();
- checkProperties();
- properties.putBytesProperty(SimpleString.toSimpleString(key, getPropertyKeysPool()), value);
+ getOrCreateProperties().putBytesProperty(key(key), value);
return this;
}
@Override
public byte[] getBytesProperty(final SimpleString key) throws ActiveMQPropertyConversionException {
- checkProperties();
- return properties.getBytesProperty(key);
+ return getOrCreateProperties().getBytesProperty(key);
}
@Override
public byte[] getBytesProperty(final String key) throws ActiveMQPropertyConversionException {
- return getBytesProperty(SimpleString.toSimpleString(key, getPropertyKeysPool()));
+ return getBytesProperty(key(key));
}
@Override
public CoreMessage putCharProperty(SimpleString key, char value) {
messageChanged();
- checkProperties();
- properties.putCharProperty(key, value);
+ getOrCreateProperties().putCharProperty(key, value);
return this;
}
@Override
public CoreMessage putCharProperty(String key, char value) {
messageChanged();
- checkProperties();
- properties.putCharProperty(SimpleString.toSimpleString(key, getPropertyKeysPool()), value);
+ getOrCreateProperties().putCharProperty(key(key), value);
--- End diff --
can be delegated as: putCharProperty(key(key), value)
---
[GitHub] activemq-artemis pull request #2427: ARTEMIS-2170 Optimized CoreMessage's ch...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2427#discussion_r234637863
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/message/impl/CoreMessage.java ---
@@ -803,139 +799,122 @@ public CoreMessage setDurable(boolean durable) {
@Override
public CoreMessage putBooleanProperty(final String key, final boolean value) {
messageChanged();
- checkProperties();
- properties.putBooleanProperty(SimpleString.toSimpleString(key, getPropertyKeysPool()), value);
+ getOrCreateProperties().putBooleanProperty(key(key), value);
return this;
}
@Override
public CoreMessage putBooleanProperty(final SimpleString key, final boolean value) {
messageChanged();
- checkProperties();
- properties.putBooleanProperty(key, value);
+ getOrCreateProperties().putBooleanProperty(key, value);
return this;
}
@Override
public Boolean getBooleanProperty(final SimpleString key) throws ActiveMQPropertyConversionException {
- checkProperties();
- return properties.getBooleanProperty(key);
+ return getOrCreateProperties().getBooleanProperty(key);
}
@Override
public Boolean getBooleanProperty(final String key) throws ActiveMQPropertyConversionException {
- checkProperties();
- return properties.getBooleanProperty(SimpleString.toSimpleString(key, getPropertyKeysPool()));
+ return getOrCreateProperties().getBooleanProperty(key(key));
}
@Override
public CoreMessage putByteProperty(final SimpleString key, final byte value) {
messageChanged();
- checkProperties();
- properties.putByteProperty(key, value);
+ getOrCreateProperties().putByteProperty(key, value);
return this;
}
@Override
public CoreMessage putByteProperty(final String key, final byte value) {
messageChanged();
- checkProperties();
- properties.putByteProperty(SimpleString.toSimpleString(key, getPropertyKeysPool()), value);
+ getOrCreateProperties().putByteProperty(key(key), value);
return this;
}
@Override
public Byte getByteProperty(final SimpleString key) throws ActiveMQPropertyConversionException {
- checkProperties();
- return properties.getByteProperty(key);
+ return getOrCreateProperties().getByteProperty(key);
}
@Override
public Byte getByteProperty(final String key) throws ActiveMQPropertyConversionException {
- return getByteProperty(SimpleString.toSimpleString(key, getPropertyKeysPool()));
+ return getByteProperty(key(key));
}
@Override
public CoreMessage putBytesProperty(final SimpleString key, final byte[] value) {
messageChanged();
- checkProperties();
- properties.putBytesProperty(key, value);
+ getOrCreateProperties().putBytesProperty(key, value);
return this;
}
@Override
public CoreMessage putBytesProperty(final String key, final byte[] value) {
messageChanged();
- checkProperties();
- properties.putBytesProperty(SimpleString.toSimpleString(key, getPropertyKeysPool()), value);
+ getOrCreateProperties().putBytesProperty(key(key), value);
return this;
}
@Override
public byte[] getBytesProperty(final SimpleString key) throws ActiveMQPropertyConversionException {
- checkProperties();
- return properties.getBytesProperty(key);
+ return getOrCreateProperties().getBytesProperty(key);
}
@Override
public byte[] getBytesProperty(final String key) throws ActiveMQPropertyConversionException {
- return getBytesProperty(SimpleString.toSimpleString(key, getPropertyKeysPool()));
+ return getBytesProperty(key(key));
}
@Override
public CoreMessage putCharProperty(SimpleString key, char value) {
messageChanged();
- checkProperties();
- properties.putCharProperty(key, value);
+ getOrCreateProperties().putCharProperty(key, value);
return this;
}
@Override
public CoreMessage putCharProperty(String key, char value) {
messageChanged();
- checkProperties();
- properties.putCharProperty(SimpleString.toSimpleString(key, getPropertyKeysPool()), value);
+ getOrCreateProperties().putCharProperty(key(key), value);
--- End diff --
By fixing those I have found some `getXXXProperty` that wasn't doing any pooling of the key property String: I hope to have done right to have turned them into pooled ones :P
---
[GitHub] activemq-artemis pull request #2427: ARTEMIS-2170 Optimized CoreMessage's ch...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2427#discussion_r234622539
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/message/impl/CoreMessage.java ---
@@ -803,139 +799,122 @@ public CoreMessage setDurable(boolean durable) {
@Override
public CoreMessage putBooleanProperty(final String key, final boolean value) {
messageChanged();
- checkProperties();
- properties.putBooleanProperty(SimpleString.toSimpleString(key, getPropertyKeysPool()), value);
+ getOrCreateProperties().putBooleanProperty(key(key), value);
return this;
}
@Override
public CoreMessage putBooleanProperty(final SimpleString key, final boolean value) {
messageChanged();
- checkProperties();
- properties.putBooleanProperty(key, value);
+ getOrCreateProperties().putBooleanProperty(key, value);
return this;
}
@Override
public Boolean getBooleanProperty(final SimpleString key) throws ActiveMQPropertyConversionException {
- checkProperties();
- return properties.getBooleanProperty(key);
+ return getOrCreateProperties().getBooleanProperty(key);
}
@Override
public Boolean getBooleanProperty(final String key) throws ActiveMQPropertyConversionException {
- checkProperties();
- return properties.getBooleanProperty(SimpleString.toSimpleString(key, getPropertyKeysPool()));
+ return getOrCreateProperties().getBooleanProperty(key(key));
}
@Override
public CoreMessage putByteProperty(final SimpleString key, final byte value) {
messageChanged();
- checkProperties();
- properties.putByteProperty(key, value);
+ getOrCreateProperties().putByteProperty(key, value);
return this;
}
@Override
public CoreMessage putByteProperty(final String key, final byte value) {
messageChanged();
- checkProperties();
- properties.putByteProperty(SimpleString.toSimpleString(key, getPropertyKeysPool()), value);
+ getOrCreateProperties().putByteProperty(key(key), value);
return this;
}
@Override
public Byte getByteProperty(final SimpleString key) throws ActiveMQPropertyConversionException {
- checkProperties();
- return properties.getByteProperty(key);
+ return getOrCreateProperties().getByteProperty(key);
}
@Override
public Byte getByteProperty(final String key) throws ActiveMQPropertyConversionException {
- return getByteProperty(SimpleString.toSimpleString(key, getPropertyKeysPool()));
+ return getByteProperty(key(key));
}
@Override
public CoreMessage putBytesProperty(final SimpleString key, final byte[] value) {
messageChanged();
- checkProperties();
- properties.putBytesProperty(key, value);
+ getOrCreateProperties().putBytesProperty(key, value);
return this;
}
@Override
public CoreMessage putBytesProperty(final String key, final byte[] value) {
messageChanged();
- checkProperties();
- properties.putBytesProperty(SimpleString.toSimpleString(key, getPropertyKeysPool()), value);
+ getOrCreateProperties().putBytesProperty(key(key), value);
--- End diff --
can be delegated as: putBytesProperty(key(key), value)
---
[GitHub] activemq-artemis pull request #2427: ARTEMIS-2170 Optimized CoreMessage's ch...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2427#discussion_r234906424
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/message/impl/CoreMessage.java ---
@@ -578,34 +567,41 @@ public CoreMessage setUserID(UUID userID) {
/**
* I am keeping this synchronized as the decode of the Properties is lazy
*/
- protected TypedProperties checkProperties() {
+ protected final TypedProperties getOrCreateProperties() {
--- End diff --
The reason why I haven't done it is due to `ClientMessageImpl extends CoreMessage implements ClientMessageInternal`: `ClientMessageInternal` has already a `geProperties` method while `CoreMessage::getProperties` has been turned into final...
I can drop the final just to have a better name, but I would prefer to keep it final to avoid any child of `CoreMessage` to broke its thread-safeness contract by overriding it.
---
[GitHub] activemq-artemis pull request #2427: ARTEMIS-2170 Optimized CoreMessage's ch...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2427#discussion_r232491023
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/message/impl/CoreMessage.java ---
@@ -803,52 +802,45 @@ public CoreMessage setDurable(boolean durable) {
@Override
public CoreMessage putBooleanProperty(final String key, final boolean value) {
messageChanged();
- checkProperties();
- properties.putBooleanProperty(SimpleString.toSimpleString(key, getPropertyKeysPool()), value);
+ checkProperties().putBooleanProperty(SimpleString.toSimpleString(key, getPropertyKeysPool()), value);
--- End diff --
Why not make this delegate to method that takes simple string
---
[GitHub] activemq-artemis pull request #2427: ARTEMIS-2170 Optimized CoreMessage's ch...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2427#discussion_r233491522
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/message/impl/CoreMessage.java ---
@@ -52,6 +52,62 @@
* consumers */
public class CoreMessage extends RefCountMessage implements ICoreMessage {
+ private static final class CoreTypedProperties extends TypedProperties {
--- End diff --
I have maintained the original logic and consistency with this PR, but we can just cleaning them without checking their presence too making a lot more simpler everything
---
[GitHub] activemq-artemis pull request #2427: ARTEMIS-2170 Optimized CoreMessage's ch...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2427#discussion_r233497082
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/message/impl/CoreMessage.java ---
@@ -52,6 +52,62 @@
* consumers */
public class CoreMessage extends RefCountMessage implements ICoreMessage {
+ private static final class CoreTypedProperties extends TypedProperties {
--- End diff --
I think go for broke, if youre doing a refactor, just to it all :).
---
[GitHub] activemq-artemis pull request #2427: ARTEMIS-2170 Optimized CoreMessage's ch...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2427#discussion_r234903527
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/message/impl/CoreMessage.java ---
@@ -803,139 +799,122 @@ public CoreMessage setDurable(boolean durable) {
@Override
public CoreMessage putBooleanProperty(final String key, final boolean value) {
messageChanged();
- checkProperties();
- properties.putBooleanProperty(SimpleString.toSimpleString(key, getPropertyKeysPool()), value);
+ getOrCreateProperties().putBooleanProperty(key(key), value);
return this;
}
@Override
public CoreMessage putBooleanProperty(final SimpleString key, final boolean value) {
messageChanged();
- checkProperties();
- properties.putBooleanProperty(key, value);
+ getOrCreateProperties().putBooleanProperty(key, value);
return this;
}
@Override
public Boolean getBooleanProperty(final SimpleString key) throws ActiveMQPropertyConversionException {
- checkProperties();
- return properties.getBooleanProperty(key);
+ return getOrCreateProperties().getBooleanProperty(key);
}
@Override
public Boolean getBooleanProperty(final String key) throws ActiveMQPropertyConversionException {
- checkProperties();
- return properties.getBooleanProperty(SimpleString.toSimpleString(key, getPropertyKeysPool()));
+ return getOrCreateProperties().getBooleanProperty(key(key));
}
@Override
public CoreMessage putByteProperty(final SimpleString key, final byte value) {
messageChanged();
- checkProperties();
- properties.putByteProperty(key, value);
+ getOrCreateProperties().putByteProperty(key, value);
return this;
}
@Override
public CoreMessage putByteProperty(final String key, final byte value) {
messageChanged();
- checkProperties();
- properties.putByteProperty(SimpleString.toSimpleString(key, getPropertyKeysPool()), value);
+ getOrCreateProperties().putByteProperty(key(key), value);
return this;
}
@Override
public Byte getByteProperty(final SimpleString key) throws ActiveMQPropertyConversionException {
- checkProperties();
- return properties.getByteProperty(key);
+ return getOrCreateProperties().getByteProperty(key);
}
@Override
public Byte getByteProperty(final String key) throws ActiveMQPropertyConversionException {
- return getByteProperty(SimpleString.toSimpleString(key, getPropertyKeysPool()));
+ return getByteProperty(key(key));
}
@Override
public CoreMessage putBytesProperty(final SimpleString key, final byte[] value) {
messageChanged();
- checkProperties();
- properties.putBytesProperty(key, value);
+ getOrCreateProperties().putBytesProperty(key, value);
return this;
}
@Override
public CoreMessage putBytesProperty(final String key, final byte[] value) {
messageChanged();
- checkProperties();
- properties.putBytesProperty(SimpleString.toSimpleString(key, getPropertyKeysPool()), value);
+ getOrCreateProperties().putBytesProperty(key(key), value);
return this;
}
@Override
public byte[] getBytesProperty(final SimpleString key) throws ActiveMQPropertyConversionException {
- checkProperties();
- return properties.getBytesProperty(key);
+ return getOrCreateProperties().getBytesProperty(key);
}
@Override
public byte[] getBytesProperty(final String key) throws ActiveMQPropertyConversionException {
- return getBytesProperty(SimpleString.toSimpleString(key, getPropertyKeysPool()));
+ return getBytesProperty(key(key));
}
@Override
public CoreMessage putCharProperty(SimpleString key, char value) {
messageChanged();
- checkProperties();
- properties.putCharProperty(key, value);
+ getOrCreateProperties().putCharProperty(key, value);
return this;
}
@Override
public CoreMessage putCharProperty(String key, char value) {
messageChanged();
- checkProperties();
- properties.putCharProperty(SimpleString.toSimpleString(key, getPropertyKeysPool()), value);
+ getOrCreateProperties().putCharProperty(key(key), value);
--- End diff --
+1
---
[GitHub] activemq-artemis pull request #2427: ARTEMIS-2170 Optimized CoreMessage's ch...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2427#discussion_r233513078
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/message/impl/CoreMessage.java ---
@@ -564,34 +604,59 @@ public CoreMessage setUserID(UUID userID) {
/**
* I am keeping this synchronized as the decode of the Properties is lazy
*/
- protected TypedProperties checkProperties() {
+ protected final TypedProperties checkProperties() {
try {
+ TypedProperties properties = this.properties;
if (properties == null) {
- synchronized (this) {
- if (properties == null) {
- TypedProperties properties = new TypedProperties();
- if (buffer != null && propertiesLocation >= 0) {
- final ByteBuf byteBuf = buffer.duplicate().readerIndex(propertiesLocation);
- properties.decode(byteBuf, coreMessageObjectPools == null ? null : coreMessageObjectPools.getPropertiesDecoderPools());
- }
- this.properties = properties;
- }
+ properties = getOrInitializeTypedProperties();
+ }
+ return properties;
+ } catch (Throwable e) {
+ throw onCheckPropertiesError(e);
+ }
+ }
+
+ private TypedProperties getOrInitializeTypedProperties() {
+ synchronized (this) {
+ TypedProperties properties = this.properties;
+ if (properties == null) {
+ properties = createTypedProperties();
+ if (buffer != null && propertiesLocation >= 0) {
+ final ByteBuf byteBuf = buffer.duplicate().readerIndex(propertiesLocation);
+ properties.decode(byteBuf, coreMessageObjectPools == null ? null : coreMessageObjectPools.getPropertiesDecoderPools());
}
+ this.properties = properties;
}
+ return properties;
+ }
+ }
- return this.properties;
- } catch (Throwable e) {
- ByteBuf duplicatebuffer = buffer.duplicate();
- duplicatebuffer.readerIndex(0);
+ private RuntimeException onCheckPropertiesError(Throwable e) {
+ ByteBuf duplicatebuffer = buffer.duplicate();
+ duplicatebuffer.readerIndex(0);
- // This is not an expected error, hence no specific logger created
- logger.warn("Could not decode properties for CoreMessage[messageID=" + messageID + ",durable=" + durable + ",userID=" + userID + ",priority=" + priority +
- ", timestamp=" + timestamp + ",expiration=" + expiration + ",address=" + address + ", propertiesLocation=" + propertiesLocation, e);
- logger.warn("Failed message has messageID=" + messageID + " and the following buffer:\n" + ByteBufUtil.prettyHexDump(duplicatebuffer));
+ // This is not an expected error, hence no specific logger created
+ logger.warn("Could not decode properties for CoreMessage[messageID=" + messageID + ",durable=" + durable + ",userID=" + userID + ",priority=" + priority +
+ ", timestamp=" + timestamp + ",expiration=" + expiration + ",address=" + address + ", propertiesLocation=" + propertiesLocation, e);
+ logger.warn("Failed message has messageID=" + messageID + " and the following buffer:\n" + ByteBufUtil.prettyHexDump(duplicatebuffer));
- throw new RuntimeException(e.getMessage(), e);
+ return new RuntimeException(e.getMessage(), e);
--- End diff --
This really should throw the original exception, dont wrap it into something else, incase other code was catching something explicit.
---
[GitHub] activemq-artemis pull request #2427: ARTEMIS-2170 Optimized CoreMessage's ch...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2427#discussion_r233197359
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/collections/TypedProperties.java ---
@@ -318,6 +313,46 @@ public synchronized boolean containsProperty(final SimpleString key) {
}
}
+ public synchronized boolean anyPropertyNameMatch(Predicate<SimpleString> propertyNamePredicate) {
+ Objects.requireNonNull(propertyNamePredicate, "propertyNamePredicate cannot be null");
+ if (properties == null) {
+ return false;
+ }
+ if (properties.isEmpty()) {
+ return false;
+ }
+ for (SimpleString propertyName : properties.keySet()) {
+ if (propertyNamePredicate.test(propertyName)) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ public synchronized boolean removeIfPropertyNameMatch(Predicate<SimpleString> propertyNamePredicate) {
--- End diff --
this is just a remove method , the args give the context that it removes only if predicate would match
---
[GitHub] activemq-artemis pull request #2427: ARTEMIS-2170 Optimized CoreMessage's ch...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2427#discussion_r233205369
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/collections/TypedProperties.java ---
@@ -318,6 +320,33 @@ public synchronized boolean containsProperty(final SimpleString key) {
}
}
+ public synchronized boolean cleanupInternalProperties(Predicate<SimpleString> propertyNamePredicate) {
+ if (!internalProperties) {
--- End diff --
I have added a super tiny optimization on `PacketImpl` turning a method into a static one: using https://github.com/AdoptOpenJDK/jitwatch to read the compilation logs seems to help the inlining of the method (for free). I would prefer to maintain separated the optimizations, but a single PR for a single word change (not a bug fix) seems really too much for me...wdyt?
---
[GitHub] activemq-artemis pull request #2427: ARTEMIS-2170 Optimized CoreMessage's ch...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2427#discussion_r233559690
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/message/impl/CoreMessage.java ---
@@ -52,6 +52,62 @@
* consumers */
public class CoreMessage extends RefCountMessage implements ICoreMessage {
+ private static final class CoreTypedProperties extends TypedProperties {
+
+ // We use properties to establish routing context on clustering.
+ // However if the client resends the message after receiving, it needs to be removed
+ private static final Predicate<SimpleString> INTERNAL_PROPERTY_NAMES_CLEANUP_FILTER =
+ name -> (name.startsWith(Message.HDR_ROUTE_TO_IDS) && !name.equals(Message.HDR_ROUTE_TO_IDS)) ||
+ (name.startsWith(Message.HDR_ROUTE_TO_ACK_IDS) && !name.equals(Message.HDR_ROUTE_TO_ACK_IDS));
+ private static final SimpleString AMQ_PROPNAME = new SimpleString("_AMQ_");
+ private boolean internalProperties;
+
+ CoreTypedProperties() {
+ super();
+ internalProperties = false;
+ }
+
+ private CoreTypedProperties(TypedProperties other) {
+ super(other);
+ if (other instanceof CoreTypedProperties) {
+ internalProperties = ((CoreTypedProperties) other).internalProperties;
+ } else {
+ internalProperties = other.containsProperty(name -> name.startsWith(AMQ_PROPNAME));
+ }
+ }
+
+ @Override
+ protected synchronized void doPutValue(SimpleString key, TypedProperties.PropertyValue value) {
+ if (!internalProperties && key.startsWith(AMQ_PROPNAME)) {
+ internalProperties = true;
+ }
+ super.doPutValue(key, value);
+ }
+
+ public synchronized boolean cleanupInternalProperties() {
+ if (!internalProperties) {
+ return false;
+ }
+ return removeProperty(INTERNAL_PROPERTY_NAMES_CLEANUP_FILTER);
+ }
+
+ public static boolean cleanupInternalProperties(TypedProperties typedProperties) {
+ if (typedProperties == null) {
+ return false;
+ }
+ if (typedProperties instanceof CoreTypedProperties) {
+ return ((CoreTypedProperties) typedProperties).cleanupInternalProperties();
+ }
+ return typedProperties.removeProperty(INTERNAL_PROPERTY_NAMES_CLEANUP_FILTER);
+ }
+
+ public static CoreTypedProperties copyOf(TypedProperties typedProperties) {
+ synchronized (typedProperties) {
--- End diff --
Of course
---
[GitHub] activemq-artemis pull request #2427: ARTEMIS-2170 Optimized CoreMessage's ch...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2427#discussion_r238867691
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/message/impl/CoreMessage.java ---
@@ -578,34 +567,41 @@ public CoreMessage setUserID(UUID userID) {
/**
* I am keeping this synchronized as the decode of the Properties is lazy
*/
- protected TypedProperties checkProperties() {
+ protected final TypedProperties getOrCreateProperties() {
--- End diff --
Nudge :) keen this doesnt lose traction
---
[GitHub] activemq-artemis pull request #2427: ARTEMIS-2170 Optimized CoreMessage's ch...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2427#discussion_r233517904
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/message/impl/CoreMessage.java ---
@@ -52,6 +52,62 @@
* consumers */
public class CoreMessage extends RefCountMessage implements ICoreMessage {
+ private static final class CoreTypedProperties extends TypedProperties {
+
+ // We use properties to establish routing context on clustering.
+ // However if the client resends the message after receiving, it needs to be removed
+ private static final Predicate<SimpleString> INTERNAL_PROPERTY_NAMES_CLEANUP_FILTER =
+ name -> (name.startsWith(Message.HDR_ROUTE_TO_IDS) && !name.equals(Message.HDR_ROUTE_TO_IDS)) ||
+ (name.startsWith(Message.HDR_ROUTE_TO_ACK_IDS) && !name.equals(Message.HDR_ROUTE_TO_ACK_IDS));
+ private static final SimpleString AMQ_PROPNAME = new SimpleString("_AMQ_");
+ private boolean internalProperties;
+
+ CoreTypedProperties() {
+ super();
+ internalProperties = false;
+ }
+
+ private CoreTypedProperties(TypedProperties other) {
+ super(other);
+ if (other instanceof CoreTypedProperties) {
--- End diff --
synchronize on other.
---
[GitHub] activemq-artemis issue #2427: ARTEMIS-2170 Optimized CoreMessage's checkProp...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/2427
Overal looks good a few comments
---
[GitHub] activemq-artemis pull request #2427: ARTEMIS-2170 Optimized CoreMessage's ch...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2427#discussion_r233513734
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/message/impl/CoreMessage.java ---
@@ -564,34 +604,59 @@ public CoreMessage setUserID(UUID userID) {
/**
* I am keeping this synchronized as the decode of the Properties is lazy
*/
- protected TypedProperties checkProperties() {
+ protected final TypedProperties checkProperties() {
try {
+ TypedProperties properties = this.properties;
if (properties == null) {
- synchronized (this) {
- if (properties == null) {
- TypedProperties properties = new TypedProperties();
- if (buffer != null && propertiesLocation >= 0) {
- final ByteBuf byteBuf = buffer.duplicate().readerIndex(propertiesLocation);
- properties.decode(byteBuf, coreMessageObjectPools == null ? null : coreMessageObjectPools.getPropertiesDecoderPools());
- }
- this.properties = properties;
- }
+ properties = getOrInitializeTypedProperties();
+ }
+ return properties;
+ } catch (Throwable e) {
+ throw onCheckPropertiesError(e);
--- End diff --
this MUST throw the original exception if such exception, to keep exception behaviour of any code upstream that maybe catching specific exceptions..
---
[GitHub] activemq-artemis pull request #2427: ARTEMIS-2170 Optimized CoreMessage's ch...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2427#discussion_r233197168
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/collections/TypedProperties.java ---
@@ -318,6 +313,46 @@ public synchronized boolean containsProperty(final SimpleString key) {
}
}
+ public synchronized boolean anyPropertyNameMatch(Predicate<SimpleString> propertyNamePredicate) {
--- End diff --
this would be a contains method
---
[GitHub] activemq-artemis pull request #2427: ARTEMIS-2170 Optimized CoreMessage's ch...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2427#discussion_r234926580
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/message/impl/CoreMessage.java ---
@@ -578,34 +567,41 @@ public CoreMessage setUserID(UUID userID) {
/**
* I am keeping this synchronized as the decode of the Properties is lazy
*/
- protected TypedProperties checkProperties() {
+ protected final TypedProperties getOrCreateProperties() {
--- End diff --
All it does it invoke / delegate this method, as such if anything i see naming CoreMessage.getProperties cleaner. And there is no need to change it from final, simply you remove the impl in ClientMessageInternalImpl as it would just inherit.
```
public TypedProperties getProperties() {
return this.checkProperties();
}
```
---
[GitHub] activemq-artemis pull request #2427: ARTEMIS-2170 Optimized CoreMessage's ch...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2427#discussion_r233518660
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/message/impl/CoreMessage.java ---
@@ -52,6 +52,62 @@
* consumers */
public class CoreMessage extends RefCountMessage implements ICoreMessage {
+ private static final class CoreTypedProperties extends TypedProperties {
+
+ // We use properties to establish routing context on clustering.
+ // However if the client resends the message after receiving, it needs to be removed
+ private static final Predicate<SimpleString> INTERNAL_PROPERTY_NAMES_CLEANUP_FILTER =
+ name -> (name.startsWith(Message.HDR_ROUTE_TO_IDS) && !name.equals(Message.HDR_ROUTE_TO_IDS)) ||
+ (name.startsWith(Message.HDR_ROUTE_TO_ACK_IDS) && !name.equals(Message.HDR_ROUTE_TO_ACK_IDS));
+ private static final SimpleString AMQ_PROPNAME = new SimpleString("_AMQ_");
+ private boolean internalProperties;
+
+ CoreTypedProperties() {
+ super();
+ internalProperties = false;
+ }
+
+ private CoreTypedProperties(TypedProperties other) {
+ super(other);
+ if (other instanceof CoreTypedProperties) {
+ internalProperties = ((CoreTypedProperties) other).internalProperties;
+ } else {
+ internalProperties = other.containsProperty(name -> name.startsWith(AMQ_PROPNAME));
+ }
+ }
+
+ @Override
+ protected synchronized void doPutValue(SimpleString key, TypedProperties.PropertyValue value) {
+ if (!internalProperties && key.startsWith(AMQ_PROPNAME)) {
+ internalProperties = true;
+ }
+ super.doPutValue(key, value);
+ }
+
+ public synchronized boolean cleanupInternalProperties() {
+ if (!internalProperties) {
+ return false;
+ }
+ return removeProperty(INTERNAL_PROPERTY_NAMES_CLEANUP_FILTER);
+ }
+
+ public static boolean cleanupInternalProperties(TypedProperties typedProperties) {
+ if (typedProperties == null) {
+ return false;
+ }
+ if (typedProperties instanceof CoreTypedProperties) {
+ return ((CoreTypedProperties) typedProperties).cleanupInternalProperties();
+ }
+ return typedProperties.removeProperty(INTERNAL_PROPERTY_NAMES_CLEANUP_FILTER);
+ }
+
+ public static CoreTypedProperties copyOf(TypedProperties typedProperties) {
+ synchronized (typedProperties) {
--- End diff --
this shouldnt be needed, as the constructor of CoreTypedProperties that takes TypedProperties should really be the thing that ensure syncronicitity
---
[GitHub] activemq-artemis pull request #2427: ARTEMIS-2170 Optimized CoreMessage's ch...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2427#discussion_r233490350
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/message/impl/CoreMessage.java ---
@@ -52,6 +52,62 @@
* consumers */
public class CoreMessage extends RefCountMessage implements ICoreMessage {
+ private static final class CoreTypedProperties extends TypedProperties {
--- End diff --
is this really needed, the way i see it, looking through the usage, these internal properties, are a bit like the extra properties that exist and introduced on AMQPMessage, e.g. theyre bits added onto the message but not part of the core message, could there just be two fields of type TypeProperties in core message, one (existing) for normal properties, and another for Internal Properties, which are these things, that then would make cleaning up / not forwarding on far easier, as then simply you clear them.
---
[GitHub] activemq-artemis pull request #2427: ARTEMIS-2170 Optimized CoreMessage's ch...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2427#discussion_r234546810
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/message/impl/CoreMessage.java ---
@@ -52,6 +52,62 @@
* consumers */
public class CoreMessage extends RefCountMessage implements ICoreMessage {
+ private static final class CoreTypedProperties extends TypedProperties {
+
+ // We use properties to establish routing context on clustering.
+ // However if the client resends the message after receiving, it needs to be removed
+ private static final Predicate<SimpleString> INTERNAL_PROPERTY_NAMES_CLEANUP_FILTER =
+ name -> (name.startsWith(Message.HDR_ROUTE_TO_IDS) && !name.equals(Message.HDR_ROUTE_TO_IDS)) ||
+ (name.startsWith(Message.HDR_ROUTE_TO_ACK_IDS) && !name.equals(Message.HDR_ROUTE_TO_ACK_IDS));
+ private static final SimpleString AMQ_PROPNAME = new SimpleString("_AMQ_");
+ private boolean internalProperties;
+
+ CoreTypedProperties() {
+ super();
+ internalProperties = false;
+ }
+
+ private CoreTypedProperties(TypedProperties other) {
+ super(other);
+ if (other instanceof CoreTypedProperties) {
+ internalProperties = ((CoreTypedProperties) other).internalProperties;
+ } else {
+ internalProperties = other.containsProperty(name -> name.startsWith(AMQ_PROPNAME));
+ }
+ }
+
+ @Override
+ protected synchronized void doPutValue(SimpleString key, TypedProperties.PropertyValue value) {
+ if (!internalProperties && key.startsWith(AMQ_PROPNAME)) {
+ internalProperties = true;
+ }
+ super.doPutValue(key, value);
+ }
+
+ public synchronized boolean cleanupInternalProperties() {
+ if (!internalProperties) {
+ return false;
+ }
+ return removeProperty(INTERNAL_PROPERTY_NAMES_CLEANUP_FILTER);
+ }
+
+ public static boolean cleanupInternalProperties(TypedProperties typedProperties) {
+ if (typedProperties == null) {
+ return false;
+ }
+ if (typedProperties instanceof CoreTypedProperties) {
+ return ((CoreTypedProperties) typedProperties).cleanupInternalProperties();
+ }
+ return typedProperties.removeProperty(INTERNAL_PROPERTY_NAMES_CLEANUP_FILTER);
+ }
+
+ public static CoreTypedProperties copyOf(TypedProperties typedProperties) {
+ synchronized (typedProperties) {
--- End diff --
So it being internal, means where this gets put MUST be somewhere in Artemis code base, maybe a small nullable boolean flag on the CoreMessage could be introduced that is set after by that code when these properties are added.
---
[GitHub] activemq-artemis pull request #2427: ARTEMIS-2170 Optimized CoreMessage's ch...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2427#discussion_r235842374
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/message/impl/CoreMessage.java ---
@@ -578,34 +567,41 @@ public CoreMessage setUserID(UUID userID) {
/**
* I am keeping this synchronized as the decode of the Properties is lazy
*/
- protected TypedProperties checkProperties() {
+ protected final TypedProperties getOrCreateProperties() {
--- End diff --
@franz1981 did you make the change?
---