refactor(allocator): introduce Alloc trait#11198
Merged
graphite-app[bot] merged 1 commit intomainfrom May 21, 2025
Merged
Conversation
Member
Author
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
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. |
CodSpeed Instrumentation Performance ReportMerging #11198 will not alter performanceComparing Summary
|
overlookmotel
changed the title
Alloc traitAlloc trait
github-actions
bot
added
the
C-cleanup
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
overlookmotel
force-pushed
the
05-20-feat_allocator_introduce_alloc_trait
branch
2 times, most recently
from
ae7a48f to
69ca688
Compare
This was referenced May 20, 2025
Dunqing
approved these changes
May 21, 2025
Member
|
Wow, that's a good idea to improve |
Contributor
Merge activity
|
First step towards replacing `bumpalo` with our own allocator (#11145). Introduce `Alloc` trait and implement it for `bumpalo::Bump`. A later PR will parameterize `Vec` with `Alloc`, instead of a static dependency on `Bump`. We can later on replace `Bump` with our own `Arena` type. `Arena` will also implement `Alloc`, so the two will be able to be swapped with a minimum of fuss. ### Why do we need a trait at all? Why couldn't `Vec` just statically depend on `Arena`, same as it now does on `Bump`? Rationale for a trait here is: My intent is to have 2 different variants of `Arena` to support filling allocator chunks from both ends (string data fills from the start, all other types fill from the end). This will have 3 advantages: 1. More tightly packed data. `&str` / `Atom` is the only type in the AST which is not aligned on 8. Strings currently often tend to end up with unnecessary padding bytes around them. 2. Packing all string data into a single immutable stretch of memory has potential to speed up all string operations (oxc-project/backlog#46 (comment)). 3. It should unblock the biggest current perf bottleneck in raw transfer, by allowing all strings in the AST to be converted to UTF-16 in a single shot (as if they were just 1 long string), rather than converting each string individually. `String` is a wrapper around `Vec`, and it's preferable not to have to implement `Vec` twice for front-filling (strings) and back-filling (all other types). `Vec<T, A: Alloc>` is a simpler solution.
graphite-app
bot
force-pushed
the
05-20-feat_allocator_introduce_alloc_trait
branch
from
69ca688 to
a2ab84b
Compare
graphite-app bot
pushed a commit
that referenced
this pull request
May 21, 2025
Parameterize `InnerVec` (otherwise known as `Vec2`) and `RawVec` with `Alloc` trait introduced in #11198. `InnerVec` and `RawVec` no longer have a dependency on `bumpalo::Bump`. The rationale for that is discussed in #11198. The only substantive change this PR makes is that all `Alloc` methods panic/abort if allocation fails, rather than returning a `Result::Err`. This alters the behavior of `try_*` methods, which previously wouldn't panic. However, I don't believe we use any of those methods, and we have no plans to, so in my view we should remove them. In practice it should be very difficult to exhaust all memory, and if we did there's no way to gracefully recover from that (what would we do? only parse half the file?). So this has little/no practical impact. That apart, this PR is pure refactor. The diff is large, but it's almost entirely adding the `A: Alloc` bound to all methods / iterators, and removing `Bump`.
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.
First step towards replacing
bumpalowith our own allocator (#11145).Introduce
Alloctrait and implement it forbumpalo::Bump. A later PR will parameterizeVecwithAlloc, instead of a static dependency onBump.We can later on replace
Bumpwith our ownArenatype.Arenawill also implementAlloc, so the two will be able to be swapped with a minimum of fuss.Why do we need a trait at all?
Why couldn't
Vecjust statically depend onArena, same as it now does onBump?Rationale for a trait here is: My intent is to have 2 different variants of
Arenato support filling allocator chunks from both ends (string data fills from the start, all other types fill from the end). This will have 3 advantages:&str/Atomis the only type in the AST which is not aligned on 8. Strings currently often tend to end up with unnecessary padding bytes around them.Atom<'a>andCompactStringnice and fast backlog#46 (comment)).Stringis a wrapper aroundVec, and it's preferable not to have to implementVectwice for front-filling (strings) and back-filling (all other types).Vec<T, A: Alloc>is a simpler solution.