Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ARTEMIS-1573 Improve UTF translation allowing zero copy #1753

Closed
wants to merge 1 commit into from
Closed

ARTEMIS-1573 Improve UTF translation allowing zero copy #1753

wants to merge 1 commit into from

Conversation

franz1981
Copy link
Contributor

@franz1981 franz1981 commented Jan 5, 2018 •
edited

The UTF translations has been improved by:

  • zero copy on array based buffers
  • zero copy UTF length calculation
  • faster array access using Netty PlatformDependent.get|putByte
  • improved perf tests UTF8Test

@franz1981
Copy link
Contributor Author

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 👍

michaelandrepearce
michaelandrepearce reviewed Jan 5, 2018
View reviewed changes
artemis-commons/src/main/java/org/apache/activemq/artemis/utils/UTF8Util.java Outdated
@@ -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<>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michaelandrepearce +1

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michaelandrepearce Good catch! I will change it in no time, thanks!

@franz1981
Copy link
Contributor Author

@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)

@michaelandrepearce
Copy link
Contributor

@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.

@franz1981
Copy link
Contributor Author

@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 👍

@franz1981
ARTEMIS-1573 Improve UTF translation allowing zero copy
dc44b69 
The UTF translations has been improved by:
- zero copy on array based buffers
- zero copy UTF length calculation
- faster array access using Netty PlatformDependent.get|putByte
- improved perf tests UTF8Test
@franz1981
Copy link
Contributor Author

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

@michaelandrepearce
Copy link
Contributor

@franz1981 looks good, ill look to merge this afternoon, just to confirm no more bits right?

@asfgit asfgit closed this in 785deb7 Jan 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clebertsuconic clebertsuconic clebertsuconic left review comments

@michaelandrepearce michaelandrepearce michaelandrepearce left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@franz1981 @michaelandrepearce @clebertsuconic