alloc: stabilise Allocator#156882
Conversation
rustbot
added
S-waiting-on-review
nia-e
added
A-allocators
nia-e
added
needs-fcp
|
r? @Amanieu |
This comment has been minimized.
This comment has been minimized.
|
I believe the safety requirements are not yet correct. See #156544 |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
@nia-e With regard to First of all, an alternative design is for This has the advantage that only those who wish to use the excess capacity (if any) pay for the cost of recovering it. Or as per C++ motto: You Don't Pay For What You Don't Use. With regard to the cost (or absence of cost) mentioned, I am not convinced the only issue is with In the absence of a separate method to query the total/excess capacity, the only way for an implementation to convey the exact capacity is to return it from the get go. This means in turn that within the implementation the exact capacity must be passed through all layers, at all times. The fact that no bloat is observed if
The exact capacity must be conveyed through all layers, some of which are not inlined so they're out of the way in the hot path. The only allocators where the entire allocation path can be inlined at no penalty are simple allocators like fixed-capacity arena allocators. All allocators with a split hot/other path will aim to mark the non-hot path as noinline to avoid bloating the hot path, specifically so the hot path can be inlined -- even a simple arena allocator with dynamic capacity.
|
This comment has been minimized.
This comment has been minimized.
I think this is a reason in favour of keeping the slice return type. Extremely simple allocators will be able to inline it away; for more complex ones, I'm doubtful that the extra pointer width is relevant. If an implementor wants to always return exactly the capacity it's asked for, it can make If it comes to it, a similar wrapper trick can be done as with |
f my gay ass puppy life
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
We do have cases that need the allocator directly though, e.g. unstable |
| #[unstable(feature = "allocator_api", issue = "32838")] | ||
| #[stable(feature = "allocator_api", since = "CURRENT_RUSTC_VERSION")] | ||
| #[derive(Copy, Clone, PartialEq, Eq, Debug)] | ||
| pub struct AllocError; |
There was a problem hiding this comment.
Asked on Zulip but mentioning here for clarity: should AllocError be #[non_exhaustive] (with some constructor function or public constant) to leave open the possibility of more detailed error reporting in the future? For Allocator::allocate it's pretty clear the only error that could occur is OOM, but for grow/etc. or other future methods there might be useful information that an allocator could return.
I believe #[non_exhaustive] can be removed without a breaking change, so aside from providing a default constructor I don't think there's any downsides to being conservative.
There was a problem hiding this comment.
AllocError being a Zero-Sized Type (ZST) is a feature, not a bug.
Specifically, it being a ZST is the only reason that Result<NonNull<[u8]>, AllocError> is only two pointers wide: there's a single niche value (null), which only a ZST error can fit it.
Richer error types should be provided by other means, for example by making the error type of Allocator customizable (type Error: Into<AllocError>; would work well).
There was a problem hiding this comment.
Specifically, it being a ZST is the only reason that
Result<NonNull<[u8]>, AllocError>is only two pointers wide: there's a single niche value (null), which only a ZST error can fit it.
That's not correct, Result<NonNull<[u8]>, *const ()> is also two pointers wide (though that's not guaranteed of course):
https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=f57891eefbf603b1719ae3c9bd725206
NonNull<[u8]> has a niche where the pointer part is null, and the length part can be anything, so the Err variant has the Ok variant's pointer part be null, and the length part's space is used for the *const () error value.
There was a problem hiding this comment.
Right, you are correct.
I'm still hoping to get a result of Result<NonNull<u8>, AllocError> at which point only a ZST does the job.
|
Given the feedback here & on Zulip, the new design for allocator_api probably needs some time to bake & iron out soundness bugs. I'll be keeping the previous document up-to-date w/ any changes or updates, but it likely makes sense to first land these changes unstably and then see if it holds up to scrutiny. I do think something could be stabilised soon-ish, but there won't be any harm in waiting a bit ^^ edit: the conversation will mainly continue on zulip and on the issues filed in response to this PR; if you have concerns not yet addressed, please ping me there and i'll happily look into seeing if the design can be adapted @rustbot author |
nia-e
removed
the
I-libs-api-nominated
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
|
☔ The latest upstream changes (presumably #157303) made this pull request unmergeable. Please resolve the merge conflicts. |
allow `Allocator`s to be used as `#[global_allocator]`s The (hopefully) immanent stabilisation of the `Allocator` trait raises the question of what is to be done about the older, already-stable `GlobalAlloc` trait. In my opinion, having two nearly-identical traits for the same purpose is needlessly confusing. Going forward, `Allocator` as the more modern interface should be _the_ allocator trait. With `Allocator` being currently unstable, there is the possibility of implementing `GlobalAlloc` for all `Allocator`s, thereby allowing them to be used as `#[global_allocator]` and allowing crates to seamlessly (and semver-compatibly) switch to `Allocator`. However, unconditionally implementing `GlobalAlloc` presents a footgun to users, as e.g. using `Global` as `#[global_allocator]` will lead to infinite recursion. @nia-e initially tried to resolve this in e1b7097 (rust-lang#156882) by using weird trait trickery to implement `GlobalAlloc` for every allocator except `Global`. But this does not go far enough, e.g. a bump allocator that itself allocates from `Global` is similarly unsuitable as global allocator. Thus, with this PR, I'd like to propose adding a new marker trait for allocators that can be used as `#[global_allocator]`: ```rust // in core::alloc trait GlobalAllocator: Allocator {} ``` `GlobalAlloc` can then be implemented for all `GlobalAllocator`s: ```rust impl<A> GlobalAlloc for A where A: GlobalAllocator { /* ... */ } ``` This provides a backwards-compatible way for allocator libraries to switch to the new interface and allows deprecating `GlobalAlloc` (not done here). Over time, I expect that `GlobalAlloc` will become more and more of an implementation detail of the `#[global_allocator]` macro (for instance, one might add perma-unstable, hidden methods for things like `grow_zeroed` that are customised only by the blanket implementation). With regards to naming, I chose `GlobalAllocator` to mirror `Allocator`. `GlobalAlloc` should probably be deprecated quickly after stabilising `GlobalAllocator` to avoid confusion. For the same reason, I think it'd be better to add `GlobalAllocator` before stabilising `Allocator` – but that is not a necessity. r? @nia-e @rustbot label +I-libs-api-nominated
allow `Allocator`s to be used as `#[global_allocator]`s The (hopefully) immanent stabilisation of the `Allocator` trait raises the question of what is to be done about the older, already-stable `GlobalAlloc` trait. In my opinion, having two nearly-identical traits for the same purpose is needlessly confusing. Going forward, `Allocator` as the more modern interface should be _the_ allocator trait. With `Allocator` being currently unstable, there is the possibility of implementing `GlobalAlloc` for all `Allocator`s, thereby allowing them to be used as `#[global_allocator]` and allowing crates to seamlessly (and semver-compatibly) switch to `Allocator`. However, unconditionally implementing `GlobalAlloc` presents a footgun to users, as e.g. using `Global` as `#[global_allocator]` will lead to infinite recursion. @nia-e initially tried to resolve this in e1b7097 (rust-lang#156882) by using weird trait trickery to implement `GlobalAlloc` for every allocator except `Global`. But this does not go far enough, e.g. a bump allocator that itself allocates from `Global` is similarly unsuitable as global allocator. Thus, with this PR, I'd like to propose adding a new marker trait for allocators that can be used as `#[global_allocator]`: ```rust // in core::alloc trait GlobalAllocator: Allocator {} ``` `GlobalAlloc` can then be implemented for all `GlobalAllocator`s: ```rust impl<A> GlobalAlloc for A where A: GlobalAllocator { /* ... */ } ``` This provides a backwards-compatible way for allocator libraries to switch to the new interface and allows deprecating `GlobalAlloc` (not done here). Over time, I expect that `GlobalAlloc` will become more and more of an implementation detail of the `#[global_allocator]` macro (for instance, one might add perma-unstable, hidden methods for things like `grow_zeroed` that are customised only by the blanket implementation). With regards to naming, I chose `GlobalAllocator` to mirror `Allocator`. `GlobalAlloc` should probably be deprecated quickly after stabilising `GlobalAllocator` to avoid confusion. For the same reason, I think it'd be better to add `GlobalAllocator` before stabilising `Allocator` – but that is not a necessity. r? @nia-e @rustbot label +I-libs-api-nominated
allow `Allocator`s to be used as `#[global_allocator]`s The (hopefully) immanent stabilisation of the `Allocator` trait raises the question of what is to be done about the older, already-stable `GlobalAlloc` trait. In my opinion, having two nearly-identical traits for the same purpose is needlessly confusing. Going forward, `Allocator` as the more modern interface should be _the_ allocator trait. With `Allocator` being currently unstable, there is the possibility of implementing `GlobalAlloc` for all `Allocator`s, thereby allowing them to be used as `#[global_allocator]` and allowing crates to seamlessly (and semver-compatibly) switch to `Allocator`. However, unconditionally implementing `GlobalAlloc` presents a footgun to users, as e.g. using `Global` as `#[global_allocator]` will lead to infinite recursion. @nia-e initially tried to resolve this in e1b7097 (rust-lang#156882) by using weird trait trickery to implement `GlobalAlloc` for every allocator except `Global`. But this does not go far enough, e.g. a bump allocator that itself allocates from `Global` is similarly unsuitable as global allocator. Thus, with this PR, I'd like to propose adding a new marker trait for allocators that can be used as `#[global_allocator]`: ```rust // in core::alloc trait GlobalAllocator: Allocator {} ``` `GlobalAlloc` can then be implemented for all `GlobalAllocator`s: ```rust impl<A> GlobalAlloc for A where A: GlobalAllocator { /* ... */ } ``` This provides a backwards-compatible way for allocator libraries to switch to the new interface and allows deprecating `GlobalAlloc` (not done here). Over time, I expect that `GlobalAlloc` will become more and more of an implementation detail of the `#[global_allocator]` macro (for instance, one might add perma-unstable, hidden methods for things like `grow_zeroed` that are customised only by the blanket implementation). With regards to naming, I chose `GlobalAllocator` to mirror `Allocator`. `GlobalAlloc` should probably be deprecated quickly after stabilising `GlobalAllocator` to avoid confusion. For the same reason, I think it'd be better to add `GlobalAllocator` before stabilising `Allocator` – but that is not a necessity. r? @nia-e @rustbot label +I-libs-api-nominated
allow `Allocator`s to be used as `#[global_allocator]`s The (hopefully) immanent stabilisation of the `Allocator` trait raises the question of what is to be done about the older, already-stable `GlobalAlloc` trait. In my opinion, having two nearly-identical traits for the same purpose is needlessly confusing. Going forward, `Allocator` as the more modern interface should be _the_ allocator trait. With `Allocator` being currently unstable, there is the possibility of implementing `GlobalAlloc` for all `Allocator`s, thereby allowing them to be used as `#[global_allocator]` and allowing crates to seamlessly (and semver-compatibly) switch to `Allocator`. However, unconditionally implementing `GlobalAlloc` presents a footgun to users, as e.g. using `Global` as `#[global_allocator]` will lead to infinite recursion. @nia-e initially tried to resolve this in e1b7097 (rust-lang#156882) by using weird trait trickery to implement `GlobalAlloc` for every allocator except `Global`. But this does not go far enough, e.g. a bump allocator that itself allocates from `Global` is similarly unsuitable as global allocator. Thus, with this PR, I'd like to propose adding a new marker trait for allocators that can be used as `#[global_allocator]`: ```rust // in core::alloc trait GlobalAllocator: Allocator {} ``` `GlobalAlloc` can then be implemented for all `GlobalAllocator`s: ```rust impl<A> GlobalAlloc for A where A: GlobalAllocator { /* ... */ } ``` This provides a backwards-compatible way for allocator libraries to switch to the new interface and allows deprecating `GlobalAlloc` (not done here). Over time, I expect that `GlobalAlloc` will become more and more of an implementation detail of the `#[global_allocator]` macro (for instance, one might add perma-unstable, hidden methods for things like `grow_zeroed` that are customised only by the blanket implementation). With regards to naming, I chose `GlobalAllocator` to mirror `Allocator`. `GlobalAlloc` should probably be deprecated quickly after stabilising `GlobalAllocator` to avoid confusion. For the same reason, I think it'd be better to add `GlobalAllocator` before stabilising `Allocator` – but that is not a necessity. r? @nia-e @rustbot label +I-libs-api-nominated
allow `Allocator`s to be used as `#[global_allocator]`s The (hopefully) immanent stabilisation of the `Allocator` trait raises the question of what is to be done about the older, already-stable `GlobalAlloc` trait. In my opinion, having two nearly-identical traits for the same purpose is needlessly confusing. Going forward, `Allocator` as the more modern interface should be _the_ allocator trait. With `Allocator` being currently unstable, there is the possibility of implementing `GlobalAlloc` for all `Allocator`s, thereby allowing them to be used as `#[global_allocator]` and allowing crates to seamlessly (and semver-compatibly) switch to `Allocator`. However, unconditionally implementing `GlobalAlloc` presents a footgun to users, as e.g. using `Global` as `#[global_allocator]` will lead to infinite recursion. @nia-e initially tried to resolve this in e1b7097 (rust-lang#156882) by using weird trait trickery to implement `GlobalAlloc` for every allocator except `Global`. But this does not go far enough, e.g. a bump allocator that itself allocates from `Global` is similarly unsuitable as global allocator. Thus, with this PR, I'd like to propose adding a new marker trait for allocators that can be used as `#[global_allocator]`: ```rust // in core::alloc trait GlobalAllocator: Allocator {} ``` `GlobalAlloc` can then be implemented for all `GlobalAllocator`s: ```rust impl<A> GlobalAlloc for A where A: GlobalAllocator { /* ... */ } ``` This provides a backwards-compatible way for allocator libraries to switch to the new interface and allows deprecating `GlobalAlloc` (not done here). Over time, I expect that `GlobalAlloc` will become more and more of an implementation detail of the `#[global_allocator]` macro (for instance, one might add perma-unstable, hidden methods for things like `grow_zeroed` that are customised only by the blanket implementation). With regards to naming, I chose `GlobalAllocator` to mirror `Allocator`. `GlobalAlloc` should probably be deprecated quickly after stabilising `GlobalAllocator` to avoid confusion. For the same reason, I think it'd be better to add `GlobalAllocator` before stabilising `Allocator` – but that is not a necessity. r? @nia-e @rustbot label +I-libs-api-nominated
allow `Allocator`s to be used as `#[global_allocator]`s The (hopefully) immanent stabilisation of the `Allocator` trait raises the question of what is to be done about the older, already-stable `GlobalAlloc` trait. In my opinion, having two nearly-identical traits for the same purpose is needlessly confusing. Going forward, `Allocator` as the more modern interface should be _the_ allocator trait. With `Allocator` being currently unstable, there is the possibility of implementing `GlobalAlloc` for all `Allocator`s, thereby allowing them to be used as `#[global_allocator]` and allowing crates to seamlessly (and semver-compatibly) switch to `Allocator`. However, unconditionally implementing `GlobalAlloc` presents a footgun to users, as e.g. using `Global` as `#[global_allocator]` will lead to infinite recursion. @nia-e initially tried to resolve this in e1b7097 (rust-lang#156882) by using weird trait trickery to implement `GlobalAlloc` for every allocator except `Global`. But this does not go far enough, e.g. a bump allocator that itself allocates from `Global` is similarly unsuitable as global allocator. Thus, with this PR, I'd like to propose adding a new marker trait for allocators that can be used as `#[global_allocator]`: ```rust // in core::alloc trait GlobalAllocator: Allocator {} ``` `GlobalAlloc` can then be implemented for all `GlobalAllocator`s: ```rust impl<A> GlobalAlloc for A where A: GlobalAllocator { /* ... */ } ``` This provides a backwards-compatible way for allocator libraries to switch to the new interface and allows deprecating `GlobalAlloc` (not done here). Over time, I expect that `GlobalAlloc` will become more and more of an implementation detail of the `#[global_allocator]` macro (for instance, one might add perma-unstable, hidden methods for things like `grow_zeroed` that are customised only by the blanket implementation). With regards to naming, I chose `GlobalAllocator` to mirror `Allocator`. `GlobalAlloc` should probably be deprecated quickly after stabilising `GlobalAllocator` to avoid confusion. For the same reason, I think it'd be better to add `GlobalAllocator` before stabilising `Allocator` – but that is not a necessity. r? @nia-e @rustbot label +I-libs-api-nominated
allow `Allocator`s to be used as `#[global_allocator]`s The (hopefully) immanent stabilisation of the `Allocator` trait raises the question of what is to be done about the older, already-stable `GlobalAlloc` trait. In my opinion, having two nearly-identical traits for the same purpose is needlessly confusing. Going forward, `Allocator` as the more modern interface should be _the_ allocator trait. With `Allocator` being currently unstable, there is the possibility of implementing `GlobalAlloc` for all `Allocator`s, thereby allowing them to be used as `#[global_allocator]` and allowing crates to seamlessly (and semver-compatibly) switch to `Allocator`. However, unconditionally implementing `GlobalAlloc` presents a footgun to users, as e.g. using `Global` as `#[global_allocator]` will lead to infinite recursion. @nia-e initially tried to resolve this in e1b7097 (rust-lang#156882) by using weird trait trickery to implement `GlobalAlloc` for every allocator except `Global`. But this does not go far enough, e.g. a bump allocator that itself allocates from `Global` is similarly unsuitable as global allocator. Thus, with this PR, I'd like to propose adding a new marker trait for allocators that can be used as `#[global_allocator]`: ```rust // in core::alloc trait GlobalAllocator: Allocator {} ``` `GlobalAlloc` can then be implemented for all `GlobalAllocator`s: ```rust impl<A> GlobalAlloc for A where A: GlobalAllocator { /* ... */ } ``` This provides a backwards-compatible way for allocator libraries to switch to the new interface and allows deprecating `GlobalAlloc` (not done here). Over time, I expect that `GlobalAlloc` will become more and more of an implementation detail of the `#[global_allocator]` macro (for instance, one might add perma-unstable, hidden methods for things like `grow_zeroed` that are customised only by the blanket implementation). With regards to naming, I chose `GlobalAllocator` to mirror `Allocator`. `GlobalAlloc` should probably be deprecated quickly after stabilising `GlobalAllocator` to avoid confusion. For the same reason, I think it'd be better to add `GlobalAllocator` before stabilising `Allocator` – but that is not a necessity. r? @nia-e @rustbot label +I-libs-api-nominated
20 participants
View all comments
Stabilise a bare-minimum (dyn-incompatible, but could be in the future)
Allocatortrait, alongsideBox::new_in(),Vec::new_in(), theSystem&GlobalAllocators, and a blanket impl ofAllocator for T: GlobalAlloc. For now, we should take care not to make it possible to instantiate anything other than aVecorBoxwith 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