Skip to content

Add MessageToBufferedByteEncoder for better performance#5510

Closed
normanmaurer wants to merge 1 commit into4.1from
buffered_encoder
Closed

Add MessageToBufferedByteEncoder for better performance#5510
normanmaurer wants to merge 1 commit into4.1from
buffered_encoder

Conversation

@normanmaurer
Copy link
Copy Markdown
Member

Motivation:

MessageToMessageEncoder always allocate a new buffer per encode call and pass it through the pipeline via a write call. This can have negative performance impact if the users does a lot of write calls before doing a flush as this may produce a lot of small buffers that needs to get allocated and also written to the underlying transport. For this situations it can be a lot better to just allocate one buffer, encoder everything into this buffer and only write it the the transport once a flush happens.

Modifications:

Add a new MessageToBufferedByteEncoder which should be used if more writes then flush operations are expected.

Result:

Better performance.

Motivation:

MessageToMessageEncoder always allocate a new buffer per encode call and pass it through the pipeline via a write call. This can have negative performance impact if the users does a lot of write calls before doing a flush as this may produce a lot of small buffers that needs to get allocated and also written to the underlying transport. For this situations it can be a lot better to just allocate one buffer, encoder everything into this buffer and only write it the the transport once a flush happens.

Modifications:

Add a new MessageToBufferedByteEncoder which should be used if more writes then flush operations are expected.

Result:

Better performance.
@normanmaurer normanmaurer added this to the 4.0.39.Final milestone Jul 7, 2016
@normanmaurer normanmaurer self-assigned this Jul 7, 2016
@normanmaurer
Copy link
Copy Markdown
Member Author

@Scottmitch @buchgr @trustin PTAL

@ninja-
Copy link
Copy Markdown

ninja- commented Aug 7, 2017

This looks good to me, want to merge?

@ninja-
Copy link
Copy Markdown

ninja- commented Aug 7, 2017

Also we might want to add option to limit max cumulation size to avoid too big buffers being allocated? In my message cumulator there's a limit of 16kb, but it was working another way(encode every message to it's own buffer and then merge to new buffer...so the limit was to avoid too much memcpy with big packets).
In this version it might be useful to avoid cumulation buffers growing too big to reduce memory waste (i.e. it grows from 8mb to 16mb and only 9mb is used...)

if (readable < 0) {
throw new IllegalStateException("readableBytes are less then before calling encode(...)");
}
outboundBuffer.incrementPendingOutboundBytes(readable);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This crosses API boundaries and exposes an internal API which I would prefer to avoid. I don't necessarily have a better idea, but we may have the same issue with SslHandler or other handlers which aggregate writes (e.g. ProxyHandler).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@Scottmitch in we use PendingWriteQueue which takes care of this for us. Maybe I could just use it here as well. Let me give it a shoot.

@normanmaurer
Copy link
Copy Markdown
Member Author

Let me just close this for now.

@normanmaurer normanmaurer deleted the buffered_encoder branch December 12, 2017 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants