Repository navigation
perf: parser clean-ups and direct nested parse calls - #997
jasperpotts wants to merge 10 commits into
Conversation
Following docs/fast-codegen-plan.md section 6.2, the generated stream parser: - looks up the field number and Schema.getField only for unknown fields, in defaultCase, instead of before the switch for every field; - parses nested messages without a try/finally per field: the limit is restored inline on success, and the final Codec.parse restores the caller's limit once if parsing fails; - calls a new public static parseFrom(...) on nested codecs generated in the same build, a static, monomorphic call (section 6.1 item 3, applied to parsing). Dependencies keep X.PROTOBUF.parse. Map entries now pass their own tag to defaultCase; before, they passed the enclosing message's tag. Fixes #993 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Jasper Potts <1466205+jasperpotts@users.noreply.github.com>
… entries Review follow-ups for the parser clean-ups: - map entries no longer wrap their parse loop in try/finally to restore the limit, like nested messages: the limit is restored inline on success, and Codec.parse restores the caller's limit on failure; ParseLimitRestoreTest gains a corrupted map-entry case; - MapEntryUnknownFieldTest covers the bug this PR fixes: an unknown field inside a map entry is now skipped (or kept, or rejected in strict mode) instead of always failing with "Bad tag", and a key with the wrong wire type is rejected; - ByteArrayWriterGenerationTest checks that the parser calls nested codecs from the same build directly and dependencies through their Codec. Refs #993 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Jasper Potts <1466205+jasperpotts@users.noreply.github.com>
|
Review follow-ups. This PR also fixes a bug that exists on The bug. In the generated map-entry parse loop, the FIXes:
The map change affects map-heavy messages only. HAPI block items, transactions and records have no maps, so I didn't re-measure them. |
…anups Signed-off-by: Jasper Potts <1466205+jasperpotts@users.noreply.github.com>
…anups Signed-off-by: Jasper Potts <1466205+jasperpotts@users.noreply.github.com>
parseFrom is public and static so generated codecs can call each other directly, but on failure it left the input's limit narrowed to a nested message and relied on Codec.parse to restore it. A direct caller reusing its buffer would then read with the wrong limit. parseFrom now restores the limit it was called with on any failure, which costs one limit() read per message. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Jasper Potts <1466205+jasperpotts@users.noreply.github.com>
…anups Signed-off-by: Jasper Potts <1466205+jasperpotts@users.noreply.github.com>
| try { | ||
| return parseImpl(input, strictMode, parseUnknownFields, maxDepth, maxSize); | ||
| } catch (final ParseException | RuntimeException e) { | ||
| input.limit(limit); |
There was a problem hiding this comment.
Minor: doing this in finally could be more robust, regardless of what the parseImpl does or throws.
What and why
These are parser clean-ups for the generated stream parser
parseImpl(ReadableSequentialData, …), which serves every PBJ parse (docs/fast-codegen-plan.mdsection 6.2):Performance (AMD EPYC 9275F): every PBJ parse is 5-11% faster, streams included: block items from
Bytes10% (612 → 551 ns), whole blocks 5.5% (byte array) to 11% (input stream). Allocation unchanged. Tier A, on by default.tag >>> 3and calledXxxSchema.getField(field)before theswitch. NowdefaultCasedoes that itself, and only fields the parser doesn't recognize reach it.try/finallyper nested message. The generated code checks the length once, narrows the limit, parses, restores the limit and checks the position. If a nested parse throws, the limit is not restored at each level. Instead, the finalCodec.parse(…)restores the caller's limit once before rethrowing. Callers see the same input limit as before, on success and on failure.public static T parseFrom(…), and nested messages generated in the same build call it directly. Before, they went throughX.PROTOBUF.parse(…)→ the virtualparseImpl, the same megamorphic call site perf: generate straight-line byte array writers with direct nested calls #995 removed for writes. Dependencies keepX.PROTOBUF.parse.A small fix on the side. Map entries now pass their own tag to
defaultCase. Before, they passed the enclosing message's tag, which was wrong for unknown fields inside a map entry.Generated code:
BlockItembefore and afterBefore (every field):
After:
Tests
ParseLimitRestoreTest(new): after a nested parse fails, the caller'sBufferedDatalimit is what it was before the call, and repeated parses of the sameBytesbehave the same.pbj-core:spotlessApply qualityGate buildpasses.pbj-integration-tests:./gradlew buildand-PpbjPerformance=maxboth pass, 326,325 tests each. That includesMaxDepthTest,MaxSizeTest,UnknownFieldsTest,MalformedMessageTest,TruncatedDataTests,ParserNeverWrapsTest, the fuzz tests, the non-canonical-order tests, andAllocationTest's parse ceilings.Performance: AMD EPYC 9275F, before = #996's branch, after = this PR
3 forks, 3 × 1 s warmup, 5 × 1 s measurement,
-prof gc.Bytes, and whole blocks 5.5% (byte array), 7% (direct buffer) and 11% (input stream). Every Hiero parse, including streams, gets this.ReadableSequentialDataand in allocatingOneOf,Bytesand lists. The opt-in byte-array parser (plan PRs 6-7) and unboxed oneofs (PR 11) address those.Generated code size (full HAPI build)
mainmain)mainclasses*ProtoCodecclassesStacked on #996.
Fixes #993
🤖 Generated with Claude Code