Repository navigation
reflect: shared_descriptor_pool_root — external path override for shared-pool mode - #399
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
a9b928c to
e1b0fe4
Compare
ed96aaf to
2e65421
Compare
|
Measured on the real ~2900-package, one-crate-per-package workspace this was built for, comparing the default
("Representative crate" = a real, large service's dependency closure in this workspace, ~40 crates; not named here.) Note on the compile-time row: an earlier pass of this same measurement showed a 2x regression, which had me worried the shared crate might be acting as a serialization bottleneck for downstream compiles. Re-ran it clean (previous attempt coincided with the machine running low on battery, which corrupted the wall-clock reading — cargo's own internally-reported build duration didn't match |
2e65421 to
1940aec
Compare
…red-pool mode buffa#310's shared_descriptor_pool assumes one crate hosts the whole package tree as nested modules, so a super::-relative path can reach the shared __buffa_fds root. That doesn't hold for a one-crate-per-package workspace (idl-gen's layout: ~2900 independent, path-dependent crates, each generated by its own generate() call) — there is no shared module tree to walk up. Add CodeGenConfig::shared_descriptor_pool_root: Option<TokenStream>. When set, its value is spliced in verbatim as the path to the shared root, instead of computing the super:: chain. Buffa has no opinion on what's on the other end — a separate crate, a re-exported alias, anything else — it only needs the tokens to resolve wherever the caller assembled shared_descriptor_root_module's output. Chose a plain TokenStream value over a trait (some `SharedPoolLocation` with a `fn root_path(&self, package: &str) -> TokenStream`) deliberately: a trait would need CodeGenConfig to hold `Box<dyn Trait>`, which breaks its current #[derive(Debug, Clone)] plain-data shape (Clone needs dyn-clone or Arc, Debug needs a supertrait) for generality nothing today needs — every known use case (ours included) wants the same fixed path regardless of package. A TokenStream costs nothing structurally (it already derives Debug + Clone) and is more general than the crate-name string this replaced: buffa never learns "crate" is a concept, the caller can point it at literally anything. `None` (the default) is byte-for-byte identical to before this change — verified via shared_pool_compile.rs's existing single-crate/super:: tests, still passing unmodified, plus a new pair of unit tests for the override path (parses correctly, uses the override verbatim, never falls back to super::). Downstream (idl-gen, not in this repo): assembles the shared root module once for a whole codegen run via encode_descriptor_set + shared_descriptor_root_module, writes it into its own generated crate, and passes that crate's path here for every one of its ~2900 per-package generate() calls. Measured on a real 2895-crate/2897-package corpus: generated source 84GB -> 1.7GB, one dependency's-worth of duplicated FileDescriptorSet bytes instead of one per crate. Full correctness verified via hash-diff against a known-good baseline (only the expected per-crate delegation-shape differences) and cargo check --workspace on the real output. Not upstreamed yet — idl-gen's one-crate-per-package topology doesn't fit any of buffa's three existing front-ends (protoc-gen-buffa / protoc-gen-buffa-packaging can't emit a second crate at all; buffa-build assumes one crate). Landing this locally first to see the real shape and size before deciding whether/how to propose it.
1940aec to
852e624
Compare
|
[claude code] The mechanism is right and
Validation accepts paths that can't compile. Docs. "Buffa has no opinion on what's there" is the opposite of the contract: the target must be Tests. Nothing runs the override across packages of different depth, which is the property that matters; a Smaller: drop the "parsing a handful of tokens costs nothing" sentence; the reservation-skip comment at the Re-review on push; this pairs with the #369/#370 shared-pool work already in 0.9.2, so I'd like to land it there. |
…d/Sync tests - shared_descriptor_pool_root becomes Option<String> (was Option<TokenStream>): TokenStream carries PhantomData<Rc<()>> by design, so the field made CodeGenConfig (and buffa_build::Config by containment) lose Send/Sync for every consumer regardless of whether they set it. Parse via the existing rust_path_to_tokens (already used ~20 places in this crate for exactly this keyword-escaping need) instead of splicing a raw TokenStream. - validate_shared_root_path rejects generic/parenthesized path segments (syn::Path alone accepts them, which would splice into uncompilable generated code) and requires a leading :: or crate, since the same value is spliced verbatim at every package depth. - Field doc now states the actual contract (must be pub, nameable from every package, gated consistently, contain every message this run reflects on — else reflect() panics at runtime, not at generate() or cargo build time), matching the equivalent warning already documented for the plugin path's shared_descriptor_pool in docs/guide.md. - New tests: depth-independence across 0/1/2 segment packages, an exact (not substring) match on the spliced path, malformed-path rejection (generics, empty, relative), a crate-relative acceptance case, a third shared_pool_compile.rs real-cargo-compile mode proving the override resolves without a second crate, and a CodeGenConfig Send+Sync pin. - Changelog fragment: drop the rustdoc intra-doc link (renders as a dead link in plain CHANGELOG.md), add (anthropics#399), note the knob is reachable only by direct buffa-codegen callers this release.
|
Thanks @iainmcgin, addressed in 8819e5a. Agree ⏺ All done and pushed. Summary of what changed on th0114nd/shared-fds-cross-crate (commit
625 unit tests pass (up from 622), all 3 ignored real-compile tests pass, MSRV (1.75), |
…e depth test's negative assertion live The validator accepted any identifier-shaped segment, so '::_::x' and 'crate::super::x' passed and spliced into uncompilable 'pub use' items — the failure class the check exists to prevent. '_', 'self', 'Self', 'super', and a non-leading 'crate' are now rejected by name; '::' alone gets its own message instead of 'must not be empty', and the segment error no longer blames generics for a non-ASCII character. The depth test asserted the absence of 'super ::', a spelling prettyplease never emits; it now checks for 'super::__buffa_fds'. The rejection table gains 'super::' and 'self::' spellings and the keyword cases; the compile-test docs mention the override and the third ignored test.
|
[claude code] The |
2 participants
Closes #397.
What this does
#310's
shared_descriptor_poolassumes one crate hosts the whole package tree as nested modules, so asuper::-relative path can reach the shared__buffa_fdsroot. That doesn't hold for a workspace that generates one independent Cargo crate per proto package (each produced by its own separategenerate()call, linked by path/registry deps rather than nested modules) — there's no module tree forsuper::to climb. See #397 for the concrete case that motivated this (a ~2900-package, one-crate-per-package workspace) and the measured impact (generated source 84GB -> 1.7GB for the full corpus).Adds
CodeGenConfig::shared_descriptor_pool_root: Option<proc_macro2::TokenStream>. When set,generate()splices this value in verbatim as the path to the shared root instead of computing thesuper::chain. Buffa doesn't need to know anything about "external crates" or similar — it just stops asserting an opinion about where the root module lives, and the caller (who already assembledshared_descriptor_root_module's output somewhere) supplies the resolved path directly.None(the default) is byte-for-byte identical to today'ssuper::-relative behavior — this is purely additive, no change to any existing consumer.Two misuse cases are rejected at
generate()time rather than left to silently no-op or fail downstream:shared_descriptor_pool_rootwithoutshared_descriptor_pool— the override only means something in shared-pool mode, so (mirroring the existingshared_descriptor_poolrequiresgenerate_reflectioncheck) it errors immediately instead of silently doing nothing.TokenStreamwould otherwise only surface as a confusing compile error in the downstream generated crate.generate()now parses it as asyn::Pathup front and errors immediately if it doesn't parse. This is a single small-token-stream parse per call, no different in cost from thesyn::parse_strcalls codegen already does per field type path — it doesn't scale with corpus size, so it doesn't need caching across the ~2900 calls in a one-crate-per-package workspace.Why a
TokenStreamvalue instead of a traitConsidered a trait (
fn root_path(&self, package: &str) -> TokenStream) so the path could vary per package, but nothing anyone's described actually needs that, and it costs real API surface:CodeGenConfigis currently plain#[derive(Debug, Clone)]data, and aBox<dyn Trait>field breaks that (needsdyn_clone/ArcforClone, aDebugsupertrait, forecloses futuredyn-safety-constrained changes). A plainTokenStreamcosts nothing structurally and is still strictly more general than the crate-name-string version this replaced in earlier iteration — buffa never learns what's on the other end.Scope
This is the
buffa-codegenmechanism only, matching how #310/#311 shipped the mechanism ahead of front-end wiring (#369/#370). I don't think the plugin path (protoc-gen-buffa/protoc-gen-buffa-packaging) can support this at all — it emits one crate'smod.rsfrom one process invocation, with no notion of a second, separately-assembled crate.buffa-buildcould plausibly expose this asConfig::shared_descriptor_pool_rootif there's interest — happy to scope that as a follow-up rather than bundling it here.Testing
reflect_pool_module_shared_with_root_override_uses_it_verbatim: low-level unit test — override path parses, is used verbatim, and the output never falls back tosuper::.shared_mode_with_root_override_allows_reserved_root_package: end-to-end — additionally asserts the actualgenerate()output contains the override path verbatim, not just that generation succeeds. The low-level unit test above bypassesgenerate_package_mod's wiring (ctx.config.shared_descriptor_pool_root.as_ref()), so this closes the gap between "the helper function works in isolation" and "the real call path uses it."shared_mode_with_root_override_requires_shared_descriptor_pool: setting the override withoutshared_descriptor_poolerrors instead of silently no-op'ing.shared_mode_with_malformed_root_override_is_rejected: a non-pathTokenStreamis rejected atgenerate()time with a clear error.reflect_pool_module_shared_delegates_without_embedding_bytesand bothshared_pool_compile.rsend-to-end compile tests (shared_pool_tree_compiles_inline/_include_bytes_sidecar) — the existing single-crate/super::path — still pass unmodified, confirming no regression to Deduplicate the embedded reflection descriptor pool across packages #310's original behavior.cargo check --workspaceon real generated output (not part of this repo, described in shared_descriptor_pool: support a caller-supplied root path for non-module-tree consumers #397).cargo fmt,cargo clippy --workspace --all-targets -- -D warnings, andcargo doc --all-featuresall clean; MSRV (1.75, workspace--all-targets) and the 32-bit (i686-unknown-linux-gnu) workspace check both verified locally as well.