Skip to content

perf: pass string literals to AST builder methods#10420

Merged
graphite-app[bot] merged 1 commit into
mainfrom
om/07-23-perf_pass_string_literals_to_ast_builder_methods
Jul 24, 2026
Merged

perf: pass string literals to AST builder methods#10420
graphite-app[bot] merged 1 commit into
mainfrom
om/07-23-perf_pass_string_literals_to_ast_builder_methods

Conversation

@overlookmotel

@overlookmotel overlookmotel commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator

Follow-on after #10018.

AST builder methods accept any type which is Into<Str<'ast>> for Str fields. This includes &'static strs. Where we have a &'static str, pass it directly to the builder method, rather than via Str::from_str_in.

This avoids unnecessarily allocating the string into the arena. As a side effect, it also makes the code shorter and more readable.

Before:

ast::BindingPattern::new_binding_identifier(
  SPAN,
  ast::Str::from_str_in("m", &self.ast_builder),
  &self.ast_builder,
)

After:

ast::BindingPattern::new_binding_identifier(SPAN, "m", &self.ast_builder)

overlookmotel commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator Author

How to use the Graphite Merge Queue

Add the label graphite: merge-when-ready to this PR to add it to 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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR is a small performance/cleanup follow-on to the oxc per-type AST construction migration: it updates AST builder call sites to pass &'static str literals directly (via Into<Str<'a>>) instead of allocating arena strings with Str::from_str_in, reducing arena allocations and simplifying code.

Changes:

  • Replaced Str::from_str_in("<literal>", builder) with direct string literals in several AST constructor calls.
  • Simplified AST construction in core finalizers, an ECMA utilities factory helper, and a Vite web worker post plugin visitor.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
crates/rolldown/src/module_finalizers/mod.rs Uses string literals directly in AST binding construction; also (unintentionally) drops a clippy suppression attribute.
crates/rolldown_plugin_vite_web_worker_post/src/ast_visitor.rs Passes "url" directly to PropertyKey::new_static_identifier to avoid arena allocation.
crates/rolldown_ecmascript_utils/src/ast_factory.rs Replaces several arena-allocated identifier strings with string literals in helper constructors.

Comment thread crates/rolldown/src/module_finalizers/mod.rs
@codspeed-hq

codspeed-hq Bot commented Jul 23, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 7 untouched benchmarks
⏩ 10 skipped benchmarks1


Comparing om/07-23-perf_pass_string_literals_to_ast_builder_methods (66a13fa) with om/07-23-refactor_pass_arrays_to_ast_builder_methods (d5762eb)

Open in CodSpeed

Footnotes

  1. 10 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.

@overlookmotel
overlookmotel force-pushed the om/07-23-refactor_pass_arrays_to_ast_builder_methods branch from de7535b to d5762eb Compare July 24, 2026 00:02
@overlookmotel
overlookmotel force-pushed the om/07-23-perf_pass_string_literals_to_ast_builder_methods branch from ea4ede0 to 66a13fa Compare July 24, 2026 00:02

@hyfdev hyfdev left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

— Review by @hyfdev, assisted by GPT-5

@graphite-app

graphite-app Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Merge activity

Follow-on after #10018.

AST builder methods accept any type which is `Into<Str<'ast>>` for `Str` fields. This includes `&'static str`s. Where we have a `&'static str`, pass it directly to the builder method, rather than via `Str::from_str_in`.

This avoids unnecessarily allocating the string into the arena. As a side effect, it also makes the code shorter and more readable.

Before:

```rust
ast::BindingPattern::new_binding_identifier(
  SPAN,
  ast::Str::from_str_in("m", &self.ast_builder),
  &self.ast_builder,
)
```

After:

```rust
ast::BindingPattern::new_binding_identifier(SPAN, "m", &self.ast_builder)
```
@graphite-app
graphite-app Bot force-pushed the om/07-23-refactor_pass_arrays_to_ast_builder_methods branch from d5762eb to 4240c14 Compare July 24, 2026 03:44
@graphite-app
graphite-app Bot requested a review from h-a-n-a as a code owner July 24, 2026 03:44
@graphite-app
graphite-app Bot force-pushed the om/07-23-perf_pass_string_literals_to_ast_builder_methods branch from 66a13fa to f38313a Compare July 24, 2026 03:44
graphite-app Bot pushed a commit that referenced this pull request Jul 24, 2026
)

Follow-on after #10018, continuation of #10420.

Custom AST builder methods (previously methods of `AstFactory`) take `&str`s. They allocate the string into arena, to obtain a `&'ast str` (allocator lifetime). Where the string is a `&'static str`, it's unnecessary to allocate into arena to extend the lifetime.

In these cases, use standard AST builder methods provided by Oxc, and skip the allocation.

Before:

```rust
BindingIdentifier::new_id(SPAN, "__rolldown_default__", &self.ast_builder)
```

After:

```rust
BindingIdentifier::new(SPAN, "__rolldown_default__", &self.ast_builder)
```
graphite-app Bot pushed a commit that referenced this pull request Jul 24, 2026
Follow-on after #10018, continuation of #10420 and #10421.

When converting `&'static str`s to `Str`s, use `Str::from` instead of `Str::from_str_in`. The difference is that `from_str_in` performs an allocation (in arena), whereas `from` is a zero-cost no-op conversion at runtime. When the `&str` already has a `'static` lifetime, it's unnecessary to extend it by allocating it into arena.

Before:

```rs
TemplateElementValue {
  raw: Str::from_str_in("/@vite/lazy?id=", &self.ast_builder),
  cooked: None,
}
```

After:

```rs
TemplateElementValue { raw: Str::from("/@vite/lazy?id="), cooked: None }
```
graphite-app Bot pushed a commit that referenced this pull request Jul 24, 2026
Follow-on after #10018, continuation of #10420 and #10421.

Where we already have a `Str<'ast>` or `Ident<'ast>`, avoid converting it into a `&str` and then allocating it back into arena again to convert back to `Str` or `Ident`. Just re-use the existing `Str` / `Ident`. This avoids unnecessary arena allocations, and in the case of `Ident`, it avoids re-hashing the string too.
Base automatically changed from om/07-23-refactor_pass_arrays_to_ast_builder_methods to main July 24, 2026 03:54
@graphite-app
graphite-app Bot merged commit f38313a into main Jul 24, 2026
32 checks passed
@graphite-app
graphite-app Bot deleted the om/07-23-perf_pass_string_literals_to_ast_builder_methods branch July 24, 2026 03:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants