Repository navigation
perf(labels)!: encode the labels snapshot by hand, drop protobuf-java - #363
Conversation
ea9008e to
f058e31
Compare
The agent pulled in the whole protobuf-java runtime, shaded it into
pyroscope.jar and exposed it as an `api` dependency, all to encode one
small message in one place and never decode anything:
message Context { map<int64,int64> labels = 1; }
message LabelsSnapshot {
map<int64, Context> contexts = 1;
map<int64, string> strings = 2;
}
Vendoring it also meant vendoring its sun.misc.Unsafe use, which trips
JEP 498's terminally deprecated warning on JDK 25 and cannot be excluded
or upgraded downstream because it is relocated (grafana#361).
LabelsSnapshotEncoder writes that message directly into one growable
byte[]: contexts are emitted while the string table is being built, then
the table is appended. Nested lengths are computed exactly before writing
instead of being back-patched with a padded varint, so output stays
byte-identical to protobuf-java's and each context needs a single
capacity check. Strings are interned in an open-addressed table with no
boxing, and ASCII (nearly all label keys and values) is written straight
into the buffer; anything else goes through String.getBytes(UTF_8), which
is exactly what protobuf-java falls back to, so even unpaired surrogates
encode identically.
JfrLabels.LabelsSnapshot keeps its name and toByteArray(), now holding
the encoded bytes, so Snapshot.labels and external Exporter
implementations still compile. protobuf-java stays as a test-only
dependency: the tests compare against golden byte vectors, cross-check
2000 randomized fixtures byte for byte against protobuf-java, and round
trip through its parser.
Incidentally fixed along the way: dump() handed out string id
`size() + 1` over a weakly consistent ConcurrentHashMap iteration, so a
dump racing registerConstant could reuse a live id; ids are now seeded
by maximum. The exporter also no longer NPEs on a null labels snapshot,
and sends the encoded prefix instead of copying it.
BREAKING CHANGE: io.pyroscope.labels.pb.JfrLabels.LabelsSnapshot is no longer a
protobuf generated message, and protobuf-java is no longer on the compile
classpath of anyone depending on io.pyroscope:agent. Snapshot.labels and
LabelsWrapper.dump() keep their type and their toByteArray(), so code that only
forwards or serializes the snapshot needs no change. Code that used the
generated-message API on it - newBuilder, toBuilder, parseFrom, getContextsMap,
getStringsMap, writeTo, getSerializedSize - or the JfrLabels.Context type has to
be rewritten; toString() is no longer protobuf text format. Anything compiled
against the old jar that hands the snapshot to a protobuf API breaks at runtime
too, since it no longer implements MessageLite.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds an end-to-end multipart test for the JFR upload path: the labels part is now sent straight out of the encoder's backing buffer, which can be longer than the encoded message, so assert the server receives exactly the [0, size()) prefix - and that a null or empty snapshot omits the part instead of throwing. Also pins the string table's constant seeding: a dump racing registerConstant can see a sparse constants map, and the ids handed out afterwards must continue past the highest seeded one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The test decoder was 200 lines of parsing logic that could itself be wrong, standing next to a real protobuf implementation that is already a test dependency. Assertions now go through pbref parseFrom. The one thing the strict reader did that protobuf's lenient parser does not is reject a non-minimal varint - protobuf reads 0x86 0x00 as 6 without complaint - and that check is worth keeping, because padded length varints are how async-profiler's writer works and drifting into them would silently inflate every upload. Rather than parse by hand for it, parseCanonical() re-serializes the parsed message and compares: protobuf reproduces the input byte for byte only if the input already looked like its own output. paddedLengthVarintParsesButIsNotCanonical() pins that the check has teeth. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…oder The encoder was one class holding three unrelated things: a growable byte buffer, an open-addressed string interning table, and the knowledge of what jfr_labels.proto looks like. Eight mutable fields, nineteen methods, and no way to test or replace any one part. Now ProtoBuffer owns the bytes and the varint primitives, StringTable owns interning and id assignment, and LabelsSnapshotEncoder owns only the schema: field numbers, nesting, record sizes. Behaviour is unchanged, including the constant-seeding semantics. The split had to not cost anything, because the hot path's whole trick is one capacity check per record and then raw stores into hoisted locals. So ProtoBuffer exposes reserve/array/commit rather than a method per byte, and callers keep writing into a local `byte[] b` with a local position. Measured on the two stress-test shapes, 2 forks x 20 iterations each: 160k contexts x 2 labels 34.654 -> 33.193 ms/op (+-2.2, +-1.9) 10240 contexts x 20 labels 27.959 -> 26.718 ms/op (+-1.7, +-1.8) allocation, both shapes 20894558 -> 20894570 B/op Both timings landed nominally faster with heavily overlapping error bars, so read it as no measurable change. The +12 B/op is the two extra object headers, against 21 MB per dump. Also drops a stringId() delegate that nothing called. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
StringTable.seed() resized the lookup index to fit the constants alone, which threw away the capacity the constructor was given. Since dump() seeds every registered constant before interning anything, a table sized for the previous dump's ~20k strings was replaced by a 64-slot one on every dump that had any constant registered, then re-grown across ~10 reallocations with a full reindex each. Roughly 0.5 MB of avoidable garbage and ~50k extra reinsertions per dump - small against a ~10 MB target, but it silently defeated the sizing hint the encoder is built around. Seeding now takes the larger of the two sizes, and rebuilds the index from strings[] so it stays correct whichever branch it takes. Worth noting why this survived review and benchmarking: the JMH dump benchmark clears CONSTANTS during setup, so seed() returned early and the path was never measured. A constants parameter on that benchmark would be a reasonable follow-up. StringTableTest covers it, and fails on the previous behaviour with expected 32768 but was 64. Those tests are only possible because the table is its own class now. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Follow-ups from review, none of them reachable from dump() today but all of them cheap to close now that the pieces are separate classes. reserve() tested `pos + n > buf.length`, which overflows int near 2 GB and silently skips the grow, turning the intended "snapshot too large" IllegalStateException into an ArrayIndexOutOfBoundsException. Compares against the remaining space instead. The encoder is now explicitly single use. finish() hands over the buffer and the string table keeps the ids it assigned, so a reused instance would emit a snapshot carrying the previous batch's ids - silently wrong labels. Reuse throws instead. finish() trims the buffer when it has real slack. Growth can leave a buffer up to twice the encoded length, and the snapshot is retained until the exporter has finished uploading, so up to ~2x the payload stayed live for the duration. Trimming only above 25% of capacity and 1 MB, so the steady state (~12% slack from the size hint) never pays a copy. varintSize(int) returns 5 for a negative value where putVarint would write 10. Not reachable - every id is 1-based and positive - but the contract is now documented and asserted rather than implied, since it became a static on ProtoBuffer. Tests: the randomized protobuf cross-check now seeds constants on half its fixtures, with id gaps and with constants that labels also use, so the seeded path is covered byte-for-byte and not just by two handwritten cases. Plus an explicit single-use test, and a note on LabelsSnapshots recording that parseCanonical leans on protobuf re-serializing maps in wire order - an implementation detail that would fail loudly, not silently, if a protobuf upgrade changed it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The trim added in the previous commit cost more than it saved. It allocates and copies the whole payload on any dump whose buffer had grown, and the final benchmark caught the consequence: on the non-ASCII shape allocation went from 67.2 MB to 76.8 MB per encode, i.e. above protobuf-java's 75.0 MB, wiping out the allocation win on that workload. It also contradicted the surrounding design. LabelsSnapshot exposes buffer() and size(), and PyroscopeExporter sends the encoded prefix rather than a copy, precisely so a multi-megabyte payload is never copied on the way out; adding a copy in finish() gave that back. The concern it addressed is real but smaller: growth can leave the buffer up to twice the encoded length, retained until the upload completes. What actually keeps that slack down is the sizing hint in dump(), which lands within ~12% in the steady state - the reverted threshold could not even fire there. Both facts are now documented on take() and finish() so this does not get re-added. Measured, hand-written, 10240 contexts x 20 labels: ASCII 28.563 ms/op, 20894564 B/op (trim never fired here) UTF8 39.193 ms/op, 67160388 B/op (was 76849439 with the trim) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three related fixes from review. writeStringEntry had no overflow guard at all, unlike writeContext. A ~2 GB label string overflows the computed body length negative, and reserve() then compares n > buf.length - pos, which is false for a negative n, so the write proceeds past the end and throws ArrayIndexOutOfBoundsException instead of the intended "too large to encode". writeContext's guard was itself unreachable in the configuration that matters: it checked body < 0 only after passing body to varintSize(int), whose assert fires first under -ea - which is exactly how the Gradle test JVM runs. The clean IllegalStateException was dead code under assertions. Both are fixed by accumulating the sizes in long, where overflow cannot happen for any possible input, and checking once against ProtoBuffer.MAX_LENGTH before narrowing to int. That removes the ordering hazard rather than reshuffling it, and the arithmetic is not measurable: allocation identical, timings +5.3% and -4.0% on the two stress shapes with overlapping error bars. finish() also returns the shared EMPTY snapshot when nothing was encoded. An app with no dynamic labels dumps zero bytes every cycle, and the snapshot stays reachable from the export queue until the upload completes, so it was carrying a few kilobytes of zeros through it for nothing. No copy involved, so this is not the buffer trim that was reverted in 1d4ab7b. Also documents on the holder that it no longer has the value equality the generated message had, and why comparing buffer() directly would be wrong past size(). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
0058999 to
cdc8cac
Compare
Benchmarked against
|
| shape | protobuf 4.36.1 | hand-written | |
|---|---|---|---|
| 160k contexts × 2 labels, ASCII | 168.65 ± 10.47 ms · 179.9 MB/op | 33.03 ± 2.59 ms · 20.9 MB/op | 5.1× · 8.6× less garbage |
| 10240 × 20, ASCII | 131.53 ± 7.12 ms · 77.5 MB/op | 27.74 ± 1.84 ms · 20.9 MB/op | 4.7× · 3.7× less garbage |
| 10240 × 20, non-ASCII values | 151.76 ± 10.56 ms · 104.1 MB/op | 39.62 ± 2.22 ms · 67.2 MB/op | 3.8× · 1.5× less garbage |
The real dump() — which reuses a sizing hint from the previous dump and so allocates its buffer
once — comes in at 20.1 ms / 10.1 MB for 10240×20 constant contexts and 31.0 ms / 11.2 MB
for 160k span contexts.
Side finding: what the 4.36.1 upgrade cost on its own
Same benchmark, protobuf only, 4.33.5 vs 4.36.1:
| shape | time | allocation |
|---|---|---|
| 160k × 2 ASCII | 164.93 → 168.65 ms (+2.3%) | 175.8 → 179.9 MB (+2.3%) |
| 10240 × 20 ASCII | 135.78 → 131.53 ms (−3.1%) | 71.5 → 77.5 MB (+8.4%) |
| 10240 × 20 UTF8 | 151.46 → 151.76 ms (+0.2%) | 75.0 → 104.1 MB (+38.8%) |
Time is unaffected — every delta sits inside overlapping error bars, so #365 costs nothing
measurable in throughput. Allocation, however, went up in all three shapes, by 29 MB per encode on
the non-ASCII one. That is consistent with the mechanism #365 relies on: with the full gencode path
no longer using UnsafeUtil, UTF-8 encoding falls back to the safe processor and allocates
intermediates. Unlike the timings, these figures have tight error bars (±20–30 bytes), so the
direction is solid.
Not an argument against #365 — it fixed the reported problem with a one-line change and no
throughput cost. Just worth knowing that label-heavy non-ASCII workloads now allocate noticeably
more per upload than they did on 4.33.5.
Shipped jar, both minimize()d
| main (#365) | this PR | |
|---|---|---|
pyroscope.jar |
5,657,995 B | 3,834,866 B (−32%) |
| classes | 1988 | 1247 |
| protobuf entries | 733 | 0 |
Reading these numbers honestly
- The
encodebenchmark builds a fresh encoder with a cold 64 KB buffer every op, so it pays the
full growth series. That is why the realdump()measures lower than the encode-only figure it
nominally contains. - Hand-written allocation is near-identical (20,894,562 vs 20,894,579 B/op) across two very
different fixtures. That is real: per-op allocation is dominated by growth-series buckets, not
input size, and both shapes land in the same buckets. - The protobuf side includes its final
toByteArray()copy, which is what the old production code
did (snapshot.labels.toByteArray()in the exporter), so it belongs in the comparison — ~1–2 ms
of the 168. - The non-ASCII row is why
writeStringEntryhas an ASCII fast path: routing every string through
String.getBytes(UTF_8)holds allocation at ~67 MB/op instead of dropping to ~21 MB. - Whole suite (119 tests) passes with the test-only protobuf dependency on 4.36.1, including the
byte-for-byte cross-check and the canonicality assertion — both of which depend on protobuf's map
iteration order, so the version bump was a real risk to them and it did not materialise.
Benchmark sources, raw JMH output and instructions to restore the :bench module:
https://github.com/proxy/gist.github.com/korniltsev-grafanista-yolo-vibecoder239/5bdc89b469381d72ff2d29cba62a4d44
🤖 Generated with Claude Code
Relates to #361, which is already fixed on main by #365 (protobuf-java 4.36.1). This PR is
therefore not a fix for that issue any more — it is a different trade for the same code: remove the
dependency instead of upgrading it. Rebased onto main with #365 in it, and the test-only protobuf
dependency now tracks 4.36.1.
Warning
Breaking change.
JfrLabels.LabelsSnapshotis no longer a protobuf generated message, and consumers no longer getprotobuf-javaon their compile classpath. See Breaking change for what breaks and what to do about it. The commit is markedperf(labels)!with aBREAKING CHANGE:footer, so release-please will cut a major release (2.9.x -> 3.0.0).protobuf-javawas a shipped, shaded,api-scoped dependency that existed to encode one small message, in one place, and never decoded anything:Vendoring the runtime also meant vendoring its
sun.misc.Unsafeuse, which trips JEP 498's terminally deprecated warning on JDK 25 — and because it is relocated, downstream consumers can neither exclude nor upgrade it (#361).LabelsSnapshotEncodernow writes that message directly into one growablebyte[].JfrLabels.LabelsSnapshotkeeps its name andtoByteArray(), holding the encoded bytes, soSnapshot.labelsand externalExporterimplementations still compile unchanged.What #365 already achieved, and what is left
#365 stopped the JDK 25 warning by upgrading to protobuf-java 4.36.1, whose full (non-lite) gencode
path no longer routes through
UnsafeUtil. I verified that independently while benchmarking: a jarbuilt with 4.33.5 emits
sun.misc.Unsafe::arrayBaseOffset has been called by …protobuf.UnsafeUtil$MemoryAccessoron encode, and a jar built with 4.36.1 is silent. So thewarning is gone on main, and nothing in this PR is needed to keep it gone.
What is left is the cost of keeping the runtime at all: ~1.8 MB and 733 protobuf classes shaded into
pyroscope.jar, protobuf on the compile classpath of every consumer, and an encode path that is3.8-5.1x slower and allocates 1.5-8.6x more than encoding the message directly. Measuring it also
turned up that the upgrade itself increased protobuf's allocation, by 2-39% depending on shape,
presumably because the full path now uses the safe (non-
Unsafe) processor — see the benchmarkcomment on this PR.
Breaking change
Snapshot.labelsandPyroscope.LabelsWrapper.dump()keep their type and theirtoByteArray(), so code that only forwards or serializes the snapshot needs no change — including customExporterandProfilerDelegateimplementations, which is the common case.What breaks:
JfrLabels.LabelsSnapshotGeneratedMessagebyte[]newBuilder(),toBuilder(),parseFrom()getContextsMap(),getStringsMap(),getStringsCount()writeTo(OutputStream),getSerializedSize()MessageLitetoByteArray(),size())JfrLabels.ContexttoString()LabelsSnapshot{size=N}equals()/hashCode()com.google.protobuf:protobuf-javaapi)toByteArray()yourself againstagent/jfr_labels.proto, or add your ownprotobuf-javadependency and generate the message.LabelsSnapshotno longer implementsMessageLite.protobuf-javafor your own protobuf use, declare it directly. Note that it was only ever usable when compiling against the unshaded artifact; in the published shaded jar it was relocated toio.pyroscope.vendor.com.google.protobuf, which is precisely why Deprecated sun.misc.Unsafe usage in vendored protobuf on JDK 25 #361 could not be worked around downstream.New on the holder, for anyone who needs the bytes without a copy:
size(),isEmpty(),buffer()(the backing array, valid over[0, size())).Performance
JMH, both encoders in the same JVM over the same fixture, JDK 17,
-f 1 -wi 3 -i 5 -prof gc. Shapes are taken from the existing stress tests inLabelsTest. The benchmark module was removed before merging — the benchmark sources, the full JMH output and instructions to restore the module are here: https://github.com/proxy/gist.github.com/korniltsev-grafanista-yolo-vibecoder239/5bdc89b469381d72ff2d29cba62a4d44 (unlisted gist). Numbers below were measured on a VM, so the ratios are more meaningful than the absolute times.The real
dump(), which reuses sizing hints from the previous dump and so allocates its buffer once, does better than the encode-only figures above: 20.1 ms/op and 10.1 MB/op for 10240x20 constant contexts, 31.0 ms/op and 11.2 MB/op for 160k span contexts.The non-ASCII row is also the justification for the ASCII fast path: with everything routed through
String.getBytes(UTF_8), allocation stays at ~67 MB/op instead of dropping to ~21 MB.Shipped jar,
minimize()d both times:pyroscope.jarThe published POM and Gradle module metadata are unchanged — the shadow publication never declared dependencies, which is exactly why #361 could not be worked around downstream.
How the encoder works
Three package-private classes, so each piece can be read, tested and swapped on its own:
ProtoBuffer(the growable byte buffer and varint primitives),StringTable(interning and idassignment), and
LabelsSnapshotEncoder(the schema — field numbers, nesting, record sizes).The buffer deliberately exposes
reserve/array/commitrather than a method per byte, so arecord still costs one capacity check and callers still write through hoisted locals; splitting it
that way measured as no change in time and +12 B/op in allocation.
src/converter/one/proto/Proto.java, and its C++ twinsrc/protobuf.h) reserves N bytes for a nested length and back-fills a padded varint. Our messages are tiny — a 2-label context is 20 bytes minimal — so padding the two nested lengths would add 20-40% to the payload and would break byte-comparability with protobuf-java. Instead, pass one interns the label strings into a reusableint[]and sums the exact sizes; pass two does a single capacity check and then plain stores. Borrowed fromProto.java: the flatbyte[] buf; int poslayout andnumberOfLeadingZeros-based varint sizing.int[]of ids plus a denseString[]by id, with an identity check beforeequals(label keys come out of the sameLabelsSetarray, so the sameStringobject recurs). The table is emitted by a linear walk in ascending id order.String.getBytes(UTF_8)— deliberately, because that is what protobuf-java's own encoder falls back to for unpaired surrogates, so even malformed input encodes identically without us hand-rolling surrogate handling.Wire compatibility
The format is frozen — Grafana Pyroscope's ingest parses these bytes — so this is verified three ways, all of which were written and made to pass against protobuf-java first, before the encoder was swapped in:
LabelsWireFormatTest) for the base case, empty snapshots, empty label sets, empty strings, varint width transitions on keys and lengths, and non-ASCII.dump()output that the fixture-driven tests cannot reach (you cannot replayConcurrentHashMapiteration order through a reference encoder).LabelsSnapshots.parseCanonical()parses with protobuf and re-serializes, which reproduces the input byte for byte only if our encoding already looked like protobuf's own output — minimal varints, both fields of every map entry, fields in order. That catches drift protobuf's parser is happy to ignore: it reads a padded0x86 0x00length as6without complaint, and padded lengths are exactly how async-profiler's writer works.paddedLengthVarintParsesButIsNotCanonical()pins that the check has teeth.LabelsEncoderCompatTest). The corpus includes hash-colliding strings, unpaired surrogates, astral chars, embedded NULs, empty strings, ids at every varint boundary, and — on half the fixtures — seeded constants with gaps in their ids and constants that labels also reference, so the seeding semantics this PR changes are covered byte-for-byte rather than by hand-written cases alone.protobuf-javais kept as atestImplementationdependency for exactly this, with the generated code repackaged toio.pyroscope.labels.pbrefso it cannot shadow the hand-written holder on the test classpath. All decoding in the tests goes through it — there is no hand-written parser to maintain on the test side either. Nothing protobuf ships at runtime, andShadowJarContentsTestnow asserts that.One intentional difference: a
LabelsSetbuilt with the same key twice emits both entries instead of paying a quadratic duplicate scan per context. Proto map merge semantics mean the decoded result is identical to protobuf's;repeatedKeysDecodeToTheLastValue()pins that.Incidental fixes
dump()handed out string idsize() + 1while iterating the weakly consistentCONSTANTSmap, so a dump racingregisterConstantcould reuse a live id and mislabel data. Ids are now seeded by maximum.PyroscopeExporterno longer NPEs on a null labels snapshot in the JFR path, and sends the encoded prefix out of the buffer instead of copying it.StringTable.seed()used to resize the lookup index to fit the registered constants alone, discarding the capacity carried over from the previous dump — so any app that callsregisterConstant()re-grew the index from 64 slots on every dump (~0.5 MB and ~50k extra reinsertions). Caught in review; the JMH benchmark missed it because it clearsCONSTANTSin setup.Notes
labelspart still is not covered by the Go integration tests — the demo app they run uses no dynamic labels. Worth a follow-up; I did not add it here because there is no Go toolchain in my environment to verify it../gradlew buildgreen (119 tests),:agent:javadocgreen, JDK 17.make itestnot run locally (no Go toolchain) — leaving that to CI.🤖 Generated with Claude Code