Skip to content

perf(ast, codegen, transformer): avoid allocations when converting RegExpFlags to string#9550

Merged
graphite-app[bot] merged 1 commit intomainfrom
03-04-perf_ast_codegen_transformer_avoid_allocations_when_converting_regexpflags_to_string
Mar 5, 2025
Merged

perf(ast, codegen, transformer): avoid allocations when converting RegExpFlags to string#9550
graphite-app[bot] merged 1 commit intomainfrom
03-04-perf_ast_codegen_transformer_avoid_allocations_when_converting_regexpflags_to_string

Conversation

@overlookmotel
Copy link
Member

@overlookmotel overlookmotel commented Mar 4, 2025

In various places, we were using RegExpFlags::to_string when we want a &str. This creates a String temporarily, which is an unnecessary allocation.

Add RegExpFlags::to_inline_string method which creates an InlineString instead, without any allocation. Use this method wherever we convert from RegExpFlags to &str.

The downside of this change is that oxc_ast gains a dependency on oxc_data_structures. Personally, I don't think this is so bad, as oxc_data_structures is a pretty lightweight crate, containing only quite minimalist data structures. The code is mostly comments! However, what we could do is put each component of oxc_data_structures behind it's own feature, so consumers can take only what they need.

Copy link
Member Author

overlookmotel commented Mar 4, 2025

@github-actions github-actions bot added A-ast Area - AST A-transformer Area - Transformer / Transpiler A-codegen Area - Code Generation labels Mar 4, 2025
@github-actions github-actions bot added the C-performance Category - Solution not expected to change functional behavior, only performance label Mar 4, 2025
@overlookmotel overlookmotel marked this pull request as ready for review March 4, 2025 21:04
@overlookmotel overlookmotel requested a review from Dunqing as a code owner March 4, 2025 21:04
@codspeed-hq
Copy link

codspeed-hq bot commented Mar 4, 2025

CodSpeed Performance Report

Merging #9550 will create unknown performance changes

Comparing 03-04-perf_ast_codegen_transformer_avoid_allocations_when_converting_regexpflags_to_string (6b4a8c6) with main (3e4f909)

Summary

🆕 39 new benchmarks

Benchmarks breakdown

Benchmark BASE HEAD Change
🆕 parser_napi[RadixUIAdoptionSection.jsx] N/A 4.8 ms N/A
🆕 parser_napi[pdf.mjs] N/A 1.8 s N/A
🆕 parser_napi_raw[RadixUIAdoptionSection.jsx] N/A 1.3 ms N/A
🆕 parser_napi_raw[cal.com.tsx] N/A 699.8 ms N/A
🆕 parser_napi_raw[checker.ts] N/A 1.1 s N/A
🆕 parser_napi_raw[pdf.mjs] N/A 266.6 ms N/A
🆕 codegen[checker.ts] N/A 23.1 ms N/A
🆕 codegen_sourcemap[checker.ts] N/A 66.2 ms N/A
🆕 isolated-declarations[vue-id.ts] N/A 57.9 ms N/A
🆕 lexer[RadixUIAdoptionSection.jsx] N/A 20.8 µs N/A
🆕 lexer[antd.js] N/A 24.1 ms N/A
🆕 lexer[cal.com.tsx] N/A 5.7 ms N/A
🆕 lexer[checker.ts] N/A 14.5 ms N/A
🆕 lexer[pdf.mjs] N/A 3.8 ms N/A
🆕 linter[RadixUIAdoptionSection.jsx] N/A 2.7 ms N/A
🆕 linter[cal.com.tsx] N/A 1.2 s N/A
🆕 linter[checker.ts] N/A 2.9 s N/A
🆕 mangler[antd.js] N/A 16 ms N/A
🆕 mangler[react.development.js] N/A 295.2 µs N/A
🆕 mangler[typescript.js] N/A 39.7 ms N/A
... ... ... ... ...

ℹ️ Only the first 20 benchmarks are displayed. Go to the app to view all benchmarks.

@camchenry
Copy link
Member

camchenry commented Mar 5, 2025

I think the additional dependency is okay personally. It's not too surprising to me that the AST depends on data structures, especially custom ones. We can always turn them into separate crates (or features like you say) later.

@graphite-app graphite-app bot added the 0-merge Merge with Graphite Merge Queue label Mar 5, 2025
@graphite-app
Copy link
Contributor

graphite-app bot commented Mar 5, 2025

Merge activity

…egExpFlags` to string (#9550)

In various places, we were using `RegExpFlags::to_string` when we want a `&str`. This creates a `String` temporarily, which is an unnecessary allocation.

Add `RegExpFlags::to_inline_string` method which creates an `InlineString` instead, without any allocation. Use this method wherever we convert from `RegExpFlags` to `&str`.

The downside of this change is that `oxc_ast` gains a dependency on `oxc_data_structures`. Personally, I don't think this is so bad, as `oxc_data_structures` is a pretty lightweight crate, containing only quite minimalist data structures. The code is mostly comments! However, what we could do is put each component of `oxc_data_structures` behind it's own feature, so consumers can take only what they need.
@graphite-app graphite-app bot force-pushed the 03-04-feat_data_structures_move_inlinestring_into_oxc_data_structures_crate branch from b28aa93 to 29041fb Compare March 5, 2025 02:23
@graphite-app graphite-app bot force-pushed the 03-04-perf_ast_codegen_transformer_avoid_allocations_when_converting_regexpflags_to_string branch from 293598d to 6b4a8c6 Compare March 5, 2025 02:23
Base automatically changed from 03-04-feat_data_structures_move_inlinestring_into_oxc_data_structures_crate to main March 5, 2025 02:43
@graphite-app graphite-app bot merged commit 6b4a8c6 into main Mar 5, 2025
35 checks passed
@graphite-app graphite-app bot deleted the 03-04-perf_ast_codegen_transformer_avoid_allocations_when_converting_regexpflags_to_string branch March 5, 2025 02:44
@oxc-bot oxc-bot mentioned this pull request Mar 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

0-merge Merge with Graphite Merge Queue A-ast Area - AST A-codegen Area - Code Generation A-transformer Area - Transformer / Transpiler C-performance Category - Solution not expected to change functional behavior, only performance

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants