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.

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

Refactor SslHandler internals to always use heap buffers for JDK SSLE… #9696

Merged
merged 3 commits into from Oct 23, 2019

Conversation

@normanmaurer
Copy link
Member

normanmaurer commented Oct 21, 2019

…ngine implementation

Motivation:

We should aim to always use heap buffers when using the JDK SSLEngine for now as it wants to operate on byte[] and so will do internal memory copies if a non heap buffer is used. Beside this it will always return BUFFER_OVERFLOW when a smaller buffer then 16kb is used when calling wrap(...) (even if a very small amount of bytes should be encrypted). This can lead to excercive direct memory usage and pressure for no good reason.

Modifications:

Refactor internals of SslHandler to ensure we use heap buffers for the JDK SSLEngine impelementation

Result:

Less direct memory usage when JDK SSLEngine implementation is used

…ngine implementation

Motivation:

We should aim to always use heap buffers when using the JDK SSLEngine for now as it wants to operate on byte[] and so will do internal memory copies if a non heap buffer is used. Beside this it will always return BUFFER_OVERFLOW when a smaller buffer then 16kb is used when calling wrap(...) (even if a very small amount of bytes should be encrypted). This can lead to excercive direct memory usage and pressure for no good reason.

Modifications:

Refactor internals of SslHandler to ensure we use heap buffers for the JDK SSLEngine impelementation

Result:

Less direct memory usage when JDK SSLEngine implementation is used
@normanmaurer

This comment has been minimized.

Copy link
Member Author

normanmaurer commented Oct 21, 2019

/cc @gmcatsf @tbrooks8 WDYT ?

@normanmaurer normanmaurer requested review from njhill and carl-mastrangelo Oct 21, 2019
Copy link
Contributor

tbrooks8 left a comment

LGTM

return handler.engine.getSession().getPacketBufferSize();
ByteBuf allocateWrapBuffer(SslHandler handler, ByteBufAllocator allocator,
int pendingBytes, int numComponents) {
// As for the JDK SSLEngine we always need to allocate buffers of the size of 16kb

This comment has been minimized.

Copy link
@tbrooks8

tbrooks8 Oct 21, 2019

Contributor

NIT: It's technically over 16KB by default. Something around 16609. And can be configured to something different based on the context. Maybe say:

// As for the JDK SSLEngine we always need to allocate buffers of the size required by the SSLEngine (normally
// ~16KB). This is required even if the amount of data to encrypt is very small. Use heap buffers to reduce

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Oct 21, 2019

Author Member

good idea... @tbrooks8 I wonder if you could also test / verify / benchmark this change as it seems you care about this :)

This comment has been minimized.

Copy link
@gmcatsf

gmcatsf Oct 21, 2019

This looks good to me and thanks!

@gmcatsf

This comment has been minimized.

Copy link

gmcatsf commented Oct 21, 2019

@normanmaurer, my understanding is that bytes in heap buffer will be copied into native memory later in the pipeline and could you comment if this change would not increase pressure down the pipeline?

@normanmaurer

This comment has been minimized.

Copy link
Member Author

normanmaurer commented Oct 22, 2019

@gmcatsf at the end you will native memory and there is no way around this requirement. That said with this change the needed native memory should be minimised as it is very unlikely that you will always need 16kb buffers.

@tbrooks8

This comment has been minimized.

Copy link
Contributor

tbrooks8 commented Oct 22, 2019

I wonder if you could also test / verify / benchmark

What are you interested in? Like a JMH comparing usage of the SslHandler with direct buffers vs. heap buffers?

I assume there are already unit tests around the SslHandler?

@normanmaurer

This comment has been minimized.

Copy link
Member Author

normanmaurer commented Oct 22, 2019

@tbrooks8 I was just interested if you have some sort of benchmark that involves SslHandler with the JDK SSLlEngine that you used for previous optimisations. We have some as well :)

@tbrooks8

This comment has been minimized.

Copy link
Contributor

tbrooks8 commented Oct 22, 2019

@normanmaurer My prior PR was motivated by the fact that I saw direct buffers being allocated (assertions triggering) in an environment without direct buffer pooling enabled. Not like a specific microbenchmark.

I can download a netty snapshot against my build and check that the test assertions are not triggering anymore?

@normanmaurer

This comment has been minimized.

Copy link
Member Author

normanmaurer commented Oct 22, 2019

@tbrooks8 I guess that is a good start :) Should I generate one for you or can you do it by yourself ?

@tbrooks8

This comment has been minimized.

Copy link
Contributor

tbrooks8 commented Oct 22, 2019

I can do it. I'll pull your branch and comment with results later today.

@normanmaurer

This comment has been minimized.

Copy link
Member Author

normanmaurer commented Oct 22, 2019

@tbrooks8 <3 ... thanks a lot!

@njhill
njhill approved these changes Oct 23, 2019
Copy link
Member

njhill left a comment

LGTM

@tbrooks8

This comment has been minimized.

Copy link
Contributor

tbrooks8 commented Oct 23, 2019

I built the jars today and ran our Netty module test suite and everything looked good to me.

We do have some full Elasticsearch benchmarks that use TLS. But unfortunately it's not the type of thing that I can easily run against a snapshot. I can monitor after the next release and reach out if I see any regressions though.

@normanmaurer normanmaurer added this to the 4.1.43.Final milestone Oct 23, 2019
@normanmaurer normanmaurer merged commit 39cc7a6 into 4.1 Oct 23, 2019
3 checks passed
3 checks passed
pull request validation (centos6-java11) Build finished.
Details
pull request validation (centos6-java12) Build finished.
Details
pull request validation (centos6-java8) Build finished.
Details
@normanmaurer normanmaurer deleted the ssl_jdk_heap branch Oct 23, 2019
normanmaurer added a commit that referenced this pull request Oct 23, 2019
#9696)


Motivation:

We should aim to always use heap buffers when using the JDK SSLEngine for now as it wants to operate on byte[] and so will do internal memory copies if a non heap buffer is used. Beside this it will always return BUFFER_OVERFLOW when a smaller buffer then 16kb is used when calling wrap(...) (even if a very small amount of bytes should be encrypted). This can lead to excercive direct memory usage and pressure for no good reason.

Modifications:

Refactor internals of SslHandler to ensure we use heap buffers for the JDK SSLEngine impelementation

Result:

Less direct memory usage when JDK SSLEngine implementation is used
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.