perf(traverse): `generate_uid` cache available binding names by overlookmotel · Pull Request #5611 · oxc-project/oxc

overlookmotel · 2024-09-08T13:45:30Z

generate_uid previously iterated through every symbol and unresolved reference in the AST to find a unique var name. If the first var name it tried was already in use, it'd iterate again.

Instead build a hash map recording existing var names in use for every name which could clash with a UID (any var name starting with _). Once built, use that hash map to generate UIDs without iterating through all symbols again.

I had hoped to make generate_uid cheaper still by just recording the highest digits postfix for each var name, and then incrementing that postfix for each UID. i.e. if AST contains vars _foo1 and _foo6, create UIDs starting at one number higher - _foo7, _foo8 etc. This method would be more efficient, but unfortunately it does not match Babel, and so causes some of Babel's tests to fail.

graphite-app · 2024-09-08T13:45:35Z

Your org has enabled the Graphite merge queue for merging into main

Add the label “0-merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

overlookmotel · 2024-09-08T13:45:41Z

perf(traverse): generate_uid cache available binding names #5611 : 2 dependent PRs (#5623 , #5626 ) 👈
feat(span): format_compact_str! macro #5610
refactor(span): move CompactStr into separate file #5609
main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @overlookmotel and the rest of your teammates on Graphite

codspeed-hq · 2024-09-08T14:04:42Z

CodSpeed Performance Report

Merging #5611 will improve performances by ×2.6

_{Comparing 09-08-perf_traverse_generate_uid_cache_available_binding_names (4996874) with main (e698418)}

Summary

⚡ 3 improvements
✅ 26 untouched benchmarks

Benchmarks breakdown

	Benchmark	`main`	`09-08-perf_traverse_generate_uid_cache_available_binding_names`	Change
⚡	`transformer[cal.com.tsx]`	62 ms	23.9 ms	×2.6
⚡	`transformer[checker.ts]`	28.8 ms	16.7 ms	+72.85%
⚡	`transformer[pdf.mjs]`	10.5 ms	6.4 ms	+63.06%

overlookmotel · 2024-09-08T17:38:51Z

I'll see if can tweak it a little to speed it up further in follow-up PRs.

overlookmotel · 2024-09-08T17:43:56Z

There was a time when this would have been pretty spectacular. But after #5518 it's futile trying to compete!

overlookmotel · 2024-09-09T08:53:23Z

Have merged the improvements made in #5623 and #5624 into this. Ready for review.

graphite-app · 2024-09-10T01:11:19Z

Merge activity

Sep 9, 9:11 PM EDT: Boshen added this pull request to the Graphite merge queue.
Sep 9, 9:16 PM EDT: Boshen merged this pull request with the Graphite merge queue.

Close #5488. `generate_uid` previously iterated through every symbol and unresolved reference in the AST to find a unique var name. If the first var name it tried was already in use, it'd iterate again. Instead build a hash map recording existing var names in use for every name which could clash with a UID (any var name starting with `_`). Once built, use that hash map to generate UIDs without iterating through all symbols again. I had hoped to make `generate_uid` cheaper still by just recording the highest digits postfix for each var name, and then incrementing that postfix for each UID. i.e. if AST contains vars `_foo1` and `_foo6`, create UIDs starting at one number higher - `_foo7`, `_foo8` etc. This method would be more efficient, but unfortunately it does not match Babel, and so causes some of Babel's tests to fail.

