Skip to content

perf(transformer/jsx): use memchr for parsing JSX pragma comments#11001

Merged
graphite-app[bot] merged 1 commit intomainfrom
05-13-perf_transformer_jsx_use_memchr_for_parsing_jsx_pragma_comments
May 14, 2025
Merged

perf(transformer/jsx): use memchr for parsing JSX pragma comments#11001
graphite-app[bot] merged 1 commit intomainfrom
05-13-perf_transformer_jsx_use_memchr_for_parsing_jsx_pragma_comments

Conversation

@overlookmotel
Copy link
Member

@overlookmotel overlookmotel commented May 13, 2025

Use memchr for finding @ when parsing JSX pragmas from comments.

This wins back most (but not all) of the perf loss of #10983 on antd.js benchmark, and preserves the perf gain of #10983 on cal.com.tsx benchmark.

Interestingly, using memchr to search just for @ and then checking next 3 bytes are jsx separately is measurably faster than using memchr::memmem::Finder to search for @jsx.

@github-actions github-actions bot added A-transformer Area - Transformer / Transpiler C-performance Category - Solution not expected to change functional behavior, only performance labels May 13, 2025
Copy link
Member Author

overlookmotel commented May 13, 2025


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 hot fixes, skip the queue and merge this PR next

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

codspeed-hq bot commented May 13, 2025

CodSpeed Instrumentation Performance Report

Merging #11001 will create unknown performance changes

Comparing 05-13-perf_transformer_jsx_use_memchr_for_parsing_jsx_pragma_comments (1aed99b) with main (6540f44)

Summary

🆕 36 new benchmarks

Benchmarks breakdown

Benchmark BASE HEAD Change
🆕 codegen[checker.ts] N/A 22.8 ms N/A
🆕 codegen_sourcemap[checker.ts] N/A 67 ms N/A
🆕 formatter[antd.js] N/A 714.8 ms N/A
🆕 formatter[react.development.js] N/A 8.1 ms N/A
🆕 formatter[typescript.js] N/A 1.1 s N/A
🆕 isolated-declarations[vue-id.ts] N/A 58.5 ms N/A
🆕 lexer[RadixUIAdoptionSection.jsx] N/A 21.7 µs N/A
🆕 lexer[antd.js] N/A 25 ms N/A
🆕 lexer[cal.com.tsx] N/A 6.1 ms N/A
🆕 lexer[checker.ts] N/A 14.9 ms N/A
🆕 lexer[pdf.mjs] N/A 4 ms N/A
🆕 linter[RadixUIAdoptionSection.jsx] N/A 2.8 ms N/A
🆕 linter[cal.com.tsx] N/A 1.2 s N/A
🆕 linter[checker.ts] N/A 3.1 s N/A
🆕 mangler[antd.js] N/A 16 ms N/A
🆕 mangler[react.development.js] N/A 295.3 µs N/A
🆕 mangler[typescript.js] N/A 39.6 ms N/A
🆕 minifier[antd.js] N/A 164.4 ms N/A
🆕 minifier[react.development.js] N/A 1.8 ms N/A
🆕 minifier[typescript.js] N/A 288.8 ms N/A
... ... ... ... ...

ℹ️ Only the first 20 benchmarks are displayed. Go to the app to view all benchmarks.

@overlookmotel overlookmotel marked this pull request as ready for review May 13, 2025 12:19
@overlookmotel overlookmotel requested a review from Dunqing as a code owner May 13, 2025 12:19
@Boshen Boshen added the 0-merge Merge with Graphite Merge Queue label May 14, 2025
Copy link
Member

Boshen commented May 14, 2025

Merge activity

…11001)

Use `memchr` for finding `@` when parsing JSX pragmas from comments.

This wins back most (but not all) of the perf loss of #10983 on `antd.js` benchmark, and preserves the perf gain of #10983 on `cal.com.tsx` benchmark.

Interestingly, using `memchr` to search just for `@` and then checking next 3 bytes are `jsx` separately is measurably faster than using `memchr::memmem::Finder` to search for `@jsx`.
@graphite-app graphite-app bot force-pushed the 05-13-test_transformer_jsx_fix_tests_for_jsx_pragma_parsing branch from 03772ba to 6540f44 Compare May 14, 2025 04:49
@graphite-app graphite-app bot force-pushed the 05-13-perf_transformer_jsx_use_memchr_for_parsing_jsx_pragma_comments branch from 861a6da to 1aed99b Compare May 14, 2025 04:50
Base automatically changed from 05-13-test_transformer_jsx_fix_tests_for_jsx_pragma_parsing to main May 14, 2025 04:56
@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label May 14, 2025
@graphite-app graphite-app bot merged commit 1aed99b into main May 14, 2025
28 checks passed
@graphite-app graphite-app bot deleted the 05-13-perf_transformer_jsx_use_memchr_for_parsing_jsx_pragma_comments branch May 14, 2025 04:57
// Search for `@`.
// Note: Using `memchr::memmem::Finder` to search for `@jsx` is slower than only using `memchr`
// to find `@` characters, and then checking if `@` is followed by `jsx` separately.
let at_sign_index = memchr(b'@', comment_str.as_bytes())?;
Copy link
Contributor

Choose a reason for hiding this comment

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

it might also be faster to use memchr_iter rather than a loop and having the searcher re-compiled again. (it also might make no difference depending on how the compiler is optimizing it)

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think memchr (unlike memchr::memmem::Finder) compiles a searcher type - it's a straight function call. If it's inlined, the "needle" (@) should be statically known to compiler, which might allow it to generate more optimal code than a more complex type.

I've taken some shortcuts here - there are some bounds checks which could be eliminated, and other stuff that could be a little tighter. But my assumption was that almost all the cost is in searching for @, so everything after that was not worthwhile optimizing.

But still, yes, memchr_iter might be faster, especially if the searcher was created outside of find_jsx_pragma and passed in so it can be re-used. Want to try it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-transformer Area - Transformer / Transpiler 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