Read 64-bit varints through unchecked Netty accessors - #14
Merged
Conversation
…t merge Measured and rejected (2026-07-30): a generated helper class in the io.netty.buffer package reaches AbstractByteBuf's protected _getByte / _getIntLE / _getLongLE and the readerIndex field directly, decoding a whole varint with zero per-byte checks and a single index store. The codec gates each read on `instanceof AbstractByteBuf && readable >= 10` and falls back to the checked chain near the buffer end. Despite removing every check, ALL deserialize benchmarks regressed (same-session baseline, 2 forks, tight bars): Simple 87.0 -> 59.1 (-32%) Simple+readString 51.3 -> 30.7 (-40%) BaseCommand 46.8 -> 32.3 (-31%) AddressBook 27.3 -> 20.2 (-26%) MessageMetadata 17.4 -> 15.9 (-8%) Interpretation: C2 optimizes the canonical checkReadableBytes0()/ readByte() pattern to near-free (the Netty kill-switch flags showed the checks cost only 8-17% when removed globally with no gate); the two-path gate plus a hand-built chain defeats that. Preserved for reference only — see the perf-plan gist, section 4.
Removing the `readableBytesFast(a) >= N` gate per read flipped the experiment from a uniform regression to a real win: the gate branch was the entire cost of the previous commit, not the unchecked chain. Safety is now enforced once per message instead of once per varint: - A truncated field always consumes past the message limit, so a single post-parse `readerIndex > endIdx` check restores the throw-on-truncated contract (covered by TruncatedInputTest across heap and pooled-direct buffers). - Overruns are bounded at 9 bytes. Heap buffers fail safely (ArrayIndexOutOfBoundsException is an IndexOutOfBoundsException) and pooled direct buffers read within their arena chunk. The residual exposure is a truncated trailing varint on a tight-capacity unpooled unsafe-direct buffer. Deserialize throughput vs the checked baseline (JMH, Apple M-series, 2-4 forks; later runs on this machine showed degraded/noisy conditions, so a long-run confirmation matrix is required before merging): MessageMetadata 17.4 -> 23.7 (+36%, reproducible across runs) Simple 87.0 -> 99.4 (+14%) AddressBook 27.3 -> 27.5 (flat) BaseCommand 46.8 -> 43.4 (-7%, within fork variance) Simple+readString 51.3 -> 48.2 (-6%)
Full 5-fork matrix vs the checked baseline: the unchecked chain is a large win exactly where multi-byte varints dominate (MessageMetadata +31%, reproducible across every run and measurement condition) and appeared to regress 1-byte-dominated shapes. The regression readings are thermally confounded, however: back-to-back matrix arms on a throttling laptop penalize later arms, and the hybrid arm "regressed" readString -17% with a provably byte-identical hot path (no varint64 fields in Frame/Point at all). Decision-grade adjudication of the checked-path shapes needs an interleaved A/B on a cool machine. This commit keeps the unchecked chain only in readVarInt64(), where the win is certain; readVarInt() and the fixed readers stay on the canonical checked path, which C2 compiles to near-free for the 1-byte case. The post-parse limit check and TruncatedInputTest cover the (at most 9-byte) overrun of a truncated varint64.
Interleaved A/B against master showed the varint64-only unchecked path regressing every benchmark without long varints: PrintInlining revealed readVarInt64Unchecked (371 bytecodes) and the readVarInt64 wrapper with the checked chain inline (303 bytecodes) both fail to inline, so 1-2 byte varints paid a call per read; and the unconditional post-parse truncation check perturbed codegen of small messages that cannot overrun at all. - Decode 1-2 byte varint64s in a small always-inlined front, falling into the (still non-inlined) nested chain only for 3+ byte values. - Split the checked fallback out of readVarInt64 so the instanceof gate always inlines. - Emit the post-parse truncation check only for messages that parse 64-bit varints; all other messages keep parseFrom byte-identical to the fully checked version.
readableBytesFast, readVarIntUnchecked and the fixed-int readers were residue of the earlier gated/gateless experiments; nothing references them since unchecked reads were restricted to 64-bit varints. Also update the class javadoc: safety is enforced by the generated parseFrom(), not by caller-side readability validation.
There was a problem hiding this comment.
Pull request overview
This PR optimizes 64-bit varint deserialization by routing AbstractByteBuf reads through a generated io.netty.buffer helper that uses unchecked Netty accessors, and then restoring the truncated-input exception contract via a post-parse bounds check emitted only for messages that parse varint64 fields.
Changes:
- Add a generated Netty-package accessor (
LightProtoByteBufAccess_*) to decode varint64 with unchecked_getByte()and a singlereaderIndexwrite. - Update
LightProtoCodec.readVarInt64()to dispatchAbstractByteBufto the unchecked decoder while keeping a checked fallback path. - Emit a conditional post-parse
readerIndex > endIdxcheck for messages that can parse varint64 (including via map key/value), and add truncation-focused tests.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/src/test/java/io/streamnative/lightproto/tests/TruncatedInputTest.java | Adds coverage for truncation behavior across heap and pooled-direct buffers, plus a tight-capacity direct sanity test. |
| code-generator/src/main/resources/io/streamnative/lightproto/generator/LightProtoCodec.java | Routes varint64 reads through unchecked access for AbstractByteBuf, with a checked fallback. |
| code-generator/src/main/resources/io/streamnative/lightproto/generator/LightProtoByteBufAccess.java | Introduces the Netty-package unchecked varint64 decoder template used for generated accessors. |
| code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoMessage.java | Conditionally emits a post-parse truncation check for messages that can parse varint64. |
| code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoMapField.java | Marks map fields as using varint64 parsing when key/value types are int64/uint64/sint64. |
| code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoGenerator.java | Generates the per-package Netty accessor class and rewrites codec references to it. |
| code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoField.java | Adds usesVarInt64Parse() / isVarInt64Type() helpers used to gate truncation checks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…te load - Add MapInt64Message and a truncation test proving the post-parse check is emitted when only map key/value types route through readVarInt64(). - Correct the helper javadoc overrun bound: at most 9 bytes past the message limit, not 10. - Narrow TruncatedInputTest's javadoc to the buffer types it covers. - Fail fast with an explicit message if the helper template resource is missing from the classpath.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoGenerator.java:96
javaPackagesis collected in a plainList, so if multiple input protos share the samejavaPackageName, this loop will regenerate/overwriteLightProtoCodec.javaand the Netty accessor class multiple times for the same package. It’s harmless but adds unnecessary work and makes outputs depend on duplicate entries. Consider deduplicating while preserving order (e.g., iterate aLinkedHashSet).
// Include the coded class once per every generated java package
for (String javaPackage : javaPackages) {
// Unchecked ByteBuf accessor companion: lives in the io.netty.buffer
// package (to reach AbstractByteBuf's protected members) and is named
// after the generated package so that multiple generated packages in
// one project don't collide.
String accessClassName = "LightProtoByteBufAccess_" + javaPackage.replace('.', '_');
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
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.
Motivation
After #11/#13, the largest remaining deserialize cost on varint64-heavy messages is Netty's per-byte read overhead: every
readByte()payscheckReadableBytes0()(bounds + accessibility) plus areaderIndexfield store. For a message like Pulsar'sMessageMetadata(sequence ids, timestamps, ledger/entry ids), that is a large fraction of the parse profile. Disabling the checks globally via-Dio.netty.buffer.checkBounds=falserecovers +8-17% deserialize, but it is a process-wide tradeoff; this PR recovers the same class of win in code, only where it pays.Changes
io.netty.bufferpackage, giving access toAbstractByteBuf's protected_getByte()and its package-privatereaderIndexfield.readVarInt64Unchecked()decodes a whole varint with no per-byte checks and a singlereaderIndexstore: 1-2 byte values decode in a small always-inlined front (the full nested chain is ~371 bytecodes, over C2'sFreqInlineSize, so it is never inlined — routing short varints through it costs a call per read and measured as a net loss); 3+ byte values fall into the chain, whose decode amortizes the call.LightProtoCodec.readVarInt64()routesAbstractByteBufinstances (heap, pooled/unpooled direct) to the unchecked reader and everything else to the existing checked chain, which moved to its own method so the dispatch always inlines. Only 64-bit varints take this path — tags, lengths and 32-bit varints measured faster on the canonical checkedreadByte()pattern.parseFrom()restores the throw-on-truncated contract with one post-loopreaderIndex > endIdxcheck — emitted only for messages that parse 64-bit varint fields (including via map key/value types). Every other reader is bounds-checked, so all other messages keepparseFrom()byte-identical to the fully checked version (an unconditionally emitted check measurably perturbed codegen of small messages).Benchmark results
Interleaved A/B against current master (alternating single-fork runs x6, JDK 26, Apple M-series; means of 6 rounds x 5 iterations):
MessageMetadata deserialize is now ~5.6x protobuf-java (23.1 vs 4.1 ops/us).
Safety on malformed input
IndexOutOfBoundsException(TruncatedInputTestcovers truncation at every wire type, on heap and pooled-direct buffers). Heap buffers degrade to anArrayIndexOutOfBoundsExceptionat the array edge at worst; pooled direct buffers stay inside the arena chunk.-Dio.netty.buffer.checkBounds=falsehas globally, here narrowed to one read type on malformed input).Caveat
The helper class makes the generated code share the
io.netty.bufferpackage: fine on the classpath (Pulsar's deployment model), but a split package under the JPMS module path.Commit history preserves the experiment sequence (gated -> gateless -> varint64-only -> inlining/conditional-check fixes); squash-merge collapses it.