Skip to content

Conversation

@Dunqing
Copy link
Member

@Dunqing Dunqing commented Apr 16, 2025

Use ctx.ast.vec_from_iter to allocate a Vec for the result of drain and use take_in to take a Vec rather than use vec_from_iter + drain to avoid intermediate allocation.

image

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.

@github-actions github-actions bot added A-minifier Area - Minifier C-performance Category - Solution not expected to change functional behavior, only performance labels Apr 16, 2025
@codspeed-hq
Copy link

codspeed-hq bot commented Apr 16, 2025

CodSpeed Instrumentation Performance Report

Merging #10435 will not alter performance

Comparing 04-16-perf_transformer_minimize_statements_reduce_allocations_of_vec (59e35c3) with main (ebe3496)

Summary

✅ 36 untouched benchmarks

@Dunqing Dunqing force-pushed the 04-16-perf_transformer_minimize_statements_reduce_allocations_of_vec branch from 609c523 to da697ef 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 from ccdd6e9 to 07fcd0b Compare April 16, 2025 08:25
@Dunqing Dunqing changed the base branch from 04-16-perf_transformer_object_rest_spread_use_arenavec_to_store_values_that_will_be_used_in_constructing_ast to graphite-base/10435 April 16, 2025 08:34
@Dunqing Dunqing force-pushed the graphite-base/10435 branch from 07fcd0b to d9f2655 Compare April 16, 2025 08:37
@Dunqing Dunqing force-pushed the 04-16-perf_transformer_minimize_statements_reduce_allocations_of_vec branch from da697ef to c7e72f5 Compare April 16, 2025 08:37
@Dunqing Dunqing changed the base branch from graphite-base/10435 to 04-16-perf_transformer_object_rest_spread_use_arenavec_to_store_values_that_will_be_used_in_constructing_ast April 16, 2025 08:43
@Dunqing Dunqing changed the title perf(transformer/minimize_statements): reduce allocations of Vec perf(minifier/minimize_statements): reduce allocations of Vec Apr 16, 2025
@Dunqing Dunqing marked this pull request as ready for review April 16, 2025 08:49
@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 04-16-perf_transformer_minimize_statements_reduce_allocations_of_vec branch from c7e72f5 to 161afb6 Compare April 16, 2025 11:51
@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

  • Apr 16, 8:41 AM EDT: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
  • Apr 16, 9:28 AM EDT: overlookmotel added this pull request to the Graphite merge queue.

@overlookmotel
Copy link
Member

overlookmotel commented Apr 16, 2025

Actually I think this might be even better:

Before:

let drained_stmts = stmts.drain(i + 1..);
let mut body = if let Some(alternate) = if_stmt.alternate.take() {
    ctx.ast.vec_from_iter(iter::once(alternate).chain(drained_stmts))
} else {
    ctx.ast.vec_from_iter(drained_stmts)
};

After:

let mut body = if let Some(alternate) = if_stmt.alternate.take() {
    let drained_stmts = stmts.drain(i + 1..);
    ctx.ast.vec_from_iter(iter::once(alternate).chain(drained_stmts))
} else {
    stmts.split_off(i + 1)
};

split_off avoids using vec_from_iter. It knows the exact required capacity of the new Vec.


Similar to my comment on preceding PR #10434 (comment), later on we can make our split_off implementation more efficient, not copying any data or performing any allocations.

Then this simpler version would be equally efficient (or possibly more, as if stmts has spare capacity, it avoids an allocation):

let mut body = stmts.split_off(i + 1);
if let Some(alternate) = if_stmt.alternate.take() {
    body.insert(0, alternate);
}

@Dunqing Dunqing force-pushed the 04-16-perf_transformer_minimize_statements_reduce_allocations_of_vec branch from 161afb6 to 59e35c3 Compare April 16, 2025 13:27
@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
Use `ctx.ast.vec_from_iter` to allocate a `Vec` for the result of `drain` and use `take_in` to take a `Vec` rather than use `vec_from_iter` + `drain` to avoid intermediate allocation.

<img width="742" alt="image" src="https://github.com/user-attachments/assets/10011073-6706-435a-8d79-0b805e14f20c" />
@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 force-pushed the 04-16-perf_transformer_minimize_statements_reduce_allocations_of_vec branch from 59e35c3 to 2eb60b3 Compare April 16, 2025 13:29
Base automatically changed from 04-16-perf_transformer_object_rest_spread_use_arenavec_to_store_values_that_will_be_used_in_constructing_ast to main April 16, 2025 13:42
@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Apr 16, 2025
@overlookmotel overlookmotel merged commit 8db9dc5 into main Apr 16, 2025
3 checks passed
@overlookmotel overlookmotel deleted the 04-16-perf_transformer_minimize_statements_reduce_allocations_of_vec branch April 16, 2025 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-minifier Area - Minifier 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