search

fitzgen / bumpalo

A fast bump allocation arena for Rust
https://docs.rs/bumpalo
Apache License 2.0
1.36k stars 111 forks source link

Question: Is it okay for Bump::grow to only receive a new_size, but not the entire new_layout? #143

Closed yanchith closed 2 years ago

yanchith commented 2 years ago

Was doing some relaxing reading of bumpalo's source (commit https://github.com/fitzgen/bumpalo/commit/b22416cfae42ad3c13814c5c97364725196f07ab) when I found the following:

Allocator::grow receives new_layout, but the Allocator trait impl in bumpalo only passes new_size to Bump::grow.

What happens, if Allocator::grow gets called with a new layout that has a greater alignment requirement? Docs of Allocator::grow say:

Returns Err if the new layout does not meet the allocator’s size and alignment constraints of the allocator, or if growing otherwise fails.

So I guess it would be ok to return AllocError if the alignment requirements got stricter with the new layout, but the trait impl currently ignores that.

I also could be missing something 😂

fitzgen commented 2 years ago

@yanchith thanks for filing an issue! Yes, I think that Bump::grow should take a new layout so that it can handle the new alignment requirements (if any) as well.

Would you be up for making these changes and sending a pull request?

yanchith commented 2 years ago

If you mean just the lazy version of returning AllocError, if the new alignment isn't automatically satisfied, then sure! I think this might also apply to Bump::shrink, so I'll look into that as well. I'll be afk for the rest of the week, but I think I could send the PR afterwards.

Btw are you privy to what lead to the decision of letting the caller request a different alignment when growing/shrinking in the allocator api?

fitzgen commented 2 years ago

If you mean just the lazy version of returning AllocError, if the new alignment isn't automatically satisfied, then sure!

That's a good place to start, but it shouldn't be hard to add the more general support (just call alloc again).

Btw are you privy to what lead to the decision of letting the caller request a different alignment when growing/shrinking in the allocator api?

I am not. Might be worth bringing up with the wg-allocators folks, if you feel like going down the rabbit hole.

yanchith commented 2 years ago

FYI I asked around and got pointed here regarding the design history: https://github.com/rust-lang/wg-allocators/issues/5

yanchith commented 2 years ago

I think we can now close this - feel free reopen if you want this for documentation.