Skip to content

Conversation

overlookmotel
Copy link
Member

@overlookmotel overlookmotel commented Mar 11, 2025

Follow-on after #9310.

Holding large types on the stack is generally best to avoid where possible. Get the data into the arena as quickly as possible, and then only need to pass around Boxes (which are only 8 bytes).

In the case of Class, previously we were unbox-ing a Class (pull it out of the arena, and onto the stack) and then allocating it back into the arena again. Class is a large type - 160 bytes - and this extra work doesn't add any value. We can just leave the Class where it is in the arena, and pass around a Box<Class>.

This is something of a micro-optimization, but they all add up...

Copy link
Member Author

overlookmotel commented Mar 11, 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.

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

codspeed-hq bot commented Mar 11, 2025

CodSpeed Performance Report

Merging #9676 will create unknown performance changes

Comparing 03-11-perf_transformer_using_avoid_large_types_on_stack (15dd0d4) with main (076d872)

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 699.7 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.1 ms N/A
🆕 isolated-declarations[vue-id.ts] N/A 58.7 ms N/A
🆕 lexer[RadixUIAdoptionSection.jsx] N/A 20.7 µ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.1 ms N/A
🆕 mangler[react.development.js] N/A 294.3 µs N/A
🆕 mangler[typescript.js] N/A 39.8 ms N/A
... ... ... ... ...

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

Copy link
Contributor

@camc314 camc314 left a comment

Choose a reason for hiding this comment

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

thanks for the detail in the description!

@camc314 camc314 force-pushed the 03-11-refactor_transformer_using_remove_.into_ branch from c1c03c8 to a4fed5a Compare March 11, 2025 09:44
@camc314 camc314 force-pushed the 03-11-perf_transformer_using_avoid_large_types_on_stack branch from 8d255b1 to 2043688 Compare March 11, 2025 09:45
@graphite-app graphite-app bot added the 0-merge Merge with Graphite Merge Queue label Mar 11, 2025
Copy link
Contributor

graphite-app bot commented Mar 11, 2025

Merge activity

@graphite-app graphite-app bot force-pushed the 03-11-refactor_transformer_using_remove_.into_ branch from a4fed5a to 786b7dc Compare March 11, 2025 10:15
graphite-app bot pushed a commit that referenced this pull request Mar 11, 2025
Follow-on after #9310.

Holding large types on the stack is generally best to avoid where possible. Get the data into the arena as quickly as possible, and then only need to pass around `Box`es (which are only 8 bytes).

In the case of `Class`, previously we were `unbox`-ing a `Class` (pull it out of the arena, and onto the stack) and then allocating it back into the arena again. `Class` is a large type - 160 bytes - and this extra work doesn't add any value. We can just leave the `Class` where it is in the arena, and pass around a `Box<Class>`.

This is something of a micro-optimization, but they all add up...
@graphite-app graphite-app bot force-pushed the 03-11-perf_transformer_using_avoid_large_types_on_stack branch from 2043688 to d06e5f2 Compare March 11, 2025 10:15
@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Mar 11, 2025
@graphite-app graphite-app bot force-pushed the 03-11-refactor_transformer_using_remove_.into_ branch from 786b7dc to 21859ca Compare March 11, 2025 10:42
graphite-app bot pushed a commit that referenced this pull request Mar 11, 2025
Follow-on after #9310.

Holding large types on the stack is generally best to avoid where possible. Get the data into the arena as quickly as possible, and then only need to pass around `Box`es (which are only 8 bytes).

In the case of `Class`, previously we were `unbox`-ing a `Class` (pull it out of the arena, and onto the stack) and then allocating it back into the arena again. `Class` is a large type - 160 bytes - and this extra work doesn't add any value. We can just leave the `Class` where it is in the arena, and pass around a `Box<Class>`.

This is something of a micro-optimization, but they all add up...
@graphite-app graphite-app bot force-pushed the 03-11-perf_transformer_using_avoid_large_types_on_stack branch from d06e5f2 to 36aa13d Compare March 11, 2025 10:43
@graphite-app graphite-app bot added the 0-merge Merge with Graphite Merge Queue label Mar 11, 2025
Follow-on after #9310.

Holding large types on the stack is generally best to avoid where possible. Get the data into the arena as quickly as possible, and then only need to pass around `Box`es (which are only 8 bytes).

In the case of `Class`, previously we were `unbox`-ing a `Class` (pull it out of the arena, and onto the stack) and then allocating it back into the arena again. `Class` is a large type - 160 bytes - and this extra work doesn't add any value. We can just leave the `Class` where it is in the arena, and pass around a `Box<Class>`.

This is something of a micro-optimization, but they all add up...
@graphite-app graphite-app bot force-pushed the 03-11-refactor_transformer_using_remove_.into_ branch from 21859ca to 076d872 Compare March 11, 2025 12:12
@graphite-app graphite-app bot force-pushed the 03-11-perf_transformer_using_avoid_large_types_on_stack branch from 36aa13d to 15dd0d4 Compare March 11, 2025 12:13
Base automatically changed from 03-11-refactor_transformer_using_remove_.into_ to main March 11, 2025 12:20
@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Mar 11, 2025
@graphite-app graphite-app bot merged commit 15dd0d4 into main Mar 11, 2025
33 checks passed
@graphite-app graphite-app bot deleted the 03-11-perf_transformer_using_avoid_large_types_on_stack branch March 11, 2025 12:30
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.

2 participants