Skip to content

perf(parser): store Modifiers on stack#20742

Merged
graphite-app[bot] merged 1 commit intomainfrom
om/03-23-perf_parser_store_modifiers_on_stack
Mar 26, 2026
Merged

perf(parser): store Modifiers on stack#20742
graphite-app[bot] merged 1 commit intomainfrom
om/03-23-perf_parser_store_modifiers_on_stack

Conversation

@overlookmotel
Copy link
Copy Markdown
Member

@overlookmotel overlookmotel commented Mar 25, 2026

Modifiers represents a set of 0 or more modifier keywords (public, private, static etc), up to a maximum of 15 possible modifiers.

Previously it was stored as:

pub struct Modifiers<'a> {
    /// Set of `Modifier`s, each containing `Span` of modifier keyword + modifier kind.
    modifiers: Option<Vec<'a, Modifier>>,
    /// Bitfield of which modifiers are present.
    kinds: ModifierKinds,
}

This is inefficient in a few ways:

  1. Modifiers is not part of the AST, only temporary data used during parsing. Using a Vec<'a, Modifier> results in this temporary data taking up space in the AST arena, which hangs around unused for the whole life of the AST.
  2. modifiers Vec can contains duplicates. This is pointless because using the same modifier twice is always a syntax error, and this is already handled by check_modifier.
  3. Modifier contains the Span of the modifier keyword. The length of modifier keywords is statically known from their kind (e.g. public is always 6 bytes). Storing just the start offsets of keywords is sufficient.

This PR switches to a more efficient representation:

pub struct Modifiers {
    /// Start offset for each modifier, indexed by `ModifierKind` discriminant.
    offsets: [MaybeUninit<u32>; 15],
    /// Bitfield of which modifiers are present.
    kinds: ModifierKinds,
}

This type contains all its data inline, and can be stored on the stack (64 bytes) - versus previous 32 bytes on stack + 16 bytes for each modifier in arena. Despite the larger footprint on stack, the use of MaybeUninit means that constructing an empty Modifiers requires less memory ops:

  • Now: 2-byte write to set kinds = ModifierKinds::none().
  • Previously: 2-byte write for kinds + 8-byte write to set modifiers = None.

Offsets are only written to offsets when a modifier is added. The kinds bitfield indicates which entries in offsets are present and initialized. All methods check kinds before accessing offsets, to avoid reading uninitialized memory (which would be UB).

Small positive impact on some benchmarks (+0.2%). To be honest, it's more of a "the principle of the matter" thing - temporary data should not be stored in the arena. This stack was also an experiment in getting Claude to create a stack of PRs unattended from an initial step-by-step plan.

Copy link
Copy Markdown
Member Author

overlookmotel commented Mar 25, 2026


How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • 0-merge - adds this PR to the back of the merge queue
  • hotfix - for urgent changes, fast-track this PR to the front of 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.

@overlookmotel overlookmotel force-pushed the om/03-23-perf_parser_store_modifiers_on_stack branch from 34034be to eb566f2 Compare March 25, 2026 15:54
@codspeed-hq
Copy link
Copy Markdown

codspeed-hq bot commented Mar 25, 2026

Merging this PR will not alter performance

✅ 53 untouched benchmarks
⏩ 3 skipped benchmarks1


Comparing om/03-23-perf_parser_store_modifiers_on_stack (4fc398f) with om/03-23-perf_parser_add_modifiers_get_method (126134b)

Open in CodSpeed

Footnotes

  1. 3 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 marked this pull request as ready for review March 25, 2026 18:58
Copilot AI review requested due to automatic review settings March 25, 2026 18:58
@overlookmotel overlookmotel force-pushed the om/03-23-perf_parser_add_modifiers_get_method branch from 3e043d7 to c37d9b5 Compare March 25, 2026 19:01
@overlookmotel overlookmotel force-pushed the om/03-23-perf_parser_store_modifiers_on_stack branch from eb566f2 to 2ac2e86 Compare March 25, 2026 19:01
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Refactors Modifiers (temporary parser-only state for TS/JS modifier keywords) to store modifier presence and keyword start offsets inline on the stack, avoiding arena allocations and duplicate storage while reconstructing Spans on demand.

Changes:

  • Replaces Modifiers<'a>’s Option<Vec<'a, Modifier>> storage with a fixed-size [MaybeUninit<u32>; 15] offsets array plus a ModifierKinds bitfield.
  • Updates parser call sites to use the new Modifiers API (no lifetime, new_single, updated iter/get semantics).
  • Adjusts invalid-modifier reporting to preserve source-order diagnostics while iterating modifiers in discriminant order.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.

