Repository navigation
perf: generate straight-line byte array writers with direct nested calls - #995
jasperpotts wants to merge 8 commits into
Conversation
The generated write(T, byte[], offset) now writes each field inline instead of through a generic ProtoArrayWriterTools helper (docs/fast-codegen-plan.md section 6.1, items 1-5): - constant tags as byte literals; - proto3 default checks inline; oneof members always written, as before; - a new public static writeTo(data, output, startOffset, end) on each codec, called directly for nested messages generated in the same build, so the call is static and inlinable; messages from dependency jars keep the Codec instance call; - single-store varints via ProtoArrayWriterTools.writeVarInt, bounded by the message end; - each run of adjacent oneof alternatives written as one switch on the kind. Map entries, google.protobuf wrapper types and repeated scalars keep their helpers. Output bytes are unchanged. Fixes #989 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Jasper Potts <1466205+jasperpotts@users.noreply.github.com>
JUnit Test Report 551 files 551 suites 51s ⏱️ Results for commit 890bc4f. ♻️ This comment has been updated with latest results. |
Integration Test Report 623 files 623 suites 41m 31s ⏱️ Results for commit 890bc4f. ♻️ This comment has been updated with latest results. |
A field whose name the generated writer uses for its own locals (size, end, offset, data, item, ...) has its local renamed with a "Value" suffix. That name could clash with another field's local: a message with fields size and size_value did not compile. Renamed locals now get a numeric suffix until they are unique against the method's names and every field of the message. localNames.proto adds such pairs, so the generated tests compile and round-trip them. Refs #989 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Jasper Potts <1466205+jasperpotts@users.noreply.github.com>
Generated test data cannot build a repeated field of a message's own type, so the item fields use a small nested message instead. Refs #989 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Jasper Potts <1466205+jasperpotts@users.noreply.github.com>
|
Review follow-ups
Writes on this branch are now also faster from #994's bulk varint stores: 12-23% on block items, measured on the #997 code. |
…egen Signed-off-by: Jasper Potts <1466205+jasperpotts@users.noreply.github.com>
…egen Signed-off-by: Jasper Potts <1466205+jasperpotts@users.noreply.github.com>
…egen Signed-off-by: Jasper Potts <1466205+jasperpotts@users.noreply.github.com>
|
@anthony-swirldslabs This sounds very similar to #546. Somehow, during the implementation of that, it was decided it cannot be done in compatible way. I haven't really followed the original reason why it wasn't possible to be done and I wonder if it still applies here. |
|
|
||
| /** | ||
| * Build the lines that write every field in field-number order, with oneof alternatives that are adjacent in that | ||
| * order grouped into one switch on the oneof's kind. |
There was a problem hiding this comment.
@abies :
@anthony-swirldslabs This sounds very similar to #546. Somehow, during the implementation of that, it was decided it cannot be done in compatible way. I haven't really followed the original reason why it wasn't possible to be done and I wonder if it still applies here.
Thanks for the note. Indeed we've discovered that a switch for oneofs would reorder fields compared to what Google Protobuf produces. While I'm unsure why we strive to maintain the binary compatibility, we nevertheless do, and it's the founding principle of PBJ. We'd need to look further into this when we get to reviewing this particular PR.
There was a problem hiding this comment.
@abies as I noted at #546 (comment) , there was a theoretical idea about grouping adjacent oneof variants in separate switch statements, and from the description of this PR ( #995 ), it sounds like exactly this idea was implemented here. So I think there's no issues, and passing integration tests (which this PR doesn't modify) prove that, too. This, of course, adds some extra complexity, but it looks like there's a performance win. I'm about to review the actual code changes here, but this concern seems resolved currently.
Please let me know if you have other concerns regarding this issue.
There was a problem hiding this comment.
No other issues. Thank you for looking into that.
| protected final int writeImpl(@NonNull $modelClass data, @NonNull byte[] output, final int startOffset) { | ||
| return writeTo(data, output, startOffset, startOffset + data.protobufSize()); |
There was a problem hiding this comment.
Minor: @artemananiev introduced a writeImpl(RandomAccessData out) at #1017 , which could benefit from a similar optimization. But this probably belongs in a separate PR.
| sb.append(""" | ||
| switch (data.$oneOf().kind()) { | ||
| $cases default -> {} | ||
| } |
There was a problem hiding this comment.
Minor:
- a degenerate case of single adjacent oneof variant could use a simple
if(){}. This might in theory require less byte code. - Could we use a normal switch/case:/break instead of exploiting the case-> to avoid break statements? This looks like a trick and may be difficult to read. case-> is totally fine when we need to return a value, but not for hiding absent fall-throughs.
| while (true) { | ||
| final int b = (tag >>> 7) == 0 ? (int) tag : (int) ((tag & 0x7F) | 0x80); | ||
| sb.append("output[offset++] = ") | ||
| .append(b > Byte.MAX_VALUE ? "(byte) " : "") | ||
| .append("0x%02X;".formatted(b)); | ||
| if (first) { | ||
| sb.append(" // tag: field %d, wire type %d".formatted(fieldNumber, wireType)); | ||
| first = false; | ||
| } | ||
| sb.append("\n"); | ||
| if ((tag >>> 7) == 0) return sb.toString(); | ||
| tag >>>= 7; | ||
| } |
There was a problem hiding this comment.
Why not use the writeVarInt() method from #994 ?
| } | ||
| final String local = localName(field, fieldNames); | ||
| // outside a oneof, var avoids naming the type; inside, the type is needed to read the OneOf's value | ||
| final String javaType = !inOneOf ? "var" : field.type() == Field.FieldType.ENUM ? "int" : field.javaFieldType(); |
There was a problem hiding this comment.
Please replace var with the actual type of the field to improve the code readability.
| .formatted(oneOfField.nameCamelFirstLower(), oneOfType, Common.camelToUpperSnake(field.name())); | ||
| prefix = oneOfGuard | ||
| ? prefix | ||
| + "if (data.%s().kind() == %s.%s)%n" |
There was a problem hiding this comment.
This will produce a multi-line if() w/o a {} block, which goes against Java Coding Conventions. Could we replace the trailing %n with a simple space character?
What and why
write(T, byte[], offset)now writes each field with straight-line generated code, instead of calling a genericProtoArrayWriterToolshelper that takes aFieldDefinition. This implementsdocs/fast-codegen-plan.mdsection 6.1, items 1-5:Performance (AMD EPYC 9275F):
write(T, byte[])is 20-22% faster for HAPI block items (cached 379 → 296 ns, fresh 602 → 483 ns) and 14-21% faster for signed transactions, still with 0 B/op. Tier A, on by default.output[offset++] = 0x0A;, with two literals for two-byte tags.if (nonce != 0) { … }. Oneof members are always written, as before.public static int writeTo(data, output, startOffset, end), and nested messages generated in the same build call it directly. The call is static, so it can be inlined; before, it went through the megamorphicCodec.write. Messages from dependency jars keep the instance call.ProtoArrayWriterTools.writeVarInt(Write array varints with exact-size bulk stores #988), bounded by the message end.switch: each run of oneof alternatives that are adjacent in field-number order becomes oneswitchon the kind.writeImpl(T, byte[], startOffset)delegates towriteTo(data, output, startOffset, startOffset + data.protobufSize()). Map entries,google.protobufwrapper types and repeated scalars keep their existing helpers.Generated code:
BlockItembefore and afterBefore:
After:
and for a message with scalars (
TransactionID):Tests
ByteArrayWriterGenerationTest(new):writeTocall;Codecinstance call;switch.pbj-core:spotlessApply qualityGate buildpasses.pbj-integration-tests:./gradlew buildpasses, 326,323 tests: protoc byte comparisons, the non-canonical-order tests, andAllocationTestwith cached-size writes still at 0 B/op;-PpbjPerformance=maxpasses, 326,323 tests.Broken pipe/Socket closed:GrpcCloseClientConnectionTestonce, thenGrpcClientComprehensiveTest.testClientStreamingMethod…InTheMiddle(two tests) once. A third run passed. These tests have the server deliberately end a call while the client is still writing, and the client's next write hits the closed socket. They pass 6 of 6 times when run alone on this branch, and they involve no generated codec. This looks like a pre-existing race under the parallel full suite, not this change.Performance: AMD EPYC 9275F, before = #988's branch, after = this PR
3 forks, 3 × 1 s warmup, 5 × 1 s measurement,
-prof gc. Per item or transaction forBlockItemBench; whole block forSampleBlockBench.write(T, byte[])is 20-22% faster for block items, both fresh and cached, and 14-21% faster for signed transactions. It still allocates 0 B/op.toBytesare unchanged within the noise. They go through the stream writer, which Generate straight-line size calculation and stream writers #990 changes; that PR also routestoBytesto this writer.SampleBlockBench.writePbjByteArrayis also a stream write, into aBufferedData, despite its name.Generated code size (full HAPI build)
main/ #988mainclasses*ProtoCodecclassesStacked on #994.
Fixes #989
🤖 Generated with Claude Code