Skip to content

perf(mangler): use shorter InlineString#9552

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

perf(mangler): use shorter InlineString#9552
graphite-app[bot] merged 1 commit intomainfrom
03-04-perf_mangler_use_shorter_inlinestring_

Conversation

@overlookmotel
Copy link
Member

@overlookmotel overlookmotel commented Mar 4, 2025

SymbolId is a u32, so there are a maximum of u32::MAX symbols. Therefore the longest mangled symbol name is 6 characters (xKrTKr for u32::MAX).

Use an 8-byte InlineString for symbol names (instead of 16 bytes).

Also, enable the tests for oxc_mangler crate. The tests were previously failing, but we didn't notice because they were disabled.

Copy link
Member Author

overlookmotel commented Mar 4, 2025

@overlookmotel overlookmotel marked this pull request as ready for review March 4, 2025 21:24
@codspeed-hq
Copy link

codspeed-hq bot commented Mar 4, 2025

CodSpeed Performance Report

Merging #9552 will create unknown performance changes

Comparing 03-04-perf_mangler_use_shorter_inlinestring_ (bc14ee5) with main (3e4f909)

Summary

🆕 39 new benchmarks

Benchmarks breakdown

Benchmark BASE HEAD Change
🆕 parser_napi[RadixUIAdoptionSection.jsx] N/A 4.9 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 699.9 ms N/A
🆕 parser_napi_raw[checker.ts] N/A 1.1 s N/A
🆕 parser_napi_raw[pdf.mjs] N/A 266.4 ms N/A
🆕 codegen[checker.ts] N/A 23.1 ms N/A
🆕 codegen_sourcemap[checker.ts] N/A 66.1 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 294.9 µ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.

@overlookmotel
Copy link
Member Author

Damn. Only +0.2% perf improvement.

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.

Maybe a small improvement but still worth it I think.

@Boshen Boshen added the 0-merge Merge with Graphite Merge Queue label Mar 5, 2025
Copy link
Member

Boshen commented Mar 5, 2025

Merge activity

  • Mar 4, 9:04 PM EST: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
  • Mar 4, 9:04 PM EST: A user added this pull request to the Graphite merge queue.
  • Mar 4, 9:53 PM EST: A user merged this pull request with the Graphite merge queue.

`SymbolId` is a `u32`, so there are a maximum of `u32::MAX` symbols. Therefore the longest mangled symbol name is 6 characters (`xKrTKr` for `u32::MAX`).

Use an 8-byte `InlineString` for symbol names (instead of 16 bytes).

Also, enable the tests for `oxc_mangler` crate. The tests were previously failing, but we didn't notice because they were disabled.
@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
@graphite-app graphite-app bot requested a review from Dunqing as a code owner March 5, 2025 02:24
@graphite-app graphite-app bot force-pushed the 03-04-perf_mangler_use_shorter_inlinestring_ branch from 1dce342 to bc14ee5 Compare March 5, 2025 02:24
Base automatically changed from 03-04-refactor_ast_estree_simplify_serializing_regexpliteral_s to main March 5, 2025 02:48
@graphite-app graphite-app bot merged commit bc14ee5 into main Mar 5, 2025
34 checks passed
@graphite-app graphite-app bot deleted the 03-04-perf_mangler_use_shorter_inlinestring_ branch March 5, 2025 02:53
@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 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