-
Notifications
You must be signed in to change notification settings - Fork 27
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
Avoid composite buffers for small messages #68
base: main
Are you sure you want to change the base?
Avoid composite buffers for small messages #68
Conversation
try { | ||
return BufferInternal.buffer(fullMsg); | ||
} finally { | ||
bbuf.release(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
vertx buffers are unreleasable and not pooled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know, but the type is still ByteBuf
so I've added it just in case we decide to change rules on the concrete type used: I can add an assertion here to be sure in the future nothing break too (or just remove it as by your suggestion)
@He-Pin I remember you have commented on the gRpc benchmark...we are still working on enabling our load generator to support gRpc, hence we have no "official" and supported benchmarks to verify this. This is mostly based on my knowledge of the Netty stack and the OpenJDK platform; do you have anything to test it and verify if it is worthy? |
if (len <= 128) { | ||
int totalBytes = 5 + len; | ||
// let's use vertx buffer here because it doesn't have any atomic release, if unpooled | ||
ByteBuf fullMsg = VertxByteBufAllocator.DEFAULT.heapBuffer(totalBytes, totalBytes); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we would like to use pooled direct buffers here instead, wdyt @vietj ?
It would save the transport later to copy it into a direct one :/
This change is optimizing the use case of Netty composite buffers in case a small payload/data is required.
The NIO Netty flow will end up copying the compontents from the CompositeBuffer in a single native direct buffer at
https://github.com/netty/netty/blob/fbb0207d5ecce39f3d63450dfd59bad5510b8e8b/transport/src/main/java/io/netty/channel/nio/AbstractNioChannel.java#L443.
For a composite buffer made of 2 components (the length and compression flag + data ones) it means iterating them (updating for each the offsets etc etc) in
CompositeByteBuf::getBytes(int index, ByteBuf dst, int dstIndex, int length)
(calling back toUnpooledHeapByteBuf::getBytes(int index, ByteBuf dst, int dstIndex, int length)
with the directdst
for each component, updating each ones offset, checking accessibility etc etc).In the case of a single merged buffer, we pay an additional allocation (and copy of data), but:
+176 bytes
, while allocating a new buffer (not composite) would costdata length + 5 + 48 bytes
and perform the additional data copy. eg for 128 bytes data, the composite would still allocate +176 bytes, while this PR will allocate 128 + 53 = 181 bytes which is similar (but we can throw away immediately the original data buffer, no longer needed!)setBytes(AbstractByteBuf buf, long addr, int index, ByteBuf src, int srcIndex, int length)
on the direct pooled buffer, using a single vertx heap buffer which contains length + data.The point about reachability of buffers is a stealthy but important consideration: