perf(transformer): faster UID generation #10759
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
CodSpeed Instrumentation Performance ReportMerging #10759 will improve performances by 22.4%Comparing Summary
Benchmarks breakdown
|
9fed3f0 to
feeb333
Compare
feeb333 to
2f82282
Compare
2123607 to
c753f75
Compare
f743e1b to
6dbb1b0
Compare
|
""" WalkthroughThis set of changes refactors the unique identifier (UID) name generation logic within the JavaScript AST traversal and transformation infrastructure. The previous approach, which manually tracked used names and generated unique names inline by maintaining a hash set of existing names and unresolved references starting with underscores, is replaced by a new Possibly related issues
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
🧰 Additional context used🪛 Biome (1.9.4)tasks/transform_conformance/tests/babel-plugin-transform-exponentiation-operator/test/fixtures/assign-to-member-expression/output.js[error] 1-1: This aliasing of this is unnecessary. Arrow functions inherits (lint/complexity/noUselessThisAlias) [error] 1-1: This aliasing of this is unnecessary. Arrow functions inherits (lint/complexity/noUselessThisAlias) [error] 1-1: This aliasing of this is unnecessary. Arrow functions inherits (lint/complexity/noUselessThisAlias) 🔇 Additional comments (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/oxc_traverse/src/context/scoping.rs (1)
287-294: Potential allocator mismatch – consider capturing the allocator once
generate_uid_nametakes an&'a Allocatorparameter every time it is called, yet theUidGeneratoris initialised only on the first call and retains that allocator reference internally.
If the caller ever passes a different allocator on a later invocation, the parameter is silently ignored, which is surprising and could lead to hard-to-debug memory bugs (strings ending up in mixed arenas).Two possible fixes:
- pub fn generate_uid_name(&mut self, name: &str, allocator: &'a Allocator) -> Atom<'a> { + pub fn generate_uid_name(&mut self, name: &str, allocator: &'a Allocator) -> Atom<'a> { // .. }
- Store the allocator in
TraverseScopingat construction and remove the extra parameter:- pub fn generate_uid_name(&mut self, name: &str, allocator: &'a Allocator) -> Atom<'a> { - let uid_generator = - self.uid_generator.get_or_insert_with(|| UidGenerator::new(&self.scoping, allocator)); + pub fn generate_uid_name(&mut self, name: &str) -> Atom<'a> { + let uid_generator = self + .uid_generator + .get_or_insert_with(|| UidGenerator::new(&self.scoping, self.allocator));
- Alternatively, assert the allocator equality on subsequent calls to catch misuse early.
Either way clarifies the ownership model and prevents accidental allocator mixing.
crates/oxc_traverse/src/context/uid.rs (1)
140-152: Minor:U32_MAX_LENconstant off by one
const U32_MAX_LEN: usize = "4294967296".len();
u32::MAXis4294967295(10 digits). While the off-by-one doesn’t break correctness (overflowing parses are rejected anyway), consider documenting the intent or using:const U32_MAX_LEN: usize = u32::MAX.to_string().len();This expresses the limit directly and avoids magic numbers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tasks/coverage/snapshots/semantic_typescript.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
crates/oxc_traverse/src/context/mod.rs(1 hunks)crates/oxc_traverse/src/context/scoping.rs(5 hunks)crates/oxc_traverse/src/context/uid.rs(1 hunks)tasks/transform_conformance/overrides/babel-plugin-transform-typescript/test/fixtures/namespace/same-name/output.mjs(1 hunks)tasks/transform_conformance/tests/babel-plugin-transform-exponentiation-operator/test/fixtures/assign-to-identifier/output.js(1 hunks)tasks/transform_conformance/tests/babel-plugin-transform-exponentiation-operator/test/fixtures/assign-to-member-expression/output.js(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
tasks/transform_conformance/tests/babel-plugin-transform-exponentiation-operator/test/fixtures/assign-to-member-expression/output.js
[error] 1-1: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
(lint/complexity/noUselessThisAlias)
[error] 1-1: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
(lint/complexity/noUselessThisAlias)
[error] 1-1: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
(lint/complexity/noUselessThisAlias)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: Conformance
- GitHub Check: Test wasm32-wasip1-threads
- GitHub Check: Test NAPI
- GitHub Check: Clippy
- GitHub Check: Benchmark (isolated_declarations)
- GitHub Check: Benchmark (semantic)
- GitHub Check: Benchmark (formatter)
- GitHub Check: Benchmark (minifier)
- GitHub Check: Benchmark (codegen)
- GitHub Check: Benchmark (parser)
- GitHub Check: Benchmark (transformer)
- GitHub Check: Benchmark (lexer)
- GitHub Check: Test (ubuntu-latest)
- GitHub Check: Test (macos-latest)
- GitHub Check: Miri
- GitHub Check: Build Linter Benchmark
🔇 Additional comments (7)
crates/oxc_traverse/src/context/mod.rs (1)
24-24: LGTM: Module import for new UID generation functionality.Adding this module import integrates the newly created UID generation module into the context module's scope, supporting the refactoring effort.
tasks/transform_conformance/tests/babel-plugin-transform-exponentiation-operator/test/fixtures/assign-to-identifier/output.js (2)
1-1: LGTM: Variable renaming aligned with new UID generation approach.The variable name change from
_unboundto___unbound2reflects the improved UID generation algorithm which now uses triple underscores and numeric suffixes to avoid collisions.
8-8: LGTM: Consistent renaming in variable usage.The variable reference is properly updated to match the declaration on line 1, maintaining consistency throughout the code.
tasks/transform_conformance/tests/babel-plugin-transform-exponentiation-operator/test/fixtures/assign-to-member-expression/output.js (2)
1-1: LGTM: Variable declarations updated with new naming convention.The variable declarations now use triple underscores and numeric suffixes (
___unbound2,___bound2,___unbound3) instead of single underscores, consistent with the improved UID generation algorithm.🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: This aliasing of this is unnecessary.
Arrow functions inherits
thisfrom their enclosing scope.(lint/complexity/noUselessThisAlias)
[error] 1-1: This aliasing of this is unnecessary.
Arrow functions inherits
thisfrom their enclosing scope.(lint/complexity/noUselessThisAlias)
[error] 1-1: This aliasing of this is unnecessary.
Arrow functions inherits
thisfrom their enclosing scope.(lint/complexity/noUselessThisAlias)
112-114: LGTM: Consistent renaming in variable usage.The variable references have been properly updated to use the new naming convention (
___unbound2,___bound2,___unbound3), maintaining consistency with their declarations.tasks/transform_conformance/overrides/babel-plugin-transform-typescript/test/fixtures/namespace/same-name/output.mjs (1)
1-22: LGTM: New test fixture with updated UID generation pattern.This fixture demonstrates the new UID generation approach with consistent underscore-prefixed names and numeric suffixes for namespace identifiers. The nested namespace structure correctly preserves the TypeScript namespace semantics while using the improved naming convention.
crates/oxc_traverse/src/context/uid.rs (1)
248-266: Empty base name edge caseWhen
nameconsists solely of underscores/digits (e.g."_"),basebecomes the empty string.
That works, but it means the empty string is a legitimate key innames. If any other part of the code assumes identifiers are non-empty, it might mis-behave.Please double-check callers or add a short comment confirming an empty
baseis intentional and safe.
6dbb1b0 to
e0a8946
Compare
Merge activity
|
Alter the algorithm for generating UIDs, to improve its performance.
Previously we stored all existing symbol names which start with `_` in an `FxHashSet`. When creating a UID based on `foo`, it would first see if `_foo` is available, then try `_foo2`, then `_foo3`, then `_foo4` etc.
When creating a lot of UIDs from the same base name (`foo` in example above), this has poor performance as each attempt requires a separate hashset lookup.
Instead, use an `FxHashMap` mapping base name to:
1. Largest number of underscores at start.
2. Largest numeric prefix on end.
i.e. if there are existing symbols `_foo`, `_bar3`, `_bar8`, `_qux`, `___qux2`, this produces a map:
```
{
"foo": { postfix: 1, underscore_count: 1 },
"bar": { postfix: 8, underscore_count: 1 },
"qux": { postfix: 2, underscore_count: 3 },
}
```
When generating a UID, `postfix` is incremented and UID is constructed as `<underscores><base><postfix>`.
This requires a maximum of 1 x hashmap lookup and 1 x hashmap insert for each UID. It also reduces the size of the hashmap where many UIDs are created for same base name.
This new algorithm produces almost same UIDs as Babel, but not quite. 3 tests require their outputs to be changed due to UIDs being different.
e0a8946 to
79e462d
Compare
Alter the algorithm for generating UIDs, to improve its performance.
Previously we stored all existing symbol names which start with
_in anFxHashSet. When creating a UID based onfoo, it would first see if_foois available, then try_foo2, then_foo3, then_foo4etc.When creating a lot of UIDs from the same base name (
fooin example above), this has poor performance as each attempt requires a separate hashset lookup.Instead, use an
FxHashMapmapping base name to:i.e. if there are existing symbols
_foo,_bar3,_bar8,_qux,___qux2, this produces a map:When generating a UID,
postfixis incremented and UID is constructed as<underscores><base><postfix>.This requires a maximum of 1 x hashmap lookup and 1 x hashmap insert for each UID. It also reduces the size of the hashmap where many UIDs are created for same base name.
This new algorithm produces almost same UIDs as Babel, but not quite. 3 tests require their outputs to be changed due to UIDs being different.