[GitHub] activemq-artemis issue #1752: ARTEMIS-1586 Reduce GC pressure due to String ...

[michaelandrepearce]

Github user michaelandrepearce commented on the issue:

https://github.com/apache/activemq-artemis/pull/1752
  
@clebertsuconic tbh, i can see the saving on property keys, and address per 
session.

Also within the consumer and producer (though for producing the saving 
would be on String to SimpleString for the keys so one extra variation needed), 
if you think typically a producer would send typically to one or a few 
destinations and then send a million of them, like wise on message properties, 
if you set one you're most likely to set it all the time, and then again 
typically a consumer would see all the same. 

The only bit I'm dubious on its benefit is property values.

Lastly on the implementation detail this i think needs some work/cleanup, 
though after finding/understanding through the discussion with @franz1981  its 
more of a last seen cache/pool so I'm more +1 it, just the name made me to 
expect something different at first.


---

[GitHub] activemq-artemis pull request #1752: ARTEMIS-1586 Reduce GC pressure due to ...

[michaelandrepearce]

Github user michaelandrepearce commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1752#discussion_r160020175
  
--- Diff: 
artemis-commons/src/main/java/org/apache/activemq/artemis/utils/collections/TypedProperties.java
 ---
@@ -329,7 +331,9 @@ public boolean containsProperty(final SimpleString key) 
{
   }
}
 
-   public synchronized void decode(final ByteBuf buffer) {
--- End diff --

haha


---

[GitHub] activemq-artemis pull request #1755: ARTEMIS-1586 Reduce GC pressure due to ...

[michaelandrepearce]

Github user michaelandrepearce closed the pull request at:

https://github.com/apache/activemq-artemis/pull/1755


---

[GitHub] activemq-artemis issue #1752: ARTEMIS-1586 Reduce GC pressure due to String ...

[franz1981]

Github user franz1981 commented on the issue:

https://github.com/apache/activemq-artemis/pull/1752
  
@clebertsuconic you're right , all good points: I was trying to understand 
which parts could benefit (if needed of course) such kind of optimizations too 
and @michaelandrepearce is helping me a lot to understand common cases that I 
haven't covered properly or that are breaking any of the assumptions I've made.
Re the benefits the one I can anticipate with a standard JMS test is a 
reduction of >3X of garbage productions without any perf regression, but as 
@michaelandrepearce pointed me maybe other tests would invalidate such 
improvements...


---

[GitHub] activemq-artemis pull request #1752: ARTEMIS-1586 Reduce GC pressure due to ...

[franz1981]

Github user franz1981 commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1752#discussion_r160014921
  
--- Diff: 
artemis-commons/src/main/java/org/apache/activemq/artemis/utils/collections/TypedProperties.java
 ---
@@ -329,7 +331,9 @@ public boolean containsProperty(final SimpleString key) 
{
   }
}
 
