[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[ https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16750864#comment-16750864 ] ASF subversion and git services commented on ARTEMIS-2170: -- Commit 6446d01a151a6e73b359c356b44fcf447961bd5a in activemq-artemis's branch refs/heads/master from Francesco Nigro [ https://gitbox.apache.org/repos/asf?p=activemq-artemis.git;h=6446d01 ] ARTEMIS-2170 Optimized CoreMessage check and cleanup methods Any checkProperties(); pattern has been replaced by an atomic checkProperties(). to help both performance and consistency. The cleanup is now performed into CoreTypedProperties both for performance reasons (avoid lock/unlock many times) and consistency, given that the operation is now atomic. > Optimized CoreMessage's checkProperties and cleanupInternalProperties methods > - > > Key: ARTEMIS-2170 > URL: https://issues.apache.org/jira/browse/ARTEMIS-2170 > Project: ActiveMQ Artemis > Issue Type: Improvement > Components: Broker >Reporter: Francesco Nigro >Assignee: Francesco Nigro >Priority: Minor > Time Spent: 6h 50m > Remaining Estimate: 0h > > CoreMessage::checkProperties perform too many volatile read/write and has a > too big body just to handle exceptional cases, while > cleanupInternalProperties is called on the hot path of session send, but is > performing too many synchronized operations and loopup on TypedProperties. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[ https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16750865#comment-16750865 ] ASF subversion and git services commented on ARTEMIS-2170: -- Commit cf65912bccf59fe381b155011d957ea04f80be96 in activemq-artemis's branch refs/heads/master from Michael André Pearce [ https://gitbox.apache.org/repos/asf?p=activemq-artemis.git;h=cf65912 ] ARTEMIS-2170 Optimized CoreMessage clearInternalProperties Ensure only iterate properties, if internal property is set. > Optimized CoreMessage's checkProperties and cleanupInternalProperties methods > - > > Key: ARTEMIS-2170 > URL: https://issues.apache.org/jira/browse/ARTEMIS-2170 > Project: ActiveMQ Artemis > Issue Type: Improvement > Components: Broker >Reporter: Francesco Nigro >Assignee: Francesco Nigro >Priority: Minor > Time Spent: 6h 50m > Remaining Estimate: 0h > > CoreMessage::checkProperties perform too many volatile read/write and has a > too big body just to handle exceptional cases, while > cleanupInternalProperties is called on the hot path of session send, but is > performing too many synchronized operations and loopup on TypedProperties. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[ https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16728451#comment-16728451 ] ASF GitHub Bot commented on ARTEMIS-2170: - 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: > Optimized CoreMessage's checkProperties and cleanupInternalProperties methods > - > > Key: ARTEMIS-2170 > URL: https://issues.apache.org/jira/browse/ARTEMIS-2170 > Project: ActiveMQ Artemis > Issue Type: Improvement > Components: Broker >Reporter: Francesco Nigro >Assignee: Francesco Nigro >Priority: Minor > > CoreMessage::checkProperties perform too many volatile read/write and has a > too big body just to handle exceptional cases, while > cleanupInternalProperties is called on the hot path of session send, but is > performing too many synchronized operations and loopup on TypedProperties. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[
https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16709367#comment-16709367
]
ASF GitHub Bot commented on ARTEMIS-2170:
-
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
> Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
> -
>
> Key: ARTEMIS-2170
> URL: https://issues.apache.org/jira/browse/ARTEMIS-2170
> Project: ActiveMQ Artemis
> Issue Type: Improvement
> Components: Broker
>Reporter: Francesco Nigro
>Assignee: Francesco Nigro
>Priority: Minor
>
> CoreMessage::checkProperties perform too many volatile read/write and has a
> too big body just to handle exceptional cases, while
> cleanupInternalProperties is called on the hot path of session send, but is
> performing too many synchronized operations and loopup on TypedProperties.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[
https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16700367#comment-16700367
]
ASF GitHub Bot commented on ARTEMIS-2170:
-
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!
> Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
> -
>
> Key: ARTEMIS-2170
> URL: https://issues.apache.org/jira/browse/ARTEMIS-2170
> Project: ActiveMQ Artemis
> Issue Type: Improvement
> Components: Broker
>Reporter: Francesco Nigro
>Assignee: Francesco Nigro
>Priority: Minor
>
> CoreMessage::checkProperties perform too many volatile read/write and has a
> too big body just to handle exceptional cases, while
> cleanupInternalProperties is called on the hot path of session send, but is
> performing too many synchronized operations and loopup on TypedProperties.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[
https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16696362#comment-16696362
]
ASF GitHub Bot commented on ARTEMIS-2170:
-
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?
> Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
> -
>
> Key: ARTEMIS-2170
> URL: https://issues.apache.org/jira/browse/ARTEMIS-2170
> Project: ActiveMQ Artemis
> Issue Type: Improvement
> Components: Broker
>Reporter: Francesco Nigro
>Assignee: Francesco Nigro
>Priority: Minor
>
> CoreMessage::checkProperties perform too many volatile read/write and has a
> too big body just to handle exceptional cases, while
> cleanupInternalProperties is called on the hot path of session send, but is
> performing too many synchronized operations and loopup on TypedProperties.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[
https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16693493#comment-16693493
]
ASF GitHub Bot commented on ARTEMIS-2170:
-
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
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.
> Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
> -
>
> Key: ARTEMIS-2170
> URL: https://issues.apache.org/jira/browse/ARTEMIS-2170
> Project: ActiveMQ Artemis
> Issue Type: Improvement
> Components: Broker
>Reporter: Francesco Nigro
>Assignee: Francesco Nigro
>Priority: Minor
>
> CoreMessage::checkProperties perform too many volatile read/write and has a
> too big body just to handle exceptional cases, while
> cleanupInternalProperties is called on the hot path of session send, but is
> performing too many synchronized operations and loopup on TypedProperties.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[
https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16693001#comment-16693001
]
ASF GitHub Bot commented on ARTEMIS-2170:
-
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
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.
> Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
> -
>
> Key: ARTEMIS-2170
> URL: https://issues.apache.org/jira/browse/ARTEMIS-2170
> Project: ActiveMQ Artemis
> Issue Type: Improvement
> Components: Broker
>Reporter: Francesco Nigro
>Assignee: Francesco Nigro
>Priority: Minor
>
> CoreMessage::checkProperties perform too many volatile read/write and has a
> too big body just to handle exceptional cases, while
> cleanupInternalProperties is called on the hot path of session send, but is
> performing too many synchronized operations and loopup on TypedProperties.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[
https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16692938#comment-16692938
]
ASF GitHub Bot commented on ARTEMIS-2170:
-
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();
}
```
> Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
> -
>
> Key: ARTEMIS-2170
> URL: https://issues.apache.org/jira/browse/ARTEMIS-2170
> Project: ActiveMQ Artemis
> Issue Type: Improvement
> Components: Broker
>Reporter: Francesco Nigro
>Assignee: Francesco Nigro
>Priority: Minor
>
> CoreMessage::checkProperties perform too many volatile read/write and has a
> too big body just to handle exceptional cases, while
> cleanupInternalProperties is called on the hot path of session send, but is
> performing too many synchronized operations and loopup on TypedProperties.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[
https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16692858#comment-16692858
]
ASF GitHub Bot commented on ARTEMIS-2170:
-
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
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.
> Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
> -
>
> Key: ARTEMIS-2170
> URL: https://issues.apache.org/jira/browse/ARTEMIS-2170
> Project: ActiveMQ Artemis
> Issue Type: Improvement
> Components: Broker
>Reporter: Francesco Nigro
>Assignee: Francesco Nigro
>Priority: Minor
>
> CoreMessage::checkProperties perform too many volatile read/write and has a
> too big body just to handle exceptional cases, while
> cleanupInternalProperties is called on the hot path of session send, but is
> performing too many synchronized operations and loopup on TypedProperties.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[
https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16692837#comment-16692837
]
ASF GitHub Bot commented on ARTEMIS-2170:
-
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.
> Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
> -
>
> Key: ARTEMIS-2170
> URL: https://issues.apache.org/jira/browse/ARTEMIS-2170
> Project: ActiveMQ Artemis
> Issue Type: Improvement
> Components: Broker
>Reporter: Francesco Nigro
>Assignee: Francesco Nigro
>Priority: Minor
>
> CoreMessage::checkProperties perform too many volatile read/write and has a
> too big body just to handle exceptional cases, while
> cleanupInternalProperties is called on the hot path of session send, but is
> performing too many synchronized operations and loopup on TypedProperties.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[
https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16692821#comment-16692821
]
ASF GitHub Bot commented on ARTEMIS-2170:
-
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();
-
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[
https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16692208#comment-16692208
]
ASF GitHub Bot commented on ARTEMIS-2170:
-
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
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.
> Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
> -
>
> Key: ARTEMIS-2170
> URL: https://issues.apache.org/jira/browse/ARTEMIS-2170
> Project: ActiveMQ Artemis
> Issue Type: Improvement
> Components: Broker
>Reporter: Francesco Nigro
>Assignee: Francesco Nigro
>Priority: Minor
>
> CoreMessage::checkProperties perform too many volatile read/write and has a
> too big body just to handle exceptional cases, while
> cleanupInternalProperties is called on the hot path of session send, but is
> performing too many synchronized operations and loopup on TypedProperties.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[
https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16691967#comment-16691967
]
ASF GitHub Bot commented on ARTEMIS-2170:
-
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
> Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
> -
>
> Key: ARTEMIS-2170
> URL: https://issues.apache.org/jira/browse/ARTEMIS-2170
> Project: ActiveMQ Artemis
> Issue Type: Improvement
> Components: Broker
>Reporter: Francesco Nigro
>Assignee: Francesco Nigro
>Priority: Minor
>
> CoreMessage::checkProperties perform too many volatile read/write and has a
> too big body just to handle exceptional cases, while
> cleanupInternalProperties is called on the hot path of session send, but is
> performing too many synchronized operations and loopup on TypedProperties.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[
https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16691763#comment-16691763
]
ASF GitHub Bot commented on ARTEMIS-2170:
-
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();
-
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[
https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16691713#comment-16691713
]
ASF GitHub Bot commented on ARTEMIS-2170:
-
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();
-
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[
https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16691706#comment-16691706
]
ASF GitHub Bot commented on ARTEMIS-2170:
-
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))
> Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
> -
>
> Key: ARTEMIS-2170
> URL: https://issues.apache.org/jira/browse/ARTEMIS-2170
> Project: ActiveMQ Artemis
> Issue Type: Improvement
> Components: Broker
>Reporter: Francesco Nigro
>Assignee: Francesco Nigro
>Priority: Minor
>
> CoreMessage::checkProperties perform too many volatile read/write and has a
> too big body just to handle exceptional cases, while
> cleanupInternalProperties is called on the hot path of session send, but is
> performing too many synchronized operations and loopup on TypedProperties.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[
https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16691710#comment-16691710
]
ASF GitHub Bot commented on ARTEMIS-2170:
-
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)
> Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
> -
>
> Key: ARTEMIS-2170
> URL: https://issues.apache.org/jira/browse/ARTEMIS-2170
> Project: ActiveMQ Artemis
> Issue Type: Improvement
> Components: Broker
>Reporter: Francesco Nigro
>Assignee: Francesco Nigro
>Priority: Minor
>
> CoreMessage::checkProperties perform too many volatile read/write and has a
> too big body just to handle exceptional cases, while
> cleanupInternalProperties is called on the hot path of session send, but is
> performing too many synchronized operations and loopup on TypedProperties.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[
https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16691708#comment-16691708
]
ASF GitHub Bot commented on ARTEMIS-2170:
-
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)
> Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
> -
>
> Key: ARTEMIS-2170
> URL: https://issues.apache.org/jira/browse/ARTEMIS-2170
> Project: ActiveMQ Artemis
> Issue Type: Improvement
> Components: Broker
>Reporter: Francesco Nigro
>Assignee: Francesco Nigro
>Priority: Minor
>
> CoreMessage::checkProperties perform too many volatile read/write and has a
> too big body just to handle exceptional cases, while
> cleanupInternalProperties is called on the hot path of session send, but is
> performing too many synchronized operations and loopup on TypedProperties.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[
https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16691463#comment-16691463
]
ASF GitHub Bot commented on ARTEMIS-2170:
-
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
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.
> Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
> -
>
> Key: ARTEMIS-2170
> URL: https://issues.apache.org/jira/browse/ARTEMIS-2170
> Project: ActiveMQ Artemis
> Issue Type: Improvement
> Components: Broker
>Reporter: Francesco Nigro
>Assignee: Francesco Nigro
>Priority: Minor
>
> CoreMessage::checkProperties perform too many volatile read/write and has a
> too big body just to handle exceptional cases, while
> cleanupInternalProperties is called on the hot path of session send, but is
> performing too many synchronized operations and loopup on TypedProperties.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[
https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16691411#comment-16691411
]
ASF GitHub Bot commented on ARTEMIS-2170:
-
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
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.
> Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
> -
>
> Key: ARTEMIS-2170
> URL: https://issues.apache.org/jira/browse/ARTEMIS-2170
> Project: ActiveMQ Artemis
> Issue Type: Improvement
> Components: Broker
>Reporter: Francesco Nigro
>Assignee: Francesco Nigro
>Priority: Minor
>
> CoreMessage::checkProperties perform too many volatile read/write and has a
> too big body just to handle exceptional cases, while
> cleanupInternalProperties is called on the hot path of session send, but is
> performing too many synchronized operations and loopup on TypedProperties.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[
https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16691389#comment-16691389
]
ASF GitHub Bot commented on ARTEMIS-2170:
-
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
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.
> Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
> -
>
> Key: ARTEMIS-2170
> URL: https://issues.apache.org/jira/browse/ARTEMIS-2170
> Project: ActiveMQ Artemis
> Issue Type: Improvement
> Components: Broker
>Reporter: Francesco Nigro
>Assignee: Francesco Nigro
>Priority: Minor
>
> CoreMessage::checkProperties perform too many volatile read/write and has a
> too big body just to handle exceptional cases, while
> cleanupInternalProperties is called on the hot path of session send, but is
> performing too many synchronized operations and loopup on TypedProperties.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[
https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16689123#comment-16689123
]
ASF GitHub Bot commented on ARTEMIS-2170:
-
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
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?
> Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
> -
>
> Key: ARTEMIS-2170
> URL: https://issues.apache.org/jira/browse/ARTEMIS-2170
> Project: ActiveMQ Artemis
> Issue Type: Improvement
> Components: Broker
>Reporter: Francesco Nigro
>Assignee: Francesco Nigro
>Priority: Minor
>
> CoreMessage::checkProperties perform too many volatile read/write and has a
> too big body just to handle exceptional cases, while
> cleanupInternalProperties is called on the hot path of session send, but is
> performing too many synchronized operations and loopup on TypedProperties.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[
https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16689122#comment-16689122
]
ASF GitHub Bot commented on ARTEMIS-2170:
-
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
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.
> Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
> -
>
> Key: ARTEMIS-2170
> URL: https://issues.apache.org/jira/browse/ARTEMIS-2170
> Project: ActiveMQ Artemis
> Issue Type: Improvement
> Components: Broker
>Reporter: Francesco Nigro
>Assignee: Francesco Nigro
>Priority: Minor
>
> CoreMessage::checkProperties perform too many volatile read/write and has a
> too big body just to handle exceptional cases, while
> cleanupInternalProperties is called on the hot path of session send, but is
> performing too many synchronized operations and loopup on TypedProperties.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[
https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16686934#comment-16686934
]
ASF GitHub Bot commented on ARTEMIS-2170:
-
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
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
> Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
> -
>
> Key: ARTEMIS-2170
> URL: https://issues.apache.org/jira/browse/ARTEMIS-2170
> Project: ActiveMQ Artemis
> Issue Type: Improvement
> Components: Broker
>Reporter: Francesco Nigro
>Assignee: Francesco Nigro
>Priority: Minor
>
> CoreMessage::checkProperties perform too many volatile read/write and has a
> too big body just to handle exceptional cases, while
> cleanupInternalProperties is called on the hot path of session send, but is
> performing too many synchronized operations and loopup on TypedProperties.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[
https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16686843#comment-16686843
]
ASF GitHub Bot commented on ARTEMIS-2170:
-
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
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?
> Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
> -
>
> Key: ARTEMIS-2170
> URL: https://issues.apache.org/jira/browse/ARTEMIS-2170
> Project: ActiveMQ Artemis
> Issue Type: Improvement
> Components: Broker
>Reporter: Francesco Nigro
>Assignee: Francesco Nigro
>Priority: Minor
>
> CoreMessage::checkProperties perform too many volatile read/write and has a
> too big body just to handle exceptional cases, while
> cleanupInternalProperties is called on the hot path of session send, but is
> performing too many synchronized operations and loopup on TypedProperties.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[
https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16686825#comment-16686825
]
ASF GitHub Bot commented on ARTEMIS-2170:
-
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
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 :)
> Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
> -
>
> Key: ARTEMIS-2170
> URL: https://issues.apache.org/jira/browse/ARTEMIS-2170
> Project: ActiveMQ Artemis
> Issue Type: Improvement
> Components: Broker
>Reporter: Francesco Nigro
>Assignee: Francesco Nigro
>Priority: Minor
>
> CoreMessage::checkProperties perform too many volatile read/write and has a
> too big body just to handle exceptional cases, while
> cleanupInternalProperties is called on the hot path of session send, but is
> performing too many synchronized operations and loopup on TypedProperties.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[
https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16686813#comment-16686813
]
ASF GitHub Bot commented on ARTEMIS-2170:
-
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
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
> Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
> -
>
> Key: ARTEMIS-2170
> URL: https://issues.apache.org/jira/browse/ARTEMIS-2170
> Project: ActiveMQ Artemis
> Issue Type: Improvement
> Components: Broker
>Reporter: Francesco Nigro
>Assignee: Francesco Nigro
>Priority: Minor
>
> CoreMessage::checkProperties perform too many volatile read/write and has a
> too big body just to handle exceptional cases, while
> cleanupInternalProperties is called on the hot path of session send, but is
> performing too many synchronized operations and loopup on TypedProperties.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[
https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16686822#comment-16686822
]
ASF GitHub Bot commented on ARTEMIS-2170:
-
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
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.
> Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
> -
>
> Key: ARTEMIS-2170
> URL: https://issues.apache.org/jira/browse/ARTEMIS-2170
> Project: ActiveMQ Artemis
> Issue Type: Improvement
> Components: Broker
>Reporter: Francesco Nigro
>Assignee: Francesco Nigro
>Priority: Minor
>
> CoreMessage::checkProperties perform too many volatile read/write and has a
> too big body just to handle exceptional cases, while
> cleanupInternalProperties is called on the hot path of session send, but is
> performing too many synchronized operations and loopup on TypedProperties.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[
https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16686793#comment-16686793
]
ASF GitHub Bot commented on ARTEMIS-2170:
-
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..
> Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
> -
>
> Key: ARTEMIS-2170
> URL: https://issues.apache.org/jira/browse/ARTEMIS-2170
> Project: ActiveMQ Artemis
> Issue Type: Improvement
> Components: Broker
>Reporter: Francesco Nigro
>Assignee: Francesco Nigro
>Priority: Minor
>
> CoreMessage::checkProperties perform too many volatile read/write and has a
> too big body just to handle exceptional cases, while
> cleanupInternalProperties is called on the hot path of session send, but is
> performing too many synchronized operations and loopup on TypedProperties.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[
https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16686809#comment-16686809
]
ASF GitHub Bot commented on ARTEMIS-2170:
-
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
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.
> Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
> -
>
> Key: ARTEMIS-2170
> URL: https://issues.apache.org/jira/browse/ARTEMIS-2170
> Project: ActiveMQ Artemis
> Issue Type: Improvement
> Components: Broker
>Reporter: Francesco Nigro
>Assignee: Francesco Nigro
>Priority: Minor
>
> CoreMessage::checkProperties perform too many volatile read/write and has a
> too big body just to handle exceptional cases, while
> cleanupInternalProperties is called on the hot path of session send, but is
> performing too many synchronized operations and loopup on TypedProperties.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[
https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16686787#comment-16686787
]
ASF GitHub Bot commented on ARTEMIS-2170:
-
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.
> Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
> -
>
> Key: ARTEMIS-2170
> URL: https://issues.apache.org/jira/browse/ARTEMIS-2170
> Project: ActiveMQ Artemis
> Issue Type: Improvement
> Components: Broker
>Reporter: Francesco Nigro
>Assignee: Francesco Nigro
>Priority: Minor
>
> CoreMessage::checkProperties perform too many volatile read/write and has a
> too big body just to handle exceptional cases, while
> cleanupInternalProperties is called on the hot path of session send, but is
> performing too many synchronized operations and loopup on TypedProperties.
--
This message was sent
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[
https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16686782#comment-16686782
]
ASF GitHub Bot commented on ARTEMIS-2170:
-
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.
> Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
> -
>
> Key: ARTEMIS-2170
> URL: https://issues.apache.org/jira/browse/ARTEMIS-2170
> Project: ActiveMQ Artemis
> Issue Type: Improvement
> Components: Broker
>Reporter: Francesco Nigro
>Assignee: Francesco Nigro
>Priority: Minor
>
> CoreMessage::checkProperties perform too many volatile read/write and has a
> too big body just to handle exceptional cases, while
> cleanupInternalProperties is called on the hot path of session send, but is
> performing too many synchronized operations and loopup on TypedProperties.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[
https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16686714#comment-16686714
]
ASF GitHub Bot commented on ARTEMIS-2170:
-
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 propertyNamePredicate) {
+ if (!internalProperties) {
--- End diff --
tbh id rather we do one refactor. so +1 for me.
> Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
> -
>
> Key: ARTEMIS-2170
> URL: https://issues.apache.org/jira/browse/ARTEMIS-2170
> Project: ActiveMQ Artemis
> Issue Type: Improvement
> Components: Broker
>Reporter: Francesco Nigro
>Assignee: Francesco Nigro
>Priority: Minor
>
> CoreMessage::checkProperties perform too many volatile read/write and has a
> too big body just to handle exceptional cases, while
> cleanupInternalProperties is called on the hot path of session send, but is
> performing too many synchronized operations and loopup on TypedProperties.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[
https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16686720#comment-16686720
]
ASF GitHub Bot commented on ARTEMIS-2170:
-
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 :).
> Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
> -
>
> Key: ARTEMIS-2170
> URL: https://issues.apache.org/jira/browse/ARTEMIS-2170
> Project: ActiveMQ Artemis
> Issue Type: Improvement
> Components: Broker
>Reporter: Francesco Nigro
>Assignee: Francesco Nigro
>Priority: Minor
>
> CoreMessage::checkProperties perform too many volatile read/write and has a
> too big body just to handle exceptional cases, while
> cleanupInternalProperties is called on the hot path of session send, but is
> performing too many synchronized operations and loopup on TypedProperties.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[
https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16686679#comment-16686679
]
ASF GitHub Bot commented on ARTEMIS-2170:
-
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
> Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
> -
>
> Key: ARTEMIS-2170
> URL: https://issues.apache.org/jira/browse/ARTEMIS-2170
> Project: ActiveMQ Artemis
> Issue Type: Improvement
> Components: Broker
>Reporter: Francesco Nigro
>Assignee: Francesco Nigro
>Priority: Minor
>
> CoreMessage::checkProperties perform too many volatile read/write and has a
> too big body just to handle exceptional cases, while
> cleanupInternalProperties is called on the hot path of session send, but is
> performing too many synchronized operations and loopup on TypedProperties.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[
https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16686712#comment-16686712
]
ASF GitHub Bot commented on ARTEMIS-2170:
-
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.
> Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
> -
>
> Key: ARTEMIS-2170
> URL: https://issues.apache.org/jira/browse/ARTEMIS-2170
> Project: ActiveMQ Artemis
> Issue Type: Improvement
> Components: Broker
>Reporter: Francesco Nigro
>Assignee: Francesco Nigro
>Priority: Minor
>
> CoreMessage::checkProperties perform too many volatile read/write and has a
> too big body just to handle exceptional cases, while
> cleanupInternalProperties is called on the hot path of session send, but is
> performing too many synchronized operations and loopup on TypedProperties.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[
https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16686698#comment-16686698
]
ASF GitHub Bot commented on ARTEMIS-2170:
-
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
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
> Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
> -
>
> Key: ARTEMIS-2170
> URL: https://issues.apache.org/jira/browse/ARTEMIS-2170
> Project: ActiveMQ Artemis
> Issue Type: Improvement
> Components: Broker
>Reporter: Francesco Nigro
>Assignee: Francesco Nigro
>Priority: Minor
>
> CoreMessage::checkProperties perform too many volatile read/write and has a
> too big body just to handle exceptional cases, while
> cleanupInternalProperties is called on the hot path of session send, but is
> performing too many synchronized operations and loopup on TypedProperties.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[
https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16686695#comment-16686695
]
ASF GitHub Bot commented on ARTEMIS-2170:
-
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
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?
> Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
> -
>
> Key: ARTEMIS-2170
> URL: https://issues.apache.org/jira/browse/ARTEMIS-2170
> Project: ActiveMQ Artemis
> Issue Type: Improvement
> Components: Broker
>Reporter: Francesco Nigro
>Assignee: Francesco Nigro
>Priority: Minor
>
> CoreMessage::checkProperties perform too many volatile read/write and has a
> too big body just to handle exceptional cases, while
> cleanupInternalProperties is called on the hot path of session send, but is
> performing too many synchronized operations and loopup on TypedProperties.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[
https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16686681#comment-16686681
]
ASF GitHub Bot commented on ARTEMIS-2170:
-
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) {
> Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
> -
>
> Key: ARTEMIS-2170
> URL: https://issues.apache.org/jira/browse/ARTEMIS-2170
> Project: ActiveMQ Artemis
> Issue Type: Improvement
> Components: Broker
>Reporter: Francesco Nigro
>Assignee: Francesco Nigro
>Priority: Minor
>
> CoreMessage::checkProperties perform too many volatile read/write and has a
> too big body just to handle exceptional cases, while
> cleanupInternalProperties is called on the hot path of session send, but is
> performing too many synchronized operations and loopup on TypedProperties.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[
https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16686687#comment-16686687
]
ASF GitHub Bot commented on ARTEMIS-2170:
-
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.
> Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
> -
>
> Key: ARTEMIS-2170
> URL: https://issues.apache.org/jira/browse/ARTEMIS-2170
> Project: ActiveMQ Artemis
> Issue Type: Improvement
> Components: Broker
>Reporter: Francesco Nigro
>Assignee: Francesco Nigro
>Priority: Minor
>
> CoreMessage::checkProperties perform too many volatile read/write and has a
> too big body just to handle exceptional cases, while
> cleanupInternalProperties is called on the hot path of session send, but is
> performing too many synchronized operations and loopup on TypedProperties.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[
https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16686678#comment-16686678
]
ASF GitHub Bot commented on ARTEMIS-2170:
-
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 {
> Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
> -
>
> Key: ARTEMIS-2170
> URL: https://issues.apache.org/jira/browse/ARTEMIS-2170
> Project: ActiveMQ Artemis
> Issue Type: Improvement
> Components: Broker
>Reporter: Francesco Nigro
>Assignee: Francesco Nigro
>Priority: Minor
>
> CoreMessage::checkProperties perform too many volatile read/write and has a
> too big body just to handle exceptional cases, while
> cleanupInternalProperties is called on the hot path of session send, but is
> performing too many synchronized operations and loopup on TypedProperties.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[
https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16686675#comment-16686675
]
ASF GitHub Bot commented on ARTEMIS-2170:
-
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()));
+
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[
https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16686667#comment-16686667
]
ASF GitHub Bot commented on ARTEMIS-2170:
-
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.
> Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
> -
>
> Key: ARTEMIS-2170
> URL: https://issues.apache.org/jira/browse/ARTEMIS-2170
> Project: ActiveMQ Artemis
> Issue Type: Improvement
> Components: Broker
>Reporter: Francesco Nigro
>Assignee: Francesco Nigro
>Priority: Minor
>
> CoreMessage::checkProperties perform too many volatile read/write and has a
> too big body just to handle exceptional cases, while
> cleanupInternalProperties is called on the hot path of session send, but is
> performing too many synchronized operations and loopup on TypedProperties.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[
https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=1668#comment-1668
]
ASF GitHub Bot commented on ARTEMIS-2170:
-
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.
> Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
> -
>
> Key: ARTEMIS-2170
> URL: https://issues.apache.org/jira/browse/ARTEMIS-2170
> Project: ActiveMQ Artemis
> Issue Type: Improvement
> Components: Broker
>Reporter: Francesco Nigro
>Assignee: Francesco Nigro
>Priority: Minor
>
> CoreMessage::checkProperties perform too many volatile read/write and has a
> too big body just to handle exceptional cases, while
> cleanupInternalProperties is called on the hot path of session send, but is
> performing too many synchronized operations and loopup on TypedProperties.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[
https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16685696#comment-16685696
]
ASF GitHub Bot commented on ARTEMIS-2170:
-
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 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?
> Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
> -
>
> Key: ARTEMIS-2170
> URL: https://issues.apache.org/jira/browse/ARTEMIS-2170
> Project: ActiveMQ Artemis
> Issue Type: Improvement
> Components: Broker
>Reporter: Francesco Nigro
>Assignee: Francesco Nigro
>Priority: Minor
>
> CoreMessage::checkProperties perform too many volatile read/write and has a
> too big body just to handle exceptional cases, while
> cleanupInternalProperties is called on the hot path of session send, but is
> performing too many synchronized operations and loopup on TypedProperties.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[
https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16685664#comment-16685664
]
ASF GitHub Bot commented on ARTEMIS-2170:
-
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 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 propertyNamePredicate) {
--- End diff --
this is just a remove method , the args give the context that it removes
only if predicate would match
> Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
> -
>
> Key: ARTEMIS-2170
> URL: https://issues.apache.org/jira/browse/ARTEMIS-2170
> Project: ActiveMQ Artemis
> Issue Type: Improvement
> Components: Broker
>Reporter: Francesco Nigro
>Assignee: Francesco Nigro
>Priority: Minor
>
> CoreMessage::checkProperties perform too many volatile read/write and has a
> too big body just to handle exceptional cases, while
> cleanupInternalProperties is called on the hot path of session send, but is
> performing too many synchronized operations and loopup on TypedProperties.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[
https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16685663#comment-16685663
]
ASF GitHub Bot commented on ARTEMIS-2170:
-
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 propertyNamePredicate) {
--- End diff --
this would be a contains method
> Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
> -
>
> Key: ARTEMIS-2170
> URL: https://issues.apache.org/jira/browse/ARTEMIS-2170
> Project: ActiveMQ Artemis
> Issue Type: Improvement
> Components: Broker
>Reporter: Francesco Nigro
>Assignee: Francesco Nigro
>Priority: Minor
>
> CoreMessage::checkProperties perform too many volatile read/write and has a
> too big body just to handle exceptional cases, while
> cleanupInternalProperties is called on the hot path of session send, but is
> performing too many synchronized operations and loopup on TypedProperties.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[
https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16684179#comment-16684179
]
ASF GitHub Bot commented on ARTEMIS-2170:
-
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 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.
> Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
> -
>
> Key: ARTEMIS-2170
> URL: https://issues.apache.org/jira/browse/ARTEMIS-2170
> Project: ActiveMQ Artemis
> Issue Type: Improvement
> Components: Broker
>Reporter: Francesco Nigro
>Assignee: Francesco Nigro
>Priority: Minor
>
> CoreMessage::checkProperties perform too many volatile read/write and has a
> too big body just to handle exceptional cases, while
> cleanupInternalProperties is called on the hot path of session send, but is
> performing too many synchronized operations and loopup on TypedProperties.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[
https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16683794#comment-16683794
]
ASF GitHub Bot commented on ARTEMIS-2170:
-
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 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.
> Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
> -
>
> Key: ARTEMIS-2170
> URL: https://issues.apache.org/jira/browse/ARTEMIS-2170
> Project: ActiveMQ Artemis
> Issue Type: Improvement
> Components: Broker
>Reporter: Francesco Nigro
>Assignee: Francesco Nigro
>Priority: Minor
>
> CoreMessage::checkProperties perform too many volatile read/write and has a
> too big body just to handle exceptional cases, while
> cleanupInternalProperties is called on the hot path of session send, but is
> performing too many synchronized operations and loopup on TypedProperties.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[
https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16683478#comment-16683478
]
ASF GitHub Bot commented on ARTEMIS-2170:
-
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 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
> Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
> -
>
> Key: ARTEMIS-2170
> URL: https://issues.apache.org/jira/browse/ARTEMIS-2170
> Project: ActiveMQ Artemis
> Issue Type: Improvement
> Components: Broker
>Reporter: Francesco Nigro
>Assignee: Francesco Nigro
>Priority: Minor
>
> CoreMessage::checkProperties perform too many volatile read/write and has a
> too big body just to handle exceptional cases, while
> cleanupInternalProperties is called on the hot path of session send, but is
> performing too many synchronized operations and loopup on TypedProperties.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[
https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16683365#comment-16683365
]
ASF GitHub Bot commented on ARTEMIS-2170:
-
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 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:
> Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
> -
>
> Key: ARTEMIS-2170
> URL: https://issues.apache.org/jira/browse/ARTEMIS-2170
> Project: ActiveMQ Artemis
> Issue Type: Improvement
> Components: Broker
>Reporter: Francesco Nigro
>Assignee: Francesco Nigro
>Priority: Minor
>
> CoreMessage::checkProperties perform too many volatile read/write and has a
> too big body just to handle exceptional cases, while
> cleanupInternalProperties is called on the hot path of session send, but is
> performing too many synchronized operations and loopup on TypedProperties.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[
https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16683349#comment-16683349
]
ASF GitHub Bot commented on ARTEMIS-2170:
-
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 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.
> Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
> -
>
> Key: ARTEMIS-2170
> URL: https://issues.apache.org/jira/browse/ARTEMIS-2170
> Project: ActiveMQ Artemis
> Issue Type: Improvement
> Components: Broker
>Reporter: Francesco Nigro
>Assignee: Francesco Nigro
>Priority: Minor
>
> CoreMessage::checkProperties perform too many volatile read/write and has a
> too big body just to handle exceptional cases, while
> cleanupInternalProperties is called on the hot path of session send, but is
> performing too many synchronized operations and loopup on TypedProperties.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[
https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16682957#comment-16682957
]
ASF GitHub Bot commented on ARTEMIS-2170:
-
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 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.
> Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
> -
>
> Key: ARTEMIS-2170
> URL: https://issues.apache.org/jira/browse/ARTEMIS-2170
> Project: ActiveMQ Artemis
> Issue Type: Improvement
> Components: Broker
>Reporter: Francesco Nigro
>Assignee: Francesco Nigro
>Priority: Minor
>
> CoreMessage::checkProperties perform too many volatile read/write and has a
> too big body just to handle exceptional cases, while
> cleanupInternalProperties is called on the hot path of session send, but is
> performing too many synchronized operations and loopup on TypedProperties.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[
https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16682901#comment-16682901
]
ASF GitHub Bot commented on ARTEMIS-2170:
-
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
> Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
> -
>
> Key: ARTEMIS-2170
> URL: https://issues.apache.org/jira/browse/ARTEMIS-2170
> Project: ActiveMQ Artemis
> Issue Type: Improvement
> Components: Broker
>Reporter: Francesco Nigro
>Assignee: Francesco Nigro
>Priority: Minor
>
> CoreMessage::checkProperties perform too many volatile read/write and has a
> too big body just to handle exceptional cases, while
> cleanupInternalProperties is called on the hot path of session send, but is
> performing too many synchronized operations and loopup on TypedProperties.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[
https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16682900#comment-16682900
]
ASF GitHub Bot commented on ARTEMIS-2170:
-
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
> Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
> -
>
> Key: ARTEMIS-2170
> URL: https://issues.apache.org/jira/browse/ARTEMIS-2170
> Project: ActiveMQ Artemis
> Issue Type: Improvement
> Components: Broker
>Reporter: Francesco Nigro
>Assignee: Francesco Nigro
>Priority: Minor
>
> CoreMessage::checkProperties perform too many volatile read/write and has a
> too big body just to handle exceptional cases, while
> cleanupInternalProperties is called on the hot path of session send, but is
> performing too many synchronized operations and loopup on TypedProperties.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[
https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16682898#comment-16682898
]
ASF GitHub Bot commented on ARTEMIS-2170:
-
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
> Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
> -
>
> Key: ARTEMIS-2170
> URL: https://issues.apache.org/jira/browse/ARTEMIS-2170
> Project: ActiveMQ Artemis
> Issue Type: Improvement
> Components: Broker
>Reporter: Francesco Nigro
>Assignee: Francesco Nigro
>Priority: Minor
>
> CoreMessage::checkProperties perform too many volatile read/write and has a
> too big body just to handle exceptional cases, while
> cleanupInternalProperties is called on the hot path of session send, but is
> performing too many synchronized operations and loopup on TypedProperties.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[
https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16682899#comment-16682899
]
ASF GitHub Bot commented on ARTEMIS-2170:
-
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 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
> Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
> -
>
> Key: ARTEMIS-2170
> URL: https://issues.apache.org/jira/browse/ARTEMIS-2170
> Project: ActiveMQ Artemis
> Issue Type: Improvement
> Components: Broker
>Reporter: Francesco Nigro
>Assignee: Francesco Nigro
>Priority: Minor
>
> CoreMessage::checkProperties perform too many volatile read/write and has a
> too big body just to handle exceptional cases, while
> cleanupInternalProperties is called on the hot path of session send, but is
> performing too many synchronized operations and loopup on TypedProperties.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[ https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16682896#comment-16682896 ] ASF GitHub Bot commented on ARTEMIS-2170: - Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2427 Overal looks good a few comments > Optimized CoreMessage's checkProperties and cleanupInternalProperties methods > - > > Key: ARTEMIS-2170 > URL: https://issues.apache.org/jira/browse/ARTEMIS-2170 > Project: ActiveMQ Artemis > Issue Type: Improvement > Components: Broker >Reporter: Francesco Nigro >Assignee: Francesco Nigro >Priority: Minor > > CoreMessage::checkProperties perform too many volatile read/write and has a > too big body just to handle exceptional cases, while > cleanupInternalProperties is called on the hot path of session send, but is > performing too many synchronized operations and loopup on TypedProperties. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[ https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16682842#comment-16682842 ] ASF GitHub Bot commented on ARTEMIS-2170: - 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: > Optimized CoreMessage's checkProperties and cleanupInternalProperties methods > - > > Key: ARTEMIS-2170 > URL: https://issues.apache.org/jira/browse/ARTEMIS-2170 > Project: ActiveMQ Artemis > Issue Type: Improvement > Components: Broker >Reporter: Francesco Nigro >Assignee: Francesco Nigro >Priority: Minor > > CoreMessage::checkProperties perform too many volatile read/write and has a > too big body just to handle exceptional cases, while > cleanupInternalProperties is called on the hot path of session send, but is > performing too many synchronized operations and loopup on TypedProperties. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (ARTEMIS-2170) Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
[ https://issues.apache.org/jira/browse/ARTEMIS-2170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16682841#comment-16682841 ] ASF GitHub Bot commented on ARTEMIS-2170: - 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 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 Date: 2018-11-11T10:32:49Z ARTEMIS-2170 Optimized CoreMessage's checkProperties method Any checkProperties(); pattern has been replaced by an atomic checkProperties(). to help both performance and consistency. > Optimized CoreMessage's checkProperties and cleanupInternalProperties methods > - > > Key: ARTEMIS-2170 > URL: https://issues.apache.org/jira/browse/ARTEMIS-2170 > Project: ActiveMQ Artemis > Issue Type: Improvement > Components: Broker >Reporter: Francesco Nigro >Assignee: Francesco Nigro >Priority: Minor > > CoreMessage::checkProperties perform too many volatile read/write and has a > too big body just to handle exceptional cases, while > cleanupInternalProperties is called on the hot path of session send, but is > performing too many synchronized operations and loopup on TypedProperties. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
