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

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

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

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

----


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

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

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



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

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

michaelandrepearce-2
In reply to this post by michaelandrepearce-2
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<SoftReference<StringUtilBuffer>> currenBuffer = new ThreadLocal<>();
    +   private static final FastThreadLocal<SoftReference<StringUtilBuffer>> 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....


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

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

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

    https://github.com/apache/activemq-artemis/pull/1753#discussion_r160160624
 
    --- 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<SoftReference<StringUtilBuffer>> currenBuffer = new ThreadLocal<>();
    +   private static final FastThreadLocal<SoftReference<StringUtilBuffer>> currentBuffer = new FastThreadLocal<>();
    --- End diff --
   
    @michaelandrepearce +1


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

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

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

    https://github.com/apache/activemq-artemis/pull/1753#discussion_r160162050
 
    --- 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<SoftReference<StringUtilBuffer>> currenBuffer = new ThreadLocal<>();
    +   private static final FastThreadLocal<SoftReference<StringUtilBuffer>> currentBuffer = new FastThreadLocal<>();
    --- End diff --
   
    @michaelandrepearce Good catch! I will change it in no time, thanks!


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

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

michaelandrepearce-2
In reply to this post by michaelandrepearce-2
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1753
 
    @clebertsuconic @michaelandrepearce
    I've improved the already existing perf tests and introduced an optimization for the common path using Netty `PlatformDependent` to avoid bounds checking: it speed up encoding/decoding by a great margin (` 40% on my box).
    The only thing I'm concerned is:
    - do not add too much complexity
    - hit the hot common case ( ie UTF encoding/decoding is actually used and most of the `ByteBuf` instances are heap based ones)


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

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

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

    https://github.com/apache/activemq-artemis/pull/1753
 
    @franz1981 LGTM.
   
    The only comment i would have, is it worth adding the code to handle the offheap now also, just while its fresh in your mind, and then it be more complete feature. And if you disagree then thats also fine, id be still happy for this to merge.


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

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

michaelandrepearce-2
In reply to this post by michaelandrepearce-2
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1753
 
    @michaelandrepearce
    > is it worth adding the code to handle the offheap now also, just while its fresh in your mind, and then it be more complete feature
   
    Agree :) I've avoided to implement them (for the `writeAsShort` case too) to not turn this PR in a total rewrite of the original code, but I already know that the gains are *huge*: I will take some time to close the circle and make it features complete by covering the remaining cases :+1:


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

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

michaelandrepearce-2
In reply to this post by michaelandrepearce-2
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1753
 
    I've changed the last few bits and these are some results:
    ```
    THIS PR:
    DIRECT:
    Throughput writeUTF = 2352 ops/ms
    Throughput readUTF = 2123 ops/ms
    HEAP:
    Throughput writeUTF = 2531 ops/ms
    Throughput readUTF = 2257 ops/ms
   
    ORIGINAL
    DIRECT;
    Throughput writeUTF = 1824 ops/ms
    Throughput readUTF = 1494 ops/ms
    HEAP:
    Throughput writeUTF = 1828 ops/ms
    Throughput readUTF = 1727 ops/ms
    ```


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

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

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

    https://github.com/apache/activemq-artemis/pull/1753
 
    @franz1981 looks good, ill look to merge this afternoon, just to confirm no more bits right?


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

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

michaelandrepearce-2
In reply to this post by michaelandrepearce-2
Github user asfgit closed the pull request at:

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


---