## [0.28.0] - 2024-09-11 - afc4548 ast: [**BREAKING**] Educe byte size of `TaggedTemplateExpression::quasi` by `Boxing` it (#5679) (Boshen) - 7415e85 ast: [**BREAKING**] Reduce byte size of `TSImportType::attributes` by `Box`ing it (#5678) (Boshen) - ee4fb42 ast: [**BREAKING**] Reduce size of `WithClause` by `Box`ing it (#5677) (Boshen) - 1fa3e56 semantic: [**BREAKING**] Rename `SymbolTable::iter` to `symbol_ids` (#5621) (overlookmotel) - 96a1552 semantic: [**BREAKING**] Remove `SymbolTable::iter_rev` (#5620) (overlookmotel) - 4a8aec1 span: [**BREAKING**] Change `SourceType::js` to `SourceType::cjs` and `SourceType::mjs` (#5606) (Boshen) - 603817b oxc: [**BREAKING**] Add `SourceType::Unambiguous`; parse `.js` as unambiguous (#5557) (Boshen) - b060525 semantic: [**BREAKING**] Remove `source_type` argument from `SemanticBuilder::new` (#5553) (Boshen) ### Features - 2da5ad1 ast: Add `JSXElementName::get_identifier` method (#5556) (overlookmotel) - 2016bae coverage: Add regular expression idempotency test (#5676) (Boshen) - 68c3cf5 minifier: Fold `void 1` -> `void 0` (#5670) (Boshen) - c6bbf94 minifier: Constant fold unary expression (#5669) (Boshen) - 86256ea minifier: Constant fold `typeof` (#5666) (Boshen) - e698418 napi/transform: Align output `SourceMap` with Rollup's `ExistingRawSourceMap` (#5657) (Boshen) - aba9194 napi/transform: Export react refresh options (#5533) (underfin) - 642295c semantic: Add `SymbolTable::delete_resolved_reference` method (#5558) (overlookmotel) - b3cbd56 span: `format_compact_str!` macro (#5610) (overlookmotel) - 95a6d99 transformer: Enable the react refresh plugin in enable_all (#5630) (Dunqing) - 7b543df transformer/react: Handle `refresh_sig` and `refresh_reg` options correctly (#5638) (Dunqing) - 17226dd traverse: Add methods for deleting references (#5559) (overlookmotel) ### Bug Fixes - d62defb codegen: Do not print trailing commas for `ArrayExpression` (#5551) (Boshen) - 1bc08e2 coverage: Parse babel unambiguously (#5579) (Boshen) - 28b934c coverage: Apply `always_strict` to test262 and typescript per the specifcation (#5555) (Boshen) - b9bf544 isolated-declarations: False positive for setter method in `interface` (#5681) (Dunqing) - 6e8409a isolated-declarations: Bindings referenced in `TSModuleDeclaration` are removed incorrectly (#5680) (Dunqing) - b8f8dd6 minifier/replace_global_defines: Do not replace shadowed identifiers (#5691) (Boshen) - 304ce25 regular_expression: Keep LegacyOctalEscape raw digits for `to_string` (#5692) (leaysgur) - 0511d55 regular_expression: Report more MayContainStrings error in (nested)class (#5661) (leaysgur) - 41582ea regular_expression: Improve RegExp `to_string()` results (#5635) (leaysgur) - 28aad28 regular_expression: Handle `-` in `/[\-]/u` as escaped character (#5631) (leaysgur) - f9e3a41 semantic: Bind `SymbolId` to function name in `if (foo) function id() {}` (#5673) (Boshen) - f49e6eb span: Treat `.js` as `module` file (reverts the previous breaking change) (#5612) (Boshen) - 919d17f transform_conformance: Only print semantic mismatch errors when output is correct (#5589) (Boshen) - 505d064 transformer: JSX transform delete references for `JSXClosingElement`s (#5560) (overlookmotel) - 9b7ecc7 transformer: RegExp transform only set span on final expression (#5508) (overlookmotel) - d1ece19 transformer: RegExp transform handle `Term::Quantifier` (#5501) (overlookmotel) - a1afd48 transformer/react: Incorrect scope_id for var hoisted in fast refresh plugin (#5695) (Dunqing) - f2f5e5a transformer/react: Missing scope_id for function in fast refresh plugin (#5693) (Dunqing) - a891c31 transformer/react: Refresh plugin has incorrect reference flags (#5656) (Dunqing) - 3e8b96f transformer/react: The refresh plugin cannot handle member expressions with React hooks (#5655) (Dunqing) - 0739b5f transformer/react: Don't transform declaration of function overloads (#5642) (Dunqing) - 3bf6aaf transformer/react: Support `emit_full_signatures` option in refresh plugin (#5629) (Dunqing) - 36d864a transformer/react: Don't transform if the variable does not have a value reference (#5528) (Dunqing) ### Performance - e8013d2 traverse: Faster string operations generating UIDs (#5626) (overlookmotel) - 4996874 traverse: `generate_uid` cache available binding names (#5611) (overlookmotel) ### Documentation - fefbbc1 sourcemap: Add trailing newline to README (#5539) (overlookmotel) - 9282647 transformer: Comment on RegExp transform for potential improvement (#5514) (overlookmotel) - 1c051ae traverse: Correct code comment 2 (#5607) (overlookmotel) - 2e24a15 traverse: Correct code comment (#5604) (overlookmotel) ### Refactor - 14ee086 ast: Inline `AstKind::as_*` methods (#5547) (overlookmotel) - 2de6ea0 index, traverse: Remove unnecessary type annotations (#5650) (overlookmotel) - 0ac420d linter: Use meaningful names for diagnostic parameters (#5564) (Don Isaac) - 2da42ef regular_expression: Improve AST docs with refactoring may_contain_strings (#5665) (leaysgur) - dec1395 regular_expression: Align diagnostics (#5543) (leaysgur) - 731ffaa semantic: Compare nodes by pointer equality (#5686) (overlookmotel) - 067f9b5 semantic: Introduce `IsGlobalReference` trait (#5672) (Boshen) - d22a9b7 semantic: `SymbolTable::is_empty` use `is_empty` (#5622) (overlookmotel) - 3d190a5 span: Move `CompactStr` into separate file (#5609) (overlookmotel) - 5532628 span: Put types and impl in the same mod file (Boshen) - ce71982 transformer: Shorten code in JSX transform (#5554) (overlookmotel) - 758a10c transformer: RegExp transform reuse var (#5527) (overlookmotel) - fad0a05 transformer: RegExp transform unbox early (#5504) (overlookmotel) - 19cdcc5 traverse: Revert changes to `walk.rs` (#5652) (overlookmotel)- 26d9235 Enable clippy::ref_as_ptr (#5577) (夕舞八弦) ### Styling - e52d006 traverse: Fix formatting of traverse codegen (#5651) (overlookmotel) - 97e99bd traverse: Remove excess line break (#5603) (overlookmotel)- 694f032 Add trailing line breaks to `package.json` files (#5542) (overlookmotel) ### Testing - 2e367c9 traverse: Enable tests for `oxc_traverse` crate (#5625) (overlookmotel)- dc92489 Add trailing line breaks to conformance fixtures (#5541) (overlookmotel) --------- Co-authored-by: Boshen <1430279+Boshen@users.noreply.github.com> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>

