Skip to content

perf(transformer/object_rest_spread): use ArenaVec to store values that will be used in constructing AST#10434

Merged
graphite-app[bot] merged 1 commit intomainfrom
04-16-perf_transformer_object_rest_spread_use_arenavec_to_store_values_that_will_be_used_in_constructing_ast
Apr 16, 2025
Merged

perf(transformer/object_rest_spread): use ArenaVec to store values that will be used in constructing AST#10434
graphite-app[bot] merged 1 commit intomainfrom
04-16-perf_transformer_object_rest_spread_use_arenavec_to_store_values_that_will_be_used_in_constructing_ast

Conversation

@Dunqing
Copy link
Member

@Dunqing Dunqing commented Apr 16, 2025

props is always used in expression_object without intermediate changes, so it is okay and good to use ArenaVec store, which also can avoid an unnecessary allocation (std vec -> arena vec converison).

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

Dunqing commented Apr 16, 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.

@Dunqing Dunqing force-pushed the 04-16-perf_transformer_object_rest_spread_use_arenavec_to_store_values_that_will_be_used_in_constructing_ast branch from 7b04309 to d09d8be Compare April 16, 2025 07:23
@codspeed-hq
Copy link

codspeed-hq bot commented Apr 16, 2025

CodSpeed Instrumentation Performance Report

Merging #10434 will not alter performance

Comparing 04-16-perf_transformer_object_rest_spread_use_arenavec_to_store_values_that_will_be_used_in_constructing_ast (595c5df) with main (ebe3496)

Summary

✅ 36 untouched benchmarks

@Dunqing Dunqing force-pushed the 04-16-perf_transformer_object_rest_spread_use_arenavec_to_store_values_that_will_be_used_in_constructing_ast branch from d09d8be to ccdd6e9 Compare April 16, 2025 07:35
@Dunqing Dunqing force-pushed the 04-15-perf_transformer_class-properties_return_early_if_no_private_fields_are_found branch from 2351230 to f431eec Compare April 16, 2025 08:25
@Dunqing Dunqing force-pushed the 04-16-perf_transformer_object_rest_spread_use_arenavec_to_store_values_that_will_be_used_in_constructing_ast branch 2 times, most recently from 07fcd0b to d9f2655 Compare April 16, 2025 08:34
@Dunqing Dunqing marked this pull request as ready for review April 16, 2025 08:43
@Dunqing Dunqing requested a review from overlookmotel as a code owner April 16, 2025 08:43
@graphite-app graphite-app bot changed the base branch from 04-15-perf_transformer_class-properties_return_early_if_no_private_fields_are_found to graphite-base/10434 April 16, 2025 10:53
@overlookmotel overlookmotel added the 0-merge Merge with Graphite Merge Queue label Apr 16, 2025
Copy link
Member

overlookmotel commented Apr 16, 2025

Merge activity

@Dunqing Dunqing force-pushed the 04-16-perf_transformer_object_rest_spread_use_arenavec_to_store_values_that_will_be_used_in_constructing_ast branch from d9f2655 to b645ac3 Compare April 16, 2025 11:51
@Dunqing Dunqing force-pushed the graphite-base/10434 branch from f431eec to 1457e68 Compare April 16, 2025 11:51
@Dunqing Dunqing changed the base branch from graphite-base/10434 to 04-15-perf_transformer_class-properties_return_early_if_no_private_fields_are_found April 16, 2025 11:51
@overlookmotel
Copy link
Member

overlookmotel commented Apr 16, 2025

There's something cool we could do here with our new Vec implementation.

We could add an API Vec::split_at which converts a Vec into 2 x Vecs, without copying any data or making any allocations.

let original_len = vec.len();
let original_ptr = vec.as_ptr();

let (vec1, vec2) = vec.split_at(2).unwrap();

assert!(vec1.len() == 2);
assert!(vec2.len() == original_len - 2);
// `vec1` and `vec2` point to chunks of the original `vec`
assert!(vec1.as_ptr() == original_ptr);
assert!(vec2.as_ptr() == original_ptr.add(2));

std's Vec cannot offer this API because when a Vec is dropped, it must deallocate with the same layout as it was originally allocated with. But because our Vec is not Drop, we don't have that restriction.

Using split_at, this transform could avoid copying any of the data from original Vec into new Vecs. It'd just create a bunch of Vecs which reference chunks of the original's data.

Base automatically changed from 04-15-perf_transformer_class-properties_return_early_if_no_private_fields_are_found to main April 16, 2025 13:26
@Dunqing Dunqing force-pushed the 04-16-perf_transformer_object_rest_spread_use_arenavec_to_store_values_that_will_be_used_in_constructing_ast branch from b645ac3 to 1f77cac Compare April 16, 2025 13:27
…that will be used in constructing AST (#10434)

`props` is always used in `expression_object` without intermediate changes, so it is okay and good to use `ArenaVec` store, which also can avoid an unnecessary allocation (std vec -> arena vec converison).
@graphite-app graphite-app bot force-pushed the 04-16-perf_transformer_object_rest_spread_use_arenavec_to_store_values_that_will_be_used_in_constructing_ast branch from 1f77cac to 595c5df Compare April 16, 2025 13:28
@graphite-app graphite-app bot merged commit 595c5df into main Apr 16, 2025
29 checks passed
@graphite-app graphite-app bot deleted the 04-16-perf_transformer_object_rest_spread_use_arenavec_to_store_values_that_will_be_used_in_constructing_ast branch April 16, 2025 13:42
@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Apr 16, 2025
@Dunqing
Copy link
Member Author

Dunqing commented Apr 21, 2025

There's something cool we could do here with our new Vec implementation.

We could add an API Vec::split_at which converts a Vec into 2 x Vecs, without copying any data or making any allocations.

let original_len = vec.len();
let original_ptr = vec.as_ptr();

let (vec1, vec2) = vec.split_at(2).unwrap();

assert!(vec1.len() == 2);
assert!(vec2.len() == original_len - 2);
// `vec1` and `vec2` point to chunks of the original `vec`
assert!(vec1.as_ptr() == original_ptr);
assert!(vec2.as_ptr() == original_ptr.add(2));

std's Vec cannot offer this API because when a Vec is dropped, it must deallocate with the same layout as it was originally allocated with. But because our Vec is not Drop, we don't have that restriction.

Using split_at, this transform could avoid copying any of the data from original Vec into new Vecs. It'd just create a bunch of Vecs which reference chunks of the original's data.

That's cool! Maybe a dumb question, what scenario do you want to use this API for? In terms of the PR changes, we don't need to use vec1, so it looks like we should have a method like cut_off?

let original_vec = vec![1, 2, 3, 4, 5];
let cut_vec = vec.cut_off(2).unwrap();

assert!(original_vec.len() == 2);
assert!(cut_vec.len() == 3);
assert!(cut_vec.as_ptr() == original_vec.as_ptr().add(2));

@overlookmotel
Copy link
Member

Yes, you're right. All we need is to optimize Vec::split_off (which is the existing method which is the same as your cut_off). I've updated oxc-project/backlog#161.

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