You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@activemq.apache.org by franz1981 <gi...@git.apache.org> on 2018/01/17 20:39:31 UTC
[GitHub] activemq-artemis pull request #1786: ARTEMIS-1616 OpenWire improvements
GitHub user franz1981 opened a pull request:
https://github.com/apache/activemq-artemis/pull/1786
ARTEMIS-1616 OpenWire improvements
It includes several improvements on OpenWire:
- Avoided copy of CoreMessage when not needed and cached lambda on hot path
- Refactored OpenWireMessageConverter::inbound in order to group help the JVM compile most used methods
- Optimized SimpleString::split because heavily used into AddressImpl::new
- Added existing queues cache to avoid multiple expensive AMQSession::checkAutoCreateQueue calls
- used SimpleString on OpenWireMessageConverter to avoid translations on CoreMessage
- cached Notification Destination check on AMQConsumer
- Used SimpleString on AMQSession explicitly with HDR_DUPLICATE_DETECTION_ID and CONNECTION_ID_PROPERTY_NAME
- Refactored toAMQMessage by grouping methods for a better readability
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/franz1981/activemq-artemis open_wire_to_amqp
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/activemq-artemis/pull/1786.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 #1786
----
commit 4d493c0eae35fb0be2e3e212eadcb657346ff8f9
Author: Francesco Nigro <ni...@...>
Date: 2018-01-16T13:24:32Z
Avoided copy of CoreMessage when not needed and cached lambda on hot path
commit e4f9a4f9a0bc3277e580a3c014ba7d422597b81e
Author: Francesco Nigro <ni...@...>
Date: 2018-01-16T16:01:54Z
Refactored inbound in order to group help the JVM compile most used methods
commit 82d1cd8c5489c106b5ff9e22b264216988b7986e
Author: Francesco Nigro <ni...@...>
Date: 2018-01-16T17:40:06Z
Optimized SimpleString::split because heavily used into AddressImpl::new
commit df18521298e8866d1e75e299336d25845b97f663
Author: Francesco Nigro <ni...@...>
Date: 2018-01-16T20:27:22Z
Added existing queues cache to avoid multiple expensive checkCreateQueue calls
commit 74eb72bffa7c723e714d4744fcc9e7207b5fe92a
Author: Francesco Nigro <ni...@...>
Date: 2018-01-16T21:24:08Z
used SimpleString to avoid translations on CoreMessage
commit 5752e30a9363bc473bd52e3ae09c677687ad1ab2
Author: Francesco Nigro <ni...@...>
Date: 2018-01-16T23:37:24Z
cached Notification Destination check on AMQConsumer
commit 94c9de15fb729d9e80000294166cc9d48fdd0e09
Author: Francesco Nigro <ni...@...>
Date: 2018-01-17T07:29:28Z
Used SimpleString explicitly with HDR_DUPLICATE_DETECTION_ID and CONNECTION_ID_PROPERTY_NAME
commit 1f53625848ae10269c9748b4125848609fd3854b
Author: Francesco Nigro <ni...@...>
Date: 2018-01-17T13:37:08Z
Refactored toAMQMessage by grouping methods for a better readability
----
---
[GitHub] activemq-artemis issue #1786: ARTEMIS-1616 OpenWire improvements
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1786
@franz1981 I assume you’ll squash commits once your ready right?
---
[GitHub] activemq-artemis pull request #1786: ARTEMIS-1616 OpenWire improvements
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1786#discussion_r162637206
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/api/core/SimpleString.java ---
@@ -361,13 +371,68 @@ public int hashCode() {
}
}
+ private static SimpleString[] splitWithCachedString(final SimpleString simpleString, final int delim) {
+ final String str = simpleString.str;
+ final byte[] data = simpleString.data;
+ final int length = str.length();
+ List<SimpleString> all = null;
+ int index = 0;
+ while (index < length) {
+ final int delimIndex = str.indexOf(delim, index);
+ if (delimIndex == -1) {
+ //just need to add the last one
+ break;
+ } else {
+ all = addSimpleStringPart(all, data, index, delimIndex);
+ }
+ index = delimIndex + 1;
+ }
+ if (all == null) {
+ return new SimpleString[]{simpleString};
+ } else {
+ // Adding the last one
+ all = addSimpleStringPart(all, data, index, length);
+ // Converting it to arrays
+ final SimpleString[] parts = new SimpleString[all.size()];
+ return all.toArray(parts);
+ }
+ }
+
+ private static List<SimpleString> addSimpleStringPart(List<SimpleString> all,
+ final byte[] data,
+ final int startIndex,
+ final int endIndex) {
+ final int expectedLength = endIndex - startIndex;
+ final SimpleString ss;
+ if (expectedLength == 0) {
+ ss = EMPTY;
+ } else {
+ //extract a byte[] copy from this
+ final int ssIndex = startIndex << 1;
+ final int delIndex = endIndex << 1;
+ final byte[] bytes = Arrays.copyOfRange(data, ssIndex, delIndex);
+ ss = new SimpleString(bytes);
+ }
+ // We will create the ArrayList lazily
+ if (all == null) {
+ // There will be at least 3 strings on this case (which is the actual common usecase)
--- End diff --
Just to be clear I’m happy with this, as a PR, eg if you get all the bits you wanted done then don’t wait on this. It shouldn’t hold a merge.
It’s just a question/query that was there before. Just a note that it be good to note what they are at some point.
---
[GitHub] activemq-artemis pull request #1786: ARTEMIS-1616 OpenWire improvements
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1786#discussion_r162272578
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/api/core/SimpleString.java ---
@@ -361,13 +371,68 @@ public int hashCode() {
}
}
+ private static SimpleString[] splitWithCachedString(final SimpleString simpleString, final int delim) {
+ final String str = simpleString.str;
+ final byte[] data = simpleString.data;
+ final int length = str.length();
+ List<SimpleString> all = null;
+ int index = 0;
+ while (index < length) {
+ final int delimIndex = str.indexOf(delim, index);
+ if (delimIndex == -1) {
+ //just need to add the last one
+ break;
+ } else {
+ all = addSimpleStringPart(all, data, index, delimIndex);
+ }
+ index = delimIndex + 1;
+ }
+ if (all == null) {
+ return new SimpleString[]{simpleString};
+ } else {
+ // Adding the last one
+ all = addSimpleStringPart(all, data, index, length);
+ // Converting it to arrays
+ final SimpleString[] parts = new SimpleString[all.size()];
+ return all.toArray(parts);
+ }
+ }
+
+ private static List<SimpleString> addSimpleStringPart(List<SimpleString> all,
+ final byte[] data,
+ final int startIndex,
+ final int endIndex) {
+ final int expectedLength = endIndex - startIndex;
+ final SimpleString ss;
+ if (expectedLength == 0) {
+ ss = EMPTY;
+ } else {
+ //extract a byte[] copy from this
+ final int ssIndex = startIndex << 1;
+ final int delIndex = endIndex << 1;
+ final byte[] bytes = Arrays.copyOfRange(data, ssIndex, delIndex);
+ ss = new SimpleString(bytes);
+ }
+ // We will create the ArrayList lazily
+ if (all == null) {
+ // There will be at least 3 strings on this case (which is the actual common usecase)
--- End diff --
Good point: the ArrayList is temporary and although it won't be allocated on the stack having 2 instead of 3 won't make a big difference considering that will die young. I've noticed that most use cases need at least 3 to avoid enlarging of capacity so I've changed it: I haven't changed the original version yet as you have noticed and I could do :)
2 or 3 are very "magical" numbers anyway :P
---
[GitHub] activemq-artemis issue #1786: ARTEMIS-1616 OpenWire improvements
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:
https://github.com/apache/activemq-artemis/pull/1786
@michaelandrepearce TBH that's what I was thinking to do, but @clebertsuconic suggested that having all the commits but just 1 pr wasn't a bad thing at all: he has more experience than me and I trust him so I think there could be some very good reasons (like reverting just what could break something, if any?) to suggest me that...I will ask him better when he will be online anyway :+1:
---
[GitHub] activemq-artemis pull request #1786: ARTEMIS-1616 OpenWire improvements
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1786#discussion_r162610245
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/api/core/SimpleString.java ---
@@ -361,13 +371,68 @@ public int hashCode() {
}
}
+ private static SimpleString[] splitWithCachedString(final SimpleString simpleString, final int delim) {
+ final String str = simpleString.str;
+ final byte[] data = simpleString.data;
+ final int length = str.length();
+ List<SimpleString> all = null;
+ int index = 0;
+ while (index < length) {
+ final int delimIndex = str.indexOf(delim, index);
+ if (delimIndex == -1) {
+ //just need to add the last one
+ break;
+ } else {
+ all = addSimpleStringPart(all, data, index, delimIndex);
+ }
+ index = delimIndex + 1;
+ }
+ if (all == null) {
+ return new SimpleString[]{simpleString};
+ } else {
+ // Adding the last one
+ all = addSimpleStringPart(all, data, index, length);
+ // Converting it to arrays
+ final SimpleString[] parts = new SimpleString[all.size()];
+ return all.toArray(parts);
+ }
+ }
+
+ private static List<SimpleString> addSimpleStringPart(List<SimpleString> all,
+ final byte[] data,
+ final int startIndex,
+ final int endIndex) {
+ final int expectedLength = endIndex - startIndex;
+ final SimpleString ss;
+ if (expectedLength == 0) {
+ ss = EMPTY;
+ } else {
+ //extract a byte[] copy from this
+ final int ssIndex = startIndex << 1;
+ final int delIndex = endIndex << 1;
+ final byte[] bytes = Arrays.copyOfRange(data, ssIndex, delIndex);
+ ss = new SimpleString(bytes);
+ }
+ // We will create the ArrayList lazily
+ if (all == null) {
+ // There will be at least 3 strings on this case (which is the actual common usecase)
--- End diff --
Difficult to say: I've found split to be used on AddressImpl mainly and openwire uses at least 3 parts addresses, while the other protocols seems to not rely heavily on split.
Probably @clebert has a better knowledge of how he has chosen 2 instead, wdyt?
---
[GitHub] activemq-artemis pull request #1786: ARTEMIS-1616 OpenWire improvements
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1786#discussion_r162492929
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/api/core/SimpleString.java ---
@@ -361,13 +371,68 @@ public int hashCode() {
}
}
+ private static SimpleString[] splitWithCachedString(final SimpleString simpleString, final int delim) {
+ final String str = simpleString.str;
+ final byte[] data = simpleString.data;
+ final int length = str.length();
+ List<SimpleString> all = null;
+ int index = 0;
+ while (index < length) {
+ final int delimIndex = str.indexOf(delim, index);
+ if (delimIndex == -1) {
+ //just need to add the last one
+ break;
+ } else {
+ all = addSimpleStringPart(all, data, index, delimIndex);
+ }
+ index = delimIndex + 1;
+ }
+ if (all == null) {
+ return new SimpleString[]{simpleString};
+ } else {
+ // Adding the last one
+ all = addSimpleStringPart(all, data, index, length);
+ // Converting it to arrays
+ final SimpleString[] parts = new SimpleString[all.size()];
+ return all.toArray(parts);
+ }
+ }
+
+ private static List<SimpleString> addSimpleStringPart(List<SimpleString> all,
+ final byte[] data,
+ final int startIndex,
+ final int endIndex) {
+ final int expectedLength = endIndex - startIndex;
+ final SimpleString ss;
+ if (expectedLength == 0) {
+ ss = EMPTY;
+ } else {
+ //extract a byte[] copy from this
+ final int ssIndex = startIndex << 1;
+ final int delIndex = endIndex << 1;
+ final byte[] bytes = Arrays.copyOfRange(data, ssIndex, delIndex);
+ ss = new SimpleString(bytes);
+ }
+ // We will create the ArrayList lazily
+ if (all == null) {
+ // There will be at least 3 strings on this case (which is the actual common usecase)
--- End diff --
as noted what are these 3 "magical" use cases, it be good to enumerate them. I'm still left wondering :)
---
[GitHub] activemq-artemis issue #1786: ARTEMIS-1616 OpenWire improvements
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1786
@franz1981 nudge, only thing holding me from merging this is the number of commits.
I guess really should be a @clebertsuconic nudge if he's asked for this on purpose.
---
[GitHub] activemq-artemis issue #1786: ARTEMIS-1616 OpenWire improvements
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:
https://github.com/apache/activemq-artemis/pull/1786
Please do not merge it yet: I need to update all the commit messages adding the JIRA.
The current PR has already passed the same OpenWire tests of master with similar results, but the most interesting part is that it improves dramatically the performances of OpenWire: on my box a 1 producer 1 consumer non durable queue with master perform ~75 K msg/sec while with this PR ~98 K msg/sec.
---
[GitHub] activemq-artemis pull request #1786: ARTEMIS-1616 OpenWire improvements
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1786#discussion_r163012542
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/api/core/SimpleString.java ---
@@ -361,13 +371,68 @@ public int hashCode() {
}
}
+ private static SimpleString[] splitWithCachedString(final SimpleString simpleString, final int delim) {
+ final String str = simpleString.str;
+ final byte[] data = simpleString.data;
+ final int length = str.length();
+ List<SimpleString> all = null;
+ int index = 0;
+ while (index < length) {
+ final int delimIndex = str.indexOf(delim, index);
+ if (delimIndex == -1) {
+ //just need to add the last one
+ break;
+ } else {
+ all = addSimpleStringPart(all, data, index, delimIndex);
+ }
+ index = delimIndex + 1;
+ }
+ if (all == null) {
+ return new SimpleString[]{simpleString};
+ } else {
+ // Adding the last one
+ all = addSimpleStringPart(all, data, index, length);
+ // Converting it to arrays
+ final SimpleString[] parts = new SimpleString[all.size()];
+ return all.toArray(parts);
+ }
+ }
+
+ private static List<SimpleString> addSimpleStringPart(List<SimpleString> all,
+ final byte[] data,
+ final int startIndex,
+ final int endIndex) {
+ final int expectedLength = endIndex - startIndex;
+ final SimpleString ss;
+ if (expectedLength == 0) {
+ ss = EMPTY;
+ } else {
+ //extract a byte[] copy from this
+ final int ssIndex = startIndex << 1;
+ final int delIndex = endIndex << 1;
+ final byte[] bytes = Arrays.copyOfRange(data, ssIndex, delIndex);
+ ss = new SimpleString(bytes);
+ }
+ // We will create the ArrayList lazily
+ if (all == null) {
+ // There will be at least 3 strings on this case (which is the actual common usecase)
--- End diff --
@franz1981 just thinking on this is it worth making the other array list also 3? Just so both is consistent, maybe make it a constant, so if in future it changes again, single change would affect both.
---
[GitHub] activemq-artemis pull request #1786: ARTEMIS-1616 OpenWire improvements
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1786#discussion_r162272711
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/api/core/SimpleString.java ---
@@ -361,13 +371,68 @@ public int hashCode() {
}
}
+ private static SimpleString[] splitWithCachedString(final SimpleString simpleString, final int delim) {
--- End diff --
To avoid the translation into into each time because String::indexOf is using int
---
[GitHub] activemq-artemis pull request #1786: ARTEMIS-1616 OpenWire improvements
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/activemq-artemis/pull/1786
---
[GitHub] activemq-artemis pull request #1786: ARTEMIS-1616 OpenWire improvements
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1786#discussion_r162636748
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/api/core/SimpleString.java ---
@@ -361,13 +371,68 @@ public int hashCode() {
}
}
+ private static SimpleString[] splitWithCachedString(final SimpleString simpleString, final int delim) {
--- End diff --
Ah :) nice, very subtle.
---
[GitHub] activemq-artemis pull request #1786: ARTEMIS-1616 OpenWire improvements
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1786#discussion_r162266546
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/api/core/SimpleString.java ---
@@ -361,13 +371,68 @@ public int hashCode() {
}
}
+ private static SimpleString[] splitWithCachedString(final SimpleString simpleString, final int delim) {
--- End diff --
why int not char, for delim
---
[GitHub] activemq-artemis pull request #1786: ARTEMIS-1616 OpenWire improvements
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1786#discussion_r162262393
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/api/core/SimpleString.java ---
@@ -361,13 +371,68 @@ public int hashCode() {
}
}
+ private static SimpleString[] splitWithCachedString(final SimpleString simpleString, final int delim) {
+ final String str = simpleString.str;
+ final byte[] data = simpleString.data;
+ final int length = str.length();
+ List<SimpleString> all = null;
+ int index = 0;
+ while (index < length) {
+ final int delimIndex = str.indexOf(delim, index);
+ if (delimIndex == -1) {
+ //just need to add the last one
+ break;
+ } else {
+ all = addSimpleStringPart(all, data, index, delimIndex);
+ }
+ index = delimIndex + 1;
+ }
+ if (all == null) {
+ return new SimpleString[]{simpleString};
+ } else {
+ // Adding the last one
+ all = addSimpleStringPart(all, data, index, length);
+ // Converting it to arrays
+ final SimpleString[] parts = new SimpleString[all.size()];
+ return all.toArray(parts);
+ }
+ }
+
+ private static List<SimpleString> addSimpleStringPart(List<SimpleString> all,
+ final byte[] data,
+ final int startIndex,
+ final int endIndex) {
+ final int expectedLength = endIndex - startIndex;
+ final SimpleString ss;
+ if (expectedLength == 0) {
+ ss = EMPTY;
--- End diff --
How we sure, that expectedLength 0 shouldn't equal null?
---
[GitHub] activemq-artemis pull request #1786: ARTEMIS-1616 OpenWire improvements
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1786#discussion_r162262699
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/api/core/SimpleString.java ---
@@ -361,13 +371,68 @@ public int hashCode() {
}
}
+ private static SimpleString[] splitWithCachedString(final SimpleString simpleString, final int delim) {
+ final String str = simpleString.str;
+ final byte[] data = simpleString.data;
+ final int length = str.length();
+ List<SimpleString> all = null;
+ int index = 0;
+ while (index < length) {
+ final int delimIndex = str.indexOf(delim, index);
+ if (delimIndex == -1) {
+ //just need to add the last one
+ break;
+ } else {
+ all = addSimpleStringPart(all, data, index, delimIndex);
+ }
+ index = delimIndex + 1;
+ }
+ if (all == null) {
+ return new SimpleString[]{simpleString};
+ } else {
+ // Adding the last one
+ all = addSimpleStringPart(all, data, index, length);
+ // Converting it to arrays
+ final SimpleString[] parts = new SimpleString[all.size()];
+ return all.toArray(parts);
+ }
+ }
+
+ private static List<SimpleString> addSimpleStringPart(List<SimpleString> all,
+ final byte[] data,
+ final int startIndex,
+ final int endIndex) {
+ final int expectedLength = endIndex - startIndex;
+ final SimpleString ss;
+ if (expectedLength == 0) {
+ ss = EMPTY;
+ } else {
+ //extract a byte[] copy from this
+ final int ssIndex = startIndex << 1;
+ final int delIndex = endIndex << 1;
+ final byte[] bytes = Arrays.copyOfRange(data, ssIndex, delIndex);
+ ss = new SimpleString(bytes);
+ }
+ // We will create the ArrayList lazily
+ if (all == null) {
+ // There will be at least 3 strings on this case (which is the actual common usecase)
--- End diff --
Is it worth iterating these cases? E.g I'm left wondering what this is about :)
---