This was referenced Sep 8, 2024

refactor(span): move CompactStr into separate file #5609

Merged

feat(span): format_compact_str! macro #5610

Merged

overlookmotel marked this pull request as ready for review September 8, 2024 13:49

overlookmotel marked this pull request as draft September 8, 2024 13:50

overlookmotel force-pushed the 09-08-feat_span_format_compact_str_macro branch from 4ea26bc to 0578754 Compare September 8, 2024 13:54

overlookmotel force-pushed the 09-08-perf_traverse_generate_uid_cache_available_binding_names branch from 1678069 to d3fcb66 Compare September 8, 2024 13:54

Boshen changed the base branch from 09-08-feat_span_format_compact_str_macro to graphite-base/5611 September 8, 2024 14:15

Boshen force-pushed the 09-08-perf_traverse_generate_uid_cache_available_binding_names branch from d3fcb66 to db35b84 Compare September 8, 2024 14:22

Boshen force-pushed the graphite-base/5611 branch from 0578754 to b3cbd56 Compare September 8, 2024 14:22

Boshen changed the base branch from graphite-base/5611 to main September 8, 2024 14:23

Boshen force-pushed the 09-08-perf_traverse_generate_uid_cache_available_binding_names branch from db35b84 to 26769b4 Compare September 8, 2024 14:23

