Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename Alloc to AllocRef #8

Closed
TimDiekmann opened this issue May 3, 2019 · 17 comments
Closed

Rename Alloc to AllocRef #8

TimDiekmann opened this issue May 3, 2019 · 17 comments

Comments

@TimDiekmann
Copy link
Member

TimDiekmann commented May 3, 2019

@SimonSapin suggested to rename Alloc to AllocHandle in rust-lang/rust#58457 (comment):

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 #8 (comment)

@TimDiekmann TimDiekmann added the Discussion Issues containing a specific idea, which needs to be discussed in the WG label May 3, 2019
@TimDiekmann TimDiekmann changed the title [Discussion] Rename Alloc to AllocHandle Rename Alloc to AllocHandle May 3, 2019
@TimDiekmann TimDiekmann added the A-Alloc Trait Issues regarding the Alloc trait label May 3, 2019
@Amanieu
Copy link
Member

Amanieu commented May 4, 2019

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
Copy link
Contributor

SimonSapin commented May 4, 2019

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
Copy link

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
Copy link

gnzlbg commented May 5, 2019

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
Copy link

glandium commented May 5, 2019

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

@TimDiekmann
Copy link
Member Author

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
Copy link
Contributor

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
Copy link

gnzlbg commented May 6, 2019

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.

SimonSapin added a commit to SimonSapin/rust that referenced this issue May 6, 2019
This is a breaking change to an unstable API.
Fixes rust-lang/wg-allocators#8

This change has some consensus.
Landing it now will hopefully help clarify the vocabulary,
including for discussion within the Allocators WG itself.
@ithinuel
Copy link

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

@TimDiekmann
Copy link
Member Author

TimDiekmann commented Sep 30, 2019

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 TimDiekmann changed the title Rename Alloc to AllocHandle Rename Alloc to AllocRef Oct 17, 2019
@TimDiekmann TimDiekmann added Proposal and removed Discussion Issues containing a specific idea, which needs to be discussed in the WG labels Oct 17, 2019
@TimDiekmann
Copy link
Member Author

TimDiekmann commented Jan 22, 2020

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 #8 (comment) 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 👍 👎 instead. I will check the boxes for you then.

@gnzlbg
Copy link

gnzlbg commented Jan 22, 2020

cc @rust-lang/libs

@TimDiekmann
Copy link
Member Author

TimDiekmann commented Jan 23, 2020

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

bors added a commit to rust-lang/rust that referenced this issue Jan 26, 2020
Rename `Alloc` to `AllocRef`

The allocator-wg has decided to merge this change upstream in rust-lang/wg-allocators#8 (comment).

This renames `Alloc` to `AllocRef` 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 rust-lang/wg-allocators#8 (comment) and the comments after that one.

Additionally it clarifies the semantics of `Clone` on an allocator. In the case of `AllocRef`, 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.

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

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

bors added a commit to rust-lang/rust that referenced this issue Jan 28, 2020
Rename `Alloc` to `AllocRef`

The allocator-wg has decided to merge this change upstream in rust-lang/wg-allocators#8 (comment).

This renames `Alloc` to `AllocRef` 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 rust-lang/wg-allocators#8 (comment) and the comments after that one.

Additionally it clarifies the semantics of `Clone` on an allocator. In the case of `AllocRef`, 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.

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

Blocked on rust-lang/miri#1160
@TimDiekmann
Copy link
Member Author

Merged in rust-lang/rust#68529

@Wodann
Copy link

Wodann commented Jan 28, 2020

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

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

@TimDiekmann
Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants