From 96cba8e8e30d2c9153f1ffebf52aabf22067379b Mon Sep 17 00:00:00 2001 From: Dmitry Petrov Date: Sat, 5 Sep 2026 23:29:43 +0100 Subject: [PATCH 1/2] perf: optimize protobuf WKT and map encoding Port measured WKT, numeric map-key, uint64 buffer, and descriptor caching improvements. Fix deprecated fields and escaped custom names across codecs, preserve map-key behavior under writer settings, and add regression tests, JMH coverage, allocation budgets, and performance evidence. --- CLAUDE.md | 2 +- allocation-check.sh | 10 + buff-json-benchmarks/CLAUDE.md | 4 +- .../benchmarks/EncodePathsBenchmark.java | 214 ++++++++++++++++++ buff-json-protoc-plugin/CLAUDE.md | 11 +- buff-json-protoc-plugin/pom.xml | 4 + .../buffjson/protoc/DecoderGenerator.java | 7 +- .../buffjson/protoc/EncoderGenerator.java | 69 +++--- .../buffjson/protoc/SourceLiterals.java | 49 ++++ buff-json-tests/CLAUDE.md | 1 + .../src/main/protobuf/conformance_test.proto | 42 ++++ .../BuffJsonEncodingRegressionTest.java | 165 ++++++++++++++ .../BuffJsonProto3ConformanceTest.java | 68 ++++++ .../BuffJsonProto3DecodeConformanceTest.java | 36 +++ buff-json/CLAUDE.md | 7 +- .../buffjson/internal/FieldWriter.java | 53 ++++- .../buffjson/internal/MessageSchema.java | 42 +--- .../internal/ProtobufMessageWriter.java | 11 +- .../buffjson/internal/WellKnownTypes.java | 91 ++++++-- .../buffjson/internal/typed/FieldName.java | 20 ++ .../internal/typed/TypedFieldAccessor.java | 26 ++- .../typed/TypedFieldAccessorFactory.java | 27 +-- .../internal/typed/TypedMessageSchema.java | 5 +- docs/performance-branch-review-2026-09-05.md | 113 +++++++++ docs/performance-port-2026-09-05.md | 60 +++++ .../2026-09-05-allocation.csv | 15 ++ .../2026-09-05-comparison.csv | 13 ++ 27 files changed, 1038 insertions(+), 127 deletions(-) create mode 100644 buff-json-benchmarks/src/main/java/io/suboptimal/buffjson/benchmarks/EncodePathsBenchmark.java create mode 100644 buff-json-protoc-plugin/src/main/java/io/suboptimal/buffjson/protoc/SourceLiterals.java create mode 100644 buff-json-tests/src/test/java/io/suboptimal/buffjson/BuffJsonEncodingRegressionTest.java create mode 100644 docs/performance-branch-review-2026-09-05.md create mode 100644 docs/performance-port-2026-09-05.md create mode 100644 docs/performance-results/2026-09-05-allocation.csv create mode 100644 docs/performance-results/2026-09-05-comparison.csv diff --git a/CLAUDE.md b/CLAUDE.md index 666222b..6656636 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -112,7 +112,7 @@ JSONFactory.getDefaultObjectReaderProvider().register(decoder.readerModule()); ## Allocation Regression Check -`./allocation-check.sh` runs JMH `-prof gc` on a representative subset of benchmarks (SimpleMessage codegen+runtime × UTF-16+UTF-8, ComplexMessage codegen+runtime, DoubleHeavy codegen × UTF-16+UTF-8) and asserts `gc.alloc.rate.norm` (bytes per `@Benchmark` invocation) stays within per-benchmark budgets defined in the script. Total runtime ~1 minute. `--quick` flag for local iteration. Wired into CI as a separate `allocation-check` job in `.github/workflows/ci.yml`. Catches regressions like a missed zero-alloc path, a forgotten try-with-resources, or a new String/byte[] allocation per call. +`./allocation-check.sh` runs JMH `-prof gc` on a representative subset of benchmarks (SimpleMessage codegen+runtime × UTF-16+UTF-8, ComplexMessage codegen+runtime, DoubleHeavy codegen × UTF-16+UTF-8, typed Struct/Timestamp × UTF-16+UTF-8, and map-heavy codegen+runtime) and asserts `gc.alloc.rate.norm` (bytes per `@Benchmark` invocation) stays within per-benchmark budgets defined in the script. Total runtime ~2 minutes. `--quick` flag for local iteration. Wired into CI as a separate `allocation-check` job in `.github/workflows/ci.yml`. Catches regressions like a missed zero-alloc path, a forgotten try-with-resources, or a new String/byte[] allocation per call. ## Build Notes diff --git a/allocation-check.sh b/allocation-check.sh index dce8617..091932c 100755 --- a/allocation-check.sh +++ b/allocation-check.sh @@ -48,6 +48,16 @@ BUDGETS=( # DoubleHeavy (25 doubles, IoT/telemetry profile) — number formatting cost. "io.suboptimal.buffjson.benchmarks.DoubleHeavyBenchmark.compiledUtf16:2200" # baseline ~1773 B/op "io.suboptimal.buffjson.benchmarks.DoubleHeavyBenchmark.compiledUtf8:2100" # baseline ~1749 B/op + + # Typed WKT helpers — guard the concrete getter paths in both encodings. + "io.suboptimal.buffjson.benchmarks.WktBenchmark.structRuntime:1050" # baseline ~847 B/op (Java 21) + "io.suboptimal.buffjson.benchmarks.WktBenchmark.timestampRuntime:550" # baseline ~464 B/op + "io.suboptimal.buffjson.benchmarks.EncodePathsBenchmark.structTypedUtf8:900" # baseline ~712 B/op + "io.suboptimal.buffjson.benchmarks.EncodePathsBenchmark.timestampTypedUtf8:520" # baseline ~440 B/op + + # Numeric map keys — no per-key numeric String on the default writer path. + "io.suboptimal.buffjson.benchmarks.RepeatedAndMapBenchmark.mapCompiled:6200" # baseline ~5025 B/op + "io.suboptimal.buffjson.benchmarks.RepeatedAndMapBenchmark.mapRuntime:6000" # baseline ~4805 B/op ) # Parse args diff --git a/buff-json-benchmarks/CLAUDE.md b/buff-json-benchmarks/CLAUDE.md index 2309ec5..8c17174 100644 --- a/buff-json-benchmarks/CLAUDE.md +++ b/buff-json-benchmarks/CLAUDE.md @@ -18,6 +18,8 @@ Older benchmarks (`ComplexMessageBenchmark`, `WktBenchmark`, etc.) still use `bu ## Benchmark Classes +`EncodePathsBenchmark` is an ordinary JMH matrix ported from the performance stash: simple, complex, map-heavy, Struct, and Timestamp shapes × codegen/typed/reflection × UTF-16/UTF-8. It has no CodSpeed dependency. + | Class | Message | Focus | |---------------------------------|-----------------------------------------------------------------|---------------------------------------------| | `SimpleMessageBenchmark` | 6-field flat message (string, int32, int64, double, bool, enum) | Scalar baseline (UTF-16/UTF-8 split) | @@ -51,7 +53,7 @@ Older benchmarks (`ComplexMessageBenchmark`, `WktBenchmark`, etc.) still use `bu ## Allocation Regression Check -`./allocation-check.sh` (at repo root) runs JMH `-prof gc` on a representative subset of benchmarks (SimpleMessage codegen+runtime × UTF-16+UTF-8, ComplexMessage codegen+runtime, DoubleHeavy codegen × UTF-16+UTF-8) and asserts `gc.alloc.rate.norm` (B/op) stays within per-benchmark budgets. Total runtime ~1 minute; `--quick` flag for local iteration. Wired into CI as a separate `allocation-check` job. Catches missed zero-alloc paths and new String/byte[] allocations on the hot path. +`./allocation-check.sh` (at repo root) runs JMH `-prof gc` on a representative subset of benchmarks (SimpleMessage codegen+runtime × UTF-16+UTF-8, ComplexMessage codegen+runtime, DoubleHeavy codegen × UTF-16+UTF-8, typed Struct/Timestamp × UTF-16+UTF-8, and map-heavy codegen+runtime) and asserts `gc.alloc.rate.norm` (B/op) stays within per-benchmark budgets. Total runtime ~2 minutes; `--quick` flag for local iteration. Wired into CI as a separate `allocation-check` job. Catches missed zero-alloc paths and new String/byte[] allocations on the hot path. ## Proto Files diff --git a/buff-json-benchmarks/src/main/java/io/suboptimal/buffjson/benchmarks/EncodePathsBenchmark.java b/buff-json-benchmarks/src/main/java/io/suboptimal/buffjson/benchmarks/EncodePathsBenchmark.java new file mode 100644 index 0000000..b8fa277 --- /dev/null +++ b/buff-json-benchmarks/src/main/java/io/suboptimal/buffjson/benchmarks/EncodePathsBenchmark.java @@ -0,0 +1,214 @@ +package io.suboptimal.buffjson.benchmarks; + +import java.util.Random; +import java.util.concurrent.TimeUnit; + +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Fork; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.Mode; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.Setup; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.annotations.Warmup; + +import io.suboptimal.buffjson.BuffJson; +import io.suboptimal.buffjson.BuffJsonEncoder; +import io.suboptimal.buffjson.proto.BenchMapHeavy; +import io.suboptimal.buffjson.proto.BenchStruct; +import io.suboptimal.buffjson.proto.BenchTimestamps; +import io.suboptimal.buffjson.proto.ComplexMessage; +import io.suboptimal.buffjson.proto.SimpleMessage; + +/** + * Stable performance-regression matrix for every encoder implementation and + * output encoding. Kept separate from the comparison suite so CI can run it + * with a fixed set of benchmark identities. + */ +@BenchmarkMode(Mode.Throughput) +@OutputTimeUnit(TimeUnit.SECONDS) +@Warmup(iterations = 3, time = 1, timeUnit = TimeUnit.SECONDS) +@Measurement(iterations = 5, time = 1, timeUnit = TimeUnit.SECONDS) +@Fork(1) +@State(Scope.Benchmark) +public class EncodePathsBenchmark { + + private static final int POOL_SIZE = 1024; + private static final int MASK = POOL_SIZE - 1; + private static final BuffJsonEncoder CODEGEN = BuffJson.encoder(); + private static final BuffJsonEncoder TYPED = BuffJson.encoder().setGeneratedEncoders(false); + private static final BuffJsonEncoder REFLECTION = BuffJson.encoder().setGeneratedEncoders(false) + .setTypedAccessors(false); + + private SimpleMessage[] simpleMessages; + private ComplexMessage[] complexMessages; + private BenchMapHeavy[] mapMessages; + private BenchStruct[] structMessages; + private BenchTimestamps[] timestampMessages; + private int mapIndex; + private int structIndex; + private int timestampIndex; + private int simpleIndex; + private int complexIndex; + + @Setup + public void setup() { + simpleMessages = BenchmarkData.createRandomSimpleMessages(new Random(42), POOL_SIZE); + complexMessages = BenchmarkData.createRandomComplexMessages(new Random(43), POOL_SIZE); + mapMessages = BenchmarkData.createRandomBenchMapHeavy(new Random(43), POOL_SIZE); + structMessages = BenchmarkData.createRandomBenchStructs(new Random(45), POOL_SIZE); + timestampMessages = BenchmarkData.createRandomBenchTimestamps(new Random(42), POOL_SIZE); + } + + @Benchmark + public String simpleCodegenUtf16() { + return CODEGEN.encode(simpleMessages[simpleIndex++ & MASK]); + } + + @Benchmark + public byte[] simpleCodegenUtf8() { + return CODEGEN.encodeToBytes(simpleMessages[simpleIndex++ & MASK]); + } + + @Benchmark + public String simpleTypedUtf16() { + return TYPED.encode(simpleMessages[simpleIndex++ & MASK]); + } + + @Benchmark + public byte[] simpleTypedUtf8() { + return TYPED.encodeToBytes(simpleMessages[simpleIndex++ & MASK]); + } + + @Benchmark + public String simpleReflectionUtf16() { + return REFLECTION.encode(simpleMessages[simpleIndex++ & MASK]); + } + + @Benchmark + public byte[] simpleReflectionUtf8() { + return REFLECTION.encodeToBytes(simpleMessages[simpleIndex++ & MASK]); + } + + @Benchmark + public String complexCodegenUtf16() { + return CODEGEN.encode(complexMessages[complexIndex++ & MASK]); + } + + @Benchmark + public byte[] complexCodegenUtf8() { + return CODEGEN.encodeToBytes(complexMessages[complexIndex++ & MASK]); + } + + @Benchmark + public String complexTypedUtf16() { + return TYPED.encode(complexMessages[complexIndex++ & MASK]); + } + + @Benchmark + public byte[] complexTypedUtf8() { + return TYPED.encodeToBytes(complexMessages[complexIndex++ & MASK]); + } + + @Benchmark + public String complexReflectionUtf16() { + return REFLECTION.encode(complexMessages[complexIndex++ & MASK]); + } + + @Benchmark + public byte[] complexReflectionUtf8() { + return REFLECTION.encodeToBytes(complexMessages[complexIndex++ & MASK]); + } + + @Benchmark + public String mapCodegenUtf16() { + return CODEGEN.encode(mapMessages[mapIndex++ & MASK]); + } + + @Benchmark + public byte[] mapCodegenUtf8() { + return CODEGEN.encodeToBytes(mapMessages[mapIndex++ & MASK]); + } + + @Benchmark + public String mapTypedUtf16() { + return TYPED.encode(mapMessages[mapIndex++ & MASK]); + } + + @Benchmark + public byte[] mapTypedUtf8() { + return TYPED.encodeToBytes(mapMessages[mapIndex++ & MASK]); + } + + @Benchmark + public String mapReflectionUtf16() { + return REFLECTION.encode(mapMessages[mapIndex++ & MASK]); + } + + @Benchmark + public byte[] mapReflectionUtf8() { + return REFLECTION.encodeToBytes(mapMessages[mapIndex++ & MASK]); + } + + @Benchmark + public String structCodegenUtf16() { + return CODEGEN.encode(structMessages[structIndex++ & MASK]); + } + + @Benchmark + public byte[] structCodegenUtf8() { + return CODEGEN.encodeToBytes(structMessages[structIndex++ & MASK]); + } + + @Benchmark + public String structTypedUtf16() { + return TYPED.encode(structMessages[structIndex++ & MASK]); + } + + @Benchmark + public byte[] structTypedUtf8() { + return TYPED.encodeToBytes(structMessages[structIndex++ & MASK]); + } + + @Benchmark + public String structReflectionUtf16() { + return REFLECTION.encode(structMessages[structIndex++ & MASK]); + } + + @Benchmark + public byte[] structReflectionUtf8() { + return REFLECTION.encodeToBytes(structMessages[structIndex++ & MASK]); + } + + @Benchmark + public String timestampCodegenUtf16() { + return CODEGEN.encode(timestampMessages[timestampIndex++ & MASK]); + } + + @Benchmark + public byte[] timestampCodegenUtf8() { + return CODEGEN.encodeToBytes(timestampMessages[timestampIndex++ & MASK]); + } + + @Benchmark + public String timestampTypedUtf16() { + return TYPED.encode(timestampMessages[timestampIndex++ & MASK]); + } + + @Benchmark + public byte[] timestampTypedUtf8() { + return TYPED.encodeToBytes(timestampMessages[timestampIndex++ & MASK]); + } + + @Benchmark + public String timestampReflectionUtf16() { + return REFLECTION.encode(timestampMessages[timestampIndex++ & MASK]); + } + + @Benchmark + public byte[] timestampReflectionUtf8() { + return REFLECTION.encodeToBytes(timestampMessages[timestampIndex++ & MASK]); + } +} diff --git a/buff-json-protoc-plugin/CLAUDE.md b/buff-json-protoc-plugin/CLAUDE.md index 16fb495..34c1ce6 100644 --- a/buff-json-protoc-plugin/CLAUDE.md +++ b/buff-json-protoc-plugin/CLAUDE.md @@ -33,7 +33,7 @@ For each non-WKT, non-map-entry message type: 1. A `FooJsonEncoder.java` class implementing `BuffJsonGeneratedEncoder` 2. A `public static final INSTANCE` singleton for direct calls from other encoders -3. Pre-computed name constants per field — both `char[] NAME_*` (UTF-16 path) and `byte[] NAME_*_BYTES` (UTF-8 path), populated from `nameChars(...)` / `nameBytes(...)` helpers at class init. ASCII-only. +3. Pre-computed name constants per field — both `char[] NAME_*` (UTF-16 path) and `byte[] NAME_*_BYTES` (UTF-8 path), JSON-escaped at generation time and emitted as Java string literals. UTF-8 bytes use `StandardCharsets.UTF_8`, supporting custom Unicode names. 4. Pre-cached `String[] ENUM_*_NAMES` arrays for each enum type (built from enum descriptor at class init, avoiding `UNRECOGNIZED` which throws from `getNumber()`) 5. A `writeFields(JSONWriter, T, ProtobufMessageWriter)` method with inlined per-field encoding logic, opening with `boolean utf8 = jsonWriter.isUTF8();` so each field-name write dispatches via `if (utf8) writeNameRaw(NAME_X_BYTES); else writeNameRaw(NAME_X);` 6. A `message_implements` insertion point per message adding `BuffJsonCodecHolder` to the implements clause @@ -55,7 +55,7 @@ For each non-WKT, non-map-entry message type: | Field name | `if (utf8) writeNameRaw(NAME_X_BYTES); else writeNameRaw(NAME_X);` — JIT-specialized branch | | Repeated | `msg.getFooList()`, check isEmpty, iterate | | Map (String key) | `msg.getFooMap()`, iterate, `entry.getKey()` directly (no `toString()`) | -| Map (non-String key) | `msg.getFooMap()`, iterate, `entry.getKey().toString()` | +| Map (non-String key) | `writeString(primitive)` or unsigned helper; no numeric String allocation | | Oneof | `switch (msg.getFooCase())` with per-case typed accessor | | Nested message (non-WKT) | `FooJsonEncoder.INSTANCE.writeFields(jw, nested, writer)` — direct call, bypasses registry | | Nested message (WKT) | `WellKnownTypes.write(jsonWriter, nested, writer)` | @@ -72,6 +72,10 @@ For each non-WKT, non-map-entry message type: ## Important Edge Cases +- **Custom `json_name`** — `SourceLiterals` escapes JSON names for encoder constants and Java literals for both encoder constants and decoder switch labels, including quotes, backslashes, control characters, and Unicode. +- **Deprecated fields/types** — included in generated codecs. Both codec classes suppress Java deprecation warnings so generated calls compile with `-Werror`; protobuf deprecation does not change JSON semantics. +- **Unsigned map keys** — uint32/fixed32 use `Integer.toUnsignedLong`; uint64/fixed64 use `WellKnownTypes.writeUnsignedLongString`. Keys always remain quoted JSON strings. Long-key writes share `FieldWriter.writeLongMapKey`, which preserves key spelling under BrowserCompatible and WriteClassName; boolean keys use constant strings. + - **`google.protobuf.Empty`** is NOT in the WKT set — it serializes as a regular empty message `{}` - **`DynamicMessage`** cannot use generated encoders (would fail cast) — guarded in `ProtobufMessageWriter` - **Map entry types** (`options.map_entry = true`) are skipped — they're synthetic @@ -90,12 +94,15 @@ For each non-WKT, non-map-entry message type: ## Build +After changing a generator, use `mvn clean verify`: incremental protobuf generation can skip unchanged `.proto` files even when the plugin implementation changed. + - Build-time deps: `protobuf-java` (CodeGeneratorRequest/descriptor APIs), `buff-json-schema` (reused to bake JSON Schema resources), and `protovalidate` (so baked schemas carry buf.validate constraints). These are **code-generation-time only** — they never become runtime dependencies of the generated code. - No shading needed — the ascopes `jvm-maven` plugin resolves the plugin's transitive deps onto the code-gen classpath automatically (verified: `buff-json-schema` + `protovalidate` load during `generate`). - Built **after** `buff-json-schema` in the reactor (it now depends on it). Still built before the consumer modules (tests/benchmarks/conformance). ## Dependencies +- `com.alibaba.fastjson2:fastjson2` — JSON name escaping at generation time (already transitively used by schema baking) - `com.google.protobuf:protobuf-java` — CodeGeneratorRequest, FileDescriptor, FieldDescriptor, ExtensionRegistry - `io.github.suboptimal-solutions:buff-json-schema` — `ProtobufSchema.generateJson(...)` for baking schema resources (build-time) - `build.buf:protovalidate` — buf.validate extensions registered into the parse `ExtensionRegistry` so constraints reach `ProtobufSchema` (build-time) diff --git a/buff-json-protoc-plugin/pom.xml b/buff-json-protoc-plugin/pom.xml index 8e7a5f5..a111073 100644 --- a/buff-json-protoc-plugin/pom.xml +++ b/buff-json-protoc-plugin/pom.xml @@ -19,6 +19,10 @@ build.buf protovalidate + + com.alibaba.fastjson2 + fastjson2 + com.google.protobuf protobuf-java diff --git a/buff-json-protoc-plugin/src/main/java/io/suboptimal/buffjson/protoc/DecoderGenerator.java b/buff-json-protoc-plugin/src/main/java/io/suboptimal/buffjson/protoc/DecoderGenerator.java index 0518769..f800579 100644 --- a/buff-json-protoc-plugin/src/main/java/io/suboptimal/buffjson/protoc/DecoderGenerator.java +++ b/buff-json-protoc-plugin/src/main/java/io/suboptimal/buffjson/protoc/DecoderGenerator.java @@ -26,6 +26,9 @@ static String generate(Descriptor msgDesc, String javaPackage, String decoderSim sb.append("package ").append(javaPackage).append(";\n\n"); sb.append("import com.alibaba.fastjson2.JSONReader;\n"); sb.append("import io.suboptimal.buffjson.BuffJsonGeneratedDecoder;\n\n"); + // [deprecated = true] fields/types get @Deprecated accessors from protoc; + // calling them from generated code would trip -Xlint:deprecation -Werror. + sb.append("@SuppressWarnings(\"deprecation\")\n"); sb.append("public final class ").append(decoderSimpleName); sb.append(" implements BuffJsonGeneratedDecoder<").append(messageClassName).append("> {\n\n"); @@ -56,9 +59,9 @@ static String generate(Descriptor msgDesc, String javaPackage, String decoderSim for (FieldDescriptor fd : msgDesc.getFields()) { String jsonName = fd.getJsonName(); - sb.append(" case \"").append(jsonName).append("\""); + sb.append(" case ").append(SourceLiterals.javaString(jsonName)); if (!fd.getName().equals(jsonName)) { - sb.append(", \"").append(fd.getName()).append("\""); + sb.append(", ").append(SourceLiterals.javaString(fd.getName())); } sb.append(" -> "); diff --git a/buff-json-protoc-plugin/src/main/java/io/suboptimal/buffjson/protoc/EncoderGenerator.java b/buff-json-protoc-plugin/src/main/java/io/suboptimal/buffjson/protoc/EncoderGenerator.java index 2835120..01183b8 100644 --- a/buff-json-protoc-plugin/src/main/java/io/suboptimal/buffjson/protoc/EncoderGenerator.java +++ b/buff-json-protoc-plugin/src/main/java/io/suboptimal/buffjson/protoc/EncoderGenerator.java @@ -53,6 +53,9 @@ static String generate(Descriptor msgDesc, String javaPackage, String encoderSim sb.append("package ").append(javaPackage).append(";\n\n"); sb.append("import com.alibaba.fastjson2.JSONWriter;\n"); sb.append("import io.suboptimal.buffjson.BuffJsonGeneratedEncoder;\n\n"); + // [deprecated = true] fields/types get @Deprecated accessors from protoc; + // calling them from generated code would trip -Xlint:deprecation -Werror. + sb.append("@SuppressWarnings(\"deprecation\")\n"); sb.append("public final class ").append(encoderSimpleName); sb.append(" implements BuffJsonGeneratedEncoder<").append(messageClassName).append("> {\n\n"); @@ -60,37 +63,17 @@ static String generate(Descriptor msgDesc, String javaPackage, String encoderSim sb.append(" public static final ").append(encoderSimpleName).append(" INSTANCE = new ") .append(encoderSimpleName).append("();\n\n"); - // Name constants: char[] for UTF-16 writers, byte[] for UTF-8 writers. - // Pre-encoded at class init; ASCII-only since proto field names are ASCII. + // Escape JSON names during generation, then quote the encoded text as Java. + // Keep the existing char[]/byte[] hot path, including for custom json_name. for (FieldDescriptor fd : msgDesc.getFields()) { - if (fd.getOptions().hasDeprecated() && fd.getOptions().getDeprecated()) - continue; - String jsonName = fd.getJsonName(); + String literal = SourceLiterals.javaString(SourceLiterals.jsonFieldName(fd.getJsonName())); sb.append(" private static final char[] NAME_").append(constantName(fd)); - sb.append(" = nameChars(\"").append(jsonName).append("\");\n"); + sb.append(" = ").append(literal).append(".toCharArray();\n"); sb.append(" private static final byte[] NAME_").append(constantName(fd)); - sb.append("_BYTES = nameBytes(\"").append(jsonName).append("\");\n"); + sb.append("_BYTES = ").append(literal).append(".getBytes(java.nio.charset.StandardCharsets.UTF_8);\n"); } sb.append("\n"); - // nameChars / nameBytes helpers - sb.append(" private static char[] nameChars(String name) {\n"); - sb.append(" char[] chars = new char[name.length() + 3];\n"); - sb.append(" chars[0] = '\"';\n"); - sb.append(" name.getChars(0, name.length(), chars, 1);\n"); - sb.append(" chars[name.length() + 1] = '\"';\n"); - sb.append(" chars[name.length() + 2] = ':';\n"); - sb.append(" return chars;\n"); - sb.append(" }\n\n"); - sb.append(" private static byte[] nameBytes(String name) {\n"); - sb.append(" byte[] bytes = new byte[name.length() + 3];\n"); - sb.append(" bytes[0] = '\"';\n"); - sb.append(" for (int i = 0; i < name.length(); i++) bytes[i + 1] = (byte) name.charAt(i);\n"); - sb.append(" bytes[name.length() + 1] = '\"';\n"); - sb.append(" bytes[name.length() + 2] = ':';\n"); - sb.append(" return bytes;\n"); - sb.append(" }\n\n"); - // Pre-collect enum types used by int-valued fields (implicit presence, // explicit presence, oneof) so we can generate cached name arrays. // Key: enum constant prefix (e.g. "STATUS"), Value: Java enum class name @@ -351,13 +334,10 @@ private static void generateMapField(StringBuilder sb, FieldDescriptor fd, Strin sb.append(" if (!map.isEmpty()) {\n"); emitWriteName(sb, constName, " "); sb.append(" jsonWriter.startObject();\n"); + sb.append(" boolean first = true;\n"); sb.append(" for (var entry : map.entrySet()) {\n"); - if (keyFd.getJavaType() == FieldDescriptor.JavaType.STRING) { - // Key is already String — call writeName directly without toString() - sb.append(" jsonWriter.writeName(entry.getKey());\n"); - } else { - sb.append(" jsonWriter.writeName(entry.getKey().toString());\n"); - } + sb.append(" if (first) first = false; else jsonWriter.writeComma();\n"); + writeMapKey(sb, keyFd, "entry.getKey()"); sb.append(" jsonWriter.writeColon();\n"); switch (valueFd.getJavaType()) { @@ -450,6 +430,33 @@ private static void writeLongValue(StringBuilder sb, FieldDescriptor fd, String } } + private static void writeMapKey(StringBuilder sb, FieldDescriptor fd, String expr) { + switch (fd.getJavaType()) { + case INT -> { + var type = fd.getType(); + if (type == FieldDescriptor.Type.UINT32 || type == FieldDescriptor.Type.FIXED32) { + sb.append( + " io.suboptimal.buffjson.internal.FieldWriter.writeLongMapKey(jsonWriter, Integer.toUnsignedLong(") + .append(expr).append("), false);\n"); + } else { + sb.append(" jsonWriter.writeString(").append(expr).append(");\n"); + } + } + case LONG -> { + var type = fd.getType(); + sb.append( + " io.suboptimal.buffjson.internal.FieldWriter.writeLongMapKey(jsonWriter, ") + .append(expr).append(", ") + .append(type == FieldDescriptor.Type.UINT64 || type == FieldDescriptor.Type.FIXED64) + .append(");\n"); + } + case BOOLEAN -> sb.append(" jsonWriter.writeString(").append(expr) + .append(" ? \"true\" : \"false\");\n"); + case STRING -> sb.append(" jsonWriter.writeString(").append(expr).append(");\n"); + default -> throw new IllegalArgumentException("Unsupported map key type: " + fd.getJavaType()); + } + } + private static void writeFloatValue(StringBuilder sb, String expr) { sb.append(" {\n"); sb.append(" float fv = ").append(expr).append(";\n"); diff --git a/buff-json-protoc-plugin/src/main/java/io/suboptimal/buffjson/protoc/SourceLiterals.java b/buff-json-protoc-plugin/src/main/java/io/suboptimal/buffjson/protoc/SourceLiterals.java new file mode 100644 index 0000000..5d4fd48 --- /dev/null +++ b/buff-json-protoc-plugin/src/main/java/io/suboptimal/buffjson/protoc/SourceLiterals.java @@ -0,0 +1,49 @@ +package io.suboptimal.buffjson.protoc; + +import com.alibaba.fastjson2.JSONFactory; +import com.alibaba.fastjson2.JSONWriter; + +/** Literal escaping used by both codec generators. */ +final class SourceLiterals { + private SourceLiterals() { + } + + /** Pre-encodes a JSON field name, including its quotes and colon. */ + static String jsonFieldName(String name) { + var context = JSONFactory.createWriteContext(); + context.setFeatures(0); + try (JSONWriter writer = JSONWriter.of(context)) { + writer.writeString(name); + writer.writeColon(); + return writer.toString(); + } + } + + /** + * Quotes a Java string literal. LF and CR must use ordinary escapes: Java + * processes Unicode escapes before tokenizing, so Unicode-escaped line breaks + * would leave the generated string literal unclosed. + */ + static String javaString(String value) { + StringBuilder sb = new StringBuilder(value.length() + 8).append('"'); + for (int i = 0; i < value.length(); i++) { + char c = value.charAt(i); + switch (c) { + case '"' -> sb.append("\\\""); + case '\\' -> sb.append("\\\\"); + case '\n' -> sb.append("\\n"); + case '\r' -> sb.append("\\r"); + case '\t' -> sb.append("\\t"); + case '\b' -> sb.append("\\b"); + case '\f' -> sb.append("\\f"); + default -> { + if (c >= 0x20 && c <= 0x7e) + sb.append(c); + else + sb.append(String.format("\\u%04x", (int) c)); + } + } + } + return sb.append('"').toString(); + } +} diff --git a/buff-json-tests/CLAUDE.md b/buff-json-tests/CLAUDE.md index bd8becd..4109f4e 100644 --- a/buff-json-tests/CLAUDE.md +++ b/buff-json-tests/CLAUDE.md @@ -10,6 +10,7 @@ pure reflection). ## Test Structure - `BuffJsonReferenceTest.java` — 5 smoke tests (scalar, default, complex, plus two `DynamicMessage` tests on UTF-16 and UTF-8 paths — `DynamicMessage` is the only thing that exclusively exercises pure reflection in production) +- `BuffJsonEncodingRegressionTest.java` — escaped custom names compile in both generated codecs, round-trip through all encoder paths (UTF-16/UTF-8) and both decoders, and accept proto-name aliases. Concrete and actual DynamicMessage WKTs share output/range validation. Deprecated-field fixtures and unsigned key/digit boundaries live in the main conformance tests. - `BuffJsonMemoryTest.java` — 8 reachability tests using `WeakReference` + `System.gc()` to confirm the encoder doesn't retain `Message` references after `encode`/`encodeToBytes`/`encode(stream)` on any of the three paths, including `DynamicMessage`. Steady-state allocation regressions are caught separately by `./allocation-check.sh` in CI (JMH `-prof gc`). - `BuffJsonCrossPathFuzzTest.java` — seeded-random (reproducible) fuzzer over `TestAllTypesProto3`. `encodePathsAgreeAndAreParseable` asserts **codegen == typed == reflection** byte-for-byte (UTF-16 and UTF-8) over 500 messages — the direct "the three paths agree" guarantee — plus a buff-json self round-trip. `decodePathsRoundTrip` asserts both decode paths reconstruct messages from `JsonFormat`-printed JSON. (It does not byte-compare encode output against `JsonFormat` because fastjson2 and protobuf may format the same float/double differently — both round-trip to the same value; curated byte-equality lives in `BuffJsonProto3ConformanceTest`.) - `BuffJsonProto3ConformanceTest.java` — proto3 JSON **encode** coverage in nested classes, each `assertMatchesReference` validating all three paths (codegen, typed-accessor, reflection) byte-for-byte against `JsonFormat`; well-known-type groups also carry out-of-range **edge cases** asserting all three paths reject identically: diff --git a/buff-json-tests/src/main/protobuf/conformance_test.proto b/buff-json-tests/src/main/protobuf/conformance_test.proto index 4854737..8071681 100644 --- a/buff-json-tests/src/main/protobuf/conformance_test.proto +++ b/buff-json-tests/src/main/protobuf/conformance_test.proto @@ -177,6 +177,22 @@ message TestCustomJsonName { string name = 2 [json_name = "Name"]; } +// Exercises JSON escaping and Java source escaping in both generated codecs. +message TestEscapedJsonNames { + int32 unicode = 1 [json_name = "naïve日本🚀"]; + string quoted = 2 [json_name = "quoted\"name"]; + bool backslash = 3 [json_name = "back\\slash"]; + int64 controls = 4 [json_name = "line\nreturn\rtab\tback\bform\f"]; + string literal_escape = 5 [json_name = "literal\\u000a"]; + repeated int32 numbers = 6 [json_name = "list\"[]"]; + map labels = 7 [json_name = "map\"key"]; + optional int32 present = 8 [json_name = "present\"zero"]; + oneof selection { + string chosen = 9 [json_name = "choice\"value"]; + } + NestedMessage nested = 10 [json_name = "nested\nvalue"]; +} + // Any type message TestAny { google.protobuf.Any value = 1; @@ -186,3 +202,29 @@ message TestAny { message TestEmpty { google.protobuf.Empty value = 1; } + +// Fields marked [deprecated = true]. Deprecation is a Java-API annotation +// concern, not a wire/JSON one, so these serialize exactly like any other +// field — matching JsonFormat.printer(). Covers every codegen branch that +// emits a NAME_ constant (implicit presence, explicit presence, +// repeated, map, nested message, enum, WKT, oneof member), so a generator +// that skips deprecated fields in one loop but not the other fails to +// compile rather than silently diverging. +message TestDeprecatedFields { + int32 not_deprecated = 1; + int32 deprecated_int32 = 2 [deprecated = true]; + int64 deprecated_int64 = 3 [deprecated = true]; + string deprecated_string = 4 [deprecated = true]; + bytes deprecated_bytes = 5 [deprecated = true]; + optional int32 deprecated_optional_int32 = 6 [deprecated = true]; + repeated int32 deprecated_repeated_int32 = 7 [deprecated = true]; + repeated string deprecated_repeated_string = 8 [deprecated = true]; + map deprecated_map = 9 [deprecated = true]; + NestedMessage deprecated_message = 10 [deprecated = true]; + TestEnum deprecated_enum = 11 [deprecated = true]; + google.protobuf.Timestamp deprecated_timestamp = 12 [deprecated = true]; + oneof deprecated_oneof { + int32 deprecated_oneof_int32 = 13 [deprecated = true]; + string deprecated_oneof_string = 14; + } +} diff --git a/buff-json-tests/src/test/java/io/suboptimal/buffjson/BuffJsonEncodingRegressionTest.java b/buff-json-tests/src/test/java/io/suboptimal/buffjson/BuffJsonEncodingRegressionTest.java new file mode 100644 index 0000000..9274ba3 --- /dev/null +++ b/buff-json-tests/src/test/java/io/suboptimal/buffjson/BuffJsonEncodingRegressionTest.java @@ -0,0 +1,165 @@ +package io.suboptimal.buffjson; + +import static org.junit.jupiter.api.Assertions.*; + +import java.nio.charset.StandardCharsets; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; + +import com.alibaba.fastjson2.JSON; +import com.alibaba.fastjson2.JSONWriter; +import com.google.protobuf.*; +import com.google.protobuf.util.JsonFormat; + +import org.junit.jupiter.api.Test; + +import io.suboptimal.buffjson.internal.ProtobufMessageWriter; +import io.suboptimal.buffjson.internal.typed.TypedMessageSchema; +import io.suboptimal.buffjson.proto.*; + +class BuffJsonEncodingRegressionTest { + + private static final JsonFormat.Printer PRINTER = JsonFormat.printer().omittingInsignificantWhitespace(); + private static List encoders() { + return List.of(BuffJson.encoder(), BuffJson.encoder().setGeneratedEncoders(false), + BuffJson.encoder().setGeneratedEncoders(false).setTypedAccessors(false)); + } + + private static TestEscapedJsonNames escapedNames() { + return TestEscapedJsonNames.newBuilder().setUnicode(42).setQuoted("quote").setBackslash(true).setControls(-1) + .setLiteralEscape("literal").addNumbers(1).addNumbers(2).putLabels("key", "value").setPresent(0) + .setChosen("choice").setNested(NestedMessage.newBuilder().setValue(3)).build(); + } + + private static String escapedNamesJson(TestEscapedJsonNames message) throws Exception { + // JsonFormat 4.34.1 prints custom json_name without escaping it. Use its + // valid proto-name output as the value oracle, then rename the keys. + var values = JSON.parseObject(PRINTER.preservingProtoFieldNames().print(message)); + Map renamed = new LinkedHashMap<>(); + for (var field : message.getDescriptorForType().getFields()) { + if (values.containsKey(field.getName())) + renamed.put(field.getJsonName(), values.get(field.getName())); + } + return JSON.toJSONString(renamed); + } + + @Test + void escapedNamesEncodeAcrossAllPaths() throws Exception { + var original = escapedNames(); + assertNotNull(TypedMessageSchema.forMessage(original.getDescriptorForType(), original.getClass()), + "Custom names must not force a fallback to reflection"); + var expected = JSON.parseObject(escapedNamesJson(original)); + assertTrue(expected.containsKey("naïve日本🚀")); + assertTrue(expected.containsKey("literal\\u000a")); + for (var message : List.of(original, + DynamicMessage.parseFrom(original.getDescriptorForType(), original.toByteString()))) { + for (var encoder : encoders()) { + for (String json : List.of(encoder.encode(message), + new String(encoder.encodeToBytes(message), StandardCharsets.UTF_8))) { + assertEquals(expected, JSON.parseObject(json)); + var builder = TestEscapedJsonNames.newBuilder(); + JsonFormat.parser().merge(json, builder); + assertEquals(original, builder.build()); + } + } + } + } + + @Test + void escapedNamesAndProtoAliasesDecode() throws Exception { + var original = escapedNames(); + for (String json : List.of(escapedNamesJson(original), PRINTER.preservingProtoFieldNames().print(original))) { + for (boolean generated : List.of(true, false)) { + var decoder = BuffJson.decoder().setGeneratedDecoders(generated); + assertEquals(original, decoder.decode(json, TestEscapedJsonNames.class)); + assertEquals(original, + decoder.decode(json.getBytes(StandardCharsets.UTF_8), TestEscapedJsonNames.class)); + } + } + } + + @Test + void wellKnownTypesMatchForConcreteAndDynamicMessages() throws Exception { + var struct = Struct.newBuilder().putFields("quoted\"key", Value.newBuilder().setStringValue("value").build()) + .build(); + var list = ListValue.newBuilder().addValues(Value.newBuilder().setNullValue(NullValue.NULL_VALUE)) + .addValues(Value.newBuilder().setBoolValue(true)).addValues(Value.newBuilder().setNumberValue(-1.5)) + .addValues(Value.newBuilder().setStringValue("text")) + .addValues(Value.newBuilder().setStructValue(struct)) + .addValues(Value.newBuilder().setListValue(ListValue.getDefaultInstance())).build(); + for (Message original : List.of( + TestTimestamp.newBuilder().setValue(Timestamp.newBuilder().setSeconds(-1).setNanos(123456789)).build(), + TestDuration.newBuilder().setValue(Duration.newBuilder().setSeconds(-2).setNanos(-123456789)).build(), + TestStruct.newBuilder().setStructValue(struct).setValue(Value.newBuilder().setListValue(list)) + .setListValue(list).build(), + TestStruct.newBuilder().setStructValue(Struct.getDefaultInstance()) + .setListValue(ListValue.getDefaultInstance()).build())) { + Message dynamic = DynamicMessage.parseFrom(original.getDescriptorForType(), original.toByteString()); + assertInstanceOf(DynamicMessage.class, + dynamic.getField(original.getDescriptorForType().getFields().getFirst())); + String expected = PRINTER.print(original); + assertEquals(expected, PRINTER.print(dynamic)); + for (var encoder : encoders()) { + for (var message : List.of(original, dynamic)) { + assertEquals(expected, encoder.encode(message)); + assertEquals(expected, new String(encoder.encodeToBytes(message), StandardCharsets.UTF_8)); + } + } + } + } + + @Test + void booleanMapKeysStayBooleanNamesWithNumericBooleanFeature() throws Exception { + var message = TestMaps.newBuilder().putBoolToString(true, "yes").putBoolToString(false, "no").build(); + String expected = PRINTER.print(message); + for (var writer : List.of(new ProtobufMessageWriter(null, true, true), + new ProtobufMessageWriter(null, false, true), new ProtobufMessageWriter(null, false, false))) { + for (boolean utf8 : List.of(false, true)) { + try (var jw = utf8 + ? JSONWriter.ofUTF8(JSONWriter.Feature.WriteBooleanAsNumber) + : JSONWriter.of(JSONWriter.Feature.WriteBooleanAsNumber)) { + writer.writeMessage(jw, message); + assertEquals(expected, jw.toString()); + } + } + } + } + + @Test + void numericMapKeysIgnoreNumberFormattingFeatures() throws Exception { + var message = TestMaps.newBuilder().putInt64ToString(Long.MIN_VALUE, "min") + .putInt64ToString(Long.MAX_VALUE, "max").putInt64ToString(1, "small") + .putUint64ToString(-1, "unsigned max").putUint64ToString(Long.MAX_VALUE, "unsigned signed max") + .putUint32ToString(1, "small uint32").putUint32ToString(-1, "uint32 max").build(); + String expected = PRINTER.print(message); + for (var feature : List.of(JSONWriter.Feature.BrowserCompatible, JSONWriter.Feature.WriteClassName)) { + for (var writer : List.of(new ProtobufMessageWriter(null, true, true), + new ProtobufMessageWriter(null, false, true), new ProtobufMessageWriter(null, false, false))) { + for (boolean utf8 : List.of(false, true)) { + try (var jw = utf8 ? JSONWriter.ofUTF8(feature) : JSONWriter.of(feature)) { + writer.writeMessage(jw, message); + assertEquals(expected, jw.toString()); + } + } + } + } + } + + @Test + void invalidWellKnownTypesStillRejectDynamicMessages() throws Exception { + for (Message original : List.of( + TestTimestamp.newBuilder().setValue(Timestamp.newBuilder().setSeconds(253402300800L)).build(), + TestTimestamp.newBuilder().setValue(Timestamp.newBuilder().setNanos(-1)).build(), + TestDuration.newBuilder().setValue(Duration.newBuilder().setSeconds(315576000001L)).build(), + TestDuration.newBuilder().setValue(Duration.newBuilder().setSeconds(1).setNanos(-1)).build())) { + var dynamic = DynamicMessage.parseFrom(original.getDescriptorForType(), original.toByteString()); + for (var encoder : encoders()) { + for (var message : List.of(original, dynamic)) { + assertThrows(IllegalArgumentException.class, () -> encoder.encode(message)); + assertThrows(IllegalArgumentException.class, () -> encoder.encodeToBytes(message)); + } + } + } + } +} diff --git a/buff-json-tests/src/test/java/io/suboptimal/buffjson/BuffJsonProto3ConformanceTest.java b/buff-json-tests/src/test/java/io/suboptimal/buffjson/BuffJsonProto3ConformanceTest.java index e584e9e..3853900 100644 --- a/buff-json-tests/src/test/java/io/suboptimal/buffjson/BuffJsonProto3ConformanceTest.java +++ b/buff-json-tests/src/test/java/io/suboptimal/buffjson/BuffJsonProto3ConformanceTest.java @@ -13,6 +13,7 @@ import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; +import io.suboptimal.buffjson.internal.typed.TypedMessageSchema; import io.suboptimal.buffjson.proto.*; class BuffJsonProto3ConformanceTest { @@ -89,6 +90,14 @@ void integerBoundaries() throws Exception { .setOptionalSint32(Integer.MIN_VALUE).setOptionalSint64(Long.MIN_VALUE).build()); } + @Test + void unsigned64DigitBoundary() throws Exception { + long last19Digit = Long.parseUnsignedLong("9999999999999999999"); + long first20Digit = Long.parseUnsignedLong("10000000000000000000"); + assertMatchesReference(TestAllScalars.newBuilder().setOptionalUint64(last19Digit).build()); + assertMatchesReference(TestAllScalars.newBuilder().setOptionalUint64(first20Digit).build()); + } + @Test void negativeNumbers() throws Exception { assertMatchesReference(TestAllScalars.newBuilder().setOptionalInt32(-1).setOptionalInt64(-1L) @@ -337,6 +346,15 @@ void intKeyMaps() throws Exception { .putSfixed32ToString(-10, "sfixed").putSfixed64ToString(-20L, "sfixed64").build()); } + @Test + void unsignedKeyBoundaries() throws Exception { + var message = TestMaps.newBuilder().putUint32ToString(-1, "uint32 max") + .putFixed32ToString(-1, "fixed32 max").putUint64ToString(-1L, "uint64 max") + .putFixed64ToString(-1L, "fixed64 max").build(); + assertMatchesReference(message); + assertMatchesReference(DynamicMessage.parseFrom(message.getDescriptorForType(), message.toByteString())); + } + @Test void boolKeyMap() throws Exception { assertMatchesReference( @@ -888,6 +906,56 @@ void emptyMessage() throws Exception { } } + // ========================================================================= + // Deprecated fields + // ========================================================================= + /** + * {@code [deprecated = true]} is a Java-API annotation concern, not a wire/JSON + * one: {@code JsonFormat.printer()} prints deprecated fields, so all three + * paths must too. Also a compile-time guard — a generator that emits + * {@code NAME_} constants for a different set of fields than it writes + * produces an encoder that does not compile. + */ + @Nested + @SuppressWarnings("deprecation") + class DeprecatedFields { + + @Test + void allDeprecatedFieldsSet() throws Exception { + var message = TestDeprecatedFields.newBuilder().setNotDeprecated(1).setDeprecatedInt32(42) + .setDeprecatedInt64(123456789012345L).setDeprecatedString("hello") + .setDeprecatedBytes(ByteString.copyFromUtf8("binary data")).setDeprecatedOptionalInt32(0) + .addDeprecatedRepeatedInt32(1).addDeprecatedRepeatedInt32(2).addDeprecatedRepeatedString("a") + .putDeprecatedMap("k", 7) + .setDeprecatedMessage(NestedMessage.newBuilder().setValue(9).setName("nested").build()) + .setDeprecatedEnum(TestEnum.TEST_ENUM_BAR) + .setDeprecatedTimestamp(Timestamp.newBuilder().setSeconds(1234567890).setNanos(123000000).build()) + .setDeprecatedOneofInt32(5).build(); + assertNotNull(TypedMessageSchema.forMessage(message.getDescriptorForType(), message.getClass()), + "Deprecated getters must remain on the typed path"); + assertMatchesReference(message); + + // Guard against the whole group being vacuously equal by omission: the + // deprecated fields must actually be present in the output. + String json = CODEGEN_ENCODER.encode(message); + for (String name : new String[]{"deprecatedInt32", "deprecatedInt64", "deprecatedString", "deprecatedBytes", + "deprecatedOptionalInt32", "deprecatedRepeatedInt32", "deprecatedRepeatedString", "deprecatedMap", + "deprecatedMessage", "deprecatedEnum", "deprecatedTimestamp", "deprecatedOneofInt32"}) { + assertTrue(json.contains("\"" + name + "\":"), "missing deprecated field " + name + " in " + json); + } + } + + @Test + void deprecatedDefaultsOmitted() throws Exception { + assertMatchesReference(TestDeprecatedFields.getDefaultInstance()); + } + + @Test + void deprecatedOneofNonDeprecatedMember() throws Exception { + assertMatchesReference(TestDeprecatedFields.newBuilder().setDeprecatedOneofString("set").build()); + } + } + // ========================================================================= // Edge cases: empty messages // ========================================================================= diff --git a/buff-json-tests/src/test/java/io/suboptimal/buffjson/BuffJsonProto3DecodeConformanceTest.java b/buff-json-tests/src/test/java/io/suboptimal/buffjson/BuffJsonProto3DecodeConformanceTest.java index f89dad5..1310698 100644 --- a/buff-json-tests/src/test/java/io/suboptimal/buffjson/BuffJsonProto3DecodeConformanceTest.java +++ b/buff-json-tests/src/test/java/io/suboptimal/buffjson/BuffJsonProto3DecodeConformanceTest.java @@ -868,6 +868,42 @@ void emptyMessage() throws Exception { } } + // ========================================================================= + // Deprecated fields + // ========================================================================= + /** + * Deprecated fields are decoded like any other field on both paths — the encode + * side emits them (matching {@code JsonFormat}), so dropping them here would + * break the round-trip. + */ + @Nested + @SuppressWarnings("deprecation") + class DeprecatedFields { + + @Test + void allDeprecatedFieldsSet() throws Exception { + assertDecodeMatchesOriginal(TestDeprecatedFields.newBuilder().setNotDeprecated(1).setDeprecatedInt32(42) + .setDeprecatedInt64(123456789012345L).setDeprecatedString("hello") + .setDeprecatedBytes(ByteString.copyFromUtf8("binary data")).setDeprecatedOptionalInt32(0) + .addDeprecatedRepeatedInt32(1).addDeprecatedRepeatedInt32(2).addDeprecatedRepeatedString("a") + .putDeprecatedMap("k", 7) + .setDeprecatedMessage(NestedMessage.newBuilder().setValue(9).setName("nested").build()) + .setDeprecatedEnum(TestEnum.TEST_ENUM_BAR) + .setDeprecatedTimestamp(Timestamp.newBuilder().setSeconds(1234567890).setNanos(123000000).build()) + .setDeprecatedOneofInt32(5).build()); + } + + @Test + void deprecatedDefaults() throws Exception { + assertDecodeMatchesOriginal(TestDeprecatedFields.getDefaultInstance()); + } + + @Test + void deprecatedOneofNonDeprecatedMember() throws Exception { + assertDecodeMatchesOriginal(TestDeprecatedFields.newBuilder().setDeprecatedOneofString("set").build()); + } + } + // ========================================================================= // Edge cases: empty messages // ========================================================================= diff --git a/buff-json/CLAUDE.md b/buff-json/CLAUDE.md index e596d03..0a95d6d 100644 --- a/buff-json/CLAUDE.md +++ b/buff-json/CLAUDE.md @@ -106,8 +106,13 @@ JSON.parseObject(json, MyMessage.class); // uses the reader's settings ## Proto3 JSON Spec: Key Gotchas +- **Cached names and descriptors**: `FieldName.of` JSON-escapes custom names once for UTF-16 and UTF-8. Map key/value descriptors and typed message WKT checks are cached in schemas. +- **Deprecated fields**: retain normal presence and encoding behavior on all three paths. +- **Numeric map keys**: written directly as quoted primitives, with unsigned conversion for uint32/fixed32 and uint64/fixed64. Boolean keys use constant strings. Long keys fall back to String formatting under BrowserCompatible or WriteClassName to prevent fastjson2 from changing their spelling. +- **Compiled WKTs**: Timestamp, Duration, Struct, Value, and ListValue use concrete getters; DynamicMessage retains descriptor-based fallback and the same range checks. + - **uint32/fixed32**: `Integer.toUnsignedLong()` for unsigned representation -- **uint64/fixed64**: `Long.toUnsignedString()` for unsigned quoted strings. On **decode**, both the quoted form and an *unquoted* JSON number up to `2^64-1` are accepted: `FieldReader.readUnsignedLong` parses the quoted form with `Long.parseUnsignedLong`, and the unquoted form via `readBigInteger` + `[0, 2^64)` range check (a plain `readInt64Value()` overflows past `Long.MAX_VALUE`), taking the low 64 bits. +- **uint64/fixed64**: `WellKnownTypes.writeUnsignedLongString()` writes quoted unsigned values without an intermediate String; large values use one exact-size 19/20-byte buffer. On **decode**, both the quoted form and an *unquoted* JSON number up to `2^64-1` are accepted: `FieldReader.readUnsignedLong` parses the quoted form with `Long.parseUnsignedLong`, and the unquoted form via `readBigInteger` + `[0, 2^64)` range check (a plain `readInt64Value()` overflows past `Long.MAX_VALUE`), taking the low 64 bits. - **int64 and all 64-bit types**: Must be quoted strings in JSON - **Enum unknown numbers**: proto3 open enums preserve an unrecognized numeric value rather than dropping it to 0. The reflection decode path uses `EnumDescriptor.findValueByNumberCreatingIfUnknown` (codegen stores via `setXxxValue(int)`), so the number survives a re-serialization to the wire — matching `JsonFormat`. - **NaN/Infinity**: fastjson2 writes `null` — we intercept and write quoted strings diff --git a/buff-json/src/main/java/io/suboptimal/buffjson/internal/FieldWriter.java b/buff-json/src/main/java/io/suboptimal/buffjson/internal/FieldWriter.java index 8d6539d..aa943c6 100644 --- a/buff-json/src/main/java/io/suboptimal/buffjson/internal/FieldWriter.java +++ b/buff-json/src/main/java/io/suboptimal/buffjson/internal/FieldWriter.java @@ -171,18 +171,61 @@ public static void writeRepeated(JSONWriter jsonWriter, FieldDescriptor fd, List * {@link com.google.protobuf.MapEntry} and * {@link com.google.protobuf.DynamicMessage} map entries. */ - public static void writeMap(JSONWriter jsonWriter, FieldDescriptor valueDescriptor, List entries, + public static void writeMap(JSONWriter jsonWriter, FieldDescriptor keyFd, FieldDescriptor valueFd, List entries, ProtobufMessageWriter writer) { - var entryDesc = valueDescriptor.getContainingType(); - var keyFd = entryDesc.findFieldByName("key"); - var valueFd = entryDesc.findFieldByName("value"); jsonWriter.startObject(); + boolean first = true; for (Object entry : entries) { Message entryMsg = (Message) entry; - jsonWriter.writeName(entryMsg.getField(keyFd).toString()); + if (first) + first = false; + else + jsonWriter.writeComma(); + writeMapKey(jsonWriter, keyFd, entryMsg.getField(keyFd)); jsonWriter.writeColon(); writeValue(jsonWriter, valueFd, entryMsg.getField(valueFd), writer); } jsonWriter.endObject(); } + + /** Writes a quoted map key; the caller writes its comma and colon. */ + public static void writeMapKey(JSONWriter jsonWriter, FieldDescriptor keyDescriptor, Object key) { + switch (keyDescriptor.getJavaType()) { + case INT -> { + int value = (int) key; + var type = keyDescriptor.getType(); + if (type == FieldDescriptor.Type.UINT32 || type == FieldDescriptor.Type.FIXED32) + writeLongMapKey(jsonWriter, Integer.toUnsignedLong(value), false); + else + jsonWriter.writeString(value); + } + case LONG -> { + long value = (long) key; + var type = keyDescriptor.getType(); + writeLongMapKey(jsonWriter, value, + type == FieldDescriptor.Type.UINT64 || type == FieldDescriptor.Type.FIXED64); + } + case BOOLEAN -> jsonWriter.writeString((boolean) key ? "true" : "false"); + case STRING -> jsonWriter.writeString((String) key); + default -> throw new IllegalArgumentException("Unsupported map key type: " + keyDescriptor.getJavaType()); + } + } + + /** + * Writes numeric keys without letting number-formatting features change their + * spelling. + */ + public static void writeLongMapKey(JSONWriter jsonWriter, long key, boolean unsigned) { + // fastjson2 2.0.63 writeString(long) delegates to writeInt64: BrowserCompatible + // can add a second pair of quotes, and WriteClassName can append an L suffix. + if ((jsonWriter.getFeatures() + & (JSONWriter.Feature.BrowserCompatible.mask | JSONWriter.Feature.WriteClassName.mask)) != 0) { + jsonWriter.writeString(unsigned ? Long.toUnsignedString(key) : Long.toString(key)); + } else if (unsigned) { + WellKnownTypes.writeUnsignedLongString(jsonWriter, key); + } else { + jsonWriter.writeString(key); + } + } + } diff --git a/buff-json/src/main/java/io/suboptimal/buffjson/internal/MessageSchema.java b/buff-json/src/main/java/io/suboptimal/buffjson/internal/MessageSchema.java index f230582..c8ffd5a 100644 --- a/buff-json/src/main/java/io/suboptimal/buffjson/internal/MessageSchema.java +++ b/buff-json/src/main/java/io/suboptimal/buffjson/internal/MessageSchema.java @@ -7,6 +7,8 @@ import com.google.protobuf.Descriptors.Descriptor; import com.google.protobuf.Descriptors.FieldDescriptor; +import io.suboptimal.buffjson.internal.typed.FieldName; + /** * Cached metadata for a protobuf message type, built from its * {@link Descriptor}. @@ -79,49 +81,23 @@ public static final class FieldInfo { private final boolean isRepeated; private final boolean isMapField; private final boolean hasPresence; + private final FieldDescriptor mapKeyDescriptor; private final FieldDescriptor mapValueDescriptor; FieldInfo(FieldDescriptor fd) { this.descriptor = fd; this.jsonName = fd.getJsonName(); - this.nameWithColon = buildNameWithColon(this.jsonName); - this.nameWithColonUtf8 = buildNameWithColonUtf8(this.jsonName); + FieldName encodedName = FieldName.of(this.jsonName); + this.nameWithColon = encodedName.chars(); + this.nameWithColonUtf8 = encodedName.utf8(); this.javaType = fd.getJavaType(); this.isRepeated = fd.isRepeated(); this.isMapField = fd.isMapField(); this.hasPresence = fd.hasPresence(); + this.mapKeyDescriptor = fd.isMapField() ? fd.getMessageType().findFieldByName("key") : null; this.mapValueDescriptor = fd.isMapField() ? fd.getMessageType().findFieldByName("value") : null; } - /** - * Pre-computes {@code "fieldName":} as a char array for the UTF-16 - * {@link com.alibaba.fastjson2.JSONWriter#writeNameRaw(char[])} path. Protobuf - * JSON field names are always ASCII. - */ - private static char[] buildNameWithColon(String name) { - char[] chars = new char[name.length() + 3]; - chars[0] = '"'; - name.getChars(0, name.length(), chars, 1); - chars[name.length() + 1] = '"'; - chars[name.length() + 2] = ':'; - return chars; - } - - /** - * Pre-computes {@code "fieldName":} as a byte array for the UTF-8 - * {@link com.alibaba.fastjson2.JSONWriter#writeNameRaw(byte[])} path, avoiding - * char→byte transcoding per field write. ASCII-only. - */ - private static byte[] buildNameWithColonUtf8(String name) { - byte[] bytes = new byte[name.length() + 3]; - bytes[0] = '"'; - for (int i = 0; i < name.length(); i++) - bytes[i + 1] = (byte) name.charAt(i); - bytes[name.length() + 1] = '"'; - bytes[name.length() + 2] = ':'; - return bytes; - } - public FieldDescriptor descriptor() { return descriptor; } @@ -154,6 +130,10 @@ public boolean hasPresence() { return hasPresence; } + public FieldDescriptor mapKeyDescriptor() { + return mapKeyDescriptor; + } + public FieldDescriptor mapValueDescriptor() { return mapValueDescriptor; } diff --git a/buff-json/src/main/java/io/suboptimal/buffjson/internal/ProtobufMessageWriter.java b/buff-json/src/main/java/io/suboptimal/buffjson/internal/ProtobufMessageWriter.java index f6e43dd..1b2459a 100644 --- a/buff-json/src/main/java/io/suboptimal/buffjson/internal/ProtobufMessageWriter.java +++ b/buff-json/src/main/java/io/suboptimal/buffjson/internal/ProtobufMessageWriter.java @@ -125,7 +125,8 @@ void writeFields(JSONWriter jsonWriter, Message message) { if (entries.isEmpty()) continue; writeName(jsonWriter, fieldInfo, utf8); - FieldWriter.writeMap(jsonWriter, fieldInfo.mapValueDescriptor(), entries, this); + FieldWriter.writeMap(jsonWriter, fieldInfo.mapKeyDescriptor(), fieldInfo.mapValueDescriptor(), entries, + this); } else if (fieldInfo.isRepeated()) { List values = (List) message.getField(fd); if (values.isEmpty()) @@ -133,13 +134,11 @@ void writeFields(JSONWriter jsonWriter, Message message) { writeName(jsonWriter, fieldInfo, utf8); FieldWriter.writeRepeated(jsonWriter, fd, values, this); } else { + if (fieldInfo.hasPresence() && !message.hasField(fd)) + continue; Object value = message.getField(fd); - if (fieldInfo.hasPresence()) { - if (!message.hasField(fd)) - continue; - } else if (isDefaultValue(fieldInfo, value)) { + if (!fieldInfo.hasPresence() && isDefaultValue(fieldInfo, value)) continue; - } writeName(jsonWriter, fieldInfo, utf8); FieldWriter.writeValue(jsonWriter, fd, value, this); } diff --git a/buff-json/src/main/java/io/suboptimal/buffjson/internal/WellKnownTypes.java b/buff-json/src/main/java/io/suboptimal/buffjson/internal/WellKnownTypes.java index 8eebd64..b551ae9 100644 --- a/buff-json/src/main/java/io/suboptimal/buffjson/internal/WellKnownTypes.java +++ b/buff-json/src/main/java/io/suboptimal/buffjson/internal/WellKnownTypes.java @@ -84,6 +84,8 @@ public final class WellKnownTypes { private static final long DURATION_SECONDS_MIN = -315576000000L; private static final long DURATION_SECONDS_MAX = 315576000000L; private static final int NANOS_MAX = 999_999_999; + // Signed bit pattern of unsigned 10^19: the boundary between 19 and 20 digits. + private static final long UNSIGNED_10_POW_19 = -8446744073709551616L; /** * Cached field descriptors for well-known types to avoid repeated @@ -173,6 +175,10 @@ private static void writeAny(JSONWriter jsonWriter, Message message, ProtobufMes } private static void writeTimestamp(JSONWriter jsonWriter, Message message) { + if (message instanceof Timestamp timestamp) { + writeTimestampDirect(jsonWriter, timestamp.getSeconds(), timestamp.getNanos()); + return; + } var fields = getFields(message, "seconds", "nanos"); long seconds = (long) message.getField(fields[0]); int nanos = (int) message.getField(fields[1]); @@ -243,6 +249,10 @@ public static void writeTimestampDirect(JSONWriter jsonWriter, long seconds, int } private static void writeDuration(JSONWriter jsonWriter, Message message) { + if (message instanceof Duration duration) { + writeDurationDirect(jsonWriter, duration.getSeconds(), duration.getNanos()); + return; + } var fields = getFields(message, "seconds", "nanos"); long seconds = (long) message.getField(fields[0]); int nanos = (int) message.getField(fields[1]); @@ -302,10 +312,10 @@ public static void writeUnsignedLongString(JSONWriter jsonWriter, long value) { return; } // Negative signed = large unsigned: format into byte[] and write as Latin1 - // Max unsigned long is 18446744073709551615 = 20 digits - byte[] buf = new byte[20]; - int off = writeUnsignedLong(buf, 0, value); - buf = java.util.Arrays.copyOf(buf, off); + // without allocating a second trimmed copy. + int length = Long.compareUnsigned(value, UNSIGNED_10_POW_19) < 0 ? 19 : 20; + byte[] buf = new byte[length]; + writeUnsignedLong(buf, 0, value); jsonWriter.writeStringLatin1(buf); } @@ -379,26 +389,65 @@ private static void writeFieldMask(JSONWriter jsonWriter, Message message) { } private static void writeStruct(JSONWriter jsonWriter, Message message, ProtobufMessageWriter writer) { + // Compiled Struct: iterate the real map instead of materializing the + // synthetic MapEntry list and pulling key/value back out through getField(). + if (message instanceof Struct struct) { + jsonWriter.startObject(); + for (var entry : struct.getFieldsMap().entrySet()) { + jsonWriter.writeName(entry.getKey()); + jsonWriter.writeColon(); + writeValue(jsonWriter, entry.getValue(), writer); + } + jsonWriter.endObject(); + return; + } + var fields = getFields(message, "fields"); @SuppressWarnings("unchecked") List entries = (List) message.getField(fields[0]); jsonWriter.startObject(); - for (var entry : entries) { - var entryFields = getFields(entry, "key", "value"); - String key = (String) entry.getField(entryFields[0]); - Message value = (Message) entry.getField(entryFields[1]); - jsonWriter.writeName(key); - jsonWriter.writeColon(); - writeValue(jsonWriter, value, writer); + if (!entries.isEmpty()) { + // Every entry shares the same map-entry descriptor, so resolve key/value once + // rather than hitting the descriptor cache per entry. + FieldDescriptor[] entryFields = getFields(entries.get(0), "key", "value"); + for (var entry : entries) { + String key = (String) entry.getField(entryFields[0]); + Message value = (Message) entry.getField(entryFields[1]); + jsonWriter.writeName(key); + jsonWriter.writeColon(); + writeValue(jsonWriter, value, writer); + } } jsonWriter.endObject(); } private static void writeValue(JSONWriter jsonWriter, Message message, ProtobufMessageWriter writer) { - var desc = message.getDescriptorForType(); - var kindOneof = desc.getOneofs().get(0); - var activeField = message.getOneofFieldDescriptor(kindOneof); + // Compiled Value: getKindCase() is an int switch on the oneof case, replacing + // a getOneofs() call (which allocates two list wrappers per invocation), a + // getOneofFieldDescriptor() scan, and a switch on the field's String name. + if (message instanceof Value value) { + switch (value.getKindCase()) { + case NULL_VALUE, KIND_NOT_SET -> jsonWriter.writeNull(); + case NUMBER_VALUE -> jsonWriter.writeDouble(value.getNumberValue()); + case STRING_VALUE -> jsonWriter.writeString(value.getStringValue()); + case BOOL_VALUE -> jsonWriter.writeBool(value.getBoolValue()); + case STRUCT_VALUE -> writeStruct(jsonWriter, value.getStructValue(), writer); + case LIST_VALUE -> writeListValue(jsonWriter, value.getListValue(), writer); + // A kind added by a future protobuf-java would otherwise write nothing at + // all after the caller has already emitted the name and colon, producing + // structurally invalid JSON. javac has no exhaustiveness lint for switch + // statements, so this arm is the guard. + default -> jsonWriter.writeNull(); + } + return; + } + + // DynamicMessage only — compiled Value returned above. getOneofs() allocates a + // list-wrapper pair per call, but caching the result would mean a second + // strong-keyed Descriptor cache pinning the descriptor pool of every schema + // ever loaded, to save two allocations on a cold path. + var activeField = message.getOneofFieldDescriptor(message.getDescriptorForType().getOneofs().get(0)); if (activeField == null) { jsonWriter.writeNull(); @@ -416,12 +465,24 @@ private static void writeValue(JSONWriter jsonWriter, Message message, ProtobufM } private static void writeListValue(JSONWriter jsonWriter, Message message, ProtobufMessageWriter writer) { + if (message instanceof ListValue listValue) { + var typedValues = listValue.getValuesList(); + jsonWriter.startArray(); + for (int i = 0, n = typedValues.size(); i < n; i++) { + if (i > 0) + jsonWriter.writeComma(); + writeValue(jsonWriter, typedValues.get(i), writer); + } + jsonWriter.endArray(); + return; + } + var fields = getFields(message, "values"); @SuppressWarnings("unchecked") List values = (List) message.getField(fields[0]); jsonWriter.startArray(); - for (int i = 0; i < values.size(); i++) { + for (int i = 0, n = values.size(); i < n; i++) { if (i > 0) jsonWriter.writeComma(); writeValue(jsonWriter, values.get(i), writer); diff --git a/buff-json/src/main/java/io/suboptimal/buffjson/internal/typed/FieldName.java b/buff-json/src/main/java/io/suboptimal/buffjson/internal/typed/FieldName.java index d9aea83..c296493 100644 --- a/buff-json/src/main/java/io/suboptimal/buffjson/internal/typed/FieldName.java +++ b/buff-json/src/main/java/io/suboptimal/buffjson/internal/typed/FieldName.java @@ -1,5 +1,8 @@ package io.suboptimal.buffjson.internal.typed; +import java.nio.charset.StandardCharsets; + +import com.alibaba.fastjson2.JSONFactory; import com.alibaba.fastjson2.JSONWriter; /** @@ -8,6 +11,23 @@ */ public record FieldName(char[] chars, byte[] utf8) { + /** + * Escapes once when building a schema, keeping both write paths + * allocation-free. + */ + public static FieldName of(String jsonName) { + var context = JSONFactory.createWriteContext(); + // Raw field names have always used double quotes, independent of writer + // features. + context.setFeatures(0); + try (JSONWriter writer = JSONWriter.of(context)) { + writer.writeString(jsonName); + writer.writeColon(); + String encoded = writer.toString(); + return new FieldName(encoded.toCharArray(), encoded.getBytes(StandardCharsets.UTF_8)); + } + } + public void writeTo(JSONWriter jw) { if (jw.isUTF8()) jw.writeNameRaw(utf8); diff --git a/buff-json/src/main/java/io/suboptimal/buffjson/internal/typed/TypedFieldAccessor.java b/buff-json/src/main/java/io/suboptimal/buffjson/internal/typed/TypedFieldAccessor.java index e11e23a..9b298fe 100644 --- a/buff-json/src/main/java/io/suboptimal/buffjson/internal/typed/TypedFieldAccessor.java +++ b/buff-json/src/main/java/io/suboptimal/buffjson/internal/typed/TypedFieldAccessor.java @@ -273,7 +273,7 @@ public void write(JSONWriter jw, Message msg, ProtobufMessageWriter writer) { } } - record PresenceMessageAccessor(Function getter, Predicate has, + record PresenceMessageAccessor(Function getter, Predicate has, boolean wellKnown, FieldName name) implements TypedFieldAccessor { @Override public void write(JSONWriter jw, Message msg, ProtobufMessageWriter writer) { @@ -281,7 +281,7 @@ public void write(JSONWriter jw, Message msg, ProtobufMessageWriter writer) { return; name.writeTo(jw); Message nested = getter.apply(msg); - if (WellKnownTypes.isWellKnownType(nested.getDescriptorForType())) + if (wellKnown) WellKnownTypes.write(jw, nested, writer); else writer.writeMessage(jw, nested); @@ -365,7 +365,7 @@ public void write(JSONWriter jw, Message msg, ProtobufMessageWriter writer) { } @SuppressWarnings("unchecked") - record RepeatedMessageAccessor(Function> listGetter, + record RepeatedMessageAccessor(Function> listGetter, boolean wellKnown, FieldName name) implements TypedFieldAccessor { @Override public void write(JSONWriter jw, Message msg, ProtobufMessageWriter writer) { @@ -378,7 +378,7 @@ public void write(JSONWriter jw, Message msg, ProtobufMessageWriter writer) { if (i > 0) jw.writeComma(); Message nested = values.get(i); - if (WellKnownTypes.isWellKnownType(nested.getDescriptorForType())) + if (wellKnown) WellKnownTypes.write(jw, nested, writer); else writer.writeMessage(jw, nested); @@ -411,20 +411,20 @@ public void write(JSONWriter jw, Message msg, ProtobufMessageWriter writer) { // --- Map fields --- - record MapAccessor(Function> entriesGetter, FieldDescriptor mapValueDescriptor, - FieldName name) implements TypedFieldAccessor { + record MapAccessor(Function> entriesGetter, FieldDescriptor mapKeyDescriptor, + FieldDescriptor mapValueDescriptor, FieldName name) implements TypedFieldAccessor { @Override public void write(JSONWriter jw, Message msg, ProtobufMessageWriter writer) { List entries = entriesGetter.apply(msg); if (entries.isEmpty()) return; name.writeTo(jw); - FieldWriter.writeMap(jw, mapValueDescriptor, entries, writer); + FieldWriter.writeMap(jw, mapKeyDescriptor, mapValueDescriptor, entries, writer); } } - record TypedMapAccessor(Function> mapGetter, FieldDescriptor valueFd, - boolean stringKey, FieldName name) implements TypedFieldAccessor { + record TypedMapAccessor(Function> mapGetter, FieldDescriptor keyFd, + FieldDescriptor valueFd, FieldName name) implements TypedFieldAccessor { @Override public void write(JSONWriter jw, Message msg, ProtobufMessageWriter writer) { java.util.Map map = mapGetter.apply(msg); @@ -432,11 +432,13 @@ public void write(JSONWriter jw, Message msg, ProtobufMessageWriter writer) { return; name.writeTo(jw); jw.startObject(); + boolean first = true; for (var entry : map.entrySet()) { - if (stringKey) - jw.writeName((String) entry.getKey()); + if (first) + first = false; else - jw.writeName(entry.getKey().toString()); + jw.writeComma(); + FieldWriter.writeMapKey(jw, keyFd, entry.getKey()); jw.writeColon(); FieldWriter.writeValue(jw, valueFd, entry.getValue(), writer); } diff --git a/buff-json/src/main/java/io/suboptimal/buffjson/internal/typed/TypedFieldAccessorFactory.java b/buff-json/src/main/java/io/suboptimal/buffjson/internal/typed/TypedFieldAccessorFactory.java index bc904af..5213c21 100644 --- a/buff-json/src/main/java/io/suboptimal/buffjson/internal/typed/TypedFieldAccessorFactory.java +++ b/buff-json/src/main/java/io/suboptimal/buffjson/internal/typed/TypedFieldAccessorFactory.java @@ -19,6 +19,8 @@ import com.google.protobuf.Descriptors.OneofDescriptor; import com.google.protobuf.Message; +import io.suboptimal.buffjson.internal.WellKnownTypes; + /** * Creates {@link TypedFieldAccessor} instances for protobuf fields using * {@link LambdaMetafactory}. Each accessor calls the typed getter directly @@ -170,7 +172,8 @@ private static TypedFieldAccessor createPresenceAccessor(FieldDescriptor fd, Cla case MESSAGE -> { var getter = createObjectGetter(messageClass, getterName); var has = createPredicate(messageClass, hasName); - yield new TypedFieldAccessor.PresenceMessageAccessor(castFunction(getter), has, name); + boolean wellKnown = WellKnownTypes.isWellKnownType(fd.getMessageType()); + yield new TypedFieldAccessor.PresenceMessageAccessor(castFunction(getter), has, wellKnown, name); } }; } @@ -196,7 +199,8 @@ private static TypedFieldAccessor createRepeatedAccessor(FieldDescriptor fd, Cla case INT -> new TypedFieldAccessor.RepeatedIntAccessor(listGetter, isUnsigned32(fd), name); case LONG -> new TypedFieldAccessor.RepeatedLongAccessor(listGetter, isUnsigned64(fd), name); case STRING -> new TypedFieldAccessor.RepeatedStringAccessor(listGetter, name); - case MESSAGE -> new TypedFieldAccessor.RepeatedMessageAccessor(listGetter, name); + case MESSAGE -> new TypedFieldAccessor.RepeatedMessageAccessor(listGetter, + WellKnownTypes.isWellKnownType(fd.getMessageType()), name); default -> new TypedFieldAccessor.RepeatedAccessor(listGetter, fd, name); }; } @@ -209,7 +213,6 @@ private static TypedFieldAccessor createMapAccessor(FieldDescriptor fd, Class. @@ -222,11 +225,11 @@ private static TypedFieldAccessor createMapAccessor(FieldDescriptor fd, Class>) (Function) createObjectGetter(messageClass, mapGetterName); - return new TypedFieldAccessor.TypedMapAccessor(mapGetter, valueFd, stringKey, name); + return new TypedFieldAccessor.TypedMapAccessor(mapGetter, keyFd, valueFd, name); } catch (NoSuchMethodException e) { // Fallback: use getField(fd) for unusual cases (custom protoc output) Function> entriesGetter = msg -> (List) msg.getField(fd); - return new TypedFieldAccessor.MapAccessor(entriesGetter, valueFd, name); + return new TypedFieldAccessor.MapAccessor(entriesGetter, keyFd, valueFd, name); } } @@ -340,19 +343,7 @@ static String toCamelCase(String name) { } static FieldName fieldName(String jsonName) { - char[] chars = new char[jsonName.length() + 3]; - chars[0] = '"'; - jsonName.getChars(0, jsonName.length(), chars, 1); - chars[jsonName.length() + 1] = '"'; - chars[jsonName.length() + 2] = ':'; - // Proto field names are always ASCII, so UTF-8 encoding is trivial - byte[] utf8 = new byte[jsonName.length() + 3]; - utf8[0] = '"'; - for (int i = 0; i < jsonName.length(); i++) - utf8[i + 1] = (byte) jsonName.charAt(i); - utf8[jsonName.length() + 1] = '"'; - utf8[jsonName.length() + 2] = ':'; - return new FieldName(chars, utf8); + return FieldName.of(jsonName); } private static boolean isUnsigned32(FieldDescriptor fd) { diff --git a/buff-json/src/main/java/io/suboptimal/buffjson/internal/typed/TypedMessageSchema.java b/buff-json/src/main/java/io/suboptimal/buffjson/internal/typed/TypedMessageSchema.java index 038f132..25830df 100644 --- a/buff-json/src/main/java/io/suboptimal/buffjson/internal/typed/TypedMessageSchema.java +++ b/buff-json/src/main/java/io/suboptimal/buffjson/internal/typed/TypedMessageSchema.java @@ -81,8 +81,9 @@ private static TypedMessageSchema build(Descriptor descriptor, Class)` | `5eefcf1` | Reproduced nesting-depth-limit bypass. The claim of one capacity check is also inaccurate for fastjson2 2.0.63’s UTF-16 implementation: it loops and calls per-string writes. | +| **Exclude** | `.claude/skills/java-nav/SKILL.md` deletion; speculative backlog proposals; stale allocation thresholds | Mixed branch material | Unrelated removal, unimplemented ideas, or machine/version-specific figures. Set new allocation budgets only for the selected combination. | + +## Compatibility findings + +1. **Escaped JSON names still break generated decoders.** At `2b68831`, DecoderGenerator emits `case "a"b", "x" -> {` for a legal `json_name` containing a quote. A compiler probe produces eight syntax errors. FieldNamesTest compiles a helper literal, not the full decoder generator output. The runtime/encoder escaping fixes remain useful but do not complete the plugin fix. +2. **Deprecated fields remain omitted by typed encoding on the remote branch.** TypedMessageSchema still skips deprecated descriptors. The separate local eager-clarke work removes that skip and adds proto fixtures covering scalar, optional, repeated, map, nested, enum, WKT, and oneof cases. Use that work rather than importing only the remote suppression/name-constant changes. +3. **Bulk repeated-string encoding bypasses maxLevel.** With a context maxLevel of 1 and a message containing `strings: ["x"]`, main and perf reject nesting level 2. The encoding branch accepts it in both generated and typed paths. This is an introduced behavior difference, even though the normal suite passes. +4. **Context caching snapshots global defaults.** Build an encoder, enable UseSingleQuotes globally, then encode a string field: main/perf observe the setting; encoding retains the earlier double-quote configuration. Allocation reduction is real; treating it as behavior-preserving is incorrect. + +## Validation and measurement + +All tests/builds were performed in exported snapshots under `/private/tmp/buff-json-branch-review-20260905`; the working branch and user worktrees were not modified. All three builds used Java 21.0.11, protobuf-java 4.34.1 and fastjson2 2.0.63, including overriding the two branches’ original 2.0.62 property. Maven command: `mvn -B -ntp package -Dspotless.skip=true -Dfastjson2.version=2.0.63` with JAVA_HOME set to Corretto 21.0.11. + +| Snapshot | Tests | Failures/errors | +|----------|------:|----------------:| +| main | 482 | 0 | +| perf | 484 | 0 | +| encoding | 560 | 0 | + +The official external protobuf conformance runner was not executed. Java 25 was used for benchmark execution, not a second full Maven test suite. Local eager-clarke uncommitted work was not included in these three Maven builds; its typed-schema fix was separately compiled for a narrow behavioral probe. + +JMH 1.37, macOS arm64, Amazon Corretto 21.0.11 and 25.0.4, one thread, fixed `-Xms256m -Xmx256m`, GC profiler. Initial screening: one fork, 2 × 500ms warmup, 3 × 500ms measurement; all 14 existing relevant benchmark methods, all three snapshots, both JDKs. Confirmation: two forks, 3 × 1s warmup, 4 × 1s measurement, targeted methods, reversed variant order on Java 25. Runs were sequential; no concurrent build or other JMH run was launched during confirmation. + +Two isolated overlays were compiled against main: **context** replaces only BuffJsonEncoder with the encoding-branch version; **wkt** replaces only WellKnownTypes with the perf-branch version. They use main’s generated encoders and all other main runtime classes, allowing separation from packed names and other branch changes. + +Timing variance on the shared desktop is material: one confirmation baseline for SimpleMessage codegen varied from 6.7M to 18.5M ops/s. Treat unstable throughput comparisons as inconclusive. Allocation measurements and the large repeated WKT effects are more useful than small percentages. These runs do not establish x86, other JVM vendors, virtual-thread, cold-start, or every Java 21+ release performance. + +### Confirmation results + +Changes below compare against main, except the packed-name row, which compares full encoding against the context-only overlay. Throughput deltas are estimates; see the raw confidence intervals. Allocation values are measured means rounded to the nearest byte. + +| Case | Java 21 throughput | Java 25 throughput | Allocation: before → after | +|----------------------------------------------------------------|-------------------------:|-----------------------------------:|---------------------------------------------------------------| +| Isolated WKT helper, Struct runtime | **+91.0%** | **+122.5%** | 21: 1,369 → 847 B/op; 25: 1,305 → 736 B/op | +| Isolated WKT helper, Timestamp runtime | **+43.0%** | **+34.6%** | Both: 568 → 464 B/op | +| perf branch, typed map-heavy | **+28.8%** | +18.6%, wide/overlapping intervals | 21: 6,983 → 4,805 B/op; 25: 6,899 → 4,889 B/op | +| Context-only, SimpleMessage | Inconclusive timing | No established throughput gain | Both paths/JDKs: **−88 B/op**; UTF-16 approximately 296 → 208 | +| Packed names beyond context-only, SimpleMessage codegen UTF-16 | +3.1%, intervals overlap | +0.8%, intervals overlap | No additional saving: 208 → 208 B/op | +| Indexed repeated getters, compiled repeated-heavy | −3.3%, intervals overlap | −4.7%, intervals overlap | No saving: 5,385 → 5,385 B/op | + +The initial short screening suggested a Java 21 typed-map regression. The longer two-fork run did not reproduce it and instead showed +28.8%. The final recommendation uses the longer run, with the Java 25 uncertainty retained. The repeated-heavy encoding-branch result likewise shows no established throughput gain; its 88-byte saving matches context reuse, so it is not evidence for the primitive-list rewrite. + +The selected map change was not isolated from every other perf-branch change. Its elimination of numeric-key String intermediates explains a concrete benefit; remeasure the extracted implementation before setting budgets. Likewise, the WKT overlay contains the other WellKnownTypes changes in that branch, but does not contain packed names, shared context, generator changes, or typed-schema changes. + +Additional probes on both JDKs passed 192 output comparisons per snapshot across 16 fixtures (WKT helpers plus Empty), compiled and DynamicMessage inputs, both writer encodings, and three nested-writer configurations. Separate probes reproduced the global-context and array-depth differences, the decoder-generation compile failure, and the remote deprecated-field omission. Compiling only the local eager-clarke TypedMessageSchema fix against main makes the standalone deprecated-field fixture agree with reflection; the full uncommitted change set was not built. + +### Evidence and reproduction + +- Raw results, confidence intervals and allocation summary (local: `benchmark-reports/branch-review-20260905/summary.csv`); revision/environment manifest (local: `benchmark-reports/branch-review-20260905/manifest.json`). +- Screening runner (local: `benchmark-reports/branch-review-20260905/run_screen.py`) and confirmation runner (local: `benchmark-reports/branch-review-20260905/run_confirm.py`). These run against the retained snapshots in `/private/tmp/buff-json-branch-review-20260905`; the saved copies can be copied there or their `root` adjusted. Both scripts include exact JVM paths, filters, heap settings, forks, iteration counts and durations. +- Behavior/DynamicMessage probe (local: `benchmark-reports/branch-review-20260905/BehaviorProbe.java`), encoding-branch Java 21 output (local: `benchmark-reports/branch-review-20260905/encoding-behavior-probe-jdk21.log`), and main output (local: `benchmark-reports/branch-review-20260905/main-behavior-probe-jdk21.log`). +- Generator probe (local: `benchmark-reports/branch-review-20260905/GeneratorProbe.java`) and compiler diagnostics (local: `benchmark-reports/branch-review-20260905/generator-probe.log`). +- Deprecated-field probe (local: `benchmark-reports/branch-review-20260905/DeprecatedBehaviorProbe.java`), remote output (local: `benchmark-reports/branch-review-20260905/encoding-deprecated-probe.log`), and local-fix output (local: `benchmark-reports/branch-review-20260905/local-deprecated-deprecated-probe.log`). + +The raw evidence is stored under the repository’s existing ignored `benchmark-reports/` directory. Only this report was added during the initial analysis; the follow-up port report includes its compact measurement data. No installed dependencies were replaced. To rebuild snapshots, export the three immutable revisions above and run the Maven command with Java 21 and fastjson2 2.0.63; compile the two specified single-class overlays against main’s benchmark jar before running the confirmation script. + +### Source pointers + +- [WKT helper optimizations](https://github.com/suboptimal-solutions/buff-fastjson-java/blob/1d8b9a7fabb2cee22277b0239fc709cd911aa843/buff-json/src/main/java/io/suboptimal/buffjson/internal/WellKnownTypes.java#L183) and [exact uint64 buffer](https://github.com/suboptimal-solutions/buff-fastjson-java/blob/1d8b9a7fabb2cee22277b0239fc709cd911aa843/buff-json/src/main/java/io/suboptimal/buffjson/internal/WellKnownTypes.java#L316). +- [Typed map-key change](https://github.com/suboptimal-solutions/buff-fastjson-java/blob/1d8b9a7fabb2cee22277b0239fc709cd911aa843/buff-json/src/main/java/io/suboptimal/buffjson/internal/typed/TypedFieldAccessor.java#L426). +- [Shared context](https://github.com/suboptimal-solutions/buff-fastjson-java/blob/2b68831737de35f7142a534d97d9115e6670b061/buff-json/src/main/java/io/suboptimal/buffjson/BuffJsonEncoder.java#L50). +- [Unescaped decoder literal](https://github.com/suboptimal-solutions/buff-fastjson-java/blob/2b68831737de35f7142a534d97d9115e6670b061/buff-json-protoc-plugin/src/main/java/io/suboptimal/buffjson/protoc/DecoderGenerator.java#L63), [remaining typed deprecated-field skip](https://github.com/suboptimal-solutions/buff-fastjson-java/blob/2b68831737de35f7142a534d97d9115e6670b061/buff-json/src/main/java/io/suboptimal/buffjson/internal/typed/TypedMessageSchema.java#L84), and [bulk-string generation](https://github.com/suboptimal-solutions/buff-fastjson-java/blob/2b68831737de35f7142a534d97d9115e6670b061/buff-json-protoc-plugin/src/main/java/io/suboptimal/buffjson/protoc/EncoderGenerator.java#L346). + +## Import sequence + +1. Preserve and finish the independent correctness work: deprecated-field cross-path behavior, escaped names in both generators, unsigned map keys. +2. Import the typed WKT helper changes and exact-size uint64 buffer as focused changes. Add DynamicMessage fallback coverage and WKT allocation benchmarks. +3. Add the small presence/descriptor hoists while retaining existing field-name infrastructure; leave the indexed reflection-map rewrite for a dedicated benchmark. Reuse the stashed benchmark matrix as ordinary JMH; expand its shapes for map-heavy and WKT cases. +4. Evaluate context reuse as a separate API/behavior change; its 88 B/op saving makes it worth pursuing. +5. Leave packed names, repeated-list rewrites, CodSpeed infrastructure, buffer-size prediction, encoder memoization, and new low-level mechanisms out of this import. Reconsider only with isolated evidence for the deployment workloads. + +No merges, cherry-picks, dependency edits, or production-code changes were made by this review. diff --git a/docs/performance-port-2026-09-05.md b/docs/performance-port-2026-09-05.md new file mode 100644 index 0000000..20c621b --- /dev/null +++ b/docs/performance-port-2026-09-05.md @@ -0,0 +1,60 @@ +# Selected performance port — 2026-09-05 + +This implements the selected changes from [the branch review](performance-branch-review-2026-09-05.md) on main (`cb106d53960bae666287b7d3ee44af1f1c65d09b`). + +## Included + +- Concrete Timestamp/Duration/Struct/Value/ListValue getters, retaining descriptor-based DynamicMessage fallback and validation. +- Direct quoted numeric map keys in generated, typed, and reflection paths, including correct unsigned uint32/fixed32/uint64/fixed64 output. Boolean keys use constant strings; long keys use a guarded String fallback under BrowserCompatible/WriteClassName to preserve key spelling. +- One exact-size buffer for large uint64 values, cached map descriptors and typed WKT checks, and presence checks before reflective reads. +- Deprecated fields on every path, with deprecation warnings suppressed in generated codecs. This reuses the independent local deprecated-field fix. +- JSON-escaped UTF-16/UTF-8 name constants and Java literal escaping in both codec generators. Custom Unicode, quotes, backslashes, control characters, and literal backslash-u names compile and round-trip. +- The standalone JMH encoding matrix from stash@0, expanded to simple, complex, map-heavy, Struct, and Timestamp messages across generated/typed/reflection and UTF-16/UTF-8 output. + +Context reuse remains a separate API/configuration change. Packed names, primitive-list rewrites, bulk string-list writes, and CodSpeed infrastructure remain deferred. + +## Validation + +Clean `mvn -B -ntp clean verify` runs build the full Maven reactor on Corretto Java 21.0.11 and 25.0.4, targeting Java 21 with `-Xlint:all,-processing -Werror`. All 496 tests pass, including actual DynamicMessage WKTs and unsigned map entries, deprecated-field fixtures, and custom-name round-trips in both output encodings and decoders. + +JsonFormat 4.34.1 itself does not escape arbitrary custom field names when printing. The custom-name regression test uses its valid proto-name output for field values, renames keys with JSON escaping, and validates BuffJson output through JsonFormat's parser. + +The official external conformance runner was unavailable (`CONF_TEST_PATH` unset; `conformance_test_runner` absent from PATH). The conformance testee builds successfully; the repository's Maven conformance tests above ran. + +## Measurements + +| Java | Benchmark | Throughput change | B/op, main → port | 99.9% CIs overlap | +|------|--------------------------------------|------------------:|------------------:|-------------------| +| 21 | SimpleMessageBenchmark.compiledUtf16 | -2.3% | 296 → 296 | yes | +| 21 | SimpleMessageBenchmark.compiledUtf8 | -1.2% | 272 → 272 | yes | +| 21 | WktBenchmark.structRuntime | +115.9% | 1369 → 847 | no | +| 21 | WktBenchmark.timestampRuntime | +37.0% | 568 → 464 | no | +| 21 | RepeatedAndMapBenchmark.mapCompiled | +4.9% | 8241 → 5025 | yes | +| 21 | RepeatedAndMapBenchmark.mapRuntime | +18.4% | 6983 → 4805 | yes | +| 25 | SimpleMessageBenchmark.compiledUtf16 | -0.5% | 296 → 296 | yes | +| 25 | SimpleMessageBenchmark.compiledUtf8 | +4.0% | 272 → 272 | yes | +| 25 | WktBenchmark.structRuntime | +124.7% | 1305 → 736 | no | +| 25 | WktBenchmark.timestampRuntime | +39.6% | 568 → 464 | no | +| 25 | RepeatedAndMapBenchmark.mapCompiled | +7.8% | 6599 → 4590 | yes | +| 25 | RepeatedAndMapBenchmark.mapRuntime | +25.8% | 6983 → 4805 | no | + +Struct and Timestamp gains are clear on both JVMs. Map allocation reduction is consistent; the Java 21 map throughput intervals overlap, as do generated-map intervals on Java 25. Scalar controls show no established throughput change and identical allocations. Overlap is a conservative reading of JMH intervals, not a separate statistical significance test. + +The map rows come from the final map comparison, rerun after the writer-feature fixes. Those fixes do not affect the measured scalar or WKT shapes. [Full rates, errors, and allocation summary](performance-results/2026-09-05-comparison.csv). + +All 14 allocation budgets pass on Java 21, including new typed Struct/Timestamp UTF-16/UTF-8 checks and generated/typed map checks. The UTF-8 Struct budget was set to 900 B/op after measuring 712 B/op. [Measured results](performance-results/2026-09-05-allocation.csv). + +The comparison runs main and the combined port sequentially, with order reversed on Java 25. It uses one thread, two forks, three 1-second warmups, four 1-second measurements, a 256 MiB fixed heap, and JMH's GC profiler. Both jars target Java 21 and use protobuf 4.34.1 and fastjson2 2.0.63. The retained main jar was built with javac 21; the port jar came from the final clean javac 25 build. Compiler version was therefore not held constant in this follow-up; the original branch review also contains same-compiler isolated measurements. These are local microbenchmarks, not application throughput guarantees. + +The compact comparison and allocation CSV files are included alongside this report. Full JMH JSON, logs, and the original run scripts remain local under the ignored `benchmark-reports/port-20260905/` directory. + +To repeat the measured subset, build main and this branch in separate checkouts and run each jar sequentially with each desired JVM: + +```bash +"$JAVA_HOME/bin/java" -jar "$BENCHMARK_JAR" \ + 'SimpleMessageBenchmark.(compiledUtf16|compiledUtf8)$|WktBenchmark.(structRuntime|timestampRuntime)$|RepeatedAndMapBenchmark.(mapCompiled|mapRuntime)$' \ + -t 1 -f 2 -wi 3 -i 4 -w 1s -r 1s -prof gc \ + -jvmArgs '-Xms256m -Xmx256m' -foe true -rf json -rff "$RESULTS_FILE" +``` + +Build with `mvn -B -ntp clean package`, set `BENCHMARK_JAR` to each checkout's `buff-json-benchmarks/target/benchmarks.jar`, and use a distinct `RESULTS_FILE` for each JVM/variant. Run `./allocation-check.sh` from this branch for the 14 allocation guards. diff --git a/docs/performance-results/2026-09-05-allocation.csv b/docs/performance-results/2026-09-05-allocation.csv new file mode 100644 index 0000000..43aecaa --- /dev/null +++ b/docs/performance-results/2026-09-05-allocation.csv @@ -0,0 +1,15 @@ +benchmark,bytes_per_op,budget_bytes_per_op +io.suboptimal.buffjson.benchmarks.ComplexMessageBenchmark.buffJsonCompiled,1483.7288867311065,2300 +io.suboptimal.buffjson.benchmarks.ComplexMessageBenchmark.buffJsonRuntime,1291.7290202525248,2500 +io.suboptimal.buffjson.benchmarks.DoubleHeavyBenchmark.compiledUtf16,1773.2836602194802,2200 +io.suboptimal.buffjson.benchmarks.DoubleHeavyBenchmark.compiledUtf8,1749.283661703668,2100 +io.suboptimal.buffjson.benchmarks.EncodePathsBenchmark.structTypedUtf8,711.6902343329957,900 +io.suboptimal.buffjson.benchmarks.EncodePathsBenchmark.timestampTypedUtf8,439.9691807059019,520 +io.suboptimal.buffjson.benchmarks.RepeatedAndMapBenchmark.mapCompiled,5025.211076782879,6200 +io.suboptimal.buffjson.benchmarks.RepeatedAndMapBenchmark.mapRuntime,4805.49474038729,6000 +io.suboptimal.buffjson.benchmarks.SimpleMessageBenchmark.compiledUtf16,295.5392393265975,380 +io.suboptimal.buffjson.benchmarks.SimpleMessageBenchmark.compiledUtf8,271.539219108242,350 +io.suboptimal.buffjson.benchmarks.SimpleMessageBenchmark.runtimeUtf16,295.5392850337711,400 +io.suboptimal.buffjson.benchmarks.SimpleMessageBenchmark.runtimeUtf8,271.5392464597608,380 +io.suboptimal.buffjson.benchmarks.WktBenchmark.structRuntime,735.6897812663278,1050 +io.suboptimal.buffjson.benchmarks.WktBenchmark.timestampRuntime,463.96922768788244,550 diff --git a/docs/performance-results/2026-09-05-comparison.csv b/docs/performance-results/2026-09-05-comparison.csv new file mode 100644 index 0000000..f0572d8 --- /dev/null +++ b/docs/performance-results/2026-09-05-comparison.csv @@ -0,0 +1,13 @@ +jdk,benchmark,main_ops_s,port_ops_s,main_error,port_error,change_percent,main_bytes_op,port_bytes_op,confidence_intervals_overlap +21,SimpleMessageBenchmark.compiledUtf16,19819831.060981713,19359661.644007273,1059311.6067293799,2860679.9277991652,-2.3217625597240943,295.5394234011239,295.5394116187978,True +21,SimpleMessageBenchmark.compiledUtf8,22603786.78887583,22326910.02835609,1134548.5442789826,433715.85874387156,-1.2249131665672985,271.53935907369333,271.5393681074146,True +21,WktBenchmark.structRuntime,594753.4632972502,1283904.6007553686,54956.349369565716,310349.1612069255,115.87173173192427,1368.7145912436313,847.3918014789379,False +21,WktBenchmark.timestampRuntime,5999595.927839809,8219843.132448305,345707.42193555436,323892.2157947425,37.006612300437205,567.9542761836005,463.96958948189257,False +21,RepeatedAndMapBenchmark.mapCompiled,193702.38742926953,203199.84277414606,11252.656730893465,25697.053151339405,4.903117339400143,8241.151748739294,5025.230520461547,True +21,RepeatedAndMapBenchmark.mapRuntime,132183.7733471802,156561.7946377851,26274.10428531031,57779.40254856843,18.442521856730565,6983.375519506152,4805.494396440039,True +25,SimpleMessageBenchmark.compiledUtf16,19854183.252377752,19753333.120199922,1650888.4854612453,1259839.5976206919,-0.5079540714209574,295.53941541640717,295.5394194018537,True +25,SimpleMessageBenchmark.compiledUtf8,22679028.966035374,23592546.109520607,2550226.6682571582,804617.6938627257,4.028025824444859,271.53936935445745,271.53935688599057,True +25,WktBenchmark.structRuntime,651781.7920683313,1464590.0175013887,17463.533185131622,50929.28570790199,124.70557406240714,1304.8840740826392,735.6920584867745,False +25,WktBenchmark.timestampRuntime,5797190.258308777,8095718.897283003,611461.6554062259,470025.8517027236,39.649011616961815,567.9543374124257,463.96961703862803,False +25,RepeatedAndMapBenchmark.mapCompiled,188541.6273728527,203262.1796007568,32574.72460861236,44554.80711236011,7.807587339210387,6599.3798079995395,4589.5008639801845,True +25,RepeatedAndMapBenchmark.mapRuntime,137890.06649574192,173511.50239247133,16052.105639812267,16256.163546552576,25.833213952238832,6983.415312904388,4805.498591330895,False From 2facf0aa5a0623217d8130bff956870a2dde8569 Mon Sep 17 00:00:00 2001 From: Dmitry Petrov Date: Sun, 6 Sep 2026 07:06:35 +0100 Subject: [PATCH 2/2] ci: compare JMH performance and publish PR reports Build base and candidate with the same JDK and shared benchmark inputs on Java 21 and 25. Keep throughput signals advisory, enforce complete allocation data, and publish validated reports from a separate trusted workflow. --- .github/performance/.gitignore | 1 + .github/performance/compare.py | 131 +++++++++++ .github/performance/publish.py | 160 +++++++++++++ .github/performance/report.py | 142 ++++++++++++ .github/performance/tests/test_performance.py | 215 ++++++++++++++++++ .github/workflows/ci.yml | 27 ++- .github/workflows/performance-report.yml | 32 +++ .github/workflows/performance.yml | 88 +++++++ allocation-check.sh | 24 +- buff-json-benchmarks/CLAUDE.md | 4 + docs/performance-ci.md | 70 ++++++ run-benchmarks.sh | 2 +- 12 files changed, 884 insertions(+), 12 deletions(-) create mode 100644 .github/performance/.gitignore create mode 100644 .github/performance/compare.py create mode 100644 .github/performance/publish.py create mode 100644 .github/performance/report.py create mode 100644 .github/performance/tests/test_performance.py create mode 100644 .github/workflows/performance-report.yml create mode 100644 .github/workflows/performance.yml create mode 100644 docs/performance-ci.md diff --git a/.github/performance/.gitignore b/.github/performance/.gitignore new file mode 100644 index 0000000..c18dd8d --- /dev/null +++ b/.github/performance/.gitignore @@ -0,0 +1 @@ +__pycache__/ diff --git a/.github/performance/compare.py b/.github/performance/compare.py new file mode 100644 index 0000000..8f331b6 --- /dev/null +++ b/.github/performance/compare.py @@ -0,0 +1,131 @@ +#!/usr/bin/env python3 +"""Build two immutable revisions, then compare JMH on this machine/JDK.""" +import argparse +import hashlib +import json +import os +from pathlib import Path +import platform +import re +import shutil +import subprocess +import tarfile +import tempfile + +from report import METHODS, SMOKE_METHODS, PREFIX, compare, render, validate + + +def output(command, cwd=None): + return subprocess.check_output(command, cwd=cwd, text=True).strip() + + +def revision(repo, ref): + return output(['git', 'rev-parse', '--verify', '--end-of-options', ref + '^{commit}'], repo) + + +def export(repo, sha, destination): + destination.mkdir() + archive = destination.parent / (destination.name + '.tar') + with archive.open('wb') as stream: + subprocess.run(['git', 'archive', '--format=tar', sha], cwd=repo, stdout=stream, check=True) + with tarfile.open(archive) as source: + # Reject archive entries escaping the exported checkout, including symlinks. + source.extractall(destination, filter='data') + archive.unlink() + + +def run_logged(command, log, cwd=None): + with log.open('w') as stream: + try: + subprocess.run(command, cwd=cwd, stdout=stream, stderr=subprocess.STDOUT, check=True) + except subprocess.CalledProcessError: + print(log.read_text()[-8000:], flush=True) + raise + + +def main(): + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument('--base', required=True, help='Immutable SHA or local ref') + parser.add_argument('--candidate', default='HEAD') + parser.add_argument('--output', required=True) + parser.add_argument('--java', required=True, choices=['21', '25']) + parser.add_argument('--smoke', action='store_true', help='Build both revisions; run only 3 short cases') + args = parser.parse_args() + repo = Path(output(['git', 'rev-parse', '--show-toplevel'])) + dest = Path(args.output).resolve() + dest.mkdir(parents=True, exist_ok=False) + java_home = Path(os.environ['JAVA_HOME']).resolve() + java = str(java_home / 'bin/java') + javac = output([str(java_home / 'bin/javac'), '-version']) + if not javac.startswith('javac ' + args.java + '.'): + raise ValueError('JAVA_HOME does not match requested Java version') + base, candidate = revision(repo, args.base), revision(repo, args.candidate) + harness = output(['git', 'rev-parse', candidate + ':buff-json-benchmarks/src'], repo) + methods = SMOKE_METHODS if args.smoke else METHODS + metadata = {'base_sha': base, 'candidate_sha': candidate, 'harness_sha': harness, 'java': args.java, + 'compiler': javac, 'os': platform.platform(), 'architecture': platform.machine(), + 'cpu': platform.processor(), 'runner_image': os.environ.get('ImageVersion', 'local'), + 'run_id': os.environ.get('GITHUB_RUN_ID', 'local')} + if Path('/proc/cpuinfo').exists(): + metadata['cpu'] = next((line.split(':', 1)[1].strip() for line in Path('/proc/cpuinfo').read_text().splitlines() + if line.startswith('model name')), metadata['cpu']) + (dest / 'metadata.json').write_text(json.dumps(metadata, indent=2) + '\n') + all_results = {'base': [], 'candidate': []} + with tempfile.TemporaryDirectory(prefix='buff-json-performance-') as work: + work = Path(work) + trees = {name: work / name for name in all_results} + for name, sha in (('base', base), ('candidate', candidate)): + export(repo, sha, trees[name]) + # Compare identical workload definitions, including generated proto fixtures. + # Each revision still uses its OWN runtime, protoc plugin and dependency versions. + source_path = 'buff-json-benchmarks/src' + shutil.rmtree(trees['base'] / source_path) + shutil.copytree(trees['candidate'] / source_path, trees['base'] / source_path) + jars = {} + for name, tree in trees.items(): + print('BUILD', name, flush=True) + cache_root = Path(os.environ.get('PERFORMANCE_MAVEN_REPO', str(work / 'maven'))) + local_repo = cache_root / name + local_repo.mkdir(parents=True, exist_ok=True) + # Cached third-party dependencies are reusable; our SNAPSHOT artifacts + # must always come from the exact revision being built. + shutil.rmtree(local_repo / 'io/github/suboptimal-solutions', ignore_errors=True) + command = ['mvn', '-B', '-ntp', 'clean', 'package', '-DskipTests', '-Dspotless.skip=true', + '-Dmaven.repo.local=' + str(local_repo)] + run_logged(command, dest / (name + '-build.log'), tree) + jars[name] = tree / 'buff-json-benchmarks/target/benchmarks.jar' + metadata[name + '_jar_sha256'] = hashlib.sha256(jars[name].read_bytes()).hexdigest() + # All builds finish before measuring. Alternate variant order per method, + # reversing it on Java 25; both forks for a method share a single JMH run. + for index, method in enumerate(methods): + order = ('base', 'candidate') if (index + int(args.java == '25')) % 2 == 0 else ('candidate', 'base') + for name in order: + stem = dest / (name + '-' + method) + command = [java, '-jar', str(jars[name]), '^' + re.escape(PREFIX + method) + '$', + '-t', '1', '-f', '1' if args.smoke else '2', + '-wi', '1' if args.smoke else '3', '-i', '3' if args.smoke else '4', + '-w', '200ms' if args.smoke else '1s', '-r', '200ms' if args.smoke else '1s', + '-prof', 'gc', '-foe', 'true', '-jvmArgs', '-Xms256m -Xmx256m', + '-rf', 'json', '-rff', str(stem) + '.json'] + print('MEASURE', method, name, flush=True) + run_logged(command, Path(str(stem) + '.log')) + measured = json.loads(Path(str(stem) + '.json').read_text()) + if len(measured) != 1: + raise ValueError('Expected exactly one completed benchmark') + all_results[name].extend(measured) + for name, results in all_results.items(): + (dest / (name + '.json')).write_text(json.dumps(results, indent=2) + '\n') + summary = compare(all_results['base'], all_results['candidate'], metadata, methods) + validate(summary, full=not args.smoke) + (dest / 'summary.json').write_text(json.dumps(summary, indent=2, allow_nan=False) + '\n') + (dest / 'metadata.json').write_text(json.dumps(metadata, indent=2) + '\n') + report = render(summary) + (dest / 'report.md').write_text(report) + if os.environ.get('GITHUB_STEP_SUMMARY'): + with open(os.environ['GITHUB_STEP_SUMMARY'], 'a') as stream: + stream.write(report) + print(report, flush=True) + + +if __name__ == '__main__': + main() diff --git a/.github/performance/publish.py b/.github/performance/publish.py new file mode 100644 index 0000000..bea0eea --- /dev/null +++ b/.github/performance/publish.py @@ -0,0 +1,160 @@ +"""Trusted workflow_run publisher; benchmark artifacts are data, never code.""" +import io +import json +import os +from pathlib import Path +import urllib.request +import urllib.parse +import zipfile + +from report import render, validate, SHA + +MARKER = '' +MAX_ARCHIVE_BYTES = 1_000_000 +MAX_JSON_BYTES = 250_000 + + +class SafeRedirect(urllib.request.HTTPRedirectHandler): + def redirect_request(self, request, fp, code, message, headers, newurl): + if urllib.parse.urlparse(newurl).scheme != 'https': + raise ValueError('Refusing non-HTTPS artifact redirect') + redirected = super().redirect_request(request, fp, code, message, headers, newurl) + if urllib.parse.urlparse(request.full_url).netloc != urllib.parse.urlparse(newurl).netloc: + redirected.remove_header('Authorization') + return redirected + + +class GitHub: + def __init__(self, token, repository): + self.repository = repository + self.prefix = 'https://api.github.com/repos/' + repository + self.token = token + + def request(self, path, method='GET', data=None, binary=False): + url = self.prefix + path + request = urllib.request.Request(url, method=method, + data=None if data is None else json.dumps(data).encode(), + headers={'Authorization': 'Bearer ' + self.token, + 'Accept': 'application/vnd.github+json', + 'X-GitHub-Api-Version': '2022-11-28', + 'Content-Type': 'application/json'}) + with urllib.request.build_opener(SafeRedirect()).open(request, timeout=60) as response: + content = response.read(MAX_ARCHIVE_BYTES + 1 if binary else 4_000_000) + if binary: + if len(content) > MAX_ARCHIVE_BYTES: + raise ValueError('Oversized artifact archive') + return content + return json.loads(content) if content else None + + def pages(self, path, key=None): + for page in range(1, 101): + separator = '&' if '?' in path else '?' + data = self.request(path + separator + f'per_page=100&page={page}') + items = data[key] if key else data + yield from items + if len(items) < 100: + return + raise ValueError('API pagination limit exceeded') + + +def read_summary(archive, java, candidate): + with zipfile.ZipFile(io.BytesIO(archive)) as package: + entries = package.infolist() + if len(entries) != 1 or entries[0].filename != 'summary.json' or entries[0].file_size > MAX_JSON_BYTES: + raise ValueError('Expected one bounded summary.json artifact') + data = json.loads(package.read(entries[0])) + validate(data) + if data['metadata']['java'] != java or data['metadata']['candidate_sha'] != candidate: + raise ValueError('Artifact does not match triggering run') + return data + + +def current_pull_request(api, run, base): + # Derive association from GitHub, not a PR number supplied by an artifact. + if run['event'] != 'pull_request': + return None + candidates = list(api.pages('/commits/' + run['head_sha'] + '/pulls')) + matching = [] + for candidate in candidates: + pr = api.request('/pulls/' + str(int(candidate['number']))) + if (pr['state'] == 'open' and pr['base']['repo']['full_name'] == api.repository + and pr['head']['sha'] == run['head_sha'] and pr['base']['sha'] == base): + matching.append(pr) + # Ambiguity or a newer PR revision must not update an unrelated/current report. + return matching[0] if len(matching) == 1 else None + + +def publish(api, run_id): + run = api.request('/actions/runs/' + str(run_id)) + workflow = api.request('/actions/workflows/' + str(run['workflow_id'])) + if (workflow['path'] != '.github/workflows/performance.yml' + or run['event'] not in ('pull_request', 'push', 'workflow_dispatch') + or run['status'] != 'completed' or not SHA.fullmatch(run['head_sha'])): + raise ValueError('Unexpected triggering workflow') + run_url = f'https://github.com/{api.repository}/actions/runs/{run_id}' + body = [MARKER, '## Performance comparison', '', f'[Workflow and raw JMH artifacts]({run_url})', f"Commit: {run['head_sha']}", ''] + summaries = [] + error = None + try: + artifacts = list(api.pages('/actions/runs/' + str(run_id) + '/artifacts', 'artifacts')) + for java in ('21', '25'): + selected = [a for a in artifacts if a['name'] == 'performance-summary-java' + java and not a['expired']] + if len(selected) != 1: + raise ValueError('Missing or ambiguous Java ' + java + ' summary') + artifact = selected[0] + if artifact['size_in_bytes'] > MAX_ARCHIVE_BYTES: + raise ValueError('Oversized summary artifact') + archive = api.request('/actions/artifacts/' + str(int(artifact['id'])) + '/zip', binary=True) + summaries.append(read_summary(archive, java, run['head_sha'])) + if len({(s['metadata']['base_sha'], s['metadata']['harness_sha']) for s in summaries}) != 1: + raise ValueError('Matrix jobs compared different sources') + except (ValueError, KeyError, TypeError, zipfile.BadZipFile) as problem: + # Never interpolate arbitrary artifact strings into a privileged comment. + print('Incomplete/invalid performance data:', type(problem).__name__) + error = 'Performance measurements are incomplete or invalid. Inspect the workflow logs; this is not a passing performance result.' + if error: + body += [error] + else: + body += [render(s) for s in summaries] + if run['conclusion'] != 'success': + body += ['', 'The measurement workflow did not succeed; results may be incomplete.'] + text = '\n'.join(body) + conclusion = 'failure' if error or run['conclusion'] != 'success' else 'neutral' + # Neutral reports intentionally do not turn noisy wall-clock changes into a gate. + check = {'name': 'Performance report', 'head_sha': run['head_sha'], 'status': 'completed', + 'conclusion': conclusion, 'details_url': run_url, 'external_id': 'performance-' + str(run_id), + 'output': {'title': 'JMH comparison (Java 21 and 25)', 'summary': text}} + existing = [c for c in api.pages('/commits/' + run['head_sha'] + '/check-runs?check_name=Performance%20report', 'check_runs') + if c.get('external_id') == check['external_id']] + if existing: + del check['head_sha'] + api.request('/check-runs/' + str(existing[0]['id']), 'PATCH', check) + else: + api.request('/check-runs', 'POST', check) + baseline = summaries[0]['metadata']['base_sha'] if not error else None + if error: + linked = [p for p in run.get('pull_requests', []) if p['head']['sha'] == run['head_sha']] + if len(linked) == 1 and SHA.fullmatch(linked[0]['base']['sha']): + baseline = linked[0]['base']['sha'] + if baseline: + pr = current_pull_request(api, run, baseline) + if pr: + path = '/issues/' + str(pr['number']) + '/comments' + comments = [c for c in api.pages(path) if c['user']['login'] == 'github-actions[bot]' + and c.get('body', '').startswith(MARKER)] + # Recheck immediately before writing to avoid replacing a newer report. + latest = api.request('/pulls/' + str(pr['number'])) + if latest['head']['sha'] == run['head_sha'] and latest['base']['sha'] == baseline: + if comments: + api.request('/issues/comments/' + str(comments[0]['id']), 'PATCH', {'body': text}) + else: + api.request(path, 'POST', {'body': text}) + return text + + +if __name__ == '__main__': + event = json.loads(Path(os.environ['GITHUB_EVENT_PATH']).read_text()) + api = GitHub(os.environ['GITHUB_TOKEN'], os.environ['GITHUB_REPOSITORY']) + result = publish(api, int(event['workflow_run']['id'])) + with open(os.environ['GITHUB_STEP_SUMMARY'], 'a') as stream: + stream.write(result) diff --git a/.github/performance/report.py b/.github/performance/report.py new file mode 100644 index 0000000..6a52887 --- /dev/null +++ b/.github/performance/report.py @@ -0,0 +1,142 @@ +"""Validate JMH measurements and render comparison reports (standard library only).""" +import math +import re + +PREFIX = 'io.suboptimal.buffjson.benchmarks.EncodePathsBenchmark.' +METHODS = tuple(shape + path + encoding for shape in ('simple', 'complex', 'map', 'struct', 'timestamp') + for path in ('Codegen', 'Typed', 'Reflection') for encoding in ('Utf16', 'Utf8')) +SMOKE_METHODS = ('simpleCodegenUtf16', 'mapTypedUtf8', 'structReflectionUtf16') +THROUGHPUT_THRESHOLD = 10.0 +ALLOCATION_THRESHOLD = 5.0 +SHA = re.compile(r'^[0-9a-f]{40}$') + + +def number(value, *, positive=False): + if isinstance(value, bool) or not isinstance(value, (int, float)) or not math.isfinite(value): + raise ValueError('Expected a finite number') + if value < 0 or value > 1e18 or (positive and value < 1e-9): + raise ValueError('Expected a nonnegative number') + return value + + +def measurements(raw, methods=METHODS): + if not isinstance(raw, list) or len(raw) != len(methods): + raise ValueError('Incomplete benchmark set') + found = {} + environment = None + for entry in raw: + name = entry['benchmark'] + if not name.startswith(PREFIX) or name[len(PREFIX):] not in methods or name in found: + raise ValueError('Unexpected or duplicate benchmark: ' + name) + if entry['mode'] != 'thrpt' or entry.get('params'): + raise ValueError('Expected unparameterized throughput benchmarks') + metric = entry['primaryMetric'] + if metric['scoreUnit'] != 'ops/s': + raise ValueError('Expected ops/s') + score = number(metric['score'], positive=True) + error = number(metric['scoreError']) + allocation = entry['secondaryMetrics']['gc.alloc.rate.norm'] + if allocation['scoreUnit'] != 'B/op': + raise ValueError('Expected B/op') + alloc = number(allocation['score']) + samples = metric['rawData'] + if len(samples) != entry['forks'] or any(len(fork) != entry['measurementIterations'] for fork in samples): + raise ValueError('Incomplete JMH forks/iterations') + for fork in samples: + for sample in fork: + number(sample, positive=True) + current = {key: entry[key] for key in ('jmhVersion', 'jdkVersion', 'vmName', 'vmVersion', + 'jvmArgs', 'forks', 'warmupIterations', 'warmupTime', 'measurementIterations', + 'measurementTime', 'threads')} + if environment is not None and current != environment: + raise ValueError('Mixed JMH/JVM settings in one result') + environment = current + found[name] = {'score': score, 'error': error, 'allocation': alloc} + return found, environment + + +def classify(base, candidate): + delta = (candidate['score'] / base['score'] - 1) * 100 + # JMH reports 99.9% confidence intervals. Separation is deliberately a + # conservative alert heuristic, not a paired statistical significance test. + slower = candidate['score'] + candidate['error'] < base['score'] - base['error'] + faster = candidate['score'] - candidate['error'] > base['score'] + base['error'] + timing = 'regression signal' if delta <= -THROUGHPUT_THRESHOLD and slower else ( + 'improvement signal' if delta >= THROUGHPUT_THRESHOLD and faster else 'inconclusive') + added = candidate['allocation'] - base['allocation'] + alloc_percent = added / base['allocation'] * 100 if base['allocation'] else None + allocation_alert = added > 16 and (alloc_percent is None or alloc_percent > ALLOCATION_THRESHOLD) + return delta, timing, added, alloc_percent, allocation_alert + + +def compare(base_raw, candidate_raw, metadata, methods=METHODS): + base, base_env = measurements(base_raw, methods) + candidate, candidate_env = measurements(candidate_raw, methods) + if base_env != candidate_env: + raise ValueError('Base and candidate JVM/JMH settings differ') + rows = [] + for method in methods: + before, after = base[PREFIX + method], candidate[PREFIX + method] + delta, timing, added, alloc_percent, alloc_alert = classify(before, after) + rows.append({'method': method, 'base': before, 'candidate': after, + 'throughput_percent': delta, 'timing': timing, + 'allocation_bytes_delta': added, 'allocation_percent': alloc_percent, + 'allocation_alert': alloc_alert}) + return {'schema': 1, 'profile': 'full' if methods == METHODS else 'smoke', + 'metadata': metadata, 'environment': base_env, 'rows': rows} + + +def validate(summary, *, full=True): + if summary['schema'] != 1 or summary['profile'] not in ('full', 'smoke'): + raise ValueError('Unsupported report schema/profile') + methods = METHODS if summary['profile'] == 'full' else SMOKE_METHODS + if full and methods != METHODS: + raise ValueError('Smoke data cannot be published as a full report') + meta = summary['metadata'] + for key in ('base_sha', 'candidate_sha', 'harness_sha'): + if not SHA.fullmatch(meta[key]): + raise ValueError('Invalid source identity') + if meta['java'] not in ('21', '25'): + raise ValueError('Unexpected Java version') + env = summary['environment'] + if not str(env['jdkVersion']).startswith(meta['java'] + '.'): + raise ValueError('JDK does not match matrix identity') + if full and (env['forks'] != 2 or env['threads'] != 1 or env['warmupIterations'] != 3 + or env['measurementIterations'] != 4 or env['warmupTime'] != '1 s' + or env['measurementTime'] != '1 s' or env['jvmArgs'] != ['-Xms256m', '-Xmx256m']): + raise ValueError('Unexpected measurement protocol') + if [r['method'] for r in summary['rows']] != list(methods): + raise ValueError('Incomplete or reordered report rows') + for row in summary['rows']: + for variant in ('base', 'candidate'): + item = row[variant] + number(item['score'], positive=True) + number(item['error']) + number(item['allocation']) + # Recompute every classification; never trust artifact-provided prose. + delta, timing, added, percent, alert = classify(row['base'], row['candidate']) + row.update(throughput_percent=delta, timing=timing, allocation_bytes_delta=added, + allocation_percent=percent, allocation_alert=alert) + return summary + + +def render(summary): + validate(summary, full=False) + meta = summary['metadata'] + lines = [f"### Java {meta['java']} performance", '', + f"Base: {meta['base_sha']} → candidate: {meta['candidate_sha']}", + f"Shared benchmark source: {meta['harness_sha']}", '', + 'Throughput alerts are advisory. Existing allocation budgets are enforced separately.', + 'A timing signal needs at least 10% change and separated JMH 99.9% intervals; otherwise it is inconclusive.', + 'Allocation alerts need both >5% and >16 B/op growth (or >16 B/op from zero).', '', + '| Benchmark | Base ops/s | Candidate ops/s | Change | Timing | B/op base → candidate | Allocation |', + '|---|---:|---:|---:|---|---:|---|'] + for row in summary['rows']: + b, c = row['base'], row['candidate'] + lines.append(f"| {row['method']} | {b['score']:,.0f} ±{b['error']:,.0f} | " + f"{c['score']:,.0f} ±{c['error']:,.0f} | {row['throughput_percent']:+.1f}% | " + f"{row['timing']} | {b['allocation']:.1f} → {c['allocation']:.1f} | " + f"{'review increase' if row['allocation_alert'] else 'within alert threshold'} |") + if summary['profile'] == 'smoke': + lines += ['', '**Smoke run: validates the pipeline only; not performance evidence.**'] + return '\n'.join(lines) + '\n' diff --git a/.github/performance/tests/test_performance.py b/.github/performance/tests/test_performance.py new file mode 100644 index 0000000..2da2d6d --- /dev/null +++ b/.github/performance/tests/test_performance.py @@ -0,0 +1,215 @@ +import copy +import io +import json +import os +import re +import subprocess +import tempfile +import urllib.request +from pathlib import Path +import sys +import unittest +import zipfile + +sys.path.insert(0, str(Path(__file__).resolve().parents[1])) +import report +import publish + +BASE = 'a' * 40 +HEAD = 'b' * 40 +HARNESS = 'c' * 40 + + +def fixture(score=1000.0, allocation=100.0, java='21'): + return [{'benchmark': report.PREFIX + method, 'mode': 'thrpt', 'jmhVersion': '1.37', + 'jdkVersion': java + '.0.1', 'vmName': 'OpenJDK', 'vmVersion': java + '.0.1', + 'jvmArgs': ['-Xms256m', '-Xmx256m'], 'forks': 2, 'warmupIterations': 3, + 'warmupTime': '1 s', 'measurementIterations': 4, 'measurementTime': '1 s', 'threads': 1, + 'primaryMetric': {'score': score, 'scoreError': 5.0, 'scoreUnit': 'ops/s', + 'rawData': [[score] * 4, [score] * 4]}, + 'secondaryMetrics': {'gc.alloc.rate.norm': {'score': allocation, 'scoreUnit': 'B/op'}}} + for method in report.METHODS] + + +def summary(java='21'): + return report.compare(fixture(java=java), fixture(1100.0, 80.0, java), + {'base_sha': BASE, 'candidate_sha': HEAD, 'harness_sha': HARNESS, 'java': java}) + + +def archive(data, name='summary.json'): + stream = io.BytesIO() + with zipfile.ZipFile(stream, 'w') as package: + package.writestr(name, json.dumps(data)) + return stream.getvalue() + + +class Reports(unittest.TestCase): + def test_throughput_direction_and_allocation(self): + result = report.compare(fixture(), fixture(800, 130), + {'base_sha': BASE, 'candidate_sha': HEAD, 'harness_sha': HARNESS, 'java': '21'}) + row = result['rows'][0] + self.assertAlmostEqual(row['throughput_percent'], -20) + self.assertEqual(row['timing'], 'regression signal') + self.assertTrue(row['allocation_alert']) + self.assertIn('regression signal', report.render(result)) + + def test_noise_is_not_a_regression(self): + before, after = fixture(), fixture(800) + for item in after: + item['primaryMetric']['scoreError'] = 300 + result = report.compare(before, after, summary()['metadata']) + self.assertEqual(result['rows'][0]['timing'], 'inconclusive') + + def test_small_allocation_deltas_and_zero_baseline(self): + b = {'score': 100, 'error': 1, 'allocation': 0} + c = {'score': 100, 'error': 1, 'allocation': 16} + self.assertFalse(report.classify(b, c)[4]) + c['allocation'] = 17 + self.assertTrue(report.classify(b, c)[4]) + b['allocation'], c['allocation'] = 1000, 1020 + self.assertFalse(report.classify(b, c)[4]) + + def test_missing_duplicate_nonfinite_and_wrong_units_fail(self): + corruptions = [] + values = fixture(); values.pop(); corruptions.append(values) + values = fixture(); values[1] = copy.deepcopy(values[0]); corruptions.append(values) + values = fixture(); values[0]['primaryMetric']['scoreError'] = float('nan'); corruptions.append(values) + values = fixture(); values[0]['primaryMetric']['score'] = 0; corruptions.append(values) + values = fixture(); values[0]['primaryMetric']['scoreUnit'] = 'ms/op'; corruptions.append(values) + values = fixture(); values[0]['secondaryMetrics'] = {}; corruptions.append(values) + values = fixture(); values[0]['primaryMetric']['rawData'][1].pop(); corruptions.append(values) + for values in corruptions: + with self.subTest(values=values[0]['benchmark']): + with self.assertRaises((ValueError, KeyError)): + report.measurements(values) + + def test_compiler_runtime_and_jmh_protocol_must_match(self): + for key, value in [('jdkVersion', '25.0.1'), ('jmhVersion', '1.38'), ('jvmArgs', ['-Xmx1g'])]: + values = fixture() + for item in values: + item[key] = value + with self.assertRaises(ValueError): + report.compare(fixture(), values, summary()['metadata']) + + def test_publisher_recomputes_labels(self): + data = summary() + data['rows'][0]['timing'] = '@everyone forged verdict' + data['rows'][0]['throughput_percent'] = -999 + result = report.render(data) + self.assertNotIn('forged', result) + self.assertIn('+10.0%', result) + + def test_unexpected_names_smoke_and_protocol_are_rejected(self): + data = summary(); data['rows'][0]['method'] = '' + with self.assertRaises(ValueError): report.validate(data) + data = summary(); data['profile'] = 'smoke' + with self.assertRaises(ValueError): report.validate(data) + data = summary(); data['environment']['forks'] = 1 + with self.assertRaises(ValueError): report.validate(data) + + +class Artifacts(unittest.TestCase): + def test_valid_identity(self): + self.assertEqual(publish.read_summary(archive(summary()), '21', HEAD)['metadata']['candidate_sha'], HEAD) + + def test_wrong_commit_jdk_path_and_oversized_json(self): + for content, java, sha in [(archive(summary()), '21', BASE), (archive(summary()), '25', HEAD), + (archive(summary(), '../summary.json'), '21', HEAD)]: + with self.assertRaises(ValueError): publish.read_summary(content, java, sha) + data = summary(); data['padding'] = 'x' * publish.MAX_JSON_BYTES + with self.assertRaises(ValueError): publish.read_summary(archive(data), '21', HEAD) + + +class FakeGitHub: + repository = 'owner/repo' + + def __init__(self, *, stale=False, missing=False, existing=False, event='pull_request'): + self.stale, self.missing, self.existing = stale, missing, existing + self.run = {'workflow_id': 7, 'event': event, 'status': 'completed', 'head_sha': HEAD, + 'conclusion': 'success', 'pull_requests': [{'head': {'sha': HEAD}, 'base': {'sha': BASE}}]} + self.writes = [] + + def request(self, path, method='GET', data=None, binary=False): + if method != 'GET': + self.writes.append((path, method, data)); return {} + if path == '/actions/runs/1': return self.run + if path == '/actions/workflows/7': return {'path': '.github/workflows/performance.yml'} + if path.startswith('/actions/artifacts/'): + return archive(summary('21' if path.endswith('/21/zip') else '25')) + if path == '/pulls/4': + return {'state': 'open', 'number': 4, 'head': {'sha': BASE if self.stale else HEAD}, + 'base': {'sha': BASE, 'repo': {'full_name': self.repository}}} + raise AssertionError(path) + + def pages(self, path, key=None): + if path.endswith('/artifacts'): + return iter([] if self.missing else [{'name': 'performance-summary-java' + j, 'id': int(j), + 'expired': False, 'size_in_bytes': 20000} for j in ('21', '25')]) + if '/check-runs?' in path: return iter([]) + if path.endswith('/pulls'): return iter([{'number': 4}]) + if path == '/issues/4/comments': + return iter([{'id': 9, 'user': {'login': 'github-actions[bot]'}, 'body': publish.MARKER}] if self.existing else []) + raise AssertionError(path) + + +class Publishing(unittest.TestCase): + def test_comment_and_check(self): + api = FakeGitHub() + publish.publish(api, 1) + self.assertEqual([w[0] for w in api.writes], ['/check-runs', '/issues/4/comments']) + self.assertEqual(api.writes[0][2]['conclusion'], 'neutral') + + def test_update_instead_of_comment_spam(self): + api = FakeGitHub(existing=True); publish.publish(api, 1) + self.assertEqual(api.writes[-1][:2], ('/issues/comments/9', 'PATCH')) + + def test_stale_pr_and_main_push_only_get_commit_check(self): + for api in (FakeGitHub(stale=True), FakeGitHub(event='push')): + publish.publish(api, 1) + self.assertEqual([w[0] for w in api.writes], ['/check-runs']) + + def test_missing_results_are_failure_not_green(self): + api = FakeGitHub(missing=True); publish.publish(api, 1) + self.assertEqual(api.writes[0][2]['conclusion'], 'failure') + self.assertEqual(api.writes[-1][0], '/issues/4/comments') + self.assertIn('incomplete or invalid', api.writes[-1][2]['body']) + + + +class Redirects(unittest.TestCase): + def test_api_token_not_forwarded_to_artifact_host(self): + request = urllib.request.Request('https://api.github.com/artifact', headers={'Authorization': 'Bearer secret'}) + redirected = publish.SafeRedirect().redirect_request(request, None, 302, 'Found', {}, 'https://artifact.example/file') + self.assertIsNone(redirected.get_header('Authorization')) + with self.assertRaises(ValueError): + publish.SafeRedirect().redirect_request(request, None, 302, 'Found', {}, 'http://artifact.example/file') + + +class AllocationGate(unittest.TestCase): + def test_valid_over_budget_missing_and_nonfinite_data(self): + root = Path(__file__).resolve().parents[3] + script = root / 'allocation-check.sh' + budgets = dict(re.findall(r'"(io\.suboptimal\.buffjson\.benchmarks\.[^":]+):(\d+)"', script.read_text())) + good = [{'benchmark': name, 'secondaryMetrics': {'gc.alloc.rate.norm': + {'score': float(limit) / 2, 'scoreUnit': 'B/op'}}} for name, limit in budgets.items()] + cases = [(good, 0)] + over = copy.deepcopy(good); over[0]['secondaryMetrics']['gc.alloc.rate.norm']['score'] = 1e9; cases.append((over, 1)) + absent = copy.deepcopy(good); absent[0]['secondaryMetrics'] = {}; cases.append((absent, 1)) + invalid = copy.deepcopy(good); invalid[0]['secondaryMetrics']['gc.alloc.rate.norm']['score'] = 'NaN'; cases.append((invalid, 1)) + cases.append((good[:-1], 1)) + with tempfile.TemporaryDirectory() as work: + work = Path(work) + (work / 'benchmarks.jar').touch() + fake_java = work / 'java' + fake_java.write_text('#!' + sys.executable + '\nimport os,shutil\nshutil.copyfile(os.environ["FAKE_RESULTS"], os.environ["RESULTS_FILE"])\n') + fake_java.chmod(0o755) + for data, expected in cases: + (work / 'input.json').write_text(json.dumps(data)) + env = dict(os.environ, PATH=str(work) + os.pathsep + os.environ['PATH'], + BENCHMARKS_JAR=str(work / 'benchmarks.jar'), FAKE_RESULTS=str(work / 'input.json'), + RESULTS_FILE=str(work / 'result.json'), LOG_FILE=str(work / 'jmh.log')) + result = subprocess.run(['bash', str(script)], env=env, capture_output=True, text=True) + self.assertEqual(result.returncode, expected, result.stdout + result.stderr) + +if __name__ == '__main__': + unittest.main() diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index fd50eb1..d23bed4 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -41,7 +41,32 @@ jobs: run: mvn package -DskipTests -B - name: Run allocation regression check - run: ./allocation-check.sh + env: + RESULTS_FILE: ${{ runner.temp }}/allocation-results.json + LOG_FILE: ${{ runner.temp }}/allocation-jmh.log + run: ./allocation-check.sh | tee "$RUNNER_TEMP/allocation-report.txt" + + - name: Include allocation budgets in job summary + if: always() + run: | + if [[ -f "$RUNNER_TEMP/allocation-report.txt" ]]; then + echo '### Allocation budgets' >> "$GITHUB_STEP_SUMMARY" + echo '```text' >> "$GITHUB_STEP_SUMMARY" + cat "$RUNNER_TEMP/allocation-report.txt" >> "$GITHUB_STEP_SUMMARY" + echo '```' >> "$GITHUB_STEP_SUMMARY" + fi + + - name: Upload allocation results + if: always() + uses: actions/upload-artifact@v4 + with: + name: allocation-results + path: | + ${{ runner.temp }}/allocation-results.json + ${{ runner.temp }}/allocation-jmh.log + ${{ runner.temp }}/allocation-report.txt + retention-days: 90 + if-no-files-found: warn conformance: # Runs the official protobuf proto3 JSON conformance suite once, on a single diff --git a/.github/workflows/performance-report.yml b/.github/workflows/performance-report.yml new file mode 100644 index 0000000..0600535 --- /dev/null +++ b/.github/workflows/performance-report.yml @@ -0,0 +1,32 @@ +name: Publish performance report + +on: + workflow_run: + workflows: [Performance] + types: [completed] + +permissions: + contents: read + actions: read + checks: write + pull-requests: write + +concurrency: + group: performance-report-${{ github.event.workflow_run.head_sha }} + cancel-in-progress: false + +jobs: + publish: + runs-on: ubuntu-24.04 + timeout-minutes: 5 + steps: + # Only trusted default-branch code executes in this write-permission job. + # Benchmark artifacts are parsed as bounded JSON, never extracted/executed. + - uses: actions/checkout@v5 + with: + ref: ${{ github.event.repository.default_branch }} + persist-credentials: false + - name: Publish commit check and update PR report + env: + GITHUB_TOKEN: ${{ github.token }} + run: python3 .github/performance/publish.py diff --git a/.github/workflows/performance.yml b/.github/workflows/performance.yml new file mode 100644 index 0000000..3150db4 --- /dev/null +++ b/.github/workflows/performance.yml @@ -0,0 +1,88 @@ +name: Performance + +on: + pull_request: + branches: [main] + push: + branches: [main] + workflow_dispatch: + inputs: + base: + description: Baseline commit/ref (empty uses the candidate's first parent) + required: false + type: string + +permissions: + contents: read + +concurrency: + group: performance-${{ github.event.pull_request.number || github.run_id }} + cancel-in-progress: ${{ github.event_name == 'pull_request' }} + +jobs: + tooling: + name: Validate performance tooling + runs-on: ubuntu-24.04 + steps: + - uses: actions/checkout@v5 + with: + persist-credentials: false + - name: Test comparison and publication logic + run: python3 -m unittest discover -s .github/performance/tests -v + + compare: + name: JMH comparison (Java ${{ matrix.java }}) + needs: tooling + runs-on: ubuntu-24.04 + timeout-minutes: 45 + strategy: + fail-fast: false + matrix: + java: ['21', '25'] + steps: + - uses: actions/checkout@v5 + with: + ref: ${{ github.event.pull_request.head.sha || github.sha }} + fetch-depth: 0 + persist-credentials: false + - uses: actions/setup-java@v5 + with: + distribution: corretto + java-version: ${{ matrix.java }} + - uses: actions/cache@v4 + with: + path: ${{ runner.temp }}/performance-maven + key: performance-${{ runner.os }}-${{ runner.arch }}-java${{ matrix.java }}-${{ hashFiles('**/pom.xml') }} + restore-keys: performance-${{ runner.os }}-${{ runner.arch }}-java${{ matrix.java }}- + - name: Build and compare immutable revisions + env: + PR_BASE: ${{ github.event.pull_request.base.sha }} + PUSH_BASE: ${{ github.event.before }} + INPUT_BASE: ${{ inputs.base }} + JAVA_VERSION: ${{ matrix.java }} + PERFORMANCE_MAVEN_REPO: ${{ runner.temp }}/performance-maven + run: | + base="${PR_BASE:-${INPUT_BASE:-${PUSH_BASE:-HEAD^}}}" + if [[ "$base" == 0000000000000000000000000000000000000000 ]]; then + base=HEAD^ + fi + python3 .github/performance/compare.py \ + --base "$base" --candidate HEAD --java "$JAVA_VERSION" \ + --output "benchmark-reports/ci-java${JAVA_VERSION}" + - name: Upload validated summary for publisher + uses: actions/upload-artifact@v4 + with: + name: performance-summary-java${{ matrix.java }} + path: benchmark-reports/ci-java${{ matrix.java }}/summary.json + if-no-files-found: error + overwrite: true + retention-days: 90 + - name: Upload raw JMH results, environment and build logs + if: always() + uses: actions/upload-artifact@v4 + with: + name: performance-raw-java${{ matrix.java }} + path: benchmark-reports/ci-java${{ matrix.java }}/ + if-no-files-found: warn + overwrite: true + retention-days: 90 diff --git a/allocation-check.sh b/allocation-check.sh index 091932c..3cecd33 100755 --- a/allocation-check.sh +++ b/allocation-check.sh @@ -23,8 +23,9 @@ set -euo pipefail cd "$(dirname "$0")" -BENCHMARKS_JAR="buff-json-benchmarks/target/benchmarks.jar" +BENCHMARKS_JAR="${BENCHMARKS_JAR:-buff-json-benchmarks/target/benchmarks.jar}" RESULTS_FILE="${RESULTS_FILE:-/tmp/buff-json-alloc-check.json}" +LOG_FILE="${LOG_FILE:-/tmp/buff-json-alloc-check.log}" # Budgets in B/op (gc.alloc.rate.norm). # Format: : @@ -80,7 +81,7 @@ if [ "$QUICK" = true ]; then WI=1; I=2; F=1; W=1; R=1 fi -# Build benchmark jar if missing or older than buff-json sources +# Build benchmark jar if missing (CI builds it from a clean checkout first) if [ ! -f "$BENCHMARKS_JAR" ]; then echo "Benchmark jar not found — building..." mvn package -DskipTests -q || { echo "Build failed" >&2; exit 2; } @@ -95,18 +96,18 @@ echo "" java -jar "$BENCHMARKS_JAR" \ "$PATTERN" \ - -prof gc \ + -prof gc -foe true \ -wi "$WI" -i "$I" -f "$F" -r "$R" -w "$W" \ -rf json -rff "$RESULTS_FILE" \ - > /tmp/buff-json-alloc-check.log 2>&1 || { - echo "JMH run failed — see /tmp/buff-json-alloc-check.log" >&2 - tail -40 /tmp/buff-json-alloc-check.log >&2 + > "$LOG_FILE" 2>&1 || { + echo "JMH run failed — see $LOG_FILE" >&2 + tail -40 "$LOG_FILE" >&2 exit 2 } # Parse JSON results and assert budgets python3 - "$RESULTS_FILE" "${BUDGETS[@]}" << 'PYTHON_EOF' -import json, sys +import json, math, sys results_file = sys.argv[1] budgets = {} @@ -128,8 +129,11 @@ for r in results: continue seen.add(name) sm = r["secondaryMetrics"].get("gc.alloc.rate.norm") - if sm is None: - print(f"{name:<70} {'no data':>14} {budgets[name]:>10.0f} {'SKIP':>8}") + if (sm is None or sm.get("scoreUnit") != "B/op" + or not isinstance(sm.get("score"), (int, float)) + or not math.isfinite(sm["score"]) or sm["score"] < 0): + print(f"{name:<70} {'invalid data':>14} {budgets[name]:>10.0f} {'FAIL':>8}") + failed.append((name, None, budgets[name])) continue score = sm["score"] budget = budgets[name] @@ -152,7 +156,7 @@ if failed: for name, score, budget in failed: short = name.replace("io.suboptimal.buffjson.benchmarks.", "") if score is None: - print(f" {short}: not reported in JMH results (budget {budget:.0f} B/op)") + print(f" {short}: missing valid allocation data (budget {budget:.0f} B/op)") else: print(f" {short}: {score:.1f} B/op > {budget:.0f} B/op (over by {score-budget:.0f})") sys.exit(1) diff --git a/buff-json-benchmarks/CLAUDE.md b/buff-json-benchmarks/CLAUDE.md index 8c17174..65ca981 100644 --- a/buff-json-benchmarks/CLAUDE.md +++ b/buff-json-benchmarks/CLAUDE.md @@ -51,6 +51,10 @@ Older benchmarks (`ComplexMessageBenchmark`, `WktBenchmark`, etc.) still use `bu - `benchmark-reports/-results.json` — machine-readable JSON - `benchmark-reports/-report.md` — markdown report with codegen/runtime/JsonFormat comparison table +## CI Performance Comparison + +The `Performance` workflow compares exact base/head revisions on Java 21 and 25, using identical candidate benchmark sources and a shared runner per JVM. `.github/performance/compare.py` runs the 30-case `EncodePathsBenchmark` matrix; `report.py` validates data and renders advisory throughput/allocation changes. A separate trusted `workflow_run` publisher updates PR comments and commit checks. See [performance CI documentation](../docs/performance-ci.md) for baselines, protocol, reporting, rollout, and local smoke runs. + ## Allocation Regression Check `./allocation-check.sh` (at repo root) runs JMH `-prof gc` on a representative subset of benchmarks (SimpleMessage codegen+runtime × UTF-16+UTF-8, ComplexMessage codegen+runtime, DoubleHeavy codegen × UTF-16+UTF-8, typed Struct/Timestamp × UTF-16+UTF-8, and map-heavy codegen+runtime) and asserts `gc.alloc.rate.norm` (B/op) stays within per-benchmark budgets. Total runtime ~2 minutes; `--quick` flag for local iteration. Wired into CI as a separate `allocation-check` job. Catches missed zero-alloc paths and new String/byte[] allocations on the hot path. diff --git a/docs/performance-ci.md b/docs/performance-ci.md new file mode 100644 index 0000000..34d6691 --- /dev/null +++ b/docs/performance-ci.md @@ -0,0 +1,70 @@ +# Performance validation in CI + +Use ordinary JMH in a separate GitHub Actions workflow, with GitHub job summaries and commit checks as the primary reports. Keep allocation budgets as a required correctness-style guard. Throughput changes are review signals until measurements on a stable runner justify a reliable gate. + +## What runs and where results live + +| Event | Comparison | Reports | +|-----------------|-------------------------------------------------------------------------|-------------------------------------------------------------------------------| +| PR into `main` | Exact base SHA against PR head SHA | Java 21/25 job summaries, raw artifacts, one updated PR comment, commit check | +| Push to `main` | Push's previous SHA against new SHA | Java 21/25 summaries and artifacts, commit check attached to the new SHA | +| Manual dispatch | Selected baseline against checked-out revision; first parent by default | Summaries, artifacts, commit check | + +The repository's default branch is `main`. Each push is measured; a multi-commit push compares the complete pushed range. A PR compares its actual head, not GitHub's synthetic merge commit; the post-merge push measures the integrated result. New PR runs cancel obsolete runs. Default-branch runs are not cancelled by newer pushes. + +- **Performance** runs 30 encoding cases on Corretto 21 and 25: simple, complex, map-heavy, Struct, and Timestamp × codegen/typed/reflection × UTF-16/UTF-8. +- Both revisions are built cleanly with the **same compiler/JDK**, on the **same runner**. Both builds finish before measurement. Benchmark sources and proto inputs come from the candidate for both builds; runtime, dependencies, and protoc generator come from their respective revisions. This also permits comparison with older commits that predate the matrix. +- Benchmark methods alternate base/candidate order, reversed for Java 25, to reduce ordering bias. Each method uses one thread, two forks, 3 × 1s warmup, 4 × 1s measurement, a fixed 256 MiB heap, and the GC profiler. Allow roughly 20–30 minutes per JVM, depending on downloads/builds; the two JVM jobs run on separate machines and are never compared against each other. +- Base/head/harness SHAs, jar digests, compiler/JVM/JMH versions, CPU, OS, runner image, raw samples, allocations, build logs, and report Markdown are retained in artifacts for 90 days. Commit checks link to the corresponding workflow. These artifacts are evidence, not the baseline: every comparison remeasures its own base. +- **CI / allocation-check** continues to enforce the 14 absolute allocation budgets. Its results and logs are now uploaded, and its table appears in the job summary. Missing/nonfinite allocation measurements fail instead of being skipped. + +## Interpreting a report + +Throughput is operations/second, so higher is better. A timing signal requires a change of at least 10% and non-overlapping JMH 99.9% confidence intervals; other results are marked **inconclusive**, not "no change." This is a conservative heuristic, not a paired statistical test. Both means and errors are shown. + +Allocation growth is flagged when it exceeds both 5% and 16 B/op; from a zero baseline the absolute threshold applies. These relative flags are advisory, while the existing absolute budgets remain enforced. A red benchmark job or failed report means the measurements are invalid/incomplete, not that a timing regression has been statistically established. + +Protocol validation rejects missing/duplicate benchmarks, missing GC data, incomplete forks, incompatible units, and changed JVM/JMH measurement settings. A benchmark that needs an API absent from the baseline will fail to build; select a compatible baseline or establish a new benchmark series rather than presenting incomparable data as an improvement. + +The matrix currently measures **encoding**. Decoder, cold-start, virtual-thread, and other architecture performance need their own benchmark series. Hosted-runner ratios reduce machine-to-machine differences, but cannot eliminate scheduling noise or time-varying load. Absolute throughput from different CPUs/JDKs should not be treated as a continuous comparable series. + +## Why this setup + +| Option | Fit here | Tradeoff | +|---------------------------------------------|------------------------------------------------------------------------------------------|---------------------------------------------------------------------------------------------------------------------------------------| +| GitHub Actions + stock JMH + native reports | Implemented; reuses the existing benchmark suite and needs no external service or secret | Shared-runner timing noise; artifacts have finite retention | +| Dedicated ephemeral runner + stock JMH | Best next step for a throughput merge gate | Requires maintained, controlled hardware, serialized measurements, and calibration | +| CodSpeed | Managed bare-metal measurements, profiling, history and PR integration | Java currently uses a JMH fork and walltime integration; enabling its service/runners is a separate dependency and organization setup | +| Bencher | Managed history, alerts, PR/commit reporting, also self-hostable | Requires a service/project and API key; reporting alone does not stabilize the benchmark machine | +| github-action-benchmark | Native JMH support and GitHub Pages history | Historical comparisons across shared runners need care; Pages introduces another published state to maintain | + +A dashboard does not improve measurement quality by itself. For this repository, start with reproducible same-run comparisons and native reports, then move the runner to controlled hardware if small throughput changes must block merges. Preserve separate series for each JDK/CPU/harness version. The raw JSON can later feed Bencher, CodSpeed, or a Pages dashboard without changing the runtime library. + +Sources checked on 2026-09-06: [OpenJDK JMH](https://github.com/openjdk/jmh), [GitHub hosted runners](https://docs.github.com/en/actions/reference/runners/github-hosted-runners), [CodSpeed Java/JMH integration](https://codspeed.io/docs/guides/how-to-benchmark-java-with-jmh), [CodSpeed macro runners](https://codspeed.io/docs/features/macro-runners), [Bencher GitHub Actions integration](https://bencher.dev/docs/how-to/github-actions/), [github-action-benchmark](https://github.com/benchmark-action/github-action-benchmark). + +## Publishing and rollout + +Benchmark execution has only read permissions and no repository secrets, including for fork PRs. A separate `workflow_run` publisher executes code from the trusted default branch. It downloads only bounded summary archives, reads JSON without extraction, validates the benchmark set/source identity/protocol, recomputes labels, and derives PR association from GitHub. It does not run code from an artifact or PR checkout with write credentials. Artifact redirects do not forward the API token to other hosts. The publisher skips stale PR heads/bases and updates a single comment instead of adding one per run. + +The publisher must exist on the default branch before GitHub can trigger it. On the PR that introduces these files, the **Performance** job summaries and downloadable reports work immediately; automatic PR comments and the extra **Performance report** check start after merge. Subsequent PRs, including forks, use the same reporting path. No branch-protection rules or external accounts are changed by this implementation. + +See [GitHub workflow_run behavior](https://docs.github.com/en/actions/reference/workflows-and-actions/events-that-trigger-workflows#workflow_run) and [GitHub's secure-use guidance](https://docs.github.com/en/actions/reference/security/secure-use). + +## Local use + +Run from the repository root, with Java 21 or 25 selected in `JAVA_HOME` and Python 3.12+: + +```bash +python3 .github/performance/compare.py \ + --base origin/main --candidate HEAD --java 21 \ + --output benchmark-reports/my-comparison +``` + +The output directory must be new. Revisions are exported to temporary directories, so the working tree is untouched. Maven caches are isolated per variant; CI retains third-party dependencies, while this project's SNAPSHOT artifacts are removed before each build. Normal library dependencies remain those declared by each revision; no JMH fork is installed. + +Add `--smoke` for a short three-case end-to-end pipeline check. Smoke reports are explicitly labelled and cannot be published as full performance evidence. Test reporting/validation with: + +```bash +python3 -m unittest discover -s .github/performance/tests -v +``` + diff --git a/run-benchmarks.sh b/run-benchmarks.sh index c31252b..8b8e3f4 100755 --- a/run-benchmarks.sh +++ b/run-benchmarks.sh @@ -85,7 +85,7 @@ fi # Always clean-rebuild to pick up code changes and regenerate JMH BenchmarkList. echo "Building benchmarks..." -mvn package -DskipTests -q +mvn clean package -DskipTests -q mkdir -p "$REPORTS_DIR"