perf: replace scope parent-chain lookup with scoped symbol table#14335
Conversation
…l table The parser resolves variables by probing one hash map per scope while walking the scope parent chain, averaging ~2.8 probes per lookup plus tombstone-insert churn for negative caching. Since scopes are entered and exited in strict stack order and all reads/writes go through the innermost scope, a single name -> binding-stack table resolves each lookup with one hash probe. Co-authored-by: Cursor <cursoragent@cursor.com>
|
📝 Benchmark detail: Open
Base persistent cache hit rate: 👍 Current persistent cache hit rate: 👍 |
📦 Binary Size-limit
❌ Size increased by 4.00KB from 62.60MB to 62.60MB (⬆️0.01%) |
Rsdoctor Bundle Diff AnalysisFound 5 projects in monorepo, 0 projects with changes. 📊 Quick Summary
Generated by Rsdoctor GitHub Action |
Merging this PR will improve performance by 6.77%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | Simulation | rust@scan_dependencies@three_module |
23.8 ms | 22.3 ms | +6.77% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing perf/ds-algo-20260610 (e650c3c) with main (b04d9f4)
Footnotes
-
40 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
|
@codspeedbot explain why this is faster |
Why
|
There was a problem hiding this comment.
Pull request overview
This PR optimizes variable resolution in the JavaScript parser by replacing per-scope hash maps + parent-chain probing with a single scoped symbol table (name → stack of bindings). This reduces variable lookup to one hash probe regardless of scope depth and removes the previous negative-cache tombstone insertion on misses.
Changes:
- Reworked
ScopeInfoDBto maintain a globalbindingsmap from identifier → binding stack, plus per-scopedefinedtracking and a newexit_scopeunwinding API. - Updated parser scope entry/exit sites to call
exit_scopewhen restoring previous scopes. - Switched “current scope variables” enumeration to a new
scope_variablesiterator and added unit tests covering shadowing, deletion masking, and enumeration behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| crates/rspack_plugin_javascript/src/visitors/scope_info.rs | Implements the binding-stack scoped symbol table, adds scope unwind (exit_scope), and adds unit tests. |
| crates/rspack_plugin_javascript/src/visitors/dependency/parser/walk.rs | Hooks scope unwinding into the three scope-restore paths to keep the binding stacks consistent. |
| crates/rspack_plugin_javascript/src/visitors/dependency/parser/mod.rs | Updates current-scope variable enumeration to use ScopeInfoDB::scope_variables. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for key in &defined { | ||
| if let Some(stack) = self.bindings.get_mut(key) | ||
| && let Some(top) = stack.last() | ||
| && top.scope == id | ||
| { | ||
| stack.pop(); | ||
| } | ||
| } |
Summary
What changed
ScopeInfoDBin the JavaScript parser previously kept oneFxHashMap<Atom, VariableInfoId>per scope and resolved a variable by probing the map of every scope along the parent chain, inserting tombstones into the starting scope as a negative cache on misses.Since the parser enters and exits scopes in strict stack order and always reads/writes through the innermost active scope, this PR replaces the per-scope maps with a classic scoped symbol table:
FxHashMap<Atom, SmallVec<[Binding; 2]>>mapping each name to a stack of bindings, innermost laststack.last()) instead of one probe per enclosing scopeexit_scope(called at the three scope-restore sites inwalk.rs) pops themdebug_asserts enforce the stack-discipline invariantWhy it should improve performance
Variable resolution is on the parse hot path and scales with identifier occurrences (dependency-level cardinality). On the
cases/allbenchmark, callgrind showed ~2.9M lookups performing 8.2M per-scope hash probes plus tombstone-insert churn, ~2.1% of all executed instructions. With the binding-stack table each lookup costs a single probe regardless of scope depth.Measured effect (callgrind, cases/all)
HashMap<Atom, VariableInfoId>::getno longer appears in the profileRelated links
Checklist
Made with Cursor