fitzgen / bumpalo

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

Account for alignment changes when growing or shrinking #144

Closed yanchith closed 2 years ago

yanchith commented 2 years ago

My riff on #143

Both Bump::grow and Bump::shrink now take in the new layout and check for alignment compatibility. Bump::grow falls back to Bump::try_alloc_layout if the alignment is not compatible, and Bump::shrink just returns AllocError in this case.

These above strategies are pretty much up to us. The Allocator trait says it is ok to error out, if it can't handle the new requirements. For bump allocation, I'd personally prefer shrinking to fail instead of allocating, if the alignment requirements changed.

Afaik, the collections in alloc don't change alignment during their life (unless transmuted), and while I can imagine legitimate code that would want to do this, the same code would probably write their own allocator.

Also added two tests, but allocator_grow_layout_change doesn't really test anything just yet. It is a bit tricky to figure out when grow falls back to Bump::try_alloc_layout without instrumenting Bump itself with counters. Ideas?

fitzgen commented 2 years ago

These above strategies are pretty much up to us. The Allocator trait says it is ok to error out, if it can't handle the new requirements. For bump allocation, I'd personally prefer shrinking to fail instead of allocating, if the alignment requirements changed.

Agreed, this sounds like the correct trade off to me.

yanchith commented 2 years ago

Sooo, I added those quickchecks, and actually found what looks like a real issue in Bump::shrink: applying the size delta to the pointer could actually produce pointers with insufficient alignment (because the size could actually be any "unround" integer once it is GTE the alignment). I fixed this by aligning the delta, so doing arithmetic with it doesn't change alignment of the pointer. We reclaim less space this way, but the new pointer stays correctly aligned.

This is all very new to me, so please double-check 😂