refactor(ast)!: simplify RegExpPattern#10834
Merged
graphite-app[bot] merged 1 commit intomainfrom May 7, 2025
Merged
Conversation
Member
Author
How to use the Graphite Merge QueueAdd either label to this PR to merge it via 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. |
github-actions
bot
added
A-linter
CodSpeed Instrumentation Performance ReportMerging #10834 will not alter performanceComparing Summary
|
overlookmotel
force-pushed
the
05-05-refactor_ast_simplify_regexppattern_
branch
3 times, most recently
from
edd6453 to
11f56a6
Compare
camc314
approved these changes
May 7, 2025
Member
Author
|
This PR produces a -1% perf regression on 4 out of 5 transformer benchmarks. I've tried to fix that, but with no success. The code in transformer barely changes in this PR, so it's a mystery where the regression is coming from - I suspect something random like it changing the compiler's decisions about inlining, which some other cosmetic change could also affect positively. Core team discussed at meet today and agreed to accept the -1%, as the benefits of this change in terms of simplicity outweigh the small perf cost. |
Member
Merge activity
|
`RegExpPattern` was an enum with `Raw`, `Invalid`, and `Pattern` variants. i.e. only the raw string *or* the `Pattern` could be stored, but not both. This required complications in various places, because if the regexp has been parsed, in order to examine or print it, you have to either convert the `Pattern` back to a `String`, or rely on the `Span` and slice the source text. The `Invalid` variant is not very useful. It's only created if `parse_regular_expression` option is enabled, in which case an error is reported from the parser anyway. Instead, always store the pattern string as an `Atom`, and store the parsed pattern as `Option<Box<Pattern>>`. This simplifies code in various places, and makes printing `RegExpLiteral`s faster in `oxc_codegen` when `parse_regular_expression` is enabled, because it can just print the pattern string, rather than converting a `Pattern` back to a `String`.
graphite-app
bot
force-pushed
the
05-05-refactor_ast_simplify_regexppattern_
branch
from
11f56a6 to
ad4fbf4
Compare
graphite-app bot
pushed a commit
that referenced
this pull request
May 7, 2025
…0855) `Pattern` and other types in `oxc_regular_expression` are not part of ESTree AST. Don't implement `ESTree`, and don't generate raw transfer deserializers for these types. None of that code is actually used. #10834 moved `Pattern` to an optional field of `RegExpPattern`, which is skipped in ESTree serialization, which enables now removing it from ESTree AST entirely. This reduces the scope of ESTree serialization.
This was referenced May 9, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
RegExpPatternwas an enum withRaw,Invalid, andPatternvariants. i.e. only the raw string or thePatterncould be stored, but not both.This required complications in various places, because if the regexp has been parsed, in order to examine or print it, you have to either convert the
Patternback to aString, or rely on theSpanand slice the source text.The
Invalidvariant is not very useful. It's only created ifparse_regular_expressionoption is enabled, in which case an error is reported from the parser anyway.Instead, always store the pattern string as an
Atom, and store the parsed pattern asOption<Box<Pattern>>.This simplifies code in various places, and makes printing
RegExpLiterals faster inoxc_codegenwhenparse_regular_expressionis enabled, because it can just print the pattern string, rather than converting aPatternback to aString.