[GitHub] activemq-artemis pull request #1786: ARTEMIS-1616 OpenWire improvements

[asfgit]

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

[michaelandrepearce]

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

[michaelandrepearce]

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

[michaelandrepearce]

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

[franz1981]

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

[michaelandrepearce]

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

[franz1981]

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

[franz1981]

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

[michaelandrepearce]

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

[michaelandrepearce]

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

[michaelandrepearce]

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

[franz1981]

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




---