Skip to content
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

Save intermediate text encoded buffer copies #5358

Merged
merged 1 commit into from
Oct 21, 2024

Conversation

franz1981
Copy link
Contributor

@franz1981 franz1981 commented Oct 15, 2024

Websocket txt messages perform an additional byte[] copy, because:

  1. Buffer::buffer(String) is performing String::getBytes -> first byte[] allocation
  2. It than allocates VertxByteBufAllocator.DEFAULT.heapBuffer to write the copy in -> second byte[] allocation + copy

VertxByteBufAllocator.DEFAULT.heapBuffer could allocate, based on Unsafe presence:

  • UnpooledHeapByteBuf
  • UnpooledUnsafeHeapByteBuf

Both the Vertx childrens of these 2 Netty buffers are NOT exposing directly the ability to wrap a given byte[] and that's why we currently perform the copy: in theory UnpooledHeapByteBuf vertx variant could be modified to do it, but UnpooledUnsafeHeapByteBuf nope, because it doesn't expose its parent constructor which enable setting the byte[] array without copying it (see https://github.com/netty/netty/blob/netty-4.1.114.Final/buffer/src/main/java/io/netty/buffer/UnpooledHeapByteBuf.java#L69).

Fixing the additional copy should require to change both Vertx and Netty, for not much gains, given that sharing a given byte[] is likely to disappear in Netty 5, regardless, so, not a good solution.

A different path would be to "guesstimate" the length of the utf8 array (there are 2 Netty method for such: eager O(1) or precise O(n)) - and using Netty's ByteBuf encoder to perform it:
this could be beneficial, but not ideal because the JVM can perform String checks to estimate the length of the encoded byte[] way faster thanks to its vectorized instructions.
This still could be benchmarked, to make sure, is a valid assumption.

For completeness, this same path is used by https://github.com/eclipse-vertx/vert.x/blob/4.5.10/src/main/java/io/vertx/core/http/impl/WebSocketImplBase.java#L240 which is using Unpooled::copiedBuffer under the hood to do what's described above and allocates io.netty.buffer.UnpooledByteBufAllocator$InstrumentedUnpooledUnsafeHeapByteBuf or io.netty.buffer.UnpooledByteBufAllocator$InstrumentedUnpooledHeapByteBuf.

The approach taken for this PR is instead to allocates a UnpooledHeapByteBuf without performing any copy:

  • it has the disadvantages of bringing another Netty buffer type in
  • it has another disadvantages because it is still a Netty reference counted type, while the Vertx heap buffers are modified to not perform reference counting
  • it has the advantage of using the (supposly) faster String::getBytes and not performing any additional copy from it (including the wrappers BufferImpl and subsequent Netty slices

@franz1981
Copy link
Contributor Author

@mkouba @vietj

@vietj vietj merged commit a368b77 into eclipse-vertx:4.x Oct 21, 2024
7 checks passed
@vietj vietj added this to the 4.5.11 milestone Oct 21, 2024
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.

2 participants