-   public synchronized void decode(final ByteBuf buffer) {
--- End diff --

Argh! My fault...github diff can't expect 100 line shift I think!


---

[GitHub] activemq-artemis pull request #:

[franz1981]

Github user franz1981 commented on the pull request:


https://github.com/apache/activemq-artemis/commit/05389b9fd830bbe82cce9b3496dcd0d82c8d9099#commitcomment-26681117
  
In 
artemis-commons/src/main/java/org/apache/activemq/artemis/api/core/SimpleString.java:
In 
artemis-commons/src/main/java/org/apache/activemq/artemis/api/core/SimpleString.java
 on line 61:
It doesn't save the cost to create the byte[] for the most common case ie 
when the string is already interned.


---

[GitHub] activemq-artemis pull request #:

[franz1981]

Github user franz1981 commented on the pull request:


https://github.com/apache/activemq-artemis/commit/05389b9fd830bbe82cce9b3496dcd0d82c8d9099#commitcomment-26681114
  
In 
artemis-commons/src/main/java/org/apache/activemq/artemis/api/core/SimpleString.java:
In 
artemis-commons/src/main/java/org/apache/activemq/artemis/api/core/SimpleString.java
 on line 40:
Guava interners are using internally concurrent hash mapS of weak/soft 
references: it is not a good way to reduce the GC pressure because it gives 
more works to the GC due to unlimited footprint, scattered stores, hidden 
additional allocations (eg map entries + soft/weak ref)...


---

[GitHub] activemq-artemis issue #1752: ARTEMIS-1586 Reduce GC pressure due to String ...

[clebertsuconic]

Github user clebertsuconic commented on the issue:

https://github.com/apache/activemq-artemis/pull/1752
  
I’m always concerned about over optimization’s on the core protocol.  
It’s already really fast. 


If we wanted to invest in core the best would be to reuse buffers from 
messages.  


I don’t see much gain here TBH. 


---

[GitHub] activemq-artemis pull request #1752: ARTEMIS-1586 Reduce GC pressure due to ...

[michaelandrepearce]

Github user michaelandrepearce commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1752#discussion_r160011868
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/core/protocol/ServerPacketDecoder.java
 ---
@@ -83,16 +85,34 @@
 
 public class ServerPacketDecoder extends ClientPacketDecoder {
 
+   private static final int UUID_LENGTH = 36;
+   private static final int DEFAULT_INTERNER_CAPACITY = 32;
private static final long serialVersionUID = 3348673114388400766L;
-   public static final ServerPacketDecoder INSTANCE = new 
ServerPacketDecoder();
+   private SimpleString.Interner keysInterner;
+   private TypedProperties.StringValue.Interner valuesInterner;
 
-   private static SessionSendMessage decodeSessionSendMessage(final 
ActiveMQBuffer in, CoreRemotingConnection connection) {
+   public ServerPacketDecoder() {
+  this.keysInterner = null;
+  this.valuesInterner = null;
+   }
+
+   private void initializeInternersIfNeeded() {
--- End diff --

I still think a global interning solution will be more beneficial 
especially with the other protocols that don't use SimpleString, but more use 
String then convert to SimpleString to keep ICoreMessage interface, but this 
will have same impact (especially in multi protocol broker env like with 
clients on AMQP and CORE).






---

[GitHub] activemq-artemis pull request #1752: ARTEMIS-1586 Reduce GC pressure due to ...

[michaelandrepearce]

Github user michaelandrepearce commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1752#discussion_r160011547
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/core/protocol/ServerPacketDecoder.java
 ---
@@ -83,16 +85,34 @@
 
 public class ServerPacketDecoder extends ClientPacketDecoder {
 
+   private static final int UUID_LENGTH = 36;
+   private static final int DEFAULT_INTERNER_CAPACITY = 32;
private static final long serialVersionUID = 3348673114388400766L;
-   public static final ServerPacketDecoder INSTANCE = new 
ServerPacketDecoder();
+   private SimpleString.Interner keysInterner;
--- End diff --

And then configure the consumer also to use it


---

[GitHub] activemq-artemis pull request #1752: ARTEMIS-1586 Reduce GC pressure due to ...

[michaelandrepearce]

Github user michaelandrepearce commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1752#discussion_r160011036
  
--- Diff: 
artemis-core-client/src/main/java/org/apache/activemq/artemis/core/message/impl/CoreMessage.java
 ---
@@ -528,8 +543,17 @@ public void decodeHeadersAndProperties(final ByteBuf 
buffer) {
private void decodeHeadersAndProperties(final ByteBuf buffer, boolean 
lazyProperties) {
   messageIDPosition = buffer.readerIndex();
   messageID = buffer.readLong();
-
-  address = SimpleString.readNullableSimpleString(buffer);
+  int b = buffer.readByte();
+  if (b != DataConstants.NULL) {
+ final int length = buffer.readInt();
+ if (keysInterner != null) {
+address = keysInterner.intern(buffer, length);
--- End diff --

btw, on another note, correlation id will kill you as each one should be 
unique, so would constantly invalidate, need to be a bit careful on property 
values to pool


---

[GitHub] activemq-artemis pull request #1752: ARTEMIS-1586 Reduce GC pressure due to ...

[franz1981]

Github user franz1981 commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1752#discussion_r160010613
  
--- Diff: 
artemis-core-client/src/main/java/org/apache/activemq/artemis/core/message/impl/CoreMessage.java
 ---
@@ -528,8 +543,17 @@ public void decodeHeadersAndProperties(final ByteBuf 
buffer) {
private void decodeHeadersAndProperties(final ByteBuf buffer, boolean 
lazyProperties) {
   messageIDPosition = buffer.readerIndex();
   messageID = buffer.readLong();
-
-  address = SimpleString.readNullableSimpleString(buffer);
+  int b = buffer.readByte();
+  if (b != DataConstants.NULL) {
+ final int length = buffer.readInt();
+ if (keysInterner != null) {
+address = keysInterner.intern(buffer, length);
--- End diff --

This is a very good point..yes, agree!


---

[GitHub] activemq-artemis pull request #1752: ARTEMIS-1586 Reduce GC pressure due to ...

[michaelandrepearce]

Github user michaelandrepearce commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1752#discussion_r160010591
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/core/protocol/ServerPacketDecoder.java
 ---
@@ -83,16 +85,34 @@
 
 public class ServerPacketDecoder extends ClientPacketDecoder {
 
+   private static final int UUID_LENGTH = 36;
+   private static final int DEFAULT_INTERNER_CAPACITY = 32;
private static final long serialVersionUID = 3348673114388400766L;
-   public static final ServerPacketDecoder INSTANCE = new 
ServerPacketDecoder();
+   private SimpleString.Interner keysInterner;
--- End diff --

Surely this is beneficial for clients also, i would look to put the Object 
pools/caches, down at PacketDecoder level, and have a number of these settable 
(address, typed property key, typed properly value) .


---

[GitHub] activemq-artemis pull request #1752: ARTEMIS-1586 Reduce GC pressure due to ...

[michaelandrepearce]

Github user michaelandrepearce commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1752#discussion_r160009754
  
--- Diff: 
artemis-core-client/src/main/java/org/apache/activemq/artemis/core/message/impl/CoreMessage.java
 ---
@@ -528,8 +543,17 @@ public void decodeHeadersAndProperties(final ByteBuf 
buffer) {
private void decodeHeadersAndProperties(final ByteBuf buffer, boolean 
lazyProperties) {
   messageIDPosition = buffer.readerIndex();
   messageID = buffer.readLong();
-
-  address = SimpleString.readNullableSimpleString(buffer);
+  int b = buffer.readByte();
+  if (b != DataConstants.NULL) {
+ final int length = buffer.readInt();
+ if (keysInterner != null) {
+address = keysInterner.intern(buffer, length);
--- End diff --

If you are having separate pools of last seen SimpleStrings instead of 
global interning for typed properties keys and values, you should probably pool 
address's separate from those. Imagine the pool is size 32 default, very 
quickly be used up and find typedproperty keys are invalidating address's in 
that last seen pool


---

[GitHub] activemq-artemis pull request #1752: ARTEMIS-1586 Reduce GC pressure due to ...

[michaelandrepearce]

Github user michaelandrepearce commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1752#discussion_r160008652
  
--- Diff: 
artemis-commons/src/main/java/org/apache/activemq/artemis/utils/collections/TypedProperties.java
 ---
@@ -329,7 +331,9 @@ public boolean containsProperty(final SimpleString key) 
{
   }
}
 
-   public synchronized void decode(final ByteBuf buffer) {
--- End diff --

Ah spotted it, line 432almost 100 line jump!


---

[GitHub] activemq-artemis pull request #1752: ARTEMIS-1586 Reduce GC pressure due to ...

[michaelandrepearce]

Github user michaelandrepearce commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1752#discussion_r160007968
  
--- Diff: 
artemis-commons/src/main/java/org/apache/activemq/artemis/utils/AbstractInterner.java
 ---
@@ -0,0 +1,157 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.activemq.artemis.utils;
+
+import io.netty.buffer.ByteBuf;
+import io.netty.util.internal.MathUtil;
+import io.netty.util.internal.PlatformDependent;
+
+/**
+ * Thread-safe {@code } interner.
+ * 
+ * Differently from {@link String#intern()} it contains a fixed amount of 
entries and
+ * when used by concurrent threads it doesn't ensure the uniqueness of the 
entries ie
+ * the same entry could be allocated multiple times by concurrent calls.
+ */
+public abstract class AbstractInterner {
+
+   private final T[] entries;
--- End diff --

Yeah but notice how its named under package pool, because really its just 
pooling the x number of last Strings that the CharSequence has been toString'd.


---

[GitHub] activemq-artemis pull request #1752: ARTEMIS-1586 Reduce GC pressure due to ...

[franz1981]

Github user franz1981 commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1752#discussion_r160007930
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/core/protocol/ServerPacketDecoder.java
 ---
@@ -83,16 +85,34 @@
 
 public class ServerPacketDecoder extends ClientPacketDecoder {
 
+   private static final int UUID_LENGTH = 36;
+   private static final int DEFAULT_INTERNER_CAPACITY = 32;
private static final long serialVersionUID = 3348673114388400766L;
-   public static final ServerPacketDecoder INSTANCE = new 
ServerPacketDecoder();
+   private SimpleString.Interner keysInterner;
+   private TypedProperties.StringValue.Interner valuesInterner;
 
-   private static SessionSendMessage decodeSessionSendMessage(final 
ActiveMQBuffer in, CoreRemotingConnection connection) {
+   public ServerPacketDecoder() {
+  this.keysInterner = null;
+  this.valuesInterner = null;
+   }
+
+   private void initializeInternersIfNeeded() {
--- End diff --

This is not how escape analysis work sadly: not on the standard Oracle JVM 
at least. 
Maybe Graal does a better job here because if allows partial escaping 
(first occurrence and never after too) while packing instances on the stack.


---

[GitHub] activemq-artemis pull request #1752: ARTEMIS-1586 Reduce GC pressure due to ...

[michaelandrepearce]

Github user michaelandrepearce commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1752#discussion_r160007505
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/core/protocol/ServerPacketDecoder.java
 ---
@@ -83,16 +85,34 @@
 
 public class ServerPacketDecoder extends ClientPacketDecoder {
 
+   private static final int UUID_LENGTH = 36;
+   private static final int DEFAULT_INTERNER_CAPACITY = 32;
private static final long serialVersionUID = 3348673114388400766L;
-   public static final ServerPacketDecoder INSTANCE = new 
ServerPacketDecoder();
+   private SimpleString.Interner keysInterner;
+   private TypedProperties.StringValue.Interner valuesInterner;
 
-   private static SessionSendMessage decodeSessionSendMessage(final 
ActiveMQBuffer in, CoreRemotingConnection connection) {
+   public ServerPacketDecoder() {
+  this.keysInterner = null;
+  this.valuesInterner = null;
+   }
+
+   private void initializeInternersIfNeeded() {
--- End diff --

intern only steals it on first occurance so only that one escapes the stack 
as thats the one held in the intern, all others that are the same as the 
interned so wouldn't escape it.




---

[GitHub] activemq-artemis pull request #1752: ARTEMIS-1586 Reduce GC pressure due to ...

[michaelandrepearce]

Github user michaelandrepearce commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1752#discussion_r160007029
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/core/protocol/ServerPacketDecoder.java
 ---
@@ -83,16 +85,34 @@
 
 public class ServerPacketDecoder extends ClientPacketDecoder {
 
+   private static final int UUID_LENGTH = 36;
+   private static final int DEFAULT_INTERNER_CAPACITY = 32;
private static final long serialVersionUID = 3348673114388400766L;
-   public static final ServerPacketDecoder INSTANCE = new 
ServerPacketDecoder();
+   private SimpleString.Interner keysInterner;
+   private TypedProperties.StringValue.Interner valuesInterner;
 
-   private static SessionSendMessage decodeSessionSendMessage(final 
ActiveMQBuffer in, CoreRemotingConnection connection) {
+   public ServerPacketDecoder() {
+  this.keysInterner = null;
+  this.valuesInterner = null;
+   }
+
+   private void initializeInternersIfNeeded() {
--- End diff --

see my note on the other comment, we can avoid the copy and byte[] object 
on heap too with keeping that on stack


---

[GitHub] activemq-artemis pull request #1752: ARTEMIS-1586 Reduce GC pressure due to ...

[franz1981]

Github user franz1981 commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1752#discussion_r160006951
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/core/protocol/ServerPacketDecoder.java
 ---
@@ -83,16 +85,34 @@
 
 public class ServerPacketDecoder extends ClientPacketDecoder {
 
+   private static final int UUID_LENGTH = 36;
+   private static final int DEFAULT_INTERNER_CAPACITY = 32;
private static final long serialVersionUID = 3348673114388400766L;
-   public static final ServerPacketDecoder INSTANCE = new 
ServerPacketDecoder();
+   private SimpleString.Interner keysInterner;
+   private TypedProperties.StringValue.Interner valuesInterner;
 
-   private static SessionSendMessage decodeSessionSendMessage(final 
ActiveMQBuffer in, CoreRemotingConnection connection) {
+   public ServerPacketDecoder() {
+  this.keysInterner = null;
+  this.valuesInterner = null;
+   }
+
+   private void initializeInternersIfNeeded() {
--- End diff --

It is fragile due to how escape analysis work: looking just at the code the 
array escapes partially so it is not always true that will live on the stack...
And considering that the intern could steal it...probably it wont's work...
I've fought many years against these weird things of the JVM dear Michael :(


---

[GitHub] activemq-artemis pull request #1752: ARTEMIS-1586 Reduce GC pressure due to ...

[michaelandrepearce]

Github user michaelandrepearce commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1752#discussion_r160006047
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/core/protocol/ServerPacketDecoder.java
 ---
@@ -83,16 +85,34 @@
 
 public class ServerPacketDecoder extends ClientPacketDecoder {
 
+   private static final int UUID_LENGTH = 36;
+   private static final int DEFAULT_INTERNER_CAPACITY = 32;
private static final long serialVersionUID = 3348673114388400766L;
-   public static final ServerPacketDecoder INSTANCE = new 
ServerPacketDecoder();
+   private SimpleString.Interner keysInterner;
+   private TypedProperties.StringValue.Interner valuesInterner;
 
-   private static SessionSendMessage decodeSessionSendMessage(final 
ActiveMQBuffer in, CoreRemotingConnection connection) {
+   public ServerPacketDecoder() {
+  this.keysInterner = null;
+  this.valuesInterner = null;
+   }
+
+   private void initializeInternersIfNeeded() {
--- End diff --

Ok, going back to previous note: 
" but it is by avoiding the byte[] that is reduced by far most of the 
garbage"

We can do this on stack though with lamda's :) 


---

[GitHub] activemq-artemis pull request #1752: ARTEMIS-1586 Reduce GC pressure due to ...

[franz1981]

Github user franz1981 commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1752#discussion_r160005561
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/core/protocol/ServerPacketDecoder.java
 ---
@@ -83,16 +85,34 @@
 
 public class ServerPacketDecoder extends ClientPacketDecoder {
 
+   private static final int UUID_LENGTH = 36;
+   private static final int DEFAULT_INTERNER_CAPACITY = 32;
private static final long serialVersionUID = 3348673114388400766L;
-   public static final ServerPacketDecoder INSTANCE = new 
ServerPacketDecoder();
+   private SimpleString.Interner keysInterner;
+   private TypedProperties.StringValue.Interner valuesInterner;
 
-   private static SessionSendMessage decodeSessionSendMessage(final 
ActiveMQBuffer in, CoreRemotingConnection connection) {
+   public ServerPacketDecoder() {
+  this.keysInterner = null;
+  this.valuesInterner = null;
+   }
+
+   private void initializeInternersIfNeeded() {
--- End diff --

sadly we can't do it with ease for different reasons:
- most byte[] operations are optimized by the JVM to avoid most of the 
bounds checks, on ByteBuf I've just provided a PR on Netty to improve this thing
- I wouldn't change SimpleString that is used everywhere (literally) on 
Artemis
- the lifecycle of the original ByteBuf is not predicatable: sometimes are 
pooled sometimes not, offheap/onheap...not that simple honestly


---

[GitHub] activemq-artemis pull request #1752: ARTEMIS-1586 Reduce GC pressure due to ...

[michaelandrepearce]

Github user michaelandrepearce commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1752#discussion_r160005095
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/core/protocol/ServerPacketDecoder.java
 ---
@@ -83,16 +85,34 @@
 
 public class ServerPacketDecoder extends ClientPacketDecoder {
 
+   private static final int UUID_LENGTH = 36;
+   private static final int DEFAULT_INTERNER_CAPACITY = 32;
private static final long serialVersionUID = 3348673114388400766L;
-   public static final ServerPacketDecoder INSTANCE = new 
ServerPacketDecoder();
+   private SimpleString.Interner keysInterner;
+   private TypedProperties.StringValue.Interner valuesInterner;
 
-   private static SessionSendMessage decodeSessionSendMessage(final 
ActiveMQBuffer in, CoreRemotingConnection connection) {
+   public ServerPacketDecoder() {
+  this.keysInterner = null;
+  this.valuesInterner = null;
+   }
+
+   private void initializeInternersIfNeeded() {
--- End diff --

Ok, so thats simple to change, why don't simple make SimpleString hold data 
as ByteBuf not byte[].

This then also would save this, and save further.


---

[GitHub] activemq-artemis pull request #1752: ARTEMIS-1586 Reduce GC pressure due to ...

[franz1981]

Github user franz1981 commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1752#discussion_r160005087
  
--- Diff: 
artemis-commons/src/main/java/org/apache/activemq/artemis/utils/AbstractInterner.java
 ---
@@ -0,0 +1,157 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.activemq.artemis.utils;
+
+import io.netty.buffer.ByteBuf;
+import io.netty.util.internal.MathUtil;
+import io.netty.util.internal.PlatformDependent;
+
+/**
+ * Thread-safe {@code } interner.
+ * 
+ * Differently from {@link String#intern()} it contains a fixed amount of 
entries and
+ * when used by concurrent threads it doesn't ensure the uniqueness of the 
entries ie
+ * the same entry could be allocated multiple times by concurrent calls.
+ */
+public abstract class AbstractInterner {
--- End diff --

Tomorrow (today in Italy :)) I will provide a JMH benchmark to show the 
difference between each approach (guava too), but consider the semantic 
differences between the interners: mine is packing all the pooled instances 
into a Object[] of fixed size not scattered along the heap and without using 
memory barriers to solve the potential races, while the Guava one is using a 
concurrent hash map with soft references, so the comparision won't be 100% fair.


---

[GitHub] activemq-artemis pull request #1752: ARTEMIS-1586 Reduce GC pressure due to ...

[franz1981]

Github user franz1981 commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1752#discussion_r160004663
  
--- Diff: 
artemis-commons/src/main/java/org/apache/activemq/artemis/utils/AbstractInterner.java
 ---
@@ -0,0 +1,157 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.activemq.artemis.utils;
+
+import io.netty.buffer.ByteBuf;
+import io.netty.util.internal.MathUtil;
+import io.netty.util.internal.PlatformDependent;
+
+/**
+ * Thread-safe {@code } interner.
+ * 
+ * Differently from {@link String#intern()} it contains a fixed amount of 
entries and
+ * when used by concurrent threads it doesn't ensure the uniqueness of the 
entries ie
+ * the same entry could be allocated multiple times by concurrent calls.
+ */
+public abstract class AbstractInterner {
--- End diff --

please use JMH if possible: if not you have to create a volatile static 
field where write the array[inx] nad read at the end of the benchmark to avoid 
the JVM to drop the code or use Escape Analysis to unpack the SimpleString on 
the stack



---

[GitHub] activemq-artemis pull request #1752: ARTEMIS-1586 Reduce GC pressure due to ...

[franz1981]

Github user franz1981 commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1752#discussion_r160004310
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/core/protocol/ServerPacketDecoder.java
 ---
@@ -83,16 +85,34 @@
 
 public class ServerPacketDecoder extends ClientPacketDecoder {
 
+   private static final int UUID_LENGTH = 36;
+   private static final int DEFAULT_INTERNER_CAPACITY = 32;
private static final long serialVersionUID = 3348673114388400766L;
-   public static final ServerPacketDecoder INSTANCE = new 
ServerPacketDecoder();
+   private SimpleString.Interner keysInterner;
+   private TypedProperties.StringValue.Interner valuesInterner;
 
-   private static SessionSendMessage decodeSessionSendMessage(final 
ActiveMQBuffer in, CoreRemotingConnection connection) {
+   public ServerPacketDecoder() {
+  this.keysInterner = null;
+  this.valuesInterner = null;
+   }
+
+   private void initializeInternersIfNeeded() {
--- End diff --

> to check if equal/hash you're still having to process through that 
bytebuf bringing the bytes (if off heap) onto heap and doing a calc on them.

That cost is reduced avoiding bounds checking and limiting the length of 
the byte stream and if compare very well vs byte[] copy + SimpleString 
allocation (measured and tested with JMH).

>All creating SimpleString with data does is simple sets the byte[] as 
data, no decoding.

It is missing a couple of parts: byte[] allocation and byte[] copy (from 
the ByteBuf).
It seems sttrange, but it is avoiding the byte[] that is reduced by far 
most of the garbage



---

[GitHub] activemq-artemis pull request #1752: ARTEMIS-1586 Reduce GC pressure due to ...

[franz1981]

Github user franz1981 commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1752#discussion_r160003657
  
--- Diff: 
artemis-commons/src/main/java/org/apache/activemq/artemis/utils/AbstractInterner.java
 ---
@@ -0,0 +1,157 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.activemq.artemis.utils;
+
+import io.netty.buffer.ByteBuf;
+import io.netty.util.internal.MathUtil;
+import io.netty.util.internal.PlatformDependent;
+
+/**
+ * Thread-safe {@code } interner.
+ * 
+ * Differently from {@link String#intern()} it contains a fixed amount of 
entries and
+ * when used by concurrent threads it doesn't ensure the uniqueness of the 
entries ie
+ * the same entry could be allocated multiple times by concurrent calls.
+ */
+public abstract class AbstractInterner {
+
+   private final T[] entries;
--- End diff --

fair enough: I've taken the name from here 
https://github.com/OpenHFT/Java-Lang/blob/master/lang/src/main/java/net/openhft/lang/pool/StringInterner.java
And I've always thought is not such a bad name


---

[GitHub] activemq-artemis pull request #1752: ARTEMIS-1586 Reduce GC pressure due to ...

[michaelandrepearce]

Github user michaelandrepearce commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1752#discussion_r160003612
  
--- Diff: 
artemis-commons/src/main/java/org/apache/activemq/artemis/utils/AbstractInterner.java
 ---
@@ -0,0 +1,157 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.activemq.artemis.utils;
+
+import io.netty.buffer.ByteBuf;
+import io.netty.util.internal.MathUtil;
+import io.netty.util.internal.PlatformDependent;
+
+/**
+ * Thread-safe {@code } interner.
+ * 
+ * Differently from {@link String#intern()} it contains a fixed amount of 
entries and
+ * when used by concurrent threads it doesn't ensure the uniqueness of the 
entries ie
+ * the same entry could be allocated multiple times by concurrent calls.
+ */
+public abstract class AbstractInterner {
--- End diff --

btw for the guava implementation i did i ran this very rudimentary test (I 
am not saying in anyway this is a true perf test, but just to provide something 
indicative), so it run for a while and take final run results from each idea to 
let jit some chance to do what ever it needs, and the array to hold objects is 
to simulate the objects sticking around for a bit, and give some gc pressure.


```
  public static void main(String... args) throws InterruptedException {
  
  byte[] testString = "hello".getBytes();

  
  int size = 1000;


  int inx = 0;
  long start2 = 0;
  long end2 = 0;
  long start = 0;
  long end = 0;
  SimpleString[] array = new SimpleString[size * 10];

  
  
  for (int k = 0; k < 5; k++) {
 array = new SimpleString[size * 10];
 inx = 0;

 System.gc();
 Thread.sleep(1000);

 for (int j = 0; j < 10; j++) {
start = System.nanoTime();
for (int i = 0; i < size; i++) {
   SimpleString simpleString = new SimpleString(testString);
   array[inx] = simpleString;
   inx++;
}
end = System.nanoTime();
 }

 array = new SimpleString[size * 10];
 inx = 0;

 System.gc();
 Thread.sleep(1000);
 for (int j = 0; j < 10; j++) {
start2 = System.nanoTime();
for (int i = 0; i < size; i++) {
   SimpleString simpleString = 
SimpleString.toSimpleString(testString);
   array[inx] = simpleString;
   inx++;
}
end2 = System.nanoTime();
 }
  }

  System.out.println("new  " + (end-start));
  System.out.println("intern   " + (end2-start2));

   }
```

outputs:

new  35102118957
intern   368258702



---

[GitHub] activemq-artemis pull request #1752: ARTEMIS-1586 Reduce GC pressure due to ...

[franz1981]

Github user franz1981 commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1752#discussion_r160003269
  
--- Diff: 
artemis-commons/src/main/java/org/apache/activemq/artemis/utils/collections/TypedProperties.java
 ---
@@ -329,7 +331,9 @@ public boolean containsProperty(final SimpleString key) 
{
   }
}
 
-   public synchronized void decode(final ByteBuf buffer) {
--- End diff --

It is a github fault, but probably dependent by how i've formatted the code 
:P
If you look at the real code you will see that the method is present and 
used :+1: 


---

[GitHub] activemq-artemis pull request #1752: ARTEMIS-1586 Reduce GC pressure due to ...

[michaelandrepearce]

Github user michaelandrepearce commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1752#discussion_r160002905
  
--- Diff: 
artemis-commons/src/main/java/org/apache/activemq/artemis/utils/AbstractInterner.java
 ---
@@ -0,0 +1,157 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.activemq.artemis.utils;
+
+import io.netty.buffer.ByteBuf;
+import io.netty.util.internal.MathUtil;
+import io.netty.util.internal.PlatformDependent;
+
+/**
+ * Thread-safe {@code } interner.
+ * 
+ * Differently from {@link String#intern()} it contains a fixed amount of 
entries and
+ * when used by concurrent threads it doesn't ensure the uniqueness of the 
entries ie
+ * the same entry could be allocated multiple times by concurrent calls.
+ */
+public abstract class AbstractInterner {
+
+   private final T[] entries;
--- End diff --

If so yes you do, and you should avoid the word intern then, as typically 
expectation is similar behaviour to String.intern (which is what guava's does)


---

[GitHub] activemq-artemis pull request #1752: ARTEMIS-1586 Reduce GC pressure due to ...

[michaelandrepearce]

Github user michaelandrepearce commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1752#discussion_r160002569
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/core/protocol/ServerPacketDecoder.java
 ---
@@ -83,16 +85,34 @@
 
 public class ServerPacketDecoder extends ClientPacketDecoder {
 
+   private static final int UUID_LENGTH = 36;
+   private static final int DEFAULT_INTERNER_CAPACITY = 32;
private static final long serialVersionUID = 3348673114388400766L;
-   public static final ServerPacketDecoder INSTANCE = new 
ServerPacketDecoder();
+   private SimpleString.Interner keysInterner;
+   private TypedProperties.StringValue.Interner valuesInterner;
 
-   private static SessionSendMessage decodeSessionSendMessage(final 
ActiveMQBuffer in, CoreRemotingConnection connection) {
+   public ServerPacketDecoder() {
+  this.keysInterner = null;
+  this.valuesInterner = null;
+   }
+
+   private void initializeInternersIfNeeded() {
--- End diff --

you're not making a saving of avoiding iterating though the bytes, to check 
if equal/hash you're still having to process through that bytebuf bringing the 
bytes (if off heap) onto heap.

All creating SimpleString with data does is simple sets the byte[] as data, 
no decoding.


---

[GitHub] activemq-artemis pull request #1752: ARTEMIS-1586 Reduce GC pressure due to ...

[michaelandrepearce]

Github user michaelandrepearce commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1752#discussion_r160001898
  
--- Diff: 
artemis-commons/src/main/java/org/apache/activemq/artemis/utils/collections/TypedProperties.java
 ---
@@ -329,7 +331,9 @@ public boolean containsProperty(final SimpleString key) 
{
   }
}
 
-   public synchronized void decode(final ByteBuf buffer) {
--- End diff --

according to diff its showing red, which means its removed.


---

[GitHub] activemq-artemis pull request #1752: ARTEMIS-1586 Reduce GC pressure due to ...

[franz1981]

Github user franz1981 commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1752#discussion_r159997984
  
--- Diff: 
artemis-commons/src/main/java/org/apache/activemq/artemis/utils/AbstractInterner.java
 ---
@@ -0,0 +1,157 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.activemq.artemis.utils;
+
+import io.netty.buffer.ByteBuf;
+import io.netty.util.internal.MathUtil;
+import io.netty.util.internal.PlatformDependent;
+
+/**
+ * Thread-safe {@code } interner.
+ * 
+ * Differently from {@link String#intern()} it contains a fixed amount of 
entries and
+ * when used by concurrent threads it doesn't ensure the uniqueness of the 
entries ie
+ * the same entry could be allocated multiple times by concurrent calls.
+ */
+public abstract class AbstractInterner {
+
+   private final T[] entries;
--- End diff --

The implementation you've shared is doing a rather different thing: so 
probably I need to explain better what this PR is aiming to do instead..


---

[GitHub] activemq-artemis pull request #1752: ARTEMIS-1586 Reduce GC pressure due to ...

[franz1981]

Github user franz1981 commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1752#discussion_r159997687
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/core/protocol/ServerPacketDecoder.java
 ---
@@ -83,16 +85,34 @@
 
 public class ServerPacketDecoder extends ClientPacketDecoder {
 
+   private static final int UUID_LENGTH = 36;
+   private static final int DEFAULT_INTERNER_CAPACITY = 32;
private static final long serialVersionUID = 3348673114388400766L;
-   public static final ServerPacketDecoder INSTANCE = new 
ServerPacketDecoder();
+   private SimpleString.Interner keysInterner;
+   private TypedProperties.StringValue.Interner valuesInterner;
 
-   private static SessionSendMessage decodeSessionSendMessage(final 
ActiveMQBuffer in, CoreRemotingConnection connection) {
+   public ServerPacketDecoder() {
+  this.keysInterner = null;
+  this.valuesInterner = null;
+   }
+
+   private void initializeInternersIfNeeded() {
--- End diff --

I'm not avoiding only that cost, but the decoding and copy too: that's why 
I'm not using byte[] but directly mapping ByteBuf to intern the keys/values.


---

[GitHub] activemq-artemis pull request #1752: ARTEMIS-1586 Reduce GC pressure due to ...

[franz1981]

Github user franz1981 commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1752#discussion_r159997343
  
--- Diff: 
artemis-commons/src/main/java/org/apache/activemq/artemis/utils/AbstractInterner.java
 ---
@@ -0,0 +1,157 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.activemq.artemis.utils;
+
+import io.netty.buffer.ByteBuf;
+import io.netty.util.internal.MathUtil;
+import io.netty.util.internal.PlatformDependent;
+
+/**
+ * Thread-safe {@code } interner.
+ * 
+ * Differently from {@link String#intern()} it contains a fixed amount of 
entries and
+ * when used by concurrent threads it doesn't ensure the uniqueness of the 
entries ie
+ * the same entry could be allocated multiple times by concurrent calls.
+ */
+public abstract class AbstractInterner {
+
+   private final T[] entries;
--- End diff --

It will have anyway a fixed (and very low) footprint, hence I don't see any 
problem on it


---

[GitHub] activemq-artemis pull request #1752: ARTEMIS-1586 Reduce GC pressure due to ...

[franz1981]

Github user franz1981 commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1752#discussion_r159997146
  
--- Diff: 
artemis-commons/src/main/java/org/apache/activemq/artemis/utils/AbstractInterner.java
 ---
@@ -0,0 +1,157 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.activemq.artemis.utils;
+
+import io.netty.buffer.ByteBuf;
+import io.netty.util.internal.MathUtil;
+import io.netty.util.internal.PlatformDependent;
+
+/**
+ * Thread-safe {@code } interner.
+ * 
+ * Differently from {@link String#intern()} it contains a fixed amount of 
entries and
+ * when used by concurrent threads it doesn't ensure the uniqueness of the 
entries ie
+ * the same entry could be allocated multiple times by concurrent calls.
+ */
+public abstract class AbstractInterner {
--- End diff --

I will provide some results for sure to justify this implementation :)
Anyway the guava one uses concurrent map hence it is very different...


---

[GitHub] activemq pull request #272: replicated LevelDB fix and debugging output

[superawesome]

GitHub user superawesome opened a pull request:

https://github.com/apache/activemq/pull/272

replicated LevelDB fix and debugging output

I know LevelDB is now deprecated, and this may not get merged because of 
that. I certainly don't want to become its maintainer. I had an unhealthy 
cluster, and did not want to try and migrate it while in that state. This is 
just a small change to get it healthy again after a slave encountered this 
error:

Unexpected session error: java.net.ProtocolException: Maximum protocol 
buffer length exeeded

In my case I believe this is due to having too many LevelDB log files to 
replicate (a separate LevelDB bug, which I intend to investigate now that this 
cluster is healthy again).

2 commits in this PR:

1) debugging output to track that down and make it more ... debuggable.

2) a 2-character change to increase a buffer size by 4x, to "fix" the 
problem.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/superawesome/activemq master

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/activemq/pull/272.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 #272


commit 4549f8ebbad4fb9191ef99da48d053864b31916c
Author: Jake Maul 
Date:   2018-01-05T22:27:44Z

adding stack trace output to debug log level

Tracing a connection failure was proving problematic. The session error is 
a warning, but the particular error *I* was having is actually rather fatal, as 
restarting the slave connections does not fix anything- it just fails again 
(and again, and again...).

This will land in the log file iff debug logging is enabled. Otherwise this 
is a no-op.

commit ce29c2f29c3d9f4ae5d930e57e573f69f09521f4
Author: Jake Maul 
Date:   2018-01-05T22:35:10Z

larger buffer for reading replication frames

The default size (1024*64=65536 bytes) was insufficient on a cluster I 
manage. I think because of an unrelated bug where LevelDB log files don't get 
purged. This is a simple fix to get it going again.

Making it 4x as big was *not* scientifically determined. I tried making it 
4x as big first, it worked, and I haven't tried any other size. I don't know 
why it was 64k in the first place, so I can't be sure this doesn't cause any 
side effects.




---

[GitHub] activemq-artemis pull request #1752: ARTEMIS-1586 Reduce GC pressure due to ...

[franz1981]

Github user franz1981 commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1752#discussion_r159996780
  
--- Diff: 
artemis-commons/src/main/java/org/apache/activemq/artemis/utils/collections/TypedProperties.java
 ---
@@ -329,7 +331,9 @@ public boolean containsProperty(final SimpleString key) 
{
   }
}
 
-   public synchronized void decode(final ByteBuf buffer) {
--- End diff --

I'm missing something for sure: I've added a method using the interners, 
but not removed the old one...or not?


---

[GitHub] activemq-artemis issue #1755: ARTEMIS-1586 Reduce GC pressure due to String ...

[michaelandrepearce]

Github user michaelandrepearce commented on the issue:

https://github.com/apache/activemq-artemis/pull/1755
  
@franz1981 i'd like to offer this alternative approach, i think its less 
invasive, deals with reducing SimpleString object creation on heap much more 
widely and is much less code / easier to maintain.


---

[GitHub] activemq-artemis pull request #1755: ARTEMIS-1586 Reduce GC pressure due to ...

[michaelandrepearce]

GitHub user michaelandrepearce opened a pull request:

https://github.com/apache/activemq-artemis/pull/1755

ARTEMIS-1586 Reduce GC pressure due to String allocations on Core protocol

Using Google guava Interners, intern SimpleString creation within stack, so 
to avoid new SimpleString objects which represent the same thing.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/michaelandrepearce/activemq-artemis 
ARTEMIS-1586

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/activemq-artemis/pull/1755.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 #1755


commit e312c4828710d3a173a0d7c67f1dfeb52f90c64f
Author: Michael André Pearce 
Date:   2018-01-05T22:16:18Z

ARTEMIS-1586 Reduce GC pressure due to String allocations on Core protocol

Using Google guava Interners, intern SimpleString creation within stack, so 
to avoid new SimpleString objects which represent the same thing.




---

[GitHub] activemq-artemis pull request #1752: ARTEMIS-1586 Reduce GC pressure due to ...

[michaelandrepearce]

Github user michaelandrepearce commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1752#discussion_r159994860
  
--- Diff: 
artemis-commons/src/main/java/org/apache/activemq/artemis/utils/AbstractInterner.java
 ---
@@ -0,0 +1,157 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.activemq.artemis.utils;
+
+import io.netty.buffer.ByteBuf;
+import io.netty.util.internal.MathUtil;
+import io.netty.util.internal.PlatformDependent;
+
+/**
+ * Thread-safe {@code } interner.
+ * 
+ * Differently from {@link String#intern()} it contains a fixed amount of 
entries and
+ * when used by concurrent threads it doesn't ensure the uniqueness of the 
entries ie
+ * the same entry could be allocated multiple times by concurrent calls.
+ */
+public abstract class AbstractInterner {
--- End diff --

Guava's is fairly good tbh. As noted else where it is heavily used in other 
projects etc so also is much more well battle tested.


---

[GitHub] activemq-artemis pull request #1752: ARTEMIS-1586 Reduce GC pressure due to ...

[michaelandrepearce]

Github user michaelandrepearce commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1752#discussion_r159993639
  
--- Diff: 
artemis-commons/src/main/java/org/apache/activemq/artemis/utils/collections/TypedProperties.java
 ---
@@ -329,7 +331,9 @@ public boolean containsProperty(final SimpleString key) 
{
   }
}
 
-   public synchronized void decode(final ByteBuf buffer) {
--- End diff --

its not, you removed a public method, from a very common class thats 
exposed to clients. 


---

[GitHub] activemq-artemis pull request #1752: ARTEMIS-1586 Reduce GC pressure due to ...

[michaelandrepearce]

Github user michaelandrepearce commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1752#discussion_r159993394
  
--- Diff: 
artemis-commons/src/main/java/org/apache/activemq/artemis/utils/AbstractInterner.java
 ---
@@ -0,0 +1,157 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.activemq.artemis.utils;
+
+import io.netty.buffer.ByteBuf;
+import io.netty.util.internal.MathUtil;
+import io.netty.util.internal.PlatformDependent;
+
+/**
+ * Thread-safe {@code } interner.
+ * 
+ * Differently from {@link String#intern()} it contains a fixed amount of 
entries and
+ * when used by concurrent threads it doesn't ensure the uniqueness of the 
entries ie
+ * the same entry could be allocated multiple times by concurrent calls.
+ */
+public abstract class AbstractInterner {
+
+   private final T[] entries;
--- End diff --

A decoder for a connection (hopefully if clients do the right thing and 
re-use them, should hang around a long time) 


---

[GitHub] activemq-artemis pull request #1752: ARTEMIS-1586 Reduce GC pressure due to ...

[michaelandrepearce]

Github user michaelandrepearce commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1752#discussion_r159993138
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/core/protocol/ServerPacketDecoder.java
 ---
@@ -83,16 +85,34 @@
 
 public class ServerPacketDecoder extends ClientPacketDecoder {
 
+   private static final int UUID_LENGTH = 36;
+   private static final int DEFAULT_INTERNER_CAPACITY = 32;
private static final long serialVersionUID = 3348673114388400766L;
-   public static final ServerPacketDecoder INSTANCE = new 
ServerPacketDecoder();
+   private SimpleString.Interner keysInterner;
+   private TypedProperties.StringValue.Interner valuesInterner;
 
-   private static SessionSendMessage decodeSessionSendMessage(final 
ActiveMQBuffer in, CoreRemotingConnection connection) {
+   public ServerPacketDecoder() {
+  this.keysInterner = null;
+  this.valuesInterner = null;
+   }
+
+   private void initializeInternersIfNeeded() {
--- End diff --

If though you think about it address SimpleString really can be shared 
every where, if the aim is to achieve / avoid creation of SimpleString's on the 
heap where its the same address really, then this can be achieved now in java 8 
easily with a normal interner and creating objects within functions.


---

[GitHub] activemq-artemis pull request #1752: ARTEMIS-1586 Reduce GC pressure due to ...

[franz1981]

Github user franz1981 commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1752#discussion_r159986257
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/core/protocol/ServerPacketDecoder.java
 ---
@@ -83,16 +85,34 @@
 
 public class ServerPacketDecoder extends ClientPacketDecoder {
 
+   private static final int UUID_LENGTH = 36;
+   private static final int DEFAULT_INTERNER_CAPACITY = 32;
private static final long serialVersionUID = 3348673114388400766L;
-   public static final ServerPacketDecoder INSTANCE = new 
ServerPacketDecoder();
+   private SimpleString.Interner keysInterner;
+   private TypedProperties.StringValue.Interner valuesInterner;
 
-   private static SessionSendMessage decodeSessionSendMessage(final 
ActiveMQBuffer in, CoreRemotingConnection connection) {
+   public ServerPacketDecoder() {
+  this.keysInterner = null;
+  this.valuesInterner = null;
+   }
+
+   private void initializeInternersIfNeeded() {
--- End diff --

It has a different usage than a JVM interner.
It Is more a "per-connection last recently use fixed size pool": I've 
chosen to call it interner just for brevity eheh


---

[GitHub] activemq-artemis pull request #1752: ARTEMIS-1586 Reduce GC pressure due to ...

[franz1981]

Github user franz1981 commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1752#discussion_r159985727
  
--- Diff: 
artemis-commons/src/main/java/org/apache/activemq/artemis/utils/AbstractInterner.java
 ---
@@ -0,0 +1,157 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.activemq.artemis.utils;
+
+import io.netty.buffer.ByteBuf;
+import io.netty.util.internal.MathUtil;
+import io.netty.util.internal.PlatformDependent;
+
+/**
+ * Thread-safe {@code } interner.
+ * 
+ * Differently from {@link String#intern()} it contains a fixed amount of 
entries and
+ * when used by concurrent threads it doesn't ensure the uniqueness of the 
entries ie
+ * the same entry could be allocated multiple times by concurrent calls.
+ */
+public abstract class AbstractInterner {
--- End diff --

Because this is pretty different implementation-wise: it acts similarly to 
a bloom filter and it has fixed memory foot-print.
And obviously the major different is in the performances :)


---

[GitHub] activemq-artemis pull request #1752: ARTEMIS-1586 Reduce GC pressure due to ...

[franz1981]

Github user franz1981 commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1752#discussion_r159985473
  
--- Diff: 
artemis-commons/src/main/java/org/apache/activemq/artemis/utils/collections/TypedProperties.java
 ---
@@ -329,7 +331,9 @@ public boolean containsProperty(final SimpleString key) 
{
   }
}
 
-   public synchronized void decode(final ByteBuf buffer) {
--- End diff --

It is back-compatible 


---

[GitHub] activemq-artemis pull request #1752: ARTEMIS-1586 Reduce GC pressure due to ...

[franz1981]

Github user franz1981 commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1752#discussion_r159985327
  
--- Diff: 
artemis-commons/src/main/java/org/apache/activemq/artemis/utils/AbstractInterner.java
 ---
@@ -0,0 +1,157 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.activemq.artemis.utils;
+
+import io.netty.buffer.ByteBuf;
+import io.netty.util.internal.MathUtil;
+import io.netty.util.internal.PlatformDependent;
+
+/**
+ * Thread-safe {@code } interner.
+ * 
+ * Differently from {@link String#intern()} it contains a fixed amount of 
entries and
+ * when used by concurrent threads it doesn't ensure the uniqueness of the 
entries ie
+ * the same entry could be allocated multiple times by concurrent calls.
+ */
+public abstract class AbstractInterner {
+
+   private final T[] entries;
--- End diff --

When the decoder will be deallocated the (few) instances will be removed or 
am I missing something?


---

[GitHub] activemq-artemis pull request #1752: ARTEMIS-1586 Reduce GC pressure due to ...

[michaelandrepearce]

Github user michaelandrepearce commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1752#discussion_r159984911
  
--- Diff: 
artemis-commons/src/main/java/org/apache/activemq/artemis/utils/collections/TypedProperties.java
 ---
@@ -93,6 +94,7 @@ public void putBooleanProperty(final SimpleString key, 
final boolean value) {
}
 
public void putByteProperty(final SimpleString key, final byte value) {
+  checkCreateProperties();
--- End diff --

am i missing something this seems like a duplicate method call.


---

[GitHub] activemq-artemis pull request #1753: ARTEMIS-1573 Improve UTF translation al...

[michaelandrepearce]

Github user michaelandrepearce commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1753#discussion_r159981865
  
--- Diff: 
artemis-commons/src/main/java/org/apache/activemq/artemis/utils/UTF8Util.java 
---
@@ -34,7 +35,7 @@
 
private static final boolean isTrace = 
ActiveMQUtilLogger.LOGGER.isTraceEnabled();
 
-   private static final ThreadLocal 
currenBuffer = new ThreadLocal<>();
+   private static final FastThreadLocal 
currentBuffer = new FastThreadLocal<>();
--- End diff --

This only benefits if all threads are FastThreadLocalThread's. Do we re-use 
Netty's thread pools? I thought we had our own


---

[GitHub] activemq-artemis pull request #1752: ARTEMIS-1586 Reduce GC pressure due to ...

[michaelandrepearce]

Github user michaelandrepearce commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1752#discussion_r159980786
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/core/protocol/ServerPacketDecoder.java
 ---
@@ -83,16 +85,34 @@
 
 public class ServerPacketDecoder extends ClientPacketDecoder {
 
+   private static final int UUID_LENGTH = 36;
+   private static final int DEFAULT_INTERNER_CAPACITY = 32;
private static final long serialVersionUID = 3348673114388400766L;
-   public static final ServerPacketDecoder INSTANCE = new 
ServerPacketDecoder();
+   private SimpleString.Interner keysInterner;
+   private TypedProperties.StringValue.Interner valuesInterner;
 
-   private static SessionSendMessage decodeSessionSendMessage(final 
ActiveMQBuffer in, CoreRemotingConnection connection) {
+   public ServerPacketDecoder() {
+  this.keysInterner = null;
+  this.valuesInterner = null;
+   }
+
+   private void initializeInternersIfNeeded() {
--- End diff --

one interner per session :O ouch - an interner ideally should be shared 
within JVM so benefits can be reaped anywhere.


---

[GitHub] activemq-artemis pull request #1752: ARTEMIS-1586 Reduce GC pressure due to ...

[michaelandrepearce]

Github user michaelandrepearce commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1752#discussion_r159978720
  
--- Diff: 
artemis-commons/src/main/java/org/apache/activemq/artemis/utils/AbstractInterner.java
 ---
@@ -0,0 +1,157 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.activemq.artemis.utils;
+
+import io.netty.buffer.ByteBuf;
+import io.netty.util.internal.MathUtil;
+import io.netty.util.internal.PlatformDependent;
+
+/**
+ * Thread-safe {@code } interner.
+ * 
+ * Differently from {@link String#intern()} it contains a fixed amount of 
entries and
+ * when used by concurrent threads it doesn't ensure the uniqueness of the 
entries ie
+ * the same entry could be allocated multiple times by concurrent calls.
+ */
+public abstract class AbstractInterner {
+
+   private final T[] entries;
--- End diff --

this doesn't allow GC to occur ever on any object held by this class, if 
the object is de-referenced. 


---

[GitHub] activemq-artemis issue #1752: ARTEMIS-1586 Reduce GC pressure due to String ...

[michaelandrepearce]

Github user michaelandrepearce commented on the issue:

https://github.com/apache/activemq-artemis/pull/1752
  
Why re-invent the wheel here, guava interners is well battle tested, and 
exists already.


---

[GitHub] activemq-artemis pull request #1752: ARTEMIS-1586 Reduce GC pressure due to ...

[michaelandrepearce]

Github user michaelandrepearce commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1752#discussion_r159977324
  
--- Diff: 
artemis-commons/src/main/java/org/apache/activemq/artemis/utils/AbstractInterner.java
 ---
@@ -0,0 +1,157 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.activemq.artemis.utils;
+
+import io.netty.buffer.ByteBuf;
+import io.netty.util.internal.MathUtil;
+import io.netty.util.internal.PlatformDependent;
+
+/**
+ * Thread-safe {@code } interner.
+ * 
+ * Differently from {@link String#intern()} it contains a fixed amount of 
entries and
+ * when used by concurrent threads it doesn't ensure the uniqueness of the 
entries ie
+ * the same entry could be allocated multiple times by concurrent calls.
+ */
+public abstract class AbstractInterner {
--- End diff --

Why not simply use Google's guava interners?


---

[GitHub] activemq-artemis pull request #1752: ARTEMIS-1586 Reduce GC pressure due to ...

[michaelandrepearce]

Github user michaelandrepearce commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1752#discussion_r159977160
  
--- Diff: 
artemis-commons/src/main/java/org/apache/activemq/artemis/utils/collections/TypedProperties.java
 ---
@@ -329,7 +331,9 @@ public boolean containsProperty(final SimpleString key) 
{
   }
}
 
-   public synchronized void decode(final ByteBuf buffer) {
--- End diff --

should keep method back compatible. 


---

[GitHub] activemq-artemis pull request #1752: ARTEMIS-1586 Reduce GC pressure due to ...

[michaelandrepearce]

Github user michaelandrepearce commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1752#discussion_r159976892
  
--- Diff: 
artemis-core-client/src/main/java/org/apache/activemq/artemis/core/message/impl/CoreMessage.java
 ---
@@ -528,8 +543,17 @@ public void decodeHeadersAndProperties(final ByteBuf 
buffer) {
private void decodeHeadersAndProperties(final ByteBuf buffer, boolean 
lazyProperties) {
   messageIDPosition = buffer.readerIndex();
   messageID = buffer.readLong();
-
-  address = SimpleString.readNullableSimpleString(buffer);
+  int b = buffer.readByte();
--- End diff --

Shouldnt this logic remain in SimpleString, as you could read strings 
(especially address in many places)


---

[GitHub] activemq-artemis issue #1752: ARTEMIS-1586 Reduce GC pressure due to String ...

[michaelandrepearce]

Github user michaelandrepearce commented on the issue:

https://github.com/apache/activemq-artemis/pull/1752
  
Bit uncertain on this, as you aren't holding weak references, as such never 
allows GC to clean objects that no longer exist.


---

[GitHub] activemq-artemis pull request #1752: ARTEMIS-1586 Reduce GC pressure due to ...

[michaelandrepearce]

Github user michaelandrepearce commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1752#discussion_r159974168
  
--- Diff: 
artemis-core-client/src/main/java/org/apache/activemq/artemis/core/protocol/core/impl/ActiveMQClientProtocolManager.java
 ---
@@ -510,7 +510,7 @@ private void notifyTopologyChange(final 
ClusterTopologyChangeMessage topMessage)
   }
}
 
-   protected PacketDecoder getPacketDecoder() {
+   protected PacketDecoder createPacketDecoder() {
--- End diff --

There's no need for this change.


---

[GitHub] activemq-artemis issue #1753: ARTEMIS-1573 Improve UTF translation allowing ...

[franz1981]

Github user franz1981 commented on the issue:

https://github.com/apache/activemq-artemis/pull/1753
  
Please do not merge yet: I've already run the CI and compatibility tests 
(kudos to @clebertsuconic work on it!), but I will need to provide some results 
on it :+1: 



---

[GitHub] activemq-artemis pull request #1753: ARTEMIS-1573 Improve UTF translation al...

[franz1981]

GitHub user franz1981 opened a pull request:

https://github.com/apache/activemq-artemis/pull/1753

ARTEMIS-1573 Improve UTF translation allowing zero copy

The UTF translations has been improved by:
- zero copy on array based buffers
- zero copy UTF length calculation

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/franz1981/activemq-artemis utf8_validation

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/activemq-artemis/pull/1753.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 #1753


commit e6bfe5132ffcb17ad7bd137b109cbb321c9fb263
Author: Francesco Nigro 
Date:   2017-12-21T13:02:34Z

ARTEMIS-1573 Improve UTF translation allowing zero copy

The UTF translations has been improved by:
- zero copy on array based buffers
- zero copy UTF length calculation




---

[GitHub] activemq-artemis issue #1752: ARTEMIS-1586 Reduce GC pressure due to String ...

[franz1981]

Github user franz1981 commented on the issue:

https://github.com/apache/activemq-artemis/pull/1752
  
Please do not merge it: I need to write down some results first and run all 
the CI tests :+1: 


---

[GitHub] activemq-artemis pull request #1752: ARTEMIS-1586 Reduce GC pressure due to ...

[franz1981]

GitHub user franz1981 opened a pull request:

https://github.com/apache/activemq-artemis/pull/1752

ARTEMIS-1586 Reduce GC pressure due to String allocations on Core protocol

The commit contains:
- a general purpose interner implementation
- StringValue/SimpleString internrs specializations
- TypedProperties keys/values string interning for SessionSendMessage 
decoding

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/franz1981/activemq-artemis decoder_interners

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/activemq-artemis/pull/1752.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 #1752


commit 2dd894c74f6dfe1e9d0579ced225a44661efcb99
Author: Francesco Nigro 
Date:   2018-01-04T14:22:05Z

ARTEMIS-1586 Reduce GC pressure due to String allocations on Core protocol

The commit contains:
- a general purpose interner implementation
- StringValue/SimpleString internrs specializations
- TypedProperties keys/values string interning for SessionSendMessage 
decoding




---

[GitHub] activemq-artemis pull request #1751: ARTEMIS-1585 - Log connector details on...

[michaelandrepearce]

GitHub user michaelandrepearce opened a pull request:

https://github.com/apache/activemq-artemis/pull/1751

ARTEMIS-1585 - Log connector details on start

Log connector details when they start. 
This is similar to that of acceptors, and helps when any issues later.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/michaelandrepearce/activemq-artemis 
ARTEMIS-1585

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/activemq-artemis/pull/1751.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 #1751


commit be5a097d9df65642b8d597ebe4e4b3a2fddd7a12
Author: Michael André Pearce 
Date:   2018-01-05T17:53:34Z

ARTEMIS-1585 - Log connector details on start

Log connector details when they start. 
This is similar to that of acceptors, and helps when any issues later.




---

[GitHub] activemq-artemis pull request #1750: ARTEMIS-1581 fix handshake-timeout prop...

[jbertram]

Github user jbertram commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1750#discussion_r159896349
  
--- Diff: 
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/jms/server/config/JMSConfigurationTest.java
 ---
@@ -97,4 +111,56 @@ public void testSetupJMSConfiguration() throws 
Exception {
 
   server.stop();
}
+
+   @Test
+   public void testHandshakeTimeoutWithValueSet() throws Exception {
+  JMSServerManager jmsServer;
--- End diff --

Or don't even use a configuration file and just programmatically configure 
the acceptor.


---

[GitHub] activemq-artemis pull request #1750: ARTEMIS-1581 fix handshake-timeout prop...

[jbertram]

Github user jbertram commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1750#discussion_r159895861
  
--- Diff: 
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/jms/server/config/JMSConfigurationTest.java
 ---
@@ -97,4 +111,56 @@ public void testSetupJMSConfiguration() throws 
Exception {
 
   server.stop();
}
+
+   @Test
+   public void testHandshakeTimeoutWithValueSet() throws Exception {
+  JMSServerManager jmsServer;
--- End diff --

The JMSServerManager is deprecated so it's probably best not to use it in 
new tests.  Check out the "embedded-simple" example for how to programmatically 
instantiate a broker with a configuration file.


---

[GitHub] activemq-artemis issue #1750: ARTEMIS-1581 fix handshake-timeout property co...

[stanlyDoge]

Github user stanlyDoge commented on the issue:

https://github.com/apache/activemq-artemis/pull/1750
  
@michaelandrepearce Done
@jbertram ahh, that explains my another conflict. Thanks.


---

[GitHub] activemq-artemis issue #1733: ARTEMIS-1545 Support JMS 2.0 Completion Listen...

[michaelandrepearce]

Github user michaelandrepearce commented on the issue:

https://github.com/apache/activemq-artemis/pull/1733
  
Still being worked on, don't merge.


---

[GitHub] activemq-artemis issue #1750: ARTEMIS-1581 fix handshake-timeout property co...

[michaelandrepearce]

Github user michaelandrepearce commented on the issue:

https://github.com/apache/activemq-artemis/pull/1750
  
@stanlyDoge could you squash the commits. Cheers.


---

[GitHub] activemq-artemis pull request #1741: NO-JIRA Diverts: updated doc & tests wi...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/activemq-artemis/pull/1741


---

[GitHub] activemq-artemis pull request #1748: ARTEMIS-1576 anon AMQP producer creates...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/activemq-artemis/pull/1748


---