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

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

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

mtaylor
GitHub user michaelandrepearce opened a pull request:

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

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

    @franz1981 i think this is cleaner merge of the two.

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

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

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

    https://github.com/apache/activemq-artemis/pull/1757.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 #1757
   
----
commit 4ab1fea88610e46381141223479aec97e7fa04ac
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

commit 794e56da99456e891284709b790bf8144d65b0f9
Author: Michael André Pearce <michael.andre.pearce@...>
Date:   2018-01-08T18:45:18Z

    ARTEMIS-1586 Refactor to make more generic
   
    * Move byte util code into ByteUtil
    * Re-use the new equals method in SimpleString
    * Apply same pools/interners to client decode
    * Create String to SimpleString pools/interners for property access via String keys (producer and consumer benefits)
    * Lazy init the pools on withing the get methods of CoreMessageObjectPools to get the specific pool, to avoid having this scattered every where.

----


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

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

mtaylor
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1757
 
    I can close the other one, but I'm just providing a couple of errors I've found too..I can do it here


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

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

mtaylor
In reply to this post by mtaylor
Github user franz1981 commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1757#discussion_r160247350
 
    --- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/api/core/SimpleString.java ---
    @@ -259,22 +281,23 @@ public boolean equals(final Object other) {
           if (other instanceof SimpleString) {
              SimpleString s = (SimpleString) other;
     
    -         if (data.length != s.data.length) {
    -            return false;
    -         }
    -
    -         for (int i = 0; i < data.length; i++) {
    -            if (data[i] != s.data[i]) {
    -               return false;
    -            }
    -         }
    -
    -         return true;
    +         return ByteUtil.equals(data, s.data);
    --- End diff --
   
    I've implemented it in the last commit as:
    ```
       @Override
       public boolean equals(final Object other) {
          if (this == other) {
             return true;
          }
   
          if (other instanceof SimpleString) {
             SimpleString s = (SimpleString) other;
             if (s.hash != 0 && this.hash != 0) {
                if (s.hash != this.hash)
                   return false;
             }
             if (s.str != null && this.str != null) {
                return this.str.equals(s.str);
             } else {
                return ByteUtil.equals(this.data, s.data);
             }
          } else {
             return false;
          }
       }
    ```


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

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

mtaylor
In reply to this post by mtaylor
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1757
 
    What I've noticed doing some fast tests is that there is just one (the same) dead living (ie detroyed but not garbage collected) `RemoteCollectionImpl` that reference the pools  after several tests: I need to undestand if we have to get rid of it in order to avoid memory leaks.
   
    Re the hashCode with ByteBuf that I've implemented, seems with UUID that is not working propertly due to integer overflow: I will review/fix it soon


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

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

mtaylor
In reply to this post by mtaylor
Github user michaelandrepearce commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1757#discussion_r160255842
 
    --- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/api/core/SimpleString.java ---
    @@ -259,22 +281,23 @@ public boolean equals(final Object other) {
           if (other instanceof SimpleString) {
              SimpleString s = (SimpleString) other;
     
    -         if (data.length != s.data.length) {
    -            return false;
    -         }
    -
    -         for (int i = 0; i < data.length; i++) {
    -            if (data[i] != s.data[i]) {
    -               return false;
    -            }
    -         }
    -
    -         return true;
    +         return ByteUtil.equals(data, s.data);
    --- End diff --
   
    I don't see the benefit of this change, for GC.
   
    Why i moved the equals code between two byte arrays into ByteUtil and re-used the equals, was to keep the logic that is checked equal between the bytebuf equals and the equals of the String the same called code. Adding these extra checks, could allow for differences in result now, and for very marginal improvement at best IMO.


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

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

mtaylor
In reply to this post by mtaylor
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1757
 
    @franz1981 which class? i don't find RemoteCollectionImpl


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

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

mtaylor
In reply to this post by mtaylor
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1757
 
    Uh typo error: I mean RemoteConnectionImpl :)


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

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

mtaylor
In reply to this post by mtaylor
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1757
 
    The RemotingConnectionImpl will hold reference to the packet decoder, until itself is GC'd. It will be long lived for bits like cluster connections etc on the server side, also it is held in maps like within RemotingServiceImpl.



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

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

mtaylor
In reply to this post by mtaylor
Github user franz1981 commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1757#discussion_r160344336
 
    --- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/api/core/SimpleString.java ---
    @@ -259,22 +281,23 @@ public boolean equals(final Object other) {
           if (other instanceof SimpleString) {
              SimpleString s = (SimpleString) other;
     
    -         if (data.length != s.data.length) {
    -            return false;
    -         }
    -
    -         for (int i = 0; i < data.length; i++) {
    -            if (data[i] != s.data[i]) {
    -               return false;
    -            }
    -         }
    -
    -         return true;
    +         return ByteUtil.equals(data, s.data);
    --- End diff --
   
    The benefit is not from the GC pov but for normal `SimpleString` usage inside Map/Set: if the `SimpleString` has already hash computed or it is string-based it won't perform array comparison (optimzed ones for sure) but will benenfit of `String::equals` that is intrinsified by the JVM.


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

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

mtaylor
In reply to this post by mtaylor
Github user franz1981 commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1757#discussion_r160344540
 
    --- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/api/core/SimpleString.java ---
    @@ -259,22 +281,23 @@ public boolean equals(final Object other) {
           if (other instanceof SimpleString) {
              SimpleString s = (SimpleString) other;
     
    -         if (data.length != s.data.length) {
    -            return false;
    -         }
    -
    -         for (int i = 0; i < data.length; i++) {
    -            if (data[i] != s.data[i]) {
    -               return false;
    -            }
    -         }
    -
    -         return true;
    +         return ByteUtil.equals(data, s.data);
    --- End diff --
   
    It is just a further improvement, but I'm already happy with the code as it is now :+1:


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

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

mtaylor
In reply to this post by mtaylor
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1757
 
    I will take a look on it now, but at a first look semms that there is a long living `RemoteConnectionImpl` that has a `destroyed` flag `true` which is not collected after a forced full GC


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

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

mtaylor
In reply to this post by mtaylor
Github user franz1981 commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1757#discussion_r160347432
 
    --- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/api/core/SimpleString.java ---
    @@ -259,22 +281,23 @@ public boolean equals(final Object other) {
           if (other instanceof SimpleString) {
              SimpleString s = (SimpleString) other;
     
    -         if (data.length != s.data.length) {
    -            return false;
    -         }
    -
    -         for (int i = 0; i < data.length; i++) {
    -            if (data[i] != s.data[i]) {
    -               return false;
    -            }
    -         }
    -
    -         return true;
    +         return ByteUtil.equals(data, s.data);
    --- End diff --
   
    And there is a correctness issue around it (in my code too!!!):
    if `s` is `null` `s.data` will throw NPE!


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

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

mtaylor
In reply to this post by mtaylor
Github user michaelandrepearce commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1757#discussion_r160352626
 
    --- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/api/core/SimpleString.java ---
    @@ -259,22 +281,23 @@ public boolean equals(final Object other) {
           if (other instanceof SimpleString) {
              SimpleString s = (SimpleString) other;
     
    -         if (data.length != s.data.length) {
    -            return false;
    -         }
    -
    -         for (int i = 0; i < data.length; i++) {
    -            if (data[i] != s.data[i]) {
    -               return false;
    -            }
    -         }
    -
    -         return true;
    +         return ByteUtil.equals(data, s.data);
    --- End diff --
   
    Cool. lets leave it then :)


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

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

mtaylor
In reply to this post by mtaylor
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1757
 
    That's what I was talking about:
    ![image](https://user-images.githubusercontent.com/13125299/34713778-8687e452-f527-11e7-8bfb-a7db8157cfb4.png)
    This `RemotingConnectionImpl` it has survived after several full GC and it is been already destroyed: it seems a leak to me, wdyt?


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

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

mtaylor
In reply to this post by mtaylor
Github user franz1981 commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1757#discussion_r160355213
 
    --- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/api/core/SimpleString.java ---
    @@ -259,22 +281,23 @@ public boolean equals(final Object other) {
           if (other instanceof SimpleString) {
              SimpleString s = (SimpleString) other;
     
    -         if (data.length != s.data.length) {
    -            return false;
    -         }
    -
    -         for (int i = 0; i < data.length; i++) {
    -            if (data[i] != s.data[i]) {
    -               return false;
    -            }
    -         }
    -
    -         return true;
    +         return ByteUtil.equals(data, s.data);
    --- End diff --
   
    I will perform some benchs to see if it will impact positively on checks and/or pattern matching based on equals: only in that case I will send a separate improvement PR :+1:


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

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

mtaylor
In reply to this post by mtaylor
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1757
 
    @michaelandrepearce
    It is near to be fixed, but I've opened a separate JIRA for it:
    ![image](https://user-images.githubusercontent.com/13125299/34717813-9b9fc4ec-f534-11e7-95c2-e7efc40db9aa.png)
    Re memory leaks of `RemotingConnectionImpl` into `JaasCallbackHandler` probably @gtully or @jbertram would be super fast to indentify why are not collected propertly...


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

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

mtaylor
In reply to this post by mtaylor
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1757
 
    @franz1981 could you fully expand the fields in references pane until root? I suspect the login context is there holding the jaas callback hadler


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

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

mtaylor
In reply to this post by mtaylor
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1757
 
    @michaelandrepearce Exactly...
    ![image](https://user-images.githubusercontent.com/13125299/34721713-241f7984-f544-11e7-874e-f79a6750db84.png)
   



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

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

mtaylor
In reply to this post by mtaylor
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1757
 
    @michaelandrepearce Probably I've fixed it: it was subtle...



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

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

mtaylor
In reply to this post by mtaylor
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1757
 
    @franz1981 once the build of that PR for fixing the leaking RemotingConnectionImpl is green, ill merge it, and then ill rebase this branch again.


---
123