DonIsaac added C-performance Category - Solution not expected to change functional behavior, only performance A-transformer Area - Transformer / Transpiler labels Sep 8, 2024

overlookmotel force-pushed the 09-08-perf_traverse_generate_uid_cache_available_binding_names branch 2 times, most recently from 42c8adc to 044958c Compare September 8, 2024 17:37

overlookmotel marked this pull request as ready for review September 8, 2024 17:38

overlookmotel requested a review from Boshen September 8, 2024 17:38

overlookmotel force-pushed the 09-08-perf_traverse_generate_uid_cache_available_binding_names branch 2 times, most recently from 8077398 to c7c4d6f Compare September 8, 2024 22:08

This was referenced Sep 8, 2024

perf(traverse): UIDs cache store used names as FxHashSet instead of Vec #5623

Closed

perf(traverse): UIDs cache store used names as single FxHashSet #5624

Closed

perf(traverse): faster string operations generating UIDs #5626

Merged

overlookmotel marked this pull request as draft September 9, 2024 00:48

overlookmotel force-pushed the 09-08-perf_traverse_generate_uid_cache_available_binding_names branch from c7c4d6f to a3ab781 Compare September 9, 2024 08:51

overlookmotel marked this pull request as ready for review September 9, 2024 08:53

Boshen requested a review from Dunqing September 9, 2024 09:18

Boshen approved these changes Sep 9, 2024

View reviewed changes

Boshen assigned Dunqing Sep 9, 2024

overlookmotel force-pushed the 09-08-perf_traverse_generate_uid_cache_available_binding_names branch from a3ab781 to 1bdd5d9 Compare September 9, 2024 10:43

Boshen added the 0-merge Merge with Graphite Merge Queue label Sep 10, 2024 — with Graphite App

Boshen force-pushed the 09-08-perf_traverse_generate_uid_cache_available_binding_names branch from 1bdd5d9 to 4996874 Compare September 10, 2024 01:12

graphite-app bot merged commit 4996874 into main Sep 10, 2024

graphite-app bot deleted the 09-08-perf_traverse_generate_uid_cache_available_binding_names branch September 10, 2024 01:16

oxc-bot mentioned this pull request Sep 11, 2024

release(oxc): crates v0.28.0 #5696

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Comments

perf(traverse): `generate_uid` cache available binding names#5611

perf(traverse): `generate_uid` cache available binding names#5611
graphite-app[bot] merged 1 commit intomainfrom
09-08-perf_traverse_generate_uid_cache_available_binding_names

overlookmotel commented Sep 8, 2024 •

edited

Loading

Uh oh!

graphite-app bot commented Sep 8, 2024

Uh oh!

overlookmotel commented Sep 8, 2024 •

edited

Loading

Uh oh!

codspeed-hq bot commented Sep 8, 2024 •

edited

Loading

Uh oh!

overlookmotel commented Sep 8, 2024

Uh oh!

overlookmotel commented Sep 8, 2024

Uh oh!

overlookmotel commented Sep 9, 2024

Uh oh!

graphite-app bot commented Sep 10, 2024 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Comments

Conversation

overlookmotel commented Sep 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

graphite-app bot commented Sep 8, 2024

Your org has enabled the Graphite merge queue for merging into main

Uh oh!

overlookmotel commented Sep 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codspeed-hq bot commented Sep 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CodSpeed Performance Report

Merging #5611 will improve performances by ×2.6

Summary

Benchmarks breakdown

Uh oh!

overlookmotel commented Sep 8, 2024

Uh oh!

overlookmotel commented Sep 8, 2024

Uh oh!

overlookmotel commented Sep 9, 2024

Uh oh!

graphite-app bot commented Sep 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge activity

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

overlookmotel commented Sep 8, 2024 •

edited

Loading

overlookmotel commented Sep 8, 2024 •

edited

Loading

codspeed-hq bot commented Sep 8, 2024 •

edited

Loading

graphite-app bot commented Sep 10, 2024 •

edited

Loading