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

Use less memory during writes when using SslHandler with SslProvider.… #6252

Closed
wants to merge 1 commit into
base: 4.1
from

Conversation

@normanmaurer
Member

normanmaurer commented Jan 19, 2017

…OpenSsl

Motivation:

In commit fc3c9c9 I changes the way how we calculate the capacity of the needed ByteBuf for wrap operations that happen during writes when the SslHandler is used. This had the effect that the same capacity for ByteBufs is needed for the JDK implementation of SSLEngine but also for our SSLEngine implementation that uses OpenSSL / BoringSSL / LibreSSL. Unfortunally this had the side-effect that applications that used our SSLEngine implementation now need a lot more memory as bascially the JDK implementation always needs a 16kb buffer for each wrap while we can do a lot better for our SSLEngine implementation.

Modification:

  • Resurrect code that calculate a better ByteBuf capacity when using our SSLEngine implementation and so be able to safe a lot of memory
  • Add test-case to ensure it works as expected and is not removed again later on.

Result:

Memory footprint of applications that uses our SSLEngine implementation based on OpenSSL / BoringSSL / LibreSSL is back to the same amount of before commit fc3c9c9.

for (int i = offset; i < endOffset; ++i) {
final ByteBuffer src = srcs[i];
if (src == null) {
throw new IllegalArgumentException("srcs[" + i + "] is null");

This comment has been minimized.

@trustin

trustin Jan 19, 2017

Member

Not strong on this, but shouldn't this be NPE for consistency? i.e. new NullPointerException("srcs[" + i + ']')

@trustin

trustin Jan 19, 2017

Member

Not strong on this, but shouldn't this be NPE for consistency? i.e. new NullPointerException("srcs[" + i + ']')

This comment has been minimized.

@normanmaurer
}
}
int maxEncryptedLen = calculateOutNetBufSize(srcsLen);

This comment has been minimized.

@nmittler

nmittler Jan 19, 2017

Member

nit: maybe just use min?

final maxEncryptedLen = Math.min(MAX_ENCRYPTED_PACKET_LENGTH,
    calculateOutNetBufSize(srcLen))
@nmittler

nmittler Jan 19, 2017

Member

nit: maybe just use min?

final maxEncryptedLen = Math.min(MAX_ENCRYPTED_PACKET_LENGTH,
    calculateOutNetBufSize(srcLen))

This comment has been minimized.

@normanmaurer
@normanmaurer
@Scottmitch

This comment has been minimized.

Show comment
Hide comment
@Scottmitch

Scottmitch Jan 19, 2017

Member

reminder: update these new tests to incorporate #6237

Member

Scottmitch commented Jan 19, 2017

reminder: update these new tests to incorporate #6237

SSLEngine clientEngine = null;
SSLEngine serverEngine = null;
try {
clientEngine = clientSslCtx.newEngine(UnpooledByteBufAllocator.DEFAULT);

This comment has been minimized.

@Scottmitch

Scottmitch Jan 19, 2017

Member

rebase reminder: update the allocators here.

@Scottmitch

Scottmitch Jan 19, 2017

Member

rebase reminder: update the allocators here.

This comment has been minimized.

@Scottmitch

Scottmitch Jan 19, 2017

Member

Do we need to update these unit tests as a result of #6237?

@Scottmitch

Scottmitch Jan 19, 2017

Member

Do we need to update these unit tests as a result of #6237?

This comment has been minimized.

@normanmaurer

normanmaurer Jan 19, 2017

Member

already done ;)

@normanmaurer

normanmaurer Jan 19, 2017

Member

already done ;)

This comment has been minimized.

@Scottmitch

Scottmitch Jan 19, 2017

Member

feel free to ignore if not ... rushing out the door at the moment :)

@Scottmitch

Scottmitch Jan 19, 2017

Member

feel free to ignore if not ... rushing out the door at the moment :)

Show outdated Hide outdated ...er/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java
Use less memory during writes when using SslHandler with SslProvider.…
…OpenSsl

Motivation:

In commit fc3c9c9 I changes the way how we calculate the capacity of the needed ByteBuf for wrap operations that happen during writes when the SslHandler is used. This had the effect that the same capacity for ByteBufs is needed for the JDK implementation of SSLEngine but also for our SSLEngine implementation that uses OpenSSL / BoringSSL / LibreSSL. Unfortunally this had the side-effect that applications that used our SSLEngine implementation now need a lot more memory as bascially the JDK implementation always needs a 16kb buffer for each wrap while we can do a lot better for our SSLEngine implementation.

Modification:

- Resurrect code that calculate a better ByteBuf capacity when using our SSLEngine implementation and so be able to safe a lot of memory
- Add test-case to ensure it works as expected and is not removed again later on.

Result:

Memory footprint of applications that uses our SSLEngine implementation based on OpenSSL / BoringSSL / LibreSSL is back to the same amount of before commit fc3c9c9.
@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Jan 19, 2017

Member

Cherry-picked into 4.1 (ac5f701) and 4.0 (ed61ec0)

Member

normanmaurer commented Jan 19, 2017

Cherry-picked into 4.1 (ac5f701) and 4.0 (ed61ec0)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment