Make message clear() O(1) with presence-guarded getters - #11
Merged
Merged
Conversation
parseFrom() begins with clear(), which physically reset every field: 13-23% of deserialization time in CPU profiles (a generated MessageMetadata.clear() executed ~60 field stores per parse). clear() now only resets presence state (bitfields, oneof cases, repeated counts, _cachedSize, _parsedBuffer): - Presence bits are tracked for all non-repeated, non-oneof fields; proto3 implicit-presence fields get an internal bit (no has() method) marking "written since last clear". - Getters return the field default while presence is unset, so stale values from previous parse/set cycles are unobservable. - String/bytes fields invalidate their cached decoded value in parse() instead of clear(), paying the cost only for fields present on the wire. - equals()/hashCode() mask bitfields to explicit-presence bits so that proto3 "set to default" remains indistinguishable from "never set". - copyFrom() guards implicit-presence fields on the source's presence bit (protobuf merge semantics). - Repeated message and map clear() reset only the element count; the parse path adds elements without clearing (parseFrom clears the child itself), while the public addX() clears reused pooled instances. This also fixes two latent bugs, now covered by InstanceReuseTest: - clearX() on a message field cleared the presence bit before the has()-guarded child clear, leaving a stale child reachable via getX(). - copyFrom() overwrote target fields with unset implicit-presence source values. Deserialization throughput (JMH, Apple M-series): AddressBook 19.4 -> 26.3 ops/us (+35%) Pulsar MessageMetadata 14.3 -> 17.4 ops/us (+22%) Simple + readString 38.6 -> 51.3 ops/us (+33%) Pulsar BaseCommand 33.2 -> 34.2 ops/us (+3%)
There was a problem hiding this comment.
Pull request overview
This PR updates the Java code generator to make Message.clear() effectively O(1) by resetting only presence/state while ensuring stale pooled values remain unobservable via presence-guarded read paths. It also adds a comprehensive regression test suite to validate instance reuse safety across parse/clear cycles.
Changes:
- Introduces internal presence bits for proto3 implicit-presence scalars and updates generated getters/materialize paths to return defaults when absent.
- Moves per-field invalidation of cached decoded
String/ByteBufreferences into the parse path and reduces per-element clearing for repeated/map fields. - Adds
InstanceReuseTestto cover reuse hazards (wide→narrow reparses, stale lazy-decode caches, repeated/map pooling, oneof switching, equality semantics, copy/merge behavior).
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/src/test/java/io/streamnative/lightproto/tests/InstanceReuseTest.java | New regression tests validating pooled instance reuse correctness under the new clear/presence model. |
| code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoField.java | Generates presence masks for all non-repeated/non-oneof fields and introduces presenceCondition() used by guarded getters/materialize. |
| code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoMessage.java | Sets internal presence bits during parse, masks bitfields for equals/hashCode proto3 semantics, and changes implicit-presence copyFrom() behavior. |
| code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoNumberField.java | Adds presence-guarded getters; adjusts equals/hashCode to avoid stale values for implicit-presence numerics. |
| code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoBooleanField.java | Adds presence-guarded getter and stale-safe equals/hashCode for implicit presence. |
| code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoEnumField.java | Adds presence-guarded getter and stale-safe equals/hashCode for implicit presence enums. |
| code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoStringField.java | Guards getter/materialize by presence; invalidates cached decoded string on parse. |
| code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoBytesField.java | Guards getters/materialize by presence; invalidates cached buffer reference on parse. |
| code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoRepeatedStringField.java | Stops per-element clear; drops stale decoded string in pooled holder on parse. |
| code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoRepeatedBytesField.java | Stops per-element clear; drops stale buffer reference in pooled holder on parse. |
| code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoRepeatedMessageField.java | Avoids double-clearing pooled children by using an internal add-for-parse path; ensures public add clears before handing out. |
| code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoMapField.java | Resets pooled message-value slots during parse and removes per-entry clear in map clear(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
211
to
+218
| if (f.isRepeated()) { | ||
| f.copy(w); | ||
| } else if (f.field.hasImplicitPresence()) { | ||
| // Always copy for proto3 implicit presence — no has() to check | ||
| // No public has() — guard on the other message's internal presence bit, | ||
| // since its field value may be stale when the bit is unset. | ||
| w.format(" if ((_other._bitField%d & %s) != 0) {\n", f.bitFieldIndex(), f.fieldMask()); | ||
| f.copy(w); | ||
| w.format(" }\n"); |
Collaborator
Author
There was a problem hiding this comment.
Good catch — fixed in e4a02aa. The copy condition for implicit-presence fields now also requires the source value to be non-default (same expression as the serialization condition), matching protobuf-java's mergeFrom behavior. Added testCopyFromTreatsSetToDefaultAsUnset to pin it down.
Review feedback: copyFrom() guarded implicit-presence fields only on the source's internal presence bit, so a field explicitly set to its default (e.g. setIntField(0)) would overwrite a non-default target value. proto3 merge semantics treat default == unset (protobuf-java mergeFrom checks `other.getX() != 0`), and the field would not survive a serialize/parse roundtrip either. The copy condition now also requires the source value to be non-default, matching the serialization condition.
This was referenced Jul 30, 2026
Merged
merlimat
added a commit
that referenced
this pull request
Sep 1, 2026
#19) * Release data references in clear() for messages above CLEAR_RETAIN_MAX Since #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. * Tighten the clear() release gate to a single branch, only where needed 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.
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
Messages are designed to be pooled and reused, and
parseFrom()begins withclear()— which physically reset every field back to its default. In CPU profiles of deserialization this is 13–23% of total time: a generatedMessageMetadata.clear()executed ~60 field stores before every parse, even when the incoming message only sets a handful of fields.Changes
clear()now only resets presence state — bitfields, oneof cases, repeated counts,_cachedSize,_parsedBuffer— and stale field values are made unobservable instead of being reset:has()method is exposed) that marks "written since last clear".MessageMetadata.clear()drops from ~60 stores to 6.parse(): the cached decodedString/ByteBufreference is dropped when the field is present on the wire, so the cost is paid per present field instead of per declared field.equals()/hashCode()mask bitfields to explicit-presence bits (_PRESENCE_CMP_MASK), preserving proto3 semantics: "explicitly set to the default" equals "never set".copyFrom()guards implicit-presence fields on the source's presence bit, matching protobuf merge semantics.clear()reset only the count. The parse path adds elements via an internal_addXForParse()that skips clearing (the child'sparseFrom()clears it anyway — previously each reused child was cleared twice per cycle), while the publicaddX()clears reused pooled instances before handing them out.Latent bugs fixed (caught by the new
InstanceReuseTest, verified failing on the previous generator)clearX()on a message field cleared the presence bit before thehas()-guarded child clear, so the child was never cleared andgetX()returned stale data.copyFrom()copied implicit-presence fields unconditionally, overwriting target fields with unset (default) source values.Results
JMH deserialization throughput (Apple M-series, higher is better):
Zero steady-state allocation on the parse path is unchanged (verified with
-prof gc, ≈10⁻⁴ B/op).One tradeoff to be aware of: since values are no longer physically reset, a cleared pooled instance can keep a previously-set
String/ByteBufreferenced until the next set/parse of that field. This is never observable through the API (every read path is presence-guarded), only a memory-retention consideration comparable to the existing_parsedBufferbehavior.Verification
InstanceReuseTestcovers the reuse hazards directly: wide→narrow reparse, stale decoded strings, pooled repeated/map element reuse, oneof case switching,materialize()on absent fields, proto3 set-to-default vs unset equality, andcopyFrommerge semantics.