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

Use `PooledByteBufAllocator` for netty internally #942

Merged
merged 20 commits into from Feb 22, 2020

Conversation

@idelpivnitskiy
Copy link
Member

idelpivnitskiy commented Feb 18, 2020

Motivation:

We configure Netty to use ST unpooled BufferAllocator.
However, there are a lot of cases when netty needs to allocate
buffers internally and these buffers will never be exposed to the
users and will be properly released by netty. For example, when
netty writes data to the transport or when it cumulates for
internal state. Allocation of unpooled memory (especially direct
memory) significantly hurts performance and increases GC
pressure.

Modifications:

  • Configure netty to use it's PooledByteBufAllocator.DEFAULT
    allocator;
  • Use allocator from the netty's ctx in HttpObjectEncoder
    instead of hardcoded POOLED_ALLOCATOR;
  • Remove RecvByteBufAllocator with pooled allocator that
    is not required anymore;
  • Do not use CopyByteBufHandlerChannelInitializer for h2;
  • Copy h2 DATA_FRAMEs from pooled to unpooled memory
    before propagating payload body to the user;
  • Do not override ByteBufAllocator for SslHandler anymore;
  • BufferAllocators.DEFAULT_ALLOCATOR should prefer heap
    memory by default;

Result:

  1. Improved performance of gRPC:
  • Aggregated API: RPS increased by 30-37%, p99 latency reduced
    by x1.8-5.7, depending on payload body size;
  • Streaming API: RPS increased by 23-29%, p99 latency reduced
    by x2.1-4.7, depending on payload body size;
  1. Improved performance of HTTP:
  • Aggregated API: RPS increased by 7-13%, depending on payload
    body size, no p99 latency change;
  • Streaming API: RPS increased by 5-9%, depending on payload
    body size, no p99 latency change;
  1. Less code to maintain.
@idelpivnitskiy idelpivnitskiy requested review from NiteshKant and Scottmitch Feb 18, 2020
@idelpivnitskiy

This comment has been minimized.

Copy link
Member Author

idelpivnitskiy commented Feb 18, 2020

Build failed due to docker issue

@servicetalk-bot test this please

Copy link
Member

NiteshKant left a comment

Overall looks good, few changes, then LGTM

@Override
protected void channelRead0(final ChannelHandlerContext ctx, final ByteBuf buf) {
// We must not release the incoming buf here because it will be released by SimpleChannelInboundHandler
ctx.fireChannelRead(alloc.buffer(buf.readableBytes()).writeBytes(buf));

This comment has been minimized.

Copy link
@NiteshKant

NiteshKant Feb 19, 2020

Member

We should also check for ByteBufHolder objects here, H2 frames may be buffer holders.

This comment has been minimized.

Copy link
@idelpivnitskiy

idelpivnitskiy Feb 20, 2020

Author Member

This handler is not used for h2 because in h2 we copy only data-frames (see AbstractH2DuplexHandler) to unpooled memory. This is currently used only for h1 (and may be used for any other protocol that requires unpooled memory before decoding) and will be placed right after SslHandler. Therefore, only ByteBufs are expected here.

This comment has been minimized.

Copy link
@NiteshKant

NiteshKant Feb 20, 2020

Member

Ok for the sake of completeness I think we should still check for BufferHolder objects and if they are not expected raise an error.

@Override
protected void channelRead0(final ChannelHandlerContext ctx, final ByteBuf buf) {
// We must not release the incoming buf here because it will be released by SimpleChannelInboundHandler
ctx.fireChannelRead(alloc.buffer(buf.readableBytes()).writeBytes(buf));

This comment has been minimized.

Copy link
@NiteshKant

NiteshKant Feb 19, 2020

Member

Should we be only doing the copy if the buffer is pooled?

This comment has been minimized.

Copy link
@idelpivnitskiy

idelpivnitskiy Feb 20, 2020

Author Member

Because we always expect pooled buffers at this point this check will be unnecessary. Let's consider it if the policy for buffer allocators will change later.

This comment has been minimized.

Copy link
@NiteshKant

NiteshKant Feb 20, 2020

Member

It is better to reduce assumptions between unrelated components (Bootstrap classes using pooled allocator and this handler assuming pooled allocators). If you think it is not expected at least add an assert and a test that leverages the assert.

@@ -49,10 +50,12 @@ private StreamingConnectionFactory() {
final HttpExecutionContext executionContext, final ReadOnlyHttpClientConfig config,
final ChannelInitializer initializer) {
final CloseHandler closeHandler = forPipelinedRequestResponse(true, channel.config());
assert config.h1Config() != null;

This comment has been minimized.

Copy link
@NiteshKant

NiteshKant Feb 19, 2020

Member

We should perhaps rename this class to H1StreamingConnectionFactory

This comment has been minimized.

Copy link
@idelpivnitskiy

idelpivnitskiy Feb 20, 2020

Author Member

Let's do that in a follow-up to minimize scope of this PR.

This comment has been minimized.

Copy link
@NiteshKant

NiteshKant Feb 20, 2020

Member

Its a package private class and it is just a rename. It is easier to do it in-place as you added the assert to make sure it is used only for H1. It is up to you if you want to do it here or in a follow up, I just want to make sure this does not get lost in comments.

@idelpivnitskiy idelpivnitskiy requested a review from NiteshKant Feb 20, 2020
@idelpivnitskiy idelpivnitskiy merged commit 82e0431 into apple:master Feb 22, 2020
3 checks passed
3 checks passed
pull request validation (jdk11) Build finished.
Details
pull request validation (jdk8) Build finished.
Details
pull request validation (quality) Build finished.
Details
@idelpivnitskiy idelpivnitskiy deleted the idelpivnitskiy:use-pooled-ba-in-netty branch Feb 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.