Skip to content

perf(transformer): optimize inserting var/let statements#10654

Merged
graphite-app[bot] merged 1 commit intomainfrom
04-28-perf_transformer_optimize_inserting_var_let_statements
Apr 29, 2025
Merged

perf(transformer): optimize inserting var/let statements#10654
graphite-app[bot] merged 1 commit intomainfrom
04-28-perf_transformer_optimize_inserting_var_let_statements

Conversation

@Dunqing
Copy link
Member

@Dunqing Dunqing commented Apr 28, 2025

Found while working on #9881, this PR aims to optimize inserting statements by reducing an unnecessary Vec allocation.

Previously, get_var_statement would collect var statement and let statement into a std::vec::Vec and return it, then immediately extend/insert to another ArenaVec. Considering here only have two elements, we don't need to store them in a Vec, just return them, and use them depending on how they are used.

Seems to have very minor improvement, and I guess transformer/checker.ts has 0.1 regression is noisy.

image

@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 28, 2025
Copy link
Member Author

Dunqing commented Apr 28, 2025

@codspeed-hq
Copy link

codspeed-hq bot commented Apr 28, 2025

CodSpeed Instrumentation Performance Report

Merging #10654 will not alter performance

Comparing 04-28-perf_transformer_optimize_inserting_var_let_statements (91df9d4) with main (4825eb5)

Summary

✅ 36 untouched benchmarks

Copy link
Member

@overlookmotel overlookmotel left a comment

Choose a reason for hiding this comment

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

It's definitely an improvement not to allocate a temp Vec.

What is less good is that we always copy the whole of stmts into a new ArenaVec. If stmts has spare capacity, it'd be preferable to shift up the existing Statements to make room for the new ones at the start, and then move the new statements into the newly-created space. That doesn't reduce the amount of memory copying that needs to happen, but at least it does it without consuming more memory in arena for a brand new Vec.

However, I was surprised to see that Vec::splice was doing the same unoptimal thing - allocate a new Vec and copy everything over. Vec::insert is also surprisingly bad! If the Vec is already full to capacity, it copies all the existing elements of the Vec twice, which is completely unnecessary.

To get this (and the many similar operations in our code) optimal, we'd need new APIs like:

impl<'a, T> Vec<'a, T> {
    /// Same as `vec.insert(0, value)`
    fn push_start(&mut self, value: T);
    /// Same as `vec.splice(0..0, iter)`
    fn extend_start<I: IntoIterator<Item = T>>(&mut self, iter: I);
}

We could optimize these to use the "shift up" method.

@overlookmotel overlookmotel added the 0-merge Merge with Graphite Merge Queue label Apr 29, 2025
Copy link
Member

overlookmotel commented Apr 29, 2025

Merge activity

Found while working on #9881, this PR aims to optimize inserting statements by reducing an unnecessary `Vec` allocation.

Previously, `get_var_statement` would collect `var` statement and `let` statement into a `std::vec::Vec` and return it, then immediately `extend`/`insert` to another `ArenaVec`. Considering here only have two elements, we don't need to store them in a `Vec`, just return them, and use them depending on how they are used.

Seems to have very minor improvement, and I guess `transformer/checker.ts` has 0.1 regression is noisy.

<img width="767" alt="image" src="https://github.com/user-attachments/assets/c0aeeb5a-e2da-4e2a-9990-56b239f6ab65" />
@graphite-app graphite-app bot force-pushed the 04-28-perf_transformer_optimize_inserting_var_let_statements branch from b44d2a9 to 91df9d4 Compare April 29, 2025 16:01
@graphite-app graphite-app bot merged commit 91df9d4 into main Apr 29, 2025
25 checks passed
@graphite-app graphite-app bot deleted the 04-28-perf_transformer_optimize_inserting_var_let_statements branch April 29, 2025 16:09
@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Apr 29, 2025
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