Skip to content

perf(codegen): use a fast hasher for internal naming/comment maps - #398

Closed
th0114nd wants to merge 3 commits into
anthropics:mainfrom
th0114nd:th0114nd/fast-hasher
Closed

th0114nd wants to merge 3 commits into
anthropics:mainfrom
th0114nd:th0114nd/fast-hasher

Conversation

@th0114nd

Copy link
Copy Markdown
Contributor

Closes #396.

What this does

CodeGenContext's internal bookkeeping (context.rs's naming/deconfliction maps, comments.rs's doc-comment lookup) used std::collections::HashMap, which defaults to SipHash. This data only ever comes from trusted, locally-generated .proto descriptors — never attacker-controlled input — so SipHash's hash-flooding resistance buys nothing here, and profiling showed it as the single largest sampled cost bucket on a large multi-thousand-file codegen run (see #396 for the numbers).

Swaps context.rs and comments.rs's internal maps to foldhash via a small hash.rs type alias (FxHashMap). foldhash is already resolved transitively via hashbrown, so this is a direct dependency on something already in the tree.

oneof.rs's maps were tried too but reverted: their types leak through public signatures in message.rs/view.rs/lazy_view.rs, too broad a blast radius for what should be a purely internal, output-invariant change. Left as std::HashMap.

One field (CodeGenContext::field_renames) stays std::collections::HashMap explicitly — it's populated from field_names::plan_field_names, which isn't touched by this change, so its type has to match.

Testing

  • Output-invariant by construction: verified with a full hash-diff of a real ~2900-crate generation run, before and after — byte-for-byte identical.
  • All existing buffa-codegen tests pass unmodified (cargo test -p buffa-codegen --lib).
  • cargo fmt, cargo clippy --workspace --all-targets -- -D warnings, and cargo doc --all-features all clean.

@github-actions

Copy link
Copy Markdown

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

CodeGenContext's internal bookkeeping (package/message name deconfliction,
doc-comment lookup, oneof/message indexing) used std::HashMap, which
defaults to SipHash. Profiling a multi-thousand-file codegen run showed
Hasher::write/hash_one (SipHash) as the single largest sampled cost bucket,
ahead of any actual codegen logic.

This data only ever comes from trusted, locally-generated .proto
descriptors, never attacker-controlled input, so SipHash's hash-flooding
resistance buys nothing here. Swap context.rs and comments.rs's maps to
foldhash (already resolved transitively via hashbrown, so this adds no new
third-party code to the tree, just a direct dependency on what's already
there).

oneof.rs was tried too but reverted: its map types leak through public
signatures in message.rs/view.rs/lazy_view.rs, too broad a blast radius for
what should be a purely internal, output-invariant change.

Verified output-invariant: full hash-diff of a real 2895-crate generation
run, before and after, is byte-for-byte identical. All 619 existing tests
pass.
@th0114nd
th0114nd force-pushed the th0114nd/fast-hasher branch from fe4244b to c42c821 Compare August 30, 2026 15:51
@th0114nd

Copy link
Copy Markdown
Contributor Author

Closing this — on reflection, a ~3-7% wall-clock improvement (verified via interleaved trials after an earlier noisy measurement) doesn't clearly justify separate review attention on its own, especially alongside #399's ~2x improvement, which deserves the focus. Not pursuing further for now; may revisit later if it's ever bundled with something else that needs the same review pass.

Thanks for reading this far regardless — no action needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Codegen: SipHash dominates CodeGenContext's internal naming/comment maps

1 participant