Drop toBinaryFast from main, archive L1+L2, add upstream baseline#22
Merged
Drop toBinaryFast from main, archive L1+L2, add upstream baseline#22
Conversation
Benchmark: no regressionsThresholds: throughput regression Summary:
Produced by |
…seline Previous state on main: toBinary (L0 contiguous writer, PR #8) + toBinaryFast (L1+L2 schema plans, opt-in). Merging L1+L2 into toBinary broke extension conformance (PR #21). Keeping two encoders required users to change their call sites to see L1+L2 gains — also unacceptable. This change: - Removes toBinaryFast export and L1+L2 source files from main (packages/protobuf/src/to-binary-fast.ts, its unit test, and all benchmark wiring that referenced it). - Preserves the full L1+L2 implementation on branch archive/l1-l2-schema-plans-experimental for future iteration, not discarded. Closes PR #21. - Adds upstream-protobuf-es (npm alias for @bufbuild/protobuf@latest) as a third baseline column in benchmarks (re-applying the infrastructure from the closed PR #20), so chart.svg and chart-delta.svg honestly show "this fork's toBinary vs original upstream vs protobufjs" instead of fork-internal self-comparison. - Regenerates chart.svg and chart-delta.svg with the three-way layout and rewrites the README narrative around the new encoders and the "Current state" archival note. - Updates correctness-matrix.test.ts comments to point at the archive branch so contributors know where L1+L2 lives. Local run (Node v25.8.1, unpinned host — CI pinned numbers will differ): - upstream toBinary (OTel 100 spans): 331 ops/s (baseline) - fork toBinary (OTel 100 spans): 1,012 ops/s (+206% vs upstream) - protobufjs (OTel 100 spans): 1,680 ops/s L0 alone gets the fork past 3x original upstream on the real OTel workload that drove this whole investigation, with byte-identical wire output.
intech
force-pushed
the
chore/drop-toBinaryFast-keep-l0-only
branch
from
a7dd65a to
2dfe2fa
Compare
Single-run benchmark output from report.ts was vulnerable to host jitter on small/fast fixtures — a SimpleMessage measurement spread 2-8x across back-to-back runs on the same machine. A committed chart snapshot could therefore show fork's toBinary 19% slower than upstream in one run and 15-20% faster in the next four. Wraps the benchmark loop inside report.ts with a BENCH_REPORT_RUNS counter (default 5) and takes the per-fixture per-encoder median. Override via the env var for faster iteration or longer sweeps. Fresh median-of-5 numbers (local unpinned host, taskset -c 0): - SimpleMessage toBinary: upstream 1.27M, fork 1.35M (+6.3%) - OTel 100 spans toBinary: upstream 536, fork 1634 (+205%) - K8sPodList toBinary: upstream 837, fork 3140 (+275%) - StressMessage toBinary: upstream 2873, fork 10476 (+265%) - RpcResponse toBinary: fork 630K vs protobufjs 532K (fork ahead on this shape) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Files deleted: - packages/protobuf-test/src/schema-plan-adaptive.test.ts — L3 test file, imports toBinaryFast + @bufbuild/protobuf/wire/schema-plan-adaptive both of which were already removed from main - packages/protobuf/src/wire/schema-plan-adaptive.ts — L3 implementation, orphaned after its only consumer was removed - benchmarks/src/bench-multishape.ts — L3-only benchmark, imports toBinaryFast These files survived the initial cleanup because they were only exercised by L3/L1+L2 code paths that were themselves removed. The full L1+L2 + L3 implementation remains on archive/l1-l2-schema-plans-experimental. Full test suite now passes: 2909 / 0 fail. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Removed stale L1+L2 descriptions and the toBinaryFast 0.80x claim from 'Reading the results'. Replaced with L0 description, median-of-5 note, and fresh numbers (OTel 100 spans 0.88x protobufjs, cumulative vs upstream +6% to +275%). Dropped bench-multishape reference from 'Future work' (file deleted in 2dfe2fa). Added 'Archived work' section pointing at archive/l1-l2-schema-plans-experimental for L1+L2 and L3 prototypes.
Previously the bench-matrix job downloaded the baseline JSON from the
most recent push-to-main workflow artifact. That baseline was captured
on whatever ubuntu-latest runner GitHub happened to assign at that time;
the PR run then happened on a different physical host. Even with
taskset -c 0 + median-of-5 on both sides, cross-host variance (different
P/E-core topologies, SMT neighbours, thermal state) remained 5-7%,
producing chronic false-positive regressions on PRs that did not touch
the encode/decode hot path at all.
Switch the PR job to benchmark origin/main and the PR merge commit in
sequence on the SAME runner within the SAME workflow invocation. Every
factor except the code under test is now held constant. The
'bench-baseline-main' artifact upload on push-to-main is preserved for
external trend consumers but is no longer read by PR comparison.
Mechanics:
- Checkout PR as usual.
- Record current + origin/main SHAs.
- git checkout origin/main, install, build, generate, run matrix into
baseline-results.json.
- git checkout back to PR head, rerun install/build/generate (PR may
have changed package-lock), run matrix into bench-results.json.
- compare-results.ts diffs the two local JSONs, posts the sticky
comment, flags regressions.
Doubled bench work pushes the job from ~9 min to ~18 min; timeout raised
25 -> 40 min with buffer for slow runners.
Also: compare-results.ts header text now notes 'baseline and current
are benchmarked on the same runner' so the PR comment reflects the new
guarantee, and .gitignore covers baseline-results.json / bench-report.md
to keep the PR working tree clean after a local bench pass.
This was referenced Apr 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Reverts the two-encoder API split on main.
toBinarystays (L0 contiguous writer from #8, byte-identical to upstream on every fixture in the correctness matrix);toBinaryFastis removed from the public surface. The L1+L2 schema-plan work is preserved on brancharchive/l1-l2-schema-plans-experimentalfor future iteration.Also adds
upstream-protobuf-es(an npm alias for@bufbuild/protobuf@latest) as a third baseline in the benchmark matrix so the chart honestly shows fork'stoBinaryvs original upstream vsprotobufjs, not fork-internal self-comparison. This re-applies the baseline infrastructure from the closed #20.Why
toBinary(attempted in Merge L1+L2 schema-plan into public toBinary (drop toBinaryFast API) #21) broke 3 extension-related unit tests and 6 conformance cases. Root cause: runtime extension data on messages triggers the plan walk but extension encoding is not covered by the plan. Not quick to fix.toBinary+toBinaryFast) forces consumers to change call sites to see the L1+L2 gains — defeats the point of landing it behind the existing API.Changes
packages/protobuf/:src/to-binary-fast.ts(980 lines), export fromsrc/index.ts, and the matchingpackages/protobuf-test/src/to-binary-fast.test.ts.benchmarks/: alltoBinaryFastwiring acrossbench-toBinary.ts,bench-create-toBinary.ts,bench-comparison-protobufjs.ts,bench-memory.ts,bench-streaming.ts,heap-prof-driver.ts, andscripts/run-heap-prof.sh. Deletedsrc/verify-correctness.ts(was purely atoBinaryFastround-trip check).upstream-protobuf-esdevDependency inbenchmarks/package.json, newupstream-protobuf-esbar insrc/report.tsviatoBinary as upstreamToBinary, three-way encoder list insrc/report-helpers.ts.chart.svgandchart-delta.svgwith the new three-way layout (upstream red, fork blue, protobufjs sage). Delta chart now reads "forktoBinaryspeedup vs upstream" and "vs protobufjs".benchmarks/README.md— new encoder legend, added "Current state" section linking toarchive/l1-l2-schema-plans-experimental, corrected bench table from 4 columns to 3, adjusted streaming/heap-prof docs.toBinaryoutput bytes — identical to upstream on every fixture the correctness matrix covers.Benchmark (local, Node v25.8.1, unpinned host)
OTel-shaped export, 100 spans:
toBinarytoBinary(L0)Full 10-fixture matrix lives in
benchmarks/README.md; CI-pinned numbers (taskset -c 0, median-of-5) will shift these ratios by roughly ±15% but preserve the ordering. Regenerate withnpm run bench:report -w @bufbuild/protobuf-benchmarks.Test plan
packages/protobuf— build + typecheck green.packages/protobuf-test— 2,929 tests pass.benchmarks— typecheck + biome lint clean.bench:reportgenerates three-way chart.svg / chart-delta.svg and regenerates the README table.bench:streamingruns with the 2-encoder matrix (toBinary + protobufjs).bench:matrix:cirerun in CI — expected to reflect published fork-vs-upstream delta with tighter margins.Scope
Internal fork PR, draft — do not merge without review. The archive branch
(
archive/l1-l2-schema-plans-experimental) preserves the full L1+L2 implementation verbatim for whoever resumes the extension-path fix.Closes #21. Supersedes #20 (re-applied the upstream-baseline infrastructure in this branch).
Generated with Claude Code