Skip to content

#9345 use default allocator when possible to avoid heap allocations#9359

Closed
doom369 wants to merge 1 commit intonetty:4.1from
doom369:user_dafault_allocator
Closed

#9345 use default allocator when possible to avoid heap allocations#9359
doom369 wants to merge 1 commit intonetty:4.1from
doom369:user_dafault_allocator

Conversation

@doom369
Copy link
Copy Markdown
Contributor

@doom369 doom369 commented Jul 12, 2019

Motivation:

In many places Netty uses Unpooled.buffer(0) while should use EMPTY_BUFFER. We can't change this due to back compatibility, however we can at least replace Unpooled.buffer(0) with ByteBufAllocator.DEFAULT.buffer(0) so in evns. with preferDirect=true heap will not be used.
This change affects mostly http and websockets users.

Modification:

  • Repalced Unpooled.buffer(0) with ByteBufAllocator.DEFAULT.buffer(0);
  • Replaced Unpooled.copiedBuffer(text, CharsetUtil.UTF_8) with
    ByteBufUtil.encodeString(ByteBufAllocator.DEFAULT, CharBuffer.wrap(text), CharsetUtil.UTF_8);

Result:

Fixes #9345

@netty-bot
Copy link
Copy Markdown

Can one of the admins verify this patch?

@normanmaurer
Copy link
Copy Markdown
Member

@netty-bot test this please

1 similar comment
@normanmaurer
Copy link
Copy Markdown
Member

@netty-bot test this please

@normanmaurer
Copy link
Copy Markdown
Member

@netty-bot test this please

@normanmaurer
Copy link
Copy Markdown
Member

@doom369 there is a leak... please fix :)

@doom369
Copy link
Copy Markdown
Contributor Author

doom369 commented Jul 12, 2019

@normanmaurer lol, this leaks mostly are due to wrong implementation / tests :). Default allocator just exposes these places. Fixing...

@normanmaurer
Copy link
Copy Markdown
Member

@doom369 I agree ;)

@normanmaurer
Copy link
Copy Markdown
Member

@netty-bot test this please

2 similar comments
@normanmaurer
Copy link
Copy Markdown
Member

@netty-bot test this please

@normanmaurer
Copy link
Copy Markdown
Member

@netty-bot test this please

@normanmaurer
Copy link
Copy Markdown
Member

@netty-bot test this please

@doom369
Copy link
Copy Markdown
Contributor Author

doom369 commented Jul 13, 2019

@normanmaurer this PR becomes more and more complex with further test fixes :). Found at least 1 place that should be fixed before the further processing.

HttpPostMultipartRequestDecoder:

    @Override
    public HttpPostMultipartRequestDecoder offer(HttpContent content) {
        checkDestroyed();

        // Maybe we should better not copy here for performance reasons but this will need
        // more care by the caller to release the content in a correct manner later
        // So maybe something to optimize on a later stage
        ByteBuf buf = content.content();
        if (undecodedChunk == null) {
            undecodedChunk = buf.copy();
        } else {
            undecodedChunk.writeBytes(buf);
        }
        if (content instanceof LastHttpContent) {
            isLastChunk = true;
        }
        parseBody();
        if (undecodedChunk != null && undecodedChunk.writerIndex() > discardThreshold) {
            undecodedChunk.discardReadBytes();
        }
        return this;
    }

This copy operation inside causes memory leak with direct buffers and look like not released in all flows.
So for now I'll revert some of changes and will split this on multiple PRs. 1 PR for 1 separate package. I'll start from WebSockets package.

@doom369
Copy link
Copy Markdown
Contributor Author

doom369 commented Jul 13, 2019

Closing this one as it is very generic. Will provide separate PR for separate package.

@doom369 doom369 closed this Jul 13, 2019
normanmaurer pushed a commit that referenced this pull request Jul 16, 2019
…possible (#9361)

Motivation:

While fixing #9359 found few places that could be patched / improved separately.

Modification:

On handshake response generation - throw exception before allocating response objects if request is invalid.

Result:

No more leaks when exception is thrown.
normanmaurer pushed a commit that referenced this pull request Jul 16, 2019
…possible (#9361)

Motivation:

While fixing #9359 found few places that could be patched / improved separately.

Modification:

On handshake response generation - throw exception before allocating response objects if request is invalid.

Result:

No more leaks when exception is thrown.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Netty still uses heap buffers while preferDirect is true

3 participants