Repository navigation
codegen: precompute corpus-wide context once and share across generate() calls - #401
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
e846e75 to
498c1c1
Compare
Update: re-measured now that #397/#399 is merged, and rebased on current mainRebased this branch on current
Bigger win than the original 1.55x: with shared-fds removing most of the per-crate descriptor-byte-encoding cost, the corpus-wide work this PR caches is a larger share of what's left per crate. RSS still improves, not regresses. |
…e() calls Each generate() call rebuilds msg_index, resolve_unboxed_variants, resolve_inlined_fields, and the per-file comment map from the whole corpus, even though none of that depends on effective_extern_paths -- the one thing that legitimately varies per call in a workspace with one crate per proto package (~2900 of them here). CodeGenConfig::shared_corpus_context lets a caller precompute this once via precompute_shared_corpus_context and reuse it across every generate() call sharing that corpus and config, turning the corpus-wide walk into an Arc clone. generate() output is byte-identical whether or not shared_corpus_context is set; the new shared_corpus_context test asserts this directly.
…orpusContext::new The cache had no key, so a context built from another corpus or under other oneof/pointer-repr rules silently miscompiled. It now records the file names and both rule lists, and generate() returns CodeGenError::SharedCorpusContextMismatch on any difference (the infallible CodeGenContext entry point asserts in debug builds). The constructor moves onto the type, the type is Clone (Arcs inside) so the config field needs no caller-side Arc, and Debug prints sizes rather than the corpus comment text. Tests now attach source_code_info so the cached comment map is exercised, and add the mismatch, Send + Sync, and Debug cases; the existing equality tests could not fail if the shared context were ignored.
498c1c1 to
06fba91
Compare
|
[claude code] The caching is sound — the comment map holds raw text and cross-reference resolution happens at emission against the per-call
Tests: the two equality tests could not fail if the shared context were ignored, so there is now a mismatch test for all three keys, the corpus carries One question for you, not blocking: the pre-pass in |
…ox substring The self-recursive view field is MessageFieldView<Self>, not a literal Box, so the tightened substring never matched. The check now pins both decisions the cached inlined_message_fields set makes: Holder.leaf is stored Inline and SelfRef.child is not.
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
SharedCorpusContext::check() treats matching file count/names as the same corpus, but the cached message-index resolutions and comment map depend on descriptor contents. Changing messages/comments in foo.proto while keeping the filename therefore passes the check and reuses stale codegen state, contrary to the documented mismatch guarantee. Please fingerprint/compare descriptor content and add a same-name/changed-schema regression test.
SharedCorpusContext carries no lifetime tying it to the caller's files, so name-only comparison was not a sound fingerprint: a Vec can keep its allocation (same pointer, same length) across an ordinary pop()+push() of a different file, and a same-named file with different content (e.g. after regenerating a FileDescriptorSet from an edited .proto) passed the old check and could silently reuse oneof/inlining decisions and comments resolved against stale content. Measured cost on the 2895-crate corpus: ~25-28% added wall/CPU time vs. the name-only check, still a ~2.3x wall-clock win over not sharing the context at all (110.8s -> 48.1s vs. shared-fds-only baseline). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
from me:
from the bot: Re-measured at
Matches the claimed numbers within ~4% on wall/CPU (normal noise). Performance is maintained through the rebase and the @iainmcgin — re: the pre-pass and @sylvesterkaczmarek — re: the fingerprint/regression-test request: you're right, and it's a real gap, not just a theoretical one. Pushed a fix as We looked at two cheaper alternatives first and rejected both:
So we measured the direct fix instead:
So versus the shared-fds-only baseline (110.8s), this PR with the correctness fix lands at ~2.3x faster, not 2.79x — a real cost, but still a substantial win, and the only version of this feature we think is actually safe to merge. If the extra ~25% becomes worth clawing back later, the Pushed |
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ontext entry The type first ships in this release, so a separate Fixed bullet for a behaviour it never had would read as a regression fix.
|
[claude code] Structural comparison is the right call — a |
Closes #400.
What this does
CodeGenContext::with_extern_resolutionrebuildsmsg_index,resolve_unboxed_variants,resolve_inlined_fields, and the per-file comment map from the whole corpus on everygenerate()call, even though none of that depends oneffective_extern_paths— the one thing that legitimately varies per call in a workspace that generates one independent Cargo crate per proto package. See #400 for the concrete case that motivated this (a ~2900-package, one-crate-per-package workspace) and the measured impact.Adds
CodeGenConfig::shared_corpus_context: Option<Arc<SharedCorpusContext>>.SharedCorpusContextis a new opaque struct, produced once viaprecompute_shared_corpus_context(files, config), holdingArc-wrapped copies of the three corpus-wide values. When set,with_extern_resolutionreuses them viaArc::cloneinstead of recomputing.None(the default) recomputes fresh every call — byte-identical to today's behavior, no change to any existing consumer.Perf numbers
Full-corpus codegen (2898 crates), same machine (Apple M4 Max, 16 cores: 12 performance + 4 efficiency, 64 GB RAM), two runs each, alternating order to cancel drift:
RSS and footprint both improve: reusing one
Arc-shared copy across ~2900 calls costs less memory than each call independently allocating and then dropping its own fresh copy of the same data. Parallelism is consistent between variants:(user+sys)/wall ≈ 14xfor both, matching this machine's 16 physical cores — the speedup comes from doing less total work, not from a change in concurrency.Public API surface
This is worth calling out explicitly since it's a permanent addition to
buffa-codegen's public surface, not just an internal change:SharedCorpusContext— opaque (no public fields, no public methods beyond the auto-derivedDebug).precompute_shared_corpus_context(files: &[FileDescriptorProto], config: &CodeGenConfig) -> SharedCorpusContext.CodeGenConfig::shared_corpus_context: Option<Arc<SharedCorpusContext>>,Noneby default.CodeGenConfigis#[non_exhaustive], so this can't break any external exhaustive-literal construction. Nothing existing changes signature or behavior. The maintenance cost going forward is the usual one for an opt-in cache-sharing knob:SharedCorpusContext's precondition (samefiles+unboxed_oneof_fields+pointer_fieldsacross every call sharing one context) is enforced only by documentation, not at runtime — a mismatch silently produces stale output rather than erroring. That mirrors howshared_descriptor_pool_root(#397/#399) already relies on caller discipline for its own precondition, so it's consistent with existing crate convention rather than a new category of risk.Relationship to #397/#399
Complementary but independent. Both target the same one-crate-per-package workspace shape, but this is orthogonal to reflection/descriptor-pool embedding — no shared code path, no ordering dependency. The perf numbers above were measured with shared-fds off specifically to isolate this change's effect. Either PR can land first or stand alone.
Testing
shared_corpus_context::shared_context_matches_fresh_computation:generate()produces byte-identical output whethershared_corpus_contextis set or not, across a corpus built to exercise the real risk cases — a non-recursive inline field, a self-recursive field (asserts its view type stays boxed either way — owned message fields useMessageField<T>, which is indirected internally regardless of recursion), and a oneof variant matched by a blanketunboxed_oneof_fieldsrule.shared_corpus_context::shared_context_across_separate_per_crate_calls: the actual production pattern — oneSharedCorpusContextreused across separategenerate()calls that each emit a different file subset with a differentextern_pathsmapping for the files they don't emit. Confirmseffective_extern_pathsstill resolves correctly per call under sharing (not just that a single combined call is internally consistent), and that one call's local config can't bleed into another's.cargo fmt,cargo clippy --workspace --all-targets -- -D warnings, andcargo doc --workspace --all-featuresall clean; full local pre-push CI-equivalent (lint, doc, test, changelog, generated-code checks) passed; MSRV (1.75, workspace--all-targets) and the 32-bit (i686-unknown-linux-gnu) workspace check both verified locally as well.