alloc: stabilise Allocator

[nia-e]

View all comments

Stabilise a bare-minimum (dyn-incompatible, but could be in the future) Allocator trait, alongside Box::new_in(), Vec::new_in(), the System & Global Allocators, and a blanket impl of Allocator for T: GlobalAlloc. For now, we should take care not to make it possible to instantiate anything other than a Vec or Box with custom allocators; it's Probably Fine, but worth a proper look before we rush in.

The soundness requirements for implementors were tightened to the most restrictive ones we could reasonably want per a conversation with @RalfJung.

This was discussed extensively at the all-hands with an apparent tentative consensus from libs and participating ecosystem stakeholders that the current design can be extended backwards-compatibly to address almost all usecases.

cc @rust-lang/libs @rust-lang/libs-api @rust-lang/opsem

r? libs

---

Edit, following more points being discovered: see the new stabilisation report

[nia-e]

r? @Amanieu

[theemathas]

I believe the safety requirements are not yet correct. See #156544

[nia-e]

Per conversations here, I've written up a new, comprehensive stabilisation report per my understanding of what a stabilised trait would look like. Note that this is not yet fully reflective of the state of this PR, as I'd like to first merge #157153 and then rebase on that. I'll update the top-level PR summary with a link as well.

If there are points not explicitly addressed, please ping me and I'll either expand on the relevant rationale or - if it is a new point - see what should be done about it ^^

[nia-e]

this also means the FCP can be restarted if the team agrees with the stabilisation report next meeting

[maxdexh]

Wouldn't the Allocator: Deallocator, impl Deallocator for T: Allocator approach prevent you from doing impl Deallocator for Arc<T: Deallocator>, since the blanket impl would conflict?

[nia-e]

We can use specialisation internally in std, so this shouldn't be an issue I suspect

[maxdexh]

What about downstream users? Also I'm pretty sure that Arc::<A>::deallocate deferring to A::deallocate would make the specialization observable outside of std in a way that allows arbitrary specialization.

[maxdexh]

As an aside, I've been trying around with my writing my own Allocator trait and I have yet to find an actual usecase for Deallocator. The way that the APIs create stuff inside an allocator makes it unclear to me how you would get a container using a Deallocator + !Allocator.

[nia-e]

Tbh I think the likelier case is that we just don't add blanket impls for Deallocator. The main usecase is for ZST/no-op deallocation where you don't need it behind a reference or smart pointer, so making the non-ZST-deallocator case a little more annoying is ultimately Whatever imo

[maxdexh]

Are there examples of deallocators in other languages or real use cases?

[nia-e]

bumpalo uses this for its boxes to be smaller ^^

[maxdexh]

How? bumpalo's box type only drops the contents without deallocating. nvm i see

[RalfJung]

@nia-e there's also #98232 which I mentioned in one of the many threads but I forgot which one.^^ I assume t-libs-api will decide not to change anything, but it'd be good to do that decision knowingly rather than accidentally. :)

[RalfJung]

The second point ("and must not rely...") is more about users of allocators than about implementors, isn't it? It's a bit confusing to handle both of these very different properties in a single sentence.

View changes since the review

[RalfJung]

Suggested change

	/// The pointer passed back in via [`shrink`], [`grow`], etc. must only be used to access
	/// memory inside of that allocation. Furthermore, if the allocator uses pointers that it tracks
	/// itself in order to access this same memory, the user-provided pointer gets invalidated.
	/// The pointer passed back in via [`deallocate`], [`shrink`], [`grow`], etc. must only be used to access
	/// memory inside of that allocation. Furthermore, this pointer should be considered "mutably borrowed"
	/// from the pointer returned by [`allocate`] etc. and the usual aliasing rules for
	/// mutable borrows apply: when their lifetime ends (e.g. because a pointer they
	/// were derived from gets used again), they are invalidated must not be used any more.

Happy to wordsmith this some more.

View changes since the review

[RalfJung]

Actually... the Box part isn't even being stabilized now, right? This is only really relevant for Box. OTOH I guess we need allocators to be forward-compatible with having Box<T, A> later...

[RalfJung]

Actually never mind, it's also relevant for when the user derived an &mut from the allocated pointer and then passes that (turned back into a raw pointer) to deallocate.

[nia-e]

also, cc @thomcc on this

[cuviper]

@maxdexh

I'm currently thinking about the danger of moving an allocator into one of its own memory blocks, and whether the std containers' usage is vulnerable against this.

An allocator should never be stored in one of its blocks, because it would cause one of the following things:

• The allocator is dropped - potential UB if this causes the allocator to clean up its memory
• The allocation is deallocated - UB, since the reference to the allocator will be dangling at the end of deallocate
• The allocator is forgotten / the program diverges - OK

@nia-e

Ultimately I think this is not an issue in std, since as you point out we always keep the allocator separate from the thing it's allocating.

I was thinking on zulip that we should do this, particularly moving Arc's allocator into ArcInner, and then Clone for Arc doesn't need any of the messy safety requirements on A: Clone. That is, we don't have to care how clone behaves if we always keep a 1:1 relationship.

Yes, this does mean we would have to be careful about how we deallocate and drop, but I think it's doable. ArcInner can hold ManuallyDrop<A>, and we can move it out before deallocating.

[maxdexh]

Hmm, if this is sound then I really like that idea. There would presumably also be many benefits to not moving around the allocator field together with the pointer all the time.

[orlp]

this also means the FCP can be restarted if the team agrees with the stabilisation report next meeting

Considering the last-minute nature in which really big lurking soundness issues were found and the scope of the proposed changes I don't think the next step would be to immediately jump into another final comment period. As much as I would like to see the allocator trait get stabilized I think this means we need a bit more time for people to play with the new design and see what they run into.

[maxdexh]

I think there may be benefits to moving the allocator inside an allocation for all containers. For box the destructor is a bit annoying though because the pointee is dropped by the compiler instead of the Drop impl.

We would need some mechanism to clone allocators regardless to justify Arc<A> and &A being allocators though.

[nia-e]

this also means the FCP can be restarted if the team agrees with the stabilisation report next meeting

Considering the last-minute nature in which really big lurking soundness issues were found and the scope of the proposed changes I don't think the next step would be to immediately jump into another final comment period. As much as I would like to see the allocator trait get stabilized I think this means we need a bit more time for people to play with the new design and see what they run into.

If there's outstanding concerns about soundness, I'm happy to postpone landing this a couple weeks. It would still be nice to know if the team is in agreement with the design though

[kpreid]

This PR is now a mix of stabilization and other changes, and I hope those other changes (even the documentation ones) can get landed for testing on nightly before stabilization, without being delayed along with the (clearly needed) delay of stabilization.

[Darksonn]

Yes, please extract the various API changes. Stabilization PRs should not change the API being stabilized.