Skip to content

Avoid composite buffers for small messages #68

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Avoid composite buffers for small messages #68

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f270026
Save using composites or using them right
franz1981 Aug 21, 2023
9ae5ae2
Using vertx default allocator to have lightweight buffer's release
franz1981 Aug 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 21 additions & 4 deletions vertx-grpc-common/src/main/java/io/vertx/grpc/common/impl/GrpcMessageImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import io.netty.buffer.Unpooled;
import io.vertx.core.buffer.Buffer;
import io.vertx.core.buffer.impl.BufferInternal;
import io.vertx.core.buffer.impl.VertxByteBufAllocator;
import io.vertx.grpc.common.GrpcMessage;

public class GrpcMessageImpl implements GrpcMessage {
Expand All @@ -38,15 +39,31 @@ public Buffer payload() {
}

public static Buffer encode(GrpcMessage message) {
ByteBuf bbuf = ((BufferInternal)message.payload()).getByteBuf();
ByteBuf bbuf = ((BufferInternal) message.payload()).getByteBuf();
int len = bbuf.readableBytes();
boolean compressed = !message.encoding().equals("identity");
// it is worthy to just copy here, 'cause composite (heap) buffers are slower to be sent to the wire or copied
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);
Copy link
Contributor Author

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 :/

fullMsg.setByte(0, compressed ? 1 : 0); // Compression flag
fullMsg.setInt(1, len); // Length
fullMsg.setBytes(5, bbuf, bbuf.readerIndex(), len);
fullMsg.writerIndex(totalBytes);
try {
return BufferInternal.buffer(fullMsg);
} finally {
bbuf.release();
Copy link
Member

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

Copy link
Contributor Author

@franz1981 franz1981 Aug 22, 2023 •
edited
Loading

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)

}
}
// slow-path
ByteBuf prefix = Unpooled.buffer(5, 5);
prefix.writeByte(compressed ? 1 : 0); // Compression flag
prefix.writeInt(len); // Length
CompositeByteBuf composite = Unpooled.compositeBuffer();
composite.addComponent(true, prefix);
composite.addComponent(true, bbuf);
CompositeByteBuf composite = Unpooled.compositeBuffer(2);
composite.addComponent(true, 0, prefix);
composite.addComponent(true, 1, bbuf);
return BufferInternal.buffer(composite);
}
}