Show a summary per file
File Description
tasks/track_memory_allocations/allocs_parser.snap Updates allocation snapshot numbers consistent with reduced arena usage.
crates/oxc_parser/src/modifiers.rs Implements new stack-based Modifiers representation and updates modifier parsing/validation logic.
crates/oxc_parser/src/js/statement.rs Switches const enum modifier construction to Modifiers::new_single.
crates/oxc_parser/src/js/module.rs Switches export default abstract class modifier construction to Modifiers::new_single.
crates/oxc_parser/src/js/function.rs Updates function parsing signatures to accept &Modifiers (no lifetime).
crates/oxc_parser/src/js/object.rs Updates method-getter/setter parsing signature to accept &Modifiers (no lifetime).
crates/oxc_parser/src/js/class.rs Updates class parsing signatures/usages to accept &Modifiers (no lifetime).
crates/oxc_parser/src/ts/statement.rs Updates TS declaration parsing signatures to accept &Modifiers (no lifetime).
crates/oxc_parser/src/ts/types.rs Updates TS signature parsing signatures to accept &Modifiers (no lifetime).

@graphite-app graphite-app bot force-pushed the om/03-23-perf_parser_add_modifiers_get_method branch from c37d9b5 to 23bf3b1 Compare March 25, 2026 19:13
@overlookmotel overlookmotel force-pushed the om/03-23-perf_parser_store_modifiers_on_stack branch from 2ac2e86 to 4fc398f Compare March 25, 2026 19:13
@overlookmotel overlookmotel force-pushed the om/03-23-perf_parser_add_modifiers_get_method branch from 23bf3b1 to 126134b Compare March 25, 2026 19:13
@graphite-app graphite-app bot force-pushed the om/03-23-perf_parser_store_modifiers_on_stack branch from 4fc398f to 24c0f9f Compare March 25, 2026 19:14
@overlookmotel overlookmotel force-pushed the om/03-23-perf_parser_store_modifiers_on_stack branch from 24c0f9f to 4fc398f Compare March 25, 2026 20:05
graphite-app bot pushed a commit that referenced this pull request Mar 26, 2026
The types for modifiers were confusing. We had `ModifierFlags` type defined with `bitflags!` macro, and `ModifierKind` enum which represents the different possible flags.

The two types weren't statically related, and consuming code needed to manually relate the two e.g. pairing usage of `ModifierFlags::DECLARE` with `ModifierKind::Declare`. This is unnecessarily complicated, and error-prone.

This PR turns `ModifierFlags` into a wrapper around a `u16` and adds methods to construct, query, and set bits in the set by passing a `ModifierKind` or a set of `ModifierKind`s. Consuming code now only has one type to deal with which is the single source of truth about what modifiers are available - `ModifierKind`.

Code is more verbose in some places, but personally I think this more explicit style is clearer. `ModifierFlags::new` is a const function, so even sizeable calls e.g. `ModifierFlags::new([ModifierKind::Public, ModifierKind::Private, ModifierKind::Protected])` is boiled down to a static `u16` at compile time. It compiles down to the same as the `bitflags!` version.

This change also enables #20742, which improves the storage of `Modifiers`.
@graphite-app
Copy link
Copy Markdown
Contributor

graphite-app bot commented Mar 26, 2026

Merge activity

@graphite-app graphite-app bot added 0-merge Merge with Graphite Merge Queue and removed 0-merge Merge with Graphite Merge Queue labels Mar 26, 2026
@camc314 camc314 added the 0-merge Merge with Graphite Merge Queue label Mar 26, 2026 — with Graphite App
@graphite-app graphite-app bot force-pushed the om/03-23-perf_parser_add_modifiers_get_method branch from 126134b to 4689d0e Compare March 26, 2026 13:38
graphite-app bot pushed a commit that referenced this pull request Mar 26, 2026
`Modifiers` represents a set of 0 or more modifier keywords (`public`, `private`, `static` etc), up to a maximum of 15 possible modifiers.

Previously it was stored as:

```rs
pub struct Modifiers<'a> {
    /// Set of `Modifier`s, each containing `Span` of modifier keyword + modifier kind.
    modifiers: Option<Vec<'a, Modifier>>,
    /// Bitfield of which modifiers are present.
    kinds: ModifierKinds,
}
```

This is inefficient in a few ways:

1. `Modifiers` is not part of the AST, only temporary data used during parsing. Using a `Vec<'a, Modifier>` results in this temporary data taking up space in the AST arena, which hangs around unused for the whole life of the AST.
2. `modifiers` `Vec` can contains duplicates. This is pointless because using the same modifier twice is always a syntax error, and this is already handled by `check_modifier`.
3. `Modifier` contains the `Span` of the modifier keyword. The length of modifier keywords is statically known from their kind (e.g. `public` is always 6 bytes). Storing just the start offsets of keywords is sufficient.

This PR switches to a more efficient representation:

```rs
pub struct Modifiers {
    /// Start offset for each modifier, indexed by `ModifierKind` discriminant.
    offsets: [MaybeUninit<u32>; 15],
    /// Bitfield of which modifiers are present.
    kinds: ModifierKinds,
}
```

This type contains all its data inline, and can be stored on the stack (64 bytes) - versus previous 32 bytes on stack + 16 bytes for each modifier in arena. Despite the larger footprint on stack, the use of `MaybeUninit` means that constructing an empty `Modifiers` requires less memory ops:

* Now: 2-byte write to set `kinds = ModifierKinds::none()`.
* Previously: 2-byte write for `kinds` + 8-byte write to set `modifiers = None`.

Offsets are only written to `offsets` when a modifier is added. The `kinds` bitfield indicates which entries in `offsets` are present and initialized. All methods check `kinds` before accessing `offsets`, to avoid reading uninitialized memory (which would be UB).

Small positive impact on some benchmarks (+0.2%). To be honest, it's more of a "the principle of the matter" thing - temporary data should not be stored in the arena. This stack was also an experiment in getting Claude to create a stack of PRs unattended from an initial step-by-step plan.
@graphite-app graphite-app bot force-pushed the om/03-23-perf_parser_store_modifiers_on_stack branch from 4fc398f to bc58fe2 Compare March 26, 2026 13:39
`Modifiers` represents a set of 0 or more modifier keywords (`public`, `private`, `static` etc), up to a maximum of 15 possible modifiers.

Previously it was stored as:

```rs
pub struct Modifiers<'a> {
    /// Set of `Modifier`s, each containing `Span` of modifier keyword + modifier kind.
    modifiers: Option<Vec<'a, Modifier>>,
    /// Bitfield of which modifiers are present.
    kinds: ModifierKinds,
}
```

This is inefficient in a few ways:

1. `Modifiers` is not part of the AST, only temporary data used during parsing. Using a `Vec<'a, Modifier>` results in this temporary data taking up space in the AST arena, which hangs around unused for the whole life of the AST.
2. `modifiers` `Vec` can contains duplicates. This is pointless because using the same modifier twice is always a syntax error, and this is already handled by `check_modifier`.
3. `Modifier` contains the `Span` of the modifier keyword. The length of modifier keywords is statically known from their kind (e.g. `public` is always 6 bytes). Storing just the start offsets of keywords is sufficient.

This PR switches to a more efficient representation:

```rs
pub struct Modifiers {
    /// Start offset for each modifier, indexed by `ModifierKind` discriminant.
    offsets: [MaybeUninit<u32>; 15],
    /// Bitfield of which modifiers are present.
    kinds: ModifierKinds,
}
```

This type contains all its data inline, and can be stored on the stack (64 bytes) - versus previous 32 bytes on stack + 16 bytes for each modifier in arena. Despite the larger footprint on stack, the use of `MaybeUninit` means that constructing an empty `Modifiers` requires less memory ops:

* Now: 2-byte write to set `kinds = ModifierKinds::none()`.
* Previously: 2-byte write for `kinds` + 8-byte write to set `modifiers = None`.

Offsets are only written to `offsets` when a modifier is added. The `kinds` bitfield indicates which entries in `offsets` are present and initialized. All methods check `kinds` before accessing `offsets`, to avoid reading uninitialized memory (which would be UB).

Small positive impact on some benchmarks (+0.2%). To be honest, it's more of a "the principle of the matter" thing - temporary data should not be stored in the arena. This stack was also an experiment in getting Claude to create a stack of PRs unattended from an initial step-by-step plan.
@graphite-app graphite-app bot force-pushed the om/03-23-perf_parser_add_modifiers_get_method branch from 4689d0e to 511d5e5 Compare March 26, 2026 13:43
@graphite-app graphite-app bot force-pushed the om/03-23-perf_parser_store_modifiers_on_stack branch from bc58fe2 to 5f9bee5 Compare March 26, 2026 13:43
@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Mar 26, 2026
Base automatically changed from om/03-23-perf_parser_add_modifiers_get_method to main March 26, 2026 13:49
@camc314 camc314 added the 0-merge Merge with Graphite Merge Queue label Mar 26, 2026
@graphite-app graphite-app bot merged commit 5f9bee5 into main Mar 26, 2026
28 checks passed
@graphite-app graphite-app bot deleted the om/03-23-perf_parser_store_modifiers_on_stack branch March 26, 2026 13:50
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-parser Area - Parser 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.

3 participants