Add DirectByteBuffer strategy for transport-nio#36289
Merged
Tim-Brooks merged 17 commits intoelastic:masterfrom Dec 7, 2018
Merged
Add DirectByteBuffer strategy for transport-nio#36289Tim-Brooks merged 17 commits intoelastic:masterfrom
Tim-Brooks merged 17 commits intoelastic:masterfrom
Conversation
Collaborator
|
Pinging @elastic/es-distributed |
| ioBuffer.flip(); | ||
| try { | ||
| return rawChannel.write(buffer); | ||
| return rawChannel.write(ioBuffer); |
Contributor
There was a problem hiding this comment.
Shouldn't we move the position on buffer back again by the number of writes not written if the write doesn't succeed in writing all of ioBuffer, i.e. when ioBuffer.remaining() < rawChannel.write(ioBuffer)?
Contributor
Author
There was a problem hiding this comment.
This is an issue. The passed buffer needs to reflect partial flushes.
Contributor
Author
|
@original-brownbear - Thanks for the review. I have added a number of tests. And fixed the issue you identified. |
original-brownbear
approved these changes
Dec 6, 2018
Contributor
original-brownbear
left a comment
There was a problem hiding this comment.
@tbrooks8 thanks LGTM :)
Contributor
Author
|
@elasticmachine run sample packaging tests |
12 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is related to #27260. In Elasticsearch all of the messages that we
serialize to write to the network are composed of heap bytes. When you
read or write to a nio socket in java, the heap memory you passed down
must be copied to/from direct memory. The JVM internally does some
buffering of the direct memory, however it is essentially unbounded.
This commit introduces a simple mechanism of buffering and copying the
memory in transport-nio. Each network event loop is given a 64kb
DirectByteBuffer. When we go to read we use this buffer and copy the
data after the read. Additionally, when we go to write, we copy the data
to the direct memory before calling write. 64KB is chosen as this is the
default receive buffer size we use for transport-netty4
(
NETTY_RECEIVE_PREDICTOR_SIZE).Since we only have one buffer per thread, we could afford larger.
However, if we the buffer is large and not all of the data is flushed in
a
writecall, we will do excess copies. This is something we canexplore in the future.