rust-lang / wg-allocators

Home of the Allocators working group: Paving a path for a standard set of allocator traits to be used in collections!
http://bit.ly/hello-wg-allocators
207 stars 9 forks source link

Rename `Alloc` to `AllocRef` #8

Closed TimDiekmann closed 4 years ago

TimDiekmann commented 5 years ago

@SimonSapin suggested to rename Alloc to AllocHandle in https://github.com/rust-lang/rust/pull/58457#issuecomment-472479293:

we might want to add Handle to the trait names [...] in order to show that they are typically implemented for a reference or smart pointer or ZST, not directly for the type that actually holds the allocator’s state.

This would also apply to #9 and rename Dealloc to DeallocHandle.

Edit: Rename AllocHandle to AllocRef as stated in https://github.com/rust-lang/wg-allocators/issues/8#issuecomment-524205326

Amanieu commented 5 years ago

IMO the main benefit here is that it clarifies the semantics of Clone on an allocator. In the case of AllocHandle, it is clear that the cloned handle still points to the same allocator instance, and that you can free data allocated from one handle with another handle.

SimonSapin commented 5 years ago

I hope this renaming would help clear some confusion that I’ve seen happen multiple times:

“If A is the type for 'the allocator' and Box<T, A> has direct ownership of a value of type A, does that means that an allocator instance can only be used in one box? Shouldn’t Box own &'a A instead or something?”

Or: “Why does Alloc::alloc take &mut self? Shouldn’t that be &self so that the allocator can be shared?”

It turns out that Box having ownership of a bare A and Alloc::alloc taking &mut self are what provides maximum flexibility, but it’s not obvious at all why. When creating your own MyAllocator type, there are typically two scenarios: the type is a zero-size unit struct that anyone can “construct” out of thin air (see std::alloc::Global), or you’d implement the Alloc trait not for your type directly but with some indirection like &'a MyAllocator or Arc<MyAllocator> (or both). Then, Alloc::alloc’s &mut self is e.g. &mut &'a MyAllocator. (Cases where &mut self over &self is important are probably less typical, but there is basically no downside in supporting them.)

Having vocabulary that separates “an allocator” and “a handle to that allocator” (and naming APIs accordingly) can hopefully make this indirection more discoverable.

scottjmaddox commented 5 years ago

I strongly support this change. Just in discussing other issues over the last day or two there have been multiple times when I felt that calling it AllocHandle would be much more clear.

gnzlbg commented 5 years ago

I think many will expect to be able to just write Vec<T, AllocStorage<[T; 16]>> and get something like ArrayVec<[T; 16]> (or SmallVec, etc.), but that won't obviously work. Renaming Alloc to AllocHandle would clarify that this is not intended to work, so I think this would be a good thing.

glandium commented 5 years ago

It should, actually, be possible to get something like ArrayVec.

TimDiekmann commented 5 years ago

I think many will expect to be able to just write Vec<T, AllocStorage<[T; 16]>> and get something like ArrayVec<[T; 16]> (or SmallVec, etc.), but that won't obviously work. Renaming Alloc to AllocHandle would clarify that this is not intended to work, so I think this would be a good thing.

Could you elaborate on this?

SimonSapin commented 5 years ago

ArrayVec seems incompatible with an API where allocating returns a pointer that is then passed again when deallocating. The ArrayVec value might have moved, and the address of its inline storage changed.

You might consider something like the Pin API to prevent moves, but that’s much less useful than an ArrayVec that can move.

gnzlbg commented 5 years ago

Basically what @SimonSapin says, but I want to expand on why it is not worth it to support this use case for an MVP.

As @SimonSapin mentions, if the allocation storage lives within the AllocHandle, then when Vec<T, A> is moved, the storage is moved. Vec has a pointer to the allocation, and this pointer would be invalidated on move.

We'd have to make those allocators !Move, or use Pin like @SimonSapin suggest.

If we wanted to support moving these, we'd need to support correcting this pointer on move. I don't see a way of achieving that without adding move constructors to the language, which would be a huge language change.

So, AFAICT, we just can't support this right now, and the changes required to the language to support these are out-of-scope of this WG.

ithinuel commented 5 years ago

bikeshed: Shouldn't that be called AllocRef rather than Handle ? The only appearance of Handle in std are for windows specific resources.

TimDiekmann commented 4 years ago

Has anyone found any drawback in renaming Alloc?

Personally, I like AllocRef more than AllocHandle. First, because of what @ithinuel was saying:

The only appearance of Handle in std are for windows specific resources.

Second, Ref expresses the usage of Alloc better than Handle.

TimDiekmann commented 4 years ago

We rename Alloc to AllocRef and push this upstream because types that implement Alloc are a reference, smart pointer, or ZSTs. It is not possible to have an allocator like MyAlloc([u8; N]), that owns the memory and also implements Alloc, since that would mean, that moving a Vec<T, MyAlloc> would need to correct the internal pointer, which is not possible as we don't have move constructors.

For further explanation please see https://github.com/rust-lang/wg-allocators/issues/8#issuecomment-489464843 and the comments after that one.

The initial proposal was to rename Alloc to AllocHandle, but Ref expresses the semantics better than Handle. Additionally, the only appearance of Handle in std are for windows specific resources, which might be confusing.

The next step is review by the rest of the tagged wg members:

No concerns currently listed. If you have any concerns regarding this change, please post those here.

If you don't have the permissions to modify comments in this repository, please vote :+1: :-1: instead. I will check the boxes for you then.

gnzlbg commented 4 years ago

cc @rust-lang/libs

TimDiekmann commented 4 years ago

I'm so excited about our first FCP, that I already implemented this change, waiting for pressing the ~"push"~ "Create pull request" button :laughing:

scottjmaddox commented 4 years ago

@TimDiekmann FYI, no need to include scott-maddox going forward. Please just tag scottjmaddox. Thanks!

TimDiekmann commented 4 years ago

Merged in https://github.com/rust-lang/rust/pull/68529

Wodann commented 4 years ago

Nice work on the merged PR, @TimDiekmann! 👏🏻

Now that we have some momentum, what is the next step?

TimDiekmann commented 4 years ago

Thanks!

I'd favor splitting dealloc into DeallocRef and keep realloc in AllocRef. Further, I'd like to introduce the BuildAllocRef trait, but I'm unsure how I should design this in the first iteration.