Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf(transformer/arrow-functions): store state of whether arguments needs transform #7321

Conversation

overlookmotel
Copy link
Contributor

@overlookmotel overlookmotel commented Nov 17, 2024

Track whether arguments needs to be transformed. If it doesn't, can skip expensive checks for whether IdentifierReferences and BindingIdentifiers names are arguments or not.

This recovers about half the performance hit of #7234.

Copy link

graphite-app bot commented Nov 17, 2024

Your org has enabled the Graphite merge queue for merging into main

Add the label “0-merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

Copy link

codspeed-hq bot commented Nov 17, 2024

CodSpeed Performance Report

Merging #7321 will create unknown performance changes

Comparing 11-17-perf_transformer_arrow-functions_store_state_of_whether_arguments_needs_transform (26d3e96) with main (ea08c1f)

Summary

🆕 30 new benchmarks

Benchmarks breakdown

Benchmark main 11-17-perf_transformer_arrow-functions_store_state_of_whether_arguments_needs_transform Change
🆕 codegen[checker.ts] N/A 24.6 ms N/A
🆕 codegen_sourcemap[checker.ts] N/A 77.4 ms N/A
🆕 isolated-declarations[vue-id.ts] N/A 397.5 ms N/A
🆕 lexer[RadixUIAdoptionSection.jsx] N/A 24.1 µs N/A
🆕 lexer[antd.js] N/A 22.3 ms N/A
🆕 lexer[cal.com.tsx] N/A 5.5 ms N/A
🆕 lexer[checker.ts] N/A 13.3 ms N/A
🆕 lexer[pdf.mjs] N/A 3.6 ms N/A
🆕 linter[RadixUIAdoptionSection.jsx] N/A 2.4 ms N/A
🆕 linter[cal.com.tsx] N/A 1.1 s N/A
🆕 linter[checker.ts] N/A 2.6 s N/A
🆕 minifier[antd.js] N/A 437.2 ms N/A
🆕 minifier[react.development.js] N/A 2.9 ms N/A
🆕 minifier[typescript.js] N/A 545.4 ms N/A
🆕 parser[RadixUIAdoptionSection.jsx] N/A 77.7 µs N/A
🆕 parser[antd.js] N/A 106.5 ms N/A
🆕 parser[cal.com.tsx] N/A 24.6 ms N/A
🆕 parser[checker.ts] N/A 53.5 ms N/A
🆕 parser[pdf.mjs] N/A 17.4 ms N/A
🆕 semantic[RadixUIAdoptionSection.jsx] N/A 98.6 µs N/A
... ... ... ... ...

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

@overlookmotel overlookmotel force-pushed the 11-17-perf_transformer_arrow-functions_store_state_of_whether_arguments_needs_transform branch from b2a68ee to 5f4e91e Compare November 17, 2024 00:53
@overlookmotel overlookmotel marked this pull request as ready for review November 17, 2024 01:24
@Dunqing Dunqing added the 0-merge Merge with Graphite Merge Queue label Nov 17, 2024
Copy link
Member

Dunqing commented Nov 17, 2024

Merge activity

@Dunqing Dunqing force-pushed the 11-16-refactor_transformer_arrow-function_comments_on_possible_improvement branch from 6b7a223 to fd2e3d8 Compare November 17, 2024 05:12
Dunqing pushed a commit that referenced this pull request Nov 17, 2024
…eeds transform (#7321)

Track whether `arguments` needs to be transformed. If it doesn't, can skip expensive checks for whether `IdentifierReference`s and `BindingIdentifier`s names are `arguments` or not.

This recovers about half the performance hit of #7234.
@Dunqing Dunqing force-pushed the 11-17-perf_transformer_arrow-functions_store_state_of_whether_arguments_needs_transform branch from f54762c to 02f345e Compare November 17, 2024 05:13
Dunqing pushed a commit that referenced this pull request Nov 17, 2024
… aid inlining (#7322)

Move the cheap "does arguments need to be transformed" checks introduced in #7321 into `enter_identifier_reference` and `enter_binding_identifier`, and mark those methods `#[inline]`. These hot paths can then usually execute without a function call.

This wins back the other half of the perf hit of #7234.
@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Nov 17, 2024
@overlookmotel overlookmotel force-pushed the 11-16-refactor_transformer_arrow-function_comments_on_possible_improvement branch from fd2e3d8 to 1995686 Compare November 17, 2024 10:01
@overlookmotel overlookmotel force-pushed the 11-17-perf_transformer_arrow-functions_store_state_of_whether_arguments_needs_transform branch from 02f345e to 1ac362d Compare November 17, 2024 10:02
overlookmotel added a commit that referenced this pull request Nov 17, 2024
… aid inlining (#7322)

Move the cheap "does arguments need to be transformed" checks introduced in #7321 into `enter_identifier_reference` and `enter_binding_identifier`, and mark those methods `#[inline]`. These hot paths can then usually execute without a function call.

This wins back the other half of the perf hit of #7234.
@graphite-app graphite-app bot added the 0-merge Merge with Graphite Merge Queue label Nov 17, 2024
@overlookmotel overlookmotel force-pushed the 11-16-refactor_transformer_arrow-function_comments_on_possible_improvement branch from 1995686 to 871e19b Compare November 17, 2024 10:11
@overlookmotel overlookmotel force-pushed the 11-17-perf_transformer_arrow-functions_store_state_of_whether_arguments_needs_transform branch from 1ac362d to 905d8b8 Compare November 17, 2024 10:11
overlookmotel added a commit that referenced this pull request Nov 17, 2024
… aid inlining (#7322)

Move the cheap "does arguments need to be transformed" checks introduced in #7321 into `enter_identifier_reference` and `enter_binding_identifier`, and mark those methods `#[inline]`. These hot paths can then usually execute without a function call.

This wins back the other half of the perf hit of #7234.
Base automatically changed from 11-16-refactor_transformer_arrow-function_comments_on_possible_improvement to main November 17, 2024 10:48
…eeds transform (#7321)

Track whether `arguments` needs to be transformed. If it doesn't, can skip expensive checks for whether `IdentifierReference`s and `BindingIdentifier`s names are `arguments` or not.

This recovers about half the performance hit of #7234.
@overlookmotel overlookmotel force-pushed the 11-17-perf_transformer_arrow-functions_store_state_of_whether_arguments_needs_transform branch from 905d8b8 to 26d3e96 Compare November 17, 2024 10:49
overlookmotel added a commit that referenced this pull request Nov 17, 2024
… aid inlining (#7322)

Move the cheap "does arguments need to be transformed" checks introduced in #7321 into `enter_identifier_reference` and `enter_binding_identifier`, and mark those methods `#[inline]`. These hot paths can then usually execute without a function call.

This wins back the other half of the perf hit of #7234.
@graphite-app graphite-app bot merged commit 26d3e96 into main Nov 17, 2024
26 checks passed
@graphite-app graphite-app bot deleted the 11-17-perf_transformer_arrow-functions_store_state_of_whether_arguments_needs_transform branch November 17, 2024 10:56
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-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.

2 participants