Skip to content

Comments

perf(transformer): arrow function transform: reduce stack memory usage#5940

Merged
graphite-app[bot] merged 1 commit intomainfrom
09-20-perf_transformer_arrow_function_transform_reduce_stack_memory_usage
Sep 23, 2024
Merged

perf(transformer): arrow function transform: reduce stack memory usage#5940
graphite-app[bot] merged 1 commit intomainfrom
09-20-perf_transformer_arrow_function_transform_reduce_stack_memory_usage

Conversation

@overlookmotel
Copy link
Member

@overlookmotel overlookmotel commented Sep 20, 2024

Arrow function transform maintains a stack for blocks which may (or may not) need a var _this = this; statement added to them.

This stack was Vec<Option<BoundIdentifier>> (24 bytes per block). Most blocks won't need a statement added, so most entries are None.

Introduce an abstraction SparseStack. This stores the stack split into 2 arrays. First array is Vec<bool> indicating if a statement needs to be added or not. Only if a statement does need to be added, then its details are pushed to a separate array Vec<BoundIdentifier>.

This means the memory taken up by the stack will be roughly 1 byte per block, instead of 24 bytes per block (assuming very few blocks need statements added).

@codspeed-hq
Copy link

codspeed-hq bot commented Sep 20, 2024

CodSpeed Performance Report

Merging #5940 will not alter performance

Comparing 09-20-perf_transformer_arrow_function_transform_reduce_stack_memory_usage (618e89e) with main (97a2c41)

Summary

✅ 29 untouched benchmarks

@overlookmotel overlookmotel force-pushed the 09-20-perf_transformer_arrow_function_transform_reduce_stack_memory_usage branch 2 times, most recently from 82874de to 319b9fe Compare September 20, 2024 22:23
@overlookmotel overlookmotel force-pushed the 09-20-perf_transformer_arrow_function_transform_reduce_stack_memory_usage branch 2 times, most recently from 39ab5a2 to 9807363 Compare September 21, 2024 00:04
@overlookmotel overlookmotel force-pushed the 09-20-perf_transformer_arrow_function_transform_reduce_stack_memory_usage branch from 9807363 to bc11788 Compare September 21, 2024 00:51
@overlookmotel overlookmotel force-pushed the 09-18-refactor_transformer_use_scopeflags.is_arrow_instead_of_inside_arrow_function_stack branch from c3e8c58 to 6979aa2 Compare September 21, 2024 01:00
@overlookmotel overlookmotel force-pushed the 09-20-perf_transformer_arrow_function_transform_reduce_stack_memory_usage branch from bc11788 to 1c3b103 Compare September 21, 2024 01:00
@Boshen Boshen force-pushed the 09-18-refactor_transformer_use_scopeflags.is_arrow_instead_of_inside_arrow_function_stack branch 2 times, most recently from 7827028 to 632b462 Compare September 21, 2024 07:53
@Boshen Boshen force-pushed the 09-20-perf_transformer_arrow_function_transform_reduce_stack_memory_usage branch from 1c3b103 to 6671dbb Compare September 21, 2024 07:53
@Boshen Boshen changed the base branch from 09-18-refactor_transformer_use_scopeflags.is_arrow_instead_of_inside_arrow_function_stack to graphite-base/5940 September 21, 2024 09:01
@Boshen Boshen force-pushed the 09-20-perf_transformer_arrow_function_transform_reduce_stack_memory_usage branch from 6671dbb to 750ef88 Compare September 21, 2024 09:05
@Boshen Boshen changed the base branch from graphite-base/5940 to main September 21, 2024 09:06
@Boshen Boshen force-pushed the 09-20-perf_transformer_arrow_function_transform_reduce_stack_memory_usage branch from 750ef88 to a98259b Compare September 21, 2024 09:06
@overlookmotel overlookmotel force-pushed the 09-20-perf_transformer_arrow_function_transform_reduce_stack_memory_usage branch from a98259b to 8cc4b35 Compare September 21, 2024 09:38
@overlookmotel overlookmotel changed the base branch from main to 09-21-fix_transformer_arrow_function_transform_prevent_stack_getting_out_of_sync September 21, 2024 09:38
@Dunqing Dunqing force-pushed the 09-21-fix_transformer_arrow_function_transform_prevent_stack_getting_out_of_sync branch 2 times, most recently from e72ab8c to 882a51d Compare September 21, 2024 09:46
@Dunqing Dunqing force-pushed the 09-20-perf_transformer_arrow_function_transform_reduce_stack_memory_usage branch from 8cc4b35 to 0dedf32 Compare September 21, 2024 09:46
@overlookmotel overlookmotel force-pushed the 09-20-perf_transformer_arrow_function_transform_reduce_stack_memory_usage branch from 0dedf32 to 5008b88 Compare September 21, 2024 09:50
@overlookmotel overlookmotel changed the base branch from 09-21-fix_transformer_arrow_function_transform_prevent_stack_getting_out_of_sync to 09-21-docs_transformer_arrow_function_transform_comment_about_incomplete_implementation September 21, 2024 09:50
@Dunqing Dunqing changed the base branch from 09-21-docs_transformer_arrow_function_transform_comment_about_incomplete_implementation to graphite-base/5940 September 21, 2024 14:09
@Dunqing Dunqing force-pushed the 09-20-perf_transformer_arrow_function_transform_reduce_stack_memory_usage branch from 5008b88 to ca3e3f6 Compare September 21, 2024 14:16
@Dunqing Dunqing changed the base branch from graphite-base/5940 to main September 21, 2024 14:17
@Dunqing Dunqing force-pushed the 09-20-perf_transformer_arrow_function_transform_reduce_stack_memory_usage branch from ca3e3f6 to 8a6b2e5 Compare September 21, 2024 14:17
@overlookmotel overlookmotel force-pushed the 09-20-perf_transformer_arrow_function_transform_reduce_stack_memory_usage branch from 8a6b2e5 to 7e76547 Compare September 21, 2024 22:11
@overlookmotel overlookmotel marked this pull request as ready for review September 22, 2024 00:49
@Boshen Boshen requested a review from Dunqing September 22, 2024 04:27
@Dunqing
Copy link
Member

Dunqing commented Sep 23, 2024

Is SparseStack an alternative for #5049?

@Dunqing Dunqing added the 0-merge Merge with Graphite Merge Queue label Sep 23, 2024 — with Graphite App
@graphite-app
Copy link
Contributor

graphite-app bot commented Sep 23, 2024

Merge activity

#5940)

Arrow function transform maintains a stack for blocks which may (or may not) need a `var _this = this;` statement added to them.

This stack was `Vec<Option<BoundIdentifier>>` (24 bytes per block). Most blocks won't need a statement added, so most entries are `None`.

Introduce an abstraction `SparseStack`. This stores the stack split into 2 arrays. First array is `Vec<bool>` indicating if a statement needs to be added or not. Only if a statement *does* need to be added, then its details are pushed to a separate array `Vec<BoundIdentifier>`.

This means the memory taken up by the stack will be roughly 1 byte per block, instead of 24 bytes per block (assuming very few blocks need statements added).
@Dunqing Dunqing force-pushed the 09-20-perf_transformer_arrow_function_transform_reduce_stack_memory_usage branch from 7e76547 to 618e89e Compare September 23, 2024 07:52
Dunqing pushed a commit that referenced this pull request Sep 23, 2024
…tack` (#5942)

Use `SparseStack` (introduced in #5940) to store the stack of blocks which may need a `var _temp;` statement added to them. This reduces the memory required for the stack, on assumption that most blocks won't need a `var` statement.
Dunqing pushed a commit that referenced this pull request Sep 23, 2024
#5959)

Use `SparseStack` (introduced in #5940) to store the stack of blocks which may need a `var _temp;` statement added to them. This reduces the memory required for the stack, on assumption that most blocks won't need a `var` statement.
Dunqing pushed a commit that referenced this pull request Sep 23, 2024
…tack` (#5960)

Use `SparseStack` (introduced in #5940) to store the stack of blocks which may need a `var _temp;` statement added to them. This reduces the memory required for the stack, on assumption that most blocks won't need a `var` statement.
Dunqing pushed a commit that referenced this pull request Sep 23, 2024
Optimize `SparseStack` (which was introduced in #5940). Initialize it with a single entry, and ensure the stack is never emptied. This makes `take`, `get_or_init` and `get_mut_or_init` methods infallible, since there is always an entry on the stack to read.
@graphite-app graphite-app bot merged commit 618e89e into main Sep 23, 2024
@graphite-app graphite-app bot deleted the 09-20-perf_transformer_arrow_function_transform_reduce_stack_memory_usage branch September 23, 2024 07:57
@overlookmotel
Copy link
Member Author

Is SparseStack an alternative for #5049?

Yes, I was planning to create a "common" transform which holds a SparseStack, and all these transforms can share it.

Boshen added a commit that referenced this pull request Sep 24, 2024
## [0.30.1] - 2024-09-24

### Features

- 5c323a2 minifier: Loop compressor passes (#6013) (Boshen)

### Bug Fixes

- 9ca202a codegen: Preserve newlines between comments (#6014) (Boshen)
- 4a99372 codegen: Print jsdoc comments for `TSEnumMember`s (#6007)
(camc314)
- 97a2c41 isolated-declarations: False positive for class private getter
with non-inferrable return type (#5987) (michaelm)

### Performance

- 2b17003 linter, prettier, diagnostics: Use `FxHashMap` instead of
`std::collections::HashMap` (#5993) (camchenry)
- 7b90d79 transformer: `SparseStack` always keep minimum 1 entry (#5962)
(overlookmotel)
- 28fe80a transformer: Logical assignment operator transform use
`SparseStack` (#5960) (overlookmotel)
- 9f7d4b7 transformer: Exponentiation operator transform use
`SparseStack` (#5959) (overlookmotel)
- 5dc0154 transformer: Nullish coalescing operator transform use
`SparseStack` (#5942) (overlookmotel)
- 618e89e transformer: Arrow function transform: reduce stack memory
usage (#5940) (overlookmotel)

### Documentation

- 5a0d17c ast: Document more AST nodes (#6000) (DonIsaac)
- 18371dd oxc: Include feature-guarded modules in docs.rs (#6012)
(DonIsaac)
- 1abfe8f semantic: Document `SymbolTable` (#5998) (DonIsaac)
- f5eee72 semantic: Correct docs for `Reference` (#5992) (overlookmotel)
- 860f108 transformer: Add to arrow functions transform docs (#5989)
(overlookmotel)

### Refactor

- 0a2f687 minifier: Move dce conditional expression to `RemoveDeadCode`
(#5971) (Boshen)
- f02bf51 transformer: Arrow function transform: remove unnecessary
assertion (#6002) (overlookmotel)

---------

Co-authored-by: Boshen <[email protected]>
Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants