You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@activemq.apache.org by franz1981 <gi...@git.apache.org> on 2018/01/05 18:26:19 UTC
[GitHub] activemq-artemis pull request #1753: ARTEMIS-1573 Improve UTF translation al...
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 <ni...@...>
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 #1753: ARTEMIS-1573 Improve UTF translation allowing ...
Posted by franz1981 <gi...@git.apache.org>.
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:
---
[GitHub] activemq-artemis issue #1753: ARTEMIS-1573 Improve UTF translation allowing ...
Posted by michaelandrepearce <gi...@git.apache.org>.
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.
---
[GitHub] activemq-artemis issue #1753: ARTEMIS-1573 Improve UTF translation allowing ...
Posted by franz1981 <gi...@git.apache.org>.
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)
---
[GitHub] activemq-artemis issue #1753: ARTEMIS-1573 Improve UTF translation allowing ...
Posted by franz1981 <gi...@git.apache.org>.
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
```
---
[GitHub] activemq-artemis pull request #1753: ARTEMIS-1573 Improve UTF translation al...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/activemq-artemis/pull/1753
---
[GitHub] activemq-artemis issue #1753: ARTEMIS-1573 Improve UTF translation allowing ...
Posted by franz1981 <gi...@git.apache.org>.
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...
Posted by clebertsuconic <gi...@git.apache.org>.
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
---
[GitHub] activemq-artemis pull request #1753: ARTEMIS-1573 Improve UTF translation al...
Posted by franz1981 <gi...@git.apache.org>.
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!
---
[GitHub] activemq-artemis issue #1753: ARTEMIS-1573 Improve UTF translation allowing ...
Posted by michaelandrepearce <gi...@git.apache.org>.
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?
---
[GitHub] activemq-artemis pull request #1753: ARTEMIS-1573 Improve UTF translation al...
Posted by michaelandrepearce <gi...@git.apache.org>.
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....
---