Skip to content

refactor(ast/estree): simplify serializing RegExpLiterals#9551

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

refactor(ast/estree): simplify serializing RegExpLiterals#9551
graphite-app[bot] merged 1 commit intomainfrom
03-04-refactor_ast_estree_simplify_serializing_regexpliteral_s

Conversation

@overlookmotel
Copy link
Member

@overlookmotel overlookmotel commented Mar 4, 2025

Similar to #9546. Acorn outputs RegExpLiteral's flags in same order as in source text. There is nothing wrong with this, but it makes our serialization/deserialization code more complex than it needs to be, just to match Acorn. I don't think we need to follow Acorn here - there's no particular "good" order for the flags.

Bump acorn-test262 to include commit oxc-project/estree-conformance@3d7013e, which reorders the flags in alphabetical order in Acorn's AST.

This allows simplifying serialization/deserialization code on our side.

Copy link
Member Author

overlookmotel commented Mar 4, 2025

@codspeed-hq
Copy link

codspeed-hq bot commented Mar 4, 2025

CodSpeed Performance Report

Merging #9551 will create unknown performance changes

Comparing 03-04-refactor_ast_estree_simplify_serializing_regexpliteral_s (c1a8cea) with main (734b6b6)

Summary

🆕 39 new benchmarks

Benchmarks breakdown

Benchmark BASE HEAD Change
🆕 parser_napi[RadixUIAdoptionSection.jsx] N/A 4.7 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 700.5 ms N/A
🆕 parser_napi_raw[checker.ts] N/A 1.1 s N/A
🆕 parser_napi_raw[pdf.mjs] N/A 266 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 296.8 µ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.

Copy link
Member

@camchenry camchenry left a comment

Choose a reason for hiding this comment

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

Was the order potentially in frequency? I'm guessing "g" and "I" are used most frequently. But I don't think it really matters, I don't see why we shouldn't keep it consistent here, plus it makes it faster

@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

Similar to #9546. Acorn outputs `RegExpLiteral`'s flags in same order as in source text. There is nothing wrong with this, but it makes our serialization/deserialization code more complex than it needs to be, just to match Acorn. I don't think we need to follow Acorn here - there's no particular "good" order for the flags.

Bump `acorn-test262` to include commit oxc-project/estree-conformance@3d7013e, which reorders the flags in alphabetical order in Acorn's AST.

This allows simplifying serialization/deserialization code on our side.
@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
@graphite-app graphite-app bot requested a review from Dunqing as a code owner March 5, 2025 02:23
@graphite-app graphite-app bot force-pushed the 03-04-refactor_ast_estree_simplify_serializing_regexpliteral_s branch from 9c22f36 to c1a8cea Compare March 5, 2025 02:24
Base automatically changed from 03-04-perf_ast_codegen_transformer_avoid_allocations_when_converting_regexpflags_to_string to main March 5, 2025 02:44
@graphite-app graphite-app bot merged commit c1a8cea into main Mar 5, 2025
34 checks passed
@graphite-app graphite-app bot deleted the 03-04-refactor_ast_estree_simplify_serializing_regexpliteral_s branch March 5, 2025 02:48
@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-ast-tools Area - AST tools C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants