Closed
Conversation
Contributor
Your org has enabled the Graphite merge queue for merging into mainAdd the label “0-merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix. You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link. |
github-actions
bot
added
A-linter
Member
Author
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
CodSpeed Performance ReportMerging #6488 will improve performances by 7.23%Comparing Summary
Benchmarks breakdown
|
Member
|
Need to measure memory (RSS) change thorough |
Member
|
First phase should not introduce breaking change. Only change the underlying |
Dunqing
force-pushed
the
10-13-test_reduce_size_of_vec
branch
from
36573f6 to
74b40d2
Compare
Dunqing
force-pushed
the
10-13-test_reduce_size_of_vec
branch
from
afd9a55 to
4f219ad
Compare
Member
Author
I have changed the underlying |
Dunqing
removed
A-linter
This was referenced Oct 16, 2024
Boshen
pushed a commit
that referenced
this pull request
Oct 19, 2024
`oxc_allocator::Vec` is intended for storing AST types in the arena. `Vec` is intended to be non-drop, because all AST types are non-drop. If they were not, it would be a memory leak, because those types will not have `drop` called on them when the allocator is dropped. However, `oxc_allocator::Vec` is currently a wrapper around `allocator_api2::vec::Vec`, which *is* drop. That unintentionally makes `oxc_allocator::Vec` drop too. This PR fixes this by wrapping the inner `allocator_api2::vec::Vec` in `ManuallyDrop`. This makes `oxc_allocator::Vec` non-drop. The wider consequence of this change is that the compiler is now able to see that loads of other types which contain `oxc_allocator::Vec` are also non-drop. Once it can prove that, it can remove a load of code which handles dropping these types in the event of a panic. This probably also then allows it to make many further optimizations on that simplified code. Strictly speaking, this PR is a breaking change. If `oxc_allocator::Vec` is abused to store drop types, then in some circumstances this change could produce a memory leak where there was none before. However, we've always been clear that only non-drop types should be stored in the arena, so such usage was always a bug. #6622 fixes the only place in Oxc where we mistakenly stored non-drop types in `Vec`. The change to `oxc_prettier` is because compiler can now deduce that `Doc` is non-drop, which causes clippy to raise a warning about using `then` instead of `then_some`. As follow-up, we should: 1. Wrap other `allocator_api2` types (e.g. `IntoIter`) in `ManuallyDrop` too, so compiler can prove they are non-drop too (or reimplement `Vec` ourselves - #6488). 2. Introduce static checks to prevent non-drop types being used in `Box` and `Vec`, to make memory leaks impossible, and detect them at compile time.
Member
|
Close as stale. This experimentation is successful, just need the time to land this properly. |
graphite-app bot
pushed a commit
that referenced
this pull request
May 15, 2025
…u32` (#10884) Related: * #6488 * #9706 Finished first step(32 -> 24) of #9706. Change `len` and `cap` fields from `usize` to `u32`, so that the `Vec` size can reduced size to `24`. Also, to avoid some unnecessary unit conversion. I changed many `RawVec` methods to take `u32` rather than `usize`. The performance looks great now! And we reduced 1%-3% memory usage in the parser.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
related: #9706
TEST ONLY!
Everything is hard-coded, just for testing