Skip to content

perf(parser): fast path for no syntax errors when checking modifiers#20748

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

perf(parser): fast path for no syntax errors when checking modifiers#20748
graphite-app[bot] merged 1 commit intomainfrom
om/03-23-perf_parser_fast_path_for_no_syntax_errors_when_checking_modifiers

Conversation

@overlookmotel
Copy link
Copy Markdown
Member

@overlookmotel overlookmotel commented Mar 25, 2026

check_modifier contained a large chunk of code with nested matches - lots of branches, all to handle cases which should never happen (syntax errors).

Instead, do a fast check for validity using only a couple of operations and a small lookup table. Only if that fast check fails, hand over to a 2nd function illegal_modifier_error which does the slow work of figuring out what error to raise and raising it. In normal operation, that path should never be taken, so mark it #[cold] and #[inline(never)].

This also replaces an unpredictable branch with a very predictable one.

Small perf improvement on some parser benchmarks (+0.2%).

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.

@codspeed-hq
Copy link
Copy Markdown

codspeed-hq bot commented Mar 25, 2026

Merging this PR will not alter performance

✅ 48 untouched benchmarks
⏩ 8 skipped benchmarks1


Comparing om/03-23-perf_parser_fast_path_for_no_syntax_errors_when_checking_modifiers (1e19687) with main (1a370a6)

Open in CodSpeed

Footnotes

  1. 8 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 changed the base branch from om/03-23-perf_parser_store_modifiers_on_stack to graphite-base/20748 March 25, 2026 21:56
@overlookmotel overlookmotel force-pushed the om/03-23-perf_parser_fast_path_for_no_syntax_errors_when_checking_modifiers branch from d6ce961 to 01e74d8 Compare March 25, 2026 21:56
@overlookmotel overlookmotel force-pushed the om/03-23-perf_parser_fast_path_for_no_syntax_errors_when_checking_modifiers branch from 01e74d8 to 85855c0 Compare March 25, 2026 22:13
@overlookmotel overlookmotel force-pushed the om/03-23-perf_parser_fast_path_for_no_syntax_errors_when_checking_modifiers branch from 85855c0 to d457c11 Compare March 26, 2026 12:44
@overlookmotel overlookmotel changed the base branch from graphite-base/20748 to om/03-25-perf_parser_remove_a_lookahead_from_eat_modifiers_before_declaration_ March 26, 2026 12:44
@graphite-app graphite-app bot changed the base branch from om/03-25-perf_parser_remove_a_lookahead_from_eat_modifiers_before_declaration_ to graphite-base/20748 March 26, 2026 13:41
@graphite-app graphite-app bot force-pushed the om/03-23-perf_parser_fast_path_for_no_syntax_errors_when_checking_modifiers branch from d457c11 to 51e6a42 Compare March 26, 2026 13:53
@graphite-app graphite-app bot force-pushed the graphite-base/20748 branch from aac33f9 to 4e8b817 Compare March 26, 2026 13:53
@graphite-app graphite-app bot changed the base branch from graphite-base/20748 to main March 26, 2026 13:53
@graphite-app graphite-app bot force-pushed the om/03-23-perf_parser_fast_path_for_no_syntax_errors_when_checking_modifiers branch from 51e6a42 to dbffd43 Compare March 26, 2026 13:53
@overlookmotel overlookmotel force-pushed the om/03-23-perf_parser_fast_path_for_no_syntax_errors_when_checking_modifiers branch 2 times, most recently from 8b10406 to b26e94c Compare March 26, 2026 23:30
@overlookmotel overlookmotel marked this pull request as ready for review March 26, 2026 23:36
Copilot AI review requested due to automatic review settings March 26, 2026 23:36
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

Improves parser modifier validation performance by adding a fast-path check that avoids the branch-heavy error-construction logic in the common (no syntax errors) case.

Changes:

  • Refactors check_modifier to first run a fast legality check (is_illegal_modifier) and only fall back to error construction when needed.
  • Adds a compile-time-generated lookup table to decide illegal modifier ordering via a single table lookup + bitset intersection.
  • Adds a unit test to ensure is_illegal_modifier stays consistent with illegal_modifier_error.

@overlookmotel overlookmotel force-pushed the om/03-23-perf_parser_fast_path_for_no_syntax_errors_when_checking_modifiers branch 2 times, most recently from 548a6ba to 1e19687 Compare March 28, 2026 11:03
@overlookmotel overlookmotel requested a review from Copilot March 28, 2026 15:37
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

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

@camc314 camc314 added the 0-merge Merge with Graphite Merge Queue label Mar 29, 2026
Copy link
Copy Markdown
Contributor

camc314 commented Mar 29, 2026

Merge activity

…20748)

`check_modifier` contained a large chunk of code with nested matches - lots of branches, all to handle cases which should never happen (syntax errors).

Instead, do a fast check for validity using only a couple of operations and a small lookup table. Only if that fast check fails, hand over to a 2nd function `illegal_modifier_error` which does the slow work of figuring out what error to raise and raising it. In normal operation, that path should never be taken, so mark it `#[cold]` and `#[inline(never)]`.

This also replaces an unpredictable branch with a very predictable one.

Small perf improvement on some parser benchmarks (+0.2%).
@graphite-app graphite-app bot force-pushed the om/03-23-perf_parser_fast_path_for_no_syntax_errors_when_checking_modifiers branch from 1e19687 to 1f57448 Compare March 29, 2026 15:30
@graphite-app graphite-app bot merged commit 1f57448 into main Mar 29, 2026
27 checks passed
@graphite-app graphite-app bot deleted the om/03-23-perf_parser_fast_path_for_no_syntax_errors_when_checking_modifiers branch March 29, 2026 15:33
@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Mar 29, 2026
graphite-app bot pushed a commit that referenced this pull request Mar 29, 2026
`illegal_modifier_error` contained a large amount of repetitious code, and was hard to read. Convert it to a shorter version which makes the logic clearer, reusing the lookup table introduced in #20748.

This change also reduces the size of generated code considerably and makes it faster, but #20748 moved this code onto a cold path, so that doesn't really matter. The main benefit is greater readability.

One test case produces a slightly different error after this change.

```ts
class D extends B {
  constructor(override readonly public foo: string) {}
}
```

```diff
- TS(1029): 'public' modifier must precede 'override' modifier.
+ TS(1029): 'public' modifier must precede 'readonly' modifier.
```

Both errors are correct, and which gets flagged was arbitrary before - it wasn't dependent on the order of modifiers in the source code, only the order of `if` branches in `illegal_modifier_error`. So IMO this change isn't important - it's just a *different* arbitrary order now.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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