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

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
73 messages Options
1234
Reply | Threaded
Open this post in threaded view
|

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

jgoodyear-2
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 <nigro.fra@...>
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

----


---
Reply | Threaded
Open this post in threaded view
|

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

jgoodyear-2
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:


---
Reply | Threaded
Open this post in threaded view
|

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

jgoodyear-2
In reply to this post by jgoodyear-2
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.


---
Reply | Threaded
Open this post in threaded view
|

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

jgoodyear-2
In reply to this post by jgoodyear-2
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.


---
Reply | Threaded
Open this post in threaded view
|

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

jgoodyear-2
In reply to this post by jgoodyear-2
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)


---
Reply | Threaded
Open this post in threaded view
|

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

jgoodyear-2
In reply to this post by jgoodyear-2
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.


---
Reply | Threaded
Open this post in threaded view
|

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

jgoodyear-2
In reply to this post by jgoodyear-2
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 <T>} interner.
    + * <p>
    + * 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<T> {
    --- End diff --
   
    Why not simply use Google's guava interners?


---
Reply | Threaded
Open this post in threaded view
|

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

jgoodyear-2
In reply to this post by jgoodyear-2
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.


---
Reply | Threaded
Open this post in threaded view
|

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

jgoodyear-2
In reply to this post by jgoodyear-2
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 <T>} interner.
    + * <p>
    + * 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<T> {
    +
    +   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.


---
Reply | Threaded
Open this post in threaded view
|

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

jgoodyear-2
In reply to this post by jgoodyear-2
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.


---
Reply | Threaded
Open this post in threaded view
|

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

jgoodyear-2
In reply to this post by jgoodyear-2
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.


---
Reply | Threaded
Open this post in threaded view
|

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

jgoodyear-2
In reply to this post by jgoodyear-2
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 <T>} interner.
    + * <p>
    + * 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<T> {
    +
    +   private final T[] entries;
    --- End diff --
   
    When the decoder will be deallocated the (few) instances will be removed or am I missing something?


---
Reply | Threaded
Open this post in threaded view
|

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

jgoodyear-2
In reply to this post by jgoodyear-2
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


---
Reply | Threaded
Open this post in threaded view
|

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

jgoodyear-2
In reply to this post by jgoodyear-2
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 <T>} interner.
    + * <p>
    + * 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<T> {
    --- 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 :)


---
Reply | Threaded
Open this post in threaded view
|

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

jgoodyear-2
In reply to this post by jgoodyear-2
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


---
Reply | Threaded
Open this post in threaded view
|

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

jgoodyear-2
In reply to this post by jgoodyear-2
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.


---
Reply | Threaded
Open this post in threaded view
|

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

jgoodyear-2
In reply to this post by jgoodyear-2
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 <T>} interner.
    + * <p>
    + * 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<T> {
    +
    +   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)


---
Reply | Threaded
Open this post in threaded view
|

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

jgoodyear-2
In reply to this post by jgoodyear-2
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.


---
Reply | Threaded
Open this post in threaded view
|

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

jgoodyear-2
In reply to this post by jgoodyear-2
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 <T>} interner.
    + * <p>
    + * 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<T> {
    --- 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.


---
Reply | Threaded
Open this post in threaded view
|

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

jgoodyear-2
In reply to this post by jgoodyear-2
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?


---
1234