feat(transformer)!: generate shorter UIDs #10763
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 #10763 will not alter performanceComparing Summary
|
42a9805 to
e5aebed
Compare
debug optione42882f to
1369244
Compare
|
The added |
WalkthroughThis change introduces a debug mode flag across the transformation and traversal subsystems, affecting Rust code, NAPI bindings, and TypeScript definitions. A new Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (15)
🔇 Additional comments (8)
✨ 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: 2
♻️ Duplicate comments (1)
crates/oxc_traverse/src/context/uid.rs (1)
569-575: Replace secondrepeat_noccurrence for MSRV consistencyFor the underscore prefix inside
DebugUidGenerator::createthe same MSRV concern applies:- uid.extend(iter::repeat_n("_", uid_name.underscore_count as usize)); + uid.extend(std::iter::repeat('_').take(uid_name.underscore_count as usize));Keeping both call-sites consistent avoids relying on recently-stabilised APIs.
🧹 Nitpick comments (2)
crates/oxc_traverse/src/context/uid.rs (2)
52-57: Avoid unused parameter in the fast path to prevent accidental misuse
UidGenerator::createalways receives aname: &str, yet the argument is ignored when the generator is inFastmode (Self::Fast(generator) => generator.create()).
This makes the API look as if the caller-supplied name had some influence even when it does not, increasing the chance of subtle bugs (e.g. the caller assuming that passing the “right” base name is important in production builds).Two lightweight options:
-pub(super) fn create(&mut self, name: &str) -> Atom<'a> { +pub(super) fn create(&mut self, name: &str) -> Atom<'a> { match self { - Self::Fast(generator) => generator.create(), + // Either document the param as “ignored when `debug == false`” … + // or make it explicit at the type level: + Self::Fast(generator) => { + debug_assert!( + name.is_empty(), + "base name is ignored in fast UID mode – pass the empty string \ + to avoid confusion" + ); + generator.create() + },Even better would be to split the API into two clearly-named methods (
create_fast()/create_debug(name)) or to expose two separate generator types behind a trait, so the compiler enforces correct usage.
323-331:Dropimplementation can leak ifBox::from_rawpanics – usedrop(Box::from_raw(...))Inside
Dropyou reconstruct theBox<str>and bind it to_box; if any future code is inserted before the end of the function and panics, the_boxbinding would be leaked.
Prefer to immediately hand ownership todropso the buffer is freed even if more code is added:- let _box = Box::from_raw(str); + drop(Box::from_raw(str));A
dropcall cannot panic, guaranteeing the memory is released.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
.typos.toml(1 hunks)crates/oxc_transformer/examples/transformer.rs(1 hunks)crates/oxc_transformer/src/context.rs(2 hunks)crates/oxc_transformer/src/es2022/class_properties/computed_key.rs(1 hunks)crates/oxc_transformer/src/es2022/class_properties/private_field.rs(1 hunks)crates/oxc_transformer/src/es2022/class_properties/super_converter.rs(2 hunks)crates/oxc_transformer/src/lib.rs(2 hunks)crates/oxc_transformer/src/options/mod.rs(3 hunks)crates/oxc_transformer/tests/integrations/es_target.rs(1 hunks)crates/oxc_traverse/Cargo.toml(1 hunks)crates/oxc_traverse/src/ast_operations/gather_node_parts.rs(1 hunks)crates/oxc_traverse/src/context/mod.rs(3 hunks)crates/oxc_traverse/src/context/reusable.rs(1 hunks)crates/oxc_traverse/src/context/scoping.rs(3 hunks)crates/oxc_traverse/src/context/uid.rs(7 hunks)napi/playground/src/lib.rs(2 hunks)napi/transform/index.d.ts(1 hunks)napi/transform/src/transformer.rs(2 hunks)napi/transform/test/transform.test.ts(1 hunks)tasks/coverage/src/tools/semantic.rs(1 hunks)tasks/transform_conformance/src/test_case.rs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
crates/oxc_transformer/src/es2022/class_properties/super_converter.rs (1)
crates/oxc_traverse/src/ast_operations/gather_node_parts.rs (1)
get_var_name_from_node(12-34)
crates/oxc_transformer/src/lib.rs (5)
crates/oxc_traverse/src/lib.rs (1)
traverse_mut_with_ctx(167-201)crates/oxc_traverse/src/context/reusable.rs (1)
new_with_debug(28-30)crates/oxc_traverse/src/context/mod.rs (1)
scoping(211-213)crates/oxc_traverse/src/context/scoping.rs (1)
scoping(61-63)crates/oxc_semantic/src/lib.rs (1)
scoping(111-113)
napi/transform/test/transform.test.ts (1)
napi/transform/src/transformer.rs (1)
transform(758-808)
crates/oxc_transformer/src/es2022/class_properties/private_field.rs (1)
crates/oxc_traverse/src/ast_operations/gather_node_parts.rs (1)
get_var_name_from_node(12-34)
🔇 Additional comments (34)
crates/oxc_transformer/examples/transformer.rs (1)
75-75: Enable debug mode for transformer exampleThe addition of
transform_options.debug = trueenables the debug mode for the transformer example, which will generate more readable, descriptive UIDs instead of the new shorter format. This is helpful for debugging purposes and aligns with the PR objective of adding a debug option.tasks/coverage/src/tools/semantic.rs (1)
19-19: Setting debug mode as default in transform optionsThis change enables debug mode by default for the semantic tests. This ensures that the tests use the more readable, descriptive UIDs, which is helpful for test readability and debugging.
crates/oxc_traverse/Cargo.toml (1)
29-29: Added 'assert_unchecked' feature for oxc_data_structuresThe addition of the
"assert_unchecked"feature to theoxc_data_structuresdependency supports the new UID generation functionality that likely relies on this feature for performance optimizations or safety checks..typos.toml (1)
18-18: Excluded UID generator file from typo checkingThe
crates/oxc_traverse/src/context/uid.rsfile has been excluded from typo checking. This is reasonable as UID generation code may contain intentional non-dictionary terms or variable names that could be falsely flagged as typos.napi/transform/index.d.ts (1)
372-373: Well-documented addition of thedebugoption.The addition of a clear and concise comment for the new
debugboolean option inTransformOptionseffectively communicates its purpose - to produce more debuggable output when enabled.crates/oxc_transformer/src/es2022/class_properties/super_converter.rs (2)
376-378: Updated function call to pass the debug flag.This change correctly passes the
debugflag from the context to theget_var_name_from_nodefunction. This aligns with the updated function signature that now requires a boolean parameter to control whether descriptive variable names are generated.
440-442: Updated function call to pass the debug flag.Similar to the previous change, this update passes the
debugflag from the context to theget_var_name_from_nodefunction, ensuring consistent behavior across the codebase.tasks/transform_conformance/src/test_case.rs (1)
55-58: Enable debug mode for transform conformance tests.This change ensures that conformance tests run with debug mode enabled, which is appropriate for testing purposes. The modification uses a clean pattern of mapping over the result of
try_fromto set the debug flag without altering the error-handling path.napi/playground/src/lib.rs (1)
192-204: Enable debug mode in the playground.The code now explicitly sets
debug = truein the playground environment, which is appropriate for a development/debugging context. This ensures that more readable UIDs are generated when experimenting with transformations in the playground.crates/oxc_transformer/src/es2022/class_properties/private_field.rs (1)
951-951: Correctly passes debug flag to get_var_name_from_node function.The function now passes the
self.ctx.debugflag toget_var_name_from_node, which determines whether to generate descriptive variable names for debugging (when true) or shorter identifiers (when false). This change aligns with the new UID generation scheme.crates/oxc_transformer/src/context.rs (2)
36-36: Added debug flag to TransformCtx struct.This new
debugboolean field propagates the debug mode setting from the transformation options through the context. It will control whether to generate longer, more descriptive UIDs (in debug mode) or shorter, more compact UIDs (in normal mode).
65-65: Properly initializes debug flag from options.The debug flag is correctly initialized from the corresponding flag in
TransformOptions, ensuring consistent debug behavior throughout the transformation process.crates/oxc_transformer/tests/integrations/es_target.rs (2)
28-29: Enabled debug mode in tests for esnext target.The test now creates a mutable options variable and explicitly enables debug mode. This ensures tests verify transformation behavior with the more descriptive debug-style UIDs.
37-38: Enabled debug mode in tests for all target configurations.Similar to the esnext case, debug mode is now enabled for all transformation options in the test. This maintains consistent testing conditions across all target configurations.
crates/oxc_transformer/src/es2022/class_properties/computed_key.rs (1)
134-134: Updated assertion to accommodate different identifier prefixes based on debug mode.The assertion now checks for different identifier name prefixes depending on the debug mode: underscore (
_) in debug mode and dollar sign ($) in standard mode. This aligns with the dual UID generation scheme introduced by this change.napi/transform/src/transformer.rs (2)
100-101: Good addition of thedebugfield with clear documentation.The documentation explains clearly that this option is for producing more debuggable output, which aligns with the PR objective of introducing a debug mode for UIDs.
162-162: Correct implementation of the debug flag parsing.The use of
is_some_andis appropriate here, setting the debug flag to true only when explicitly provided and set to true. This ensures that the default behavior remains the more efficient non-debug mode.napi/transform/test/transform.test.ts (1)
4-9: Clean approach to enabling debug mode in tests.Creating a wrapper function that automatically sets
debug: trueis an elegant solution that ensures all tests run with debug mode enabled. This approach:
- Preserves the original function signature
- Allows test-specific options to be merged in
- Makes it easy to toggle the default debug behavior for all tests at once
This is better than modifying each individual test case.
crates/oxc_transformer/src/options/mod.rs (3)
49-50: Clear documentation of the debug flag behavior.The comment clearly explains the purpose of the debug flag: to produce code with inserted UIDs in a more easily debuggable form. This is consistent with the PR objective.
86-86: Appropriate default for the debug flag inenable_all().Setting
debug: falseby default is the right choice since the PR's goal is to optimize performance with shorter UIDs. Debug mode should be explicitly enabled when needed rather than being the default behavior.
267-267: Correct default for BabelOptions conversion.Setting
debug: falsewhen converting fromBabelOptionsmaintains consistency with the default behavior and ensures that the more performant UID generation is used by default.crates/oxc_traverse/src/ast_operations/gather_node_parts.rs (1)
12-15: Key optimization for non-debug mode.This change implements the core performance enhancement by skipping the expensive node name gathering operation when debug mode is off. Returning an empty string when
!debugis an effective way to implement the shorter UID generation approach described in the PR objectives.This aligns perfectly with the goal of making UID generation cheaper and faster by eliminating the need for descriptive base names when not in debug mode.
crates/oxc_traverse/src/context/reusable.rs (2)
24-25: Modified constructor to support debug modeThe
newconstructor now passesfalseas the default for the newdebugparameter when creating the innerTraverseCtx. This maintains backward compatibility while integrating with the new debug feature.
27-29: New constructor added for explicit debug mode controlThe
new_with_debugconstructor allows creating aReusableTraverseCtxwith an explicit debug flag setting. This is a clean addition that follows Rust's idioms for providing optional functionality while maintaining the existing API.crates/oxc_transformer/src/lib.rs (2)
15-15: Updated import to include new traverse functionsThe import now includes
ReusableTraverseCtxandtraverse_mut_with_ctxwhich are needed for the updated traversal approach with debug support.
160-164: Refactored traversal to support debug modeThis change replaces the previous traversal approach with a new pattern that uses
ReusableTraverseCtxandtraverse_mut_with_ctx. The debug flag is now correctly propagated from the transform context to the traversal context. This is a well-structured refactoring that properly integrates the debug mode throughout the transformation pipeline.The method:
- Creates a reusable traversal context with the scoping, allocator, and debug flag
- Performs the traversal with this context
- Retrieves the updated scoping from the context
This approach maintains the same functionality while adding debug capabilities.
crates/oxc_traverse/src/context/mod.rs (3)
460-460: Updated to pass debug flag to variable name generationThe
generate_uid_based_on_nodemethod now passes the debug flag to theget_var_name_from_nodefunction, ensuring that variable names derived from nodes follow the desired debugging style.
495-495: Propagated debug flag to variable name extractionSimilar to the previous change, the debug flag is now passed to
get_var_name_from_nodein thegenerate_uid_in_current_hoist_scope_based_on_nodemethod, maintaining consistency in how variable names are generated throughout the codebase.
652-655: Added debug flag to context creationThe
TraverseCtx::newmethod now accepts and propagates a debug flag to the innerTraverseScopingcomponent. This ensures the debug setting is consistently available throughout the traversal context hierarchy.crates/oxc_traverse/src/context/scoping.rs (5)
27-27: Added debug flag to TraverseScopingA new
debugboolean field withpub(super)visibility is added to theTraverseScopingstruct. This field controls whether debug-friendly UIDs are generated or fast, compact UIDs are used.
290-296: Optimized UID generation with fast pathThe
generate_uid_namemethod has been refactored to separate fast and slow paths based on the type of UID generator. When a fast generator is available, it directly callscreate()without arguments. Otherwise, it falls back to the slow path that requires more setup.This optimization recognizes that most usage will likely be in non-debug mode and optimizes for that case.
298-307: Added separate method for slow path UID generationThe new
generate_uid_name_slowmethod handles the debug-friendly UID generation path. It's marked with#[cold]and#[inline(never)]attributes to help the compiler optimize the common (non-debug) case. This separation is a good optimization practice for code paths that are expected to be rarely taken.
302-304: Debug flag passed to UID generatorThe debug flag is now properly passed to
UidGenerator::new, ensuring the correct type of UID generator is created based on the debug setting.
377-382: Updated constructor to accept debug flagThe
TraverseScoping::newmethod now accepts adebugparameter and initializes the new debug field with the provided value. This allows controlling the UID generation strategy from the creation of the scoping context.
c300235 to
fd37a53
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
crates/oxc_traverse/src/context/uid.rs (2)
549-550: 🛠️ Refactor suggestionReplace
std::iter::repeat_nwith a stable alternativeSame MSRV compatibility issue as earlier in the file.
- uid.extend(iter::repeat_n("_", uid_name.underscore_count as usize)); + uid.extend(iter::repeat("_").take(uid_name.underscore_count as usize));
159-160: 🛠️ Refactor suggestionReplace
std::iter::repeat_nwith a stable alternative
repeat_nwas stabilized only in Rust 1.72. If the project's minimum supported Rust version (MSRV) is lower, this code will fail to compile on older toolchains.- buffer.extend(iter::repeat_n('$', len - 1)); + buffer.extend(iter::repeat('$').take(len - 1));
🧹 Nitpick comments (2)
crates/oxc_traverse/src/context/uid.rs (2)
246-252: The unsafe pointer subtraction looks safe but could use an explicit checkThe comment above explains why the pointer subtraction should be safe, but adding an explicit check would make the code more robust against future changes.
+ // Double-check that buffer is large enough for this operation + debug_assert!( + self.last_letter_ptr as usize >= self.buffer_start_ptr as usize + (POSTFIX_BYTES - 1), + "Buffer too small for pointer subtraction" + ); let earliest_letter_ptr = unsafe { self.last_letter_ptr.sub(POSTFIX_BYTES - 1) };
24-27: Enum variants should be non-exhaustive for future compatibilitySince this is a public enum that might need additional variants in the future, consider making it non-exhaustive to avoid breaking changes in the public API.
#[expect(private_interfaces)] pub enum UidGenerator<'a> { Fast(FastUidGenerator<'a>), Debug(DebugUidGenerator<'a>), + #[doc(hidden)] + __NonExhaustive, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/oxc_traverse/src/context/uid.rs(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: Benchmark (formatter)
- GitHub Check: Benchmark (codegen)
- GitHub Check: Benchmark (minifier)
- GitHub Check: Test wasm32-wasip1-threads
- GitHub Check: Benchmark (semantic)
- GitHub Check: Test Linux
- GitHub Check: Benchmark (isolated_declarations)
- GitHub Check: Test NAPI
- GitHub Check: Benchmark (transformer)
- GitHub Check: Conformance
- GitHub Check: Benchmark (parser)
- GitHub Check: Build Linter Benchmark
- GitHub Check: Benchmark (lexer)
- GitHub Check: Miri
- GitHub Check: Clippy
🔇 Additional comments (5)
crates/oxc_traverse/src/context/uid.rs (5)
52-57: LGTM: Enum dispatch is correct and comprehensiveThe implementation correctly dispatches to the appropriate generator based on the enum variant.
60-74: Well-documented UID generation algorithm with clear collision avoidanceThe approach of finding the longest
$prefix in the code and then using one more$to avoid collisions is clever and properly explained.
141-144: Proper checking of both symbol names and unresolved referencesThe generator examines both symbol names and unresolved references to determine the longest
$prefix, ensuring no naming conflicts.
289-309: The Drop implementation properly cleans up the allocated bufferThe code correctly reconstitutes the original Box and drops it, avoiding memory leaks. The safety comments explain the invariants well.
568-673: Comprehensive tests for FastUidGenerator cover edge casesThe tests verify UID generation with various scenarios including empty initial sets, existing symbols with
$prefixes, and corner cases with long prefixes.
This reverts commit ebc2730.
|
It looks like the +4% benchmark result was an illusion. The benchmark on a previous commit was atypically slow just due to noise, and that was the comparison point when I got the +4% speed-up. In later runs compared to other commits on main, the speed up is consistently around +2%. I am tending to agree with Boshen that the complication is not worthwhile for only +2%. But @Dunqing would like your opinion please before I close this. |
|
I don't think this is worthwhile. Closing. |
|
Sorry for the late response. Indeed, this is not worth getting 2% performance and leading |
Transformer produce more compact output by generating shorter UIDs. Producing these shorter UIDs is also faster.
Previously UIDs would depend on the base name passed to
generate_uid_namee.g.foo->_foo,_foo2,_foo3etc. This requires maintaining aHashMapof used names and a hashmap lookup for every UID.New scheme in this PR is much simpler. UIDs are
$a,$b,$c,$detc. The name passed togenerate_uid_nameis ignored. If the AST contains existing identifiers starting with$, UIDs will be$$a,$$betc, to avoid naming clashes. This does not require a hashmap, so is much cheaper.The rationale is that when building for production, there isn't much point spending cycles producing lengthy and descriptive
_foo_bar_qux2UIDs in the transformer, when the mangler will blow them all away anyway.Introduce a
debugoption to use the old lengthy UID style. We need this option for transformer conformance tests, and it can be useful for debugging. But setdebug: falseas the default.