Release data references in clear() for messages above CLEAR_RETAIN_MAX - #19
Merged
Merged
Conversation
Since streamnative#11, clear() is O(1): it resets counts and presence bits but leaves data references in place (cached Strings in StringHolders, ByteBuf refs in BytesHolders, singular string/bytes values). A reused message instance — one per connection in Pulsar's PulsarDecoder, thread-locals in Commands — therefore pins the last message's data until the same field is overwritten. For multi-MB messages this is a leak-shaped retention: in the proxy back-pressure test every connection that ever parsed the ~4.6 MB topic-list response kept it on the decoder's BaseCommand, ~900 MB across 200 connections, OOMing the run even with the write-side scratch fix in place. clear() now gates on the previous message's size, which it already knows in O(1): _cachedSize is maintained by parseFrom() and getSerializedSize(). At or below CLEAR_RETAIN_MAX (64 KiB) nothing changes — the O(1) clear runs bit for bit as before, retaining at most that much per instance. Above it (or at -1: mutated since, unknown fields on the wire, or already cleared — over cleared fields the walk touches nothing), clear() takes a generated _clearAndRelease() path that nulls the retained references and recurses into nested messages. The recursion is forced — children do not re-check their own size gate — so a large message spread across many small children (each below the threshold) still releases everything. The release walk costs O(element count), which is noise for any message large enough to trigger it. The gate sits on the whole-message size rather than per field or per holder: per-holder gating misses many-small-elements aggregates (8192 x 560-byte topic names), and per-node gating misses fan-out shapes. ClearReleaseTest covers both sides via WeakReferences: release of built, parsed-and-materialized, fan-out-nested and bytes-payload data above the threshold; retention (the deliberate O(1) behavior) below it; the conservative -1 path; and byte-identical reuse after a deep clear.
Messages with no reference-bearing fields (numbers/enums/bools only, e.g. nested coordinate/child types) skip the gate entirely — their release path is behaviorally identical to the plain clear, so their clear() compiles byte-identical to the pre-gate version. This matters because nested-child clear() runs once per child per parse on hot paths. Where the gate remains, the two comparisons (_cachedSize > CLEAR_RETAIN_MAX || _cachedSize < 0) fold into one unsigned compare: -1 is huge unsigned, so Integer.compareUnsigned covers the conservative unknown-size case in the same branch. Interleaved JMH (3 alternating rounds vs 0.8.0) showed the two-branch gate costing 2-5% on ~15-75 ns parse loops; this recovers most of it.
merlimat
added a commit
that referenced
this pull request
Sep 1, 2026
…20) * Add large-message serialize benchmark and non-array target identity sweep LargeMessageBenchmark serializes the Pulsar topic-list shape from 600 B to 4.6 MB, a varint-dense repeated-int64 message at 2 KB and 8 KB, and a 2 MB bytes payload into pooled direct buffers. NonArrayTargetIdentityTest checks that writeTo() to direct, offset and multi-component composite targets is byte-identical to the heap-array path for sizes swept byte by byte across every plausible internal boundary, for built and parsed messages and across repeated writes. * Write direct buffers in place through their NIO view above 512 bytes Since #12, writeTo() to a non-array buffer stages the whole message in a heap byte[] scratch and bulk-copies it. Messages above SCRATCH_RETAIN_MAX (1 MiB) never retain that scratch, so every write allocated a fresh full-size array — multi-MB G1-humongous allocations that OOMed Pulsar's proxy back-pressure test (apache/pulsar#26256, together with the clear() retention fixed in #19). Below the cap the copy itself was still paid. A single-region direct buffer exposes its memory as a java.nio.ByteBuffer through ByteBuf.internalNioBuffer(). Absolute puts on a DirectByteBuffer compile to a bounds check plus a jdk.internal.misc.Unsafe store — which, unlike sun.misc.Unsafe, carries no JDK 24+ deprecation check — so the message can be written in place: no scratch array and no bulk copy, at any size. writeTo() now dispatches heap buffers in place through the backing array (unchanged), single-region direct buffers larger than NIO_WRITE_MIN (512 bytes) through the NIO view, and everything else (small messages; composites and other buffers without a single NIO region) through the scratch path as before. Above the threshold no direct-buffer write touches the scratch, so it only grows past 512 bytes for composite targets. The threshold exists because the view's per-put cost is a fixed tax per message while the copy it saves grows with size. Interleaved JMH on pooled direct buffers (JDK 21/26): the view is 15-19% slower on the ~70-byte varint-dense MessageMetadata, at parity on BaseCommand, 20% faster at 600 bytes, and 35-40% faster from 6 KB to 100 KB; on the 2 MB / 4.6 MB cases it removes the per-write allocation (-70% / -55%) and matches the per-field ByteBuf-API write-through of #18, which it replaces. The field emitters are parameterized over the write sink (WriteSink.ARRAY / WriteSink.NIO): one emitter produces both _writeTo(byte[], int) and _writeTo(ByteBuffer, int), differing only in the sink variable and in how bulk data is copied out of a ByteBuf; every raw writer in LightProtoCodec is overloaded for both sinks. NonArrayTargetIdentityTest sweeps sizes byte by byte across every boundary (64 B .. 1 MiB) on direct, offset and multi-component composite targets, for built and parsed messages, repeated strings incl. non-ASCII, bytes payloads, nested trees and the Pulsar BaseCommand shape. NioWriteTest checks the routing flips exactly at NIO_WRITE_MIN, that composites keep the scratch path, and (via ThreadMXBean.getThreadAllocatedBytes) that 5 writes of a 4.6 MB topic list or a 5 MB payload allocate less than a quarter of one message. LargeMessageBenchmark covers 600 B .. 4.6 MB topic lists, varint-dense 2 KB / 8 KB messages and a 2 MB payload.
11 tasks
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.
Problem
Since #11,
clear()is O(1): it resets counts and presence bits but leaves data references in place (StringHolder.s,BytesHolder.b, singular string/bytes values). A reused message instance therefore pins the previous message's data until the same field is overwritten.Pulsar keeps one
BaseCommandper connection inPulsarDecoder(plus thread-locals inCommands), so every connection that ever parsed a ~4.6 MB topic-list response kept it alive afterclear()— up to ~900 MB across the 200 connections ofProxyPatternConsumerBackPressureMultipleConsumersTest, which still OOMed with the write-side fix (#18) in place. This is the second of the two 0.8.0 regressions behind apache/pulsar#26256.Fix
clear()gates on the previous message's size, which it already knows in O(1):_cachedSizeis maintained byparseFrom()andgetSerializedSize().CLEAR_RETAIN_MAX(64 KiB): the O(1) clear runs bit for bit as before, retaining at most that much per instance.-1(mutated since, unknown fields on the wire, or already cleared): a generated_clearAndRelease()nulls the retained references and recurses into nested messages. The recursion is forced — children don't re-check their own size gate — so a large message spread across many small children (each below the threshold) still releases everything. The release walk is O(element count), which is noise for any message large enough to trigger it.The gate sits on the whole-message size rather than per field or per holder: per-holder gating misses many-small-element aggregates (8192 × 560-byte topic names), and per-node gating misses fan-out shapes.
The second commit tightens the cost on hot paths: messages with no reference-bearing fields (numbers/enums/bools only — e.g. small nested child types cleared once per parse) skip the gate entirely, since their release path is behaviorally identical to the plain clear; where the gate remains, the two comparisons fold into a single
Integer.compareUnsignedbranch (-1is huge unsigned, covering the conservative unknown-size case).Verification
ClearReleaseTest(WeakReference-based): release of built, parsed-and-materialized, fan-out-nested and bytes-payload data above the threshold; the deliberate O(1) retention below it; the conservative unknown-size path; and byte-identical reuse after a deep clear.BaseCommand/MessageMetadataserialize and deserialize benchmarks; worst consistent residual ≈ −2 % on ~15 ns synthetic microloops (one predicted branch inclear()).Independent of #18; the two merge cleanly in either order (verified by merge simulation plus the full suite on each branch).