[GitHub] activemq-artemis pull request #1786: ARTEMIS-1616 OpenWire improvements
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
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 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 addSimpleStringPart(List 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
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 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 addSimpleStringPart(List 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
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
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 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 addSimpleStringPart(List 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
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 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 addSimpleStringPart(List 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 pull request #1786: ARTEMIS-1616 OpenWire improvements
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
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 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 addSimpleStringPart(List 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 pull request #1786: ARTEMIS-1616 OpenWire improvements
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
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 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 addSimpleStringPart(List 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 :) ---
[GitHub] activemq-artemis pull request #1786: ARTEMIS-1616 OpenWire improvements
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 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 addSimpleStringPart(List 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
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 Date: 2018-01-16T13:24:32Z Avoided copy of CoreMessage when not needed and cached lambda on hot path commit e4f9a4f9a0bc3277e580a3c014ba7d422597b81e Author: Francesco Nigro Date: 2018-01-16T16:01:54Z Refactored inbound in order to group help the JVM compile most used methods commit 82d1cd8c5489c106b5ff9e22b264216988b7986e Author: Francesco Nigro Date: 2018-01-16T17:40:06Z Optimized SimpleString::split because heavily used into AddressImpl::new commit df18521298e8866d1e75e299336d25845b97f663 Author: Francesco Nigro Date: 2018-01-16T20:27:22Z Added existing queues cache to avoid multiple expensive checkCreateQueue calls commit 74eb72bffa7c723e714d4744fcc9e7207b5fe92a Author: Francesco Nigro Date: 2018-01-16T21:24:08Z used SimpleString to avoid translations on CoreMessage commit 5752e30a9363bc473bd52e3ae09c677687ad1ab2 Author: Francesco Nigro Date: 2018-01-16T23:37:24Z cached Notification Destination check on AMQConsumer commit 94c9de15fb729d9e8294166cc9d48fdd0e09 Author: Francesco Nigro Date: 2018-01-17T07:29:28Z Used SimpleString explicitly with HDR_DUPLICATE_DETECTION_ID and CONNECTION_ID_PROPERTY_NAME commit 1f53625848ae10269c9748b4125848609fd3854b Author: Francesco Nigro Date: 2018-01-17T13:37:08Z Refactored toAMQMessage by grouping methods for a better readability ---