Feature/nio direct bytebuffer encoder - #345
Draft
agentgt wants to merge 3 commits into
Draft
Conversation
No output in the codebase today actually relies on LogOutput's default write(LogEvent, ByteBuffer, ContentType) - FileChannelOutput (the only output advertising WriteMethod.BYTE_BUFFER) overrides that method itself and bypasses the default entirely, and the only Buffer implementations that exist (StringBuilderBuffer, JsonBuffer) both drain through the byte[]/String overloads instead. So the default's correctness has never actually been exercised. Builds a real java.nio FileChannel-backed LogOutput plus a real ByteBuffer-backed encoder Buffer (no mocks), driven through a full RainbowGum route, to check it directly. Two scenarios: - unflippedPositionAsLengthConventionWritesFullContent: the buffer is left unflipped (position == written length) after encoding, matching what the default method's `new byte[buf.position()]` sizing expects. Writes correctly. - standardFlippedBufferConventionSilentlyWritesNothing: the buffer is flip()'d before drain, the standard java.nio idiom (write, then flip before handing off to be read/written) that any real future NIO output would very plausibly use. The default method still sizes off buf.position(), which is now 0 post-flip, so it silently writes a zero-length array instead of the actual content - quiet data loss, not an exception. Confirms Adam's suspicion: LogOutput.write(LogEvent, ByteBuffer, ContentType)'s default implementation only works for the non-standard "position tracks written length, never flipped" convention, and produces silent data loss for the conventional flipped-buffer NIO idiom. Not fixed here - test only, pending a decision on which convention to standardize on (switching to buf.remaining()/buf.get(arr) without rewind() would fix the flipped case but break the unflipped one, and nothing in the codebase depends on either today).
Sized its array off buf.position() and rewind()'d, which only worked for a non-standard "buffer never flipped, position tracks written length" convention - the standard java.nio idiom of filling a buffer then flip()'ing it before handing it off (or building one with ByteBuffer.wrap(byte[])) silently produced a zero-length write instead of the actual content. Switched to buf.remaining()/buf.get(arr), the conventional way to drain a buffer that's ready for reading, and documented the expected contract. Nothing in the codebase depended on the old convention (see previous commit), so this is a pure fix, not a behavior change for any real output. Updates NioByteBufferOutputTest to match: the flipped-buffer test now asserts correct full content instead of demonstrating the bug, and adds a second test confirming ByteBuffer.wrap(byte[]) (also standard, and what a real caller would likely reach for first) works too.
…coder DirectByteBufferEncoder/DirectByteBufferBuffer format into a reused StringBuilder (same as the standard path) but then encode directly into a reused ByteBuffer via a CharsetEncoder, skipping the intermediate String/byte[] allocation that LogOutput.write(LogEvent, String)'s default otherwise does on every call - the same technique Log4j2's garbage-free encoders use, and one of the likely reasons it edges out RainbowGum in some non-virtual-thread benchmarks. Sizes the byte buffer upfront from CharsetEncoder.maxBytesPerChar() so a single encode() call can't overflow, rather than a more complex overflow/retry loop. Depends on the LogOutput.write(LogEvent, ByteBuffer, ContentType) fix from the previous commit - this is exactly the API it exists to future- proof for. Pair with LogAppender.AppenderFlag#REUSE_BUFFER to get the full benefit, since the byte buffer (and any grown capacity) only persists across events if the appender keeps one Buffer instance instead of creating a fresh one per event. New module, no ServiceProvider registration - opt-in via .encoder(DirectByteBufferEncoder.of(formatter)) like any other encoder, not wired into any default. Tests are real full RainbowGum loads into a real ListLogOutput (no mocks): ASCII, multi-byte UTF-8 (including a surrogate-pair emoji), a message forcing buffer growth well past the initial 8KB capacity, and a REUSE_BUFFER two-events-in-a-row case guarding against a longer message's bytes leaking into a following shorter one.
agentgt
marked this pull request as draft
August 24, 2026 16:08
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
1 participant
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.
No description provided.