Associate an allocator to boxes #58457
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
Cc: @SimonSapin |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Aha, this is something I had already hit in the allocator_api crate but didn't transpose to my rustc branch. |
|
How is |
It's not related, what is is PhantomData. Default is used to get an allocator from nothing. |
And you won't be able to do anything useful with a non-ZST allocator since we're always using default(). |
|
This implementation means I think it's better just to implement the "quick and dirty" solution from #50882 (comment). |
2 things:
I'd rather have /something/ than wait even more for the above to be addressed. |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Haha, moving off the |
|
r? @sfackler |
|
Also, the codegen/box-maybe-uninit.rs test is kind of biased... If it was using What do we do about this? Keeping the box keyword where it's currently used means:
That's all artificial to keep things nicer for Global, at the cost of silently making things "bad" for other allocators. |
|
In fact, it doesn't even pass with |
|
And it fails for |
|
I see, in this PR the I think this hack can be useful to experiment with e.g. adding a type parameter and see the effects on type inference, find bugs in the compiler, etc. However I think we maybe shouldn’t land it: it’s reasonable for an allocator that has per-instance state to implement |
|
@SimonSapin Your message makes me realize there's something fundamentally dangerous with In case the above is not entirely clear, take this example: struct StatefulAlloc {
next_free: *mut u8,
}
impl Alloc for StatefulAlloc { ... }
// Do stuff with Box<T, StatefulAlloc>I don't see actual examples where the above uses would make sense. However impl<'a> Alloc for &'a StatefulAlloc { ... }
// Do stuff with Box<T, &StatefulAlloc>does make sense. An alternative would be struct MyGlobalStatefulAlloc;
static mut MyGlobalAllocState: StatefulAlloc = StatefulAlloc::default();
impl Alloc for MyGlobalStatefulAlloc { /* impl using MyGlobalAllocState */ }IOW, the usecases are either ZSTs or refs, and other kinds of types make little sense. And a generic |
|
That’s a good point. In my previous message I somewhat confused an allocator "instance" (which could be shared between many So it’s true that the "bad" cases are probably not useful, but
Those might be dependent bugs that should be fixed before we can land this. |
Maybe not for |
|
I really would like to land something, no matter how piecemeal, just to make forward progress. For example, once @glandium lands something, I can use the same hacks that end up being used to get #52420 to work, in parallel to people trying to obviate those hacks. As long as the stable APIs of things aren't broken, it should be OK, right? |
src/liballoc/boxed.rs
Outdated
| @@ -83,7 +84,7 @@ use crate::str::from_boxed_utf8_unchecked; | |||
| #[lang = "owned_box"] | |||
| #[fundamental] | |||
| #[stable(feature = "rust1", since = "1.0.0")] | |||
| pub struct Box<T: ?Sized>(Unique<T>); | |||
| pub struct Box<T: ?Sized, A: Alloc + Default = Global>(Unique<T>, PhantomData<A>); | |||
There was a problem hiding this comment.
Curiosity: Why include the trait bounds here? I don't recall anywhere else the libraries do that when the definition of the type doesn't need something from one of the traits.
Also, this new type parameter is insta-stable, right? Is that a concern? (Would it work to make the lang item an unstable CustomBox<T, A>, and have type Box<T> = CustomBox<T, Global>; for now?)
There was a problem hiding this comment.
Curiosity: Why include the trait bounds here?
I don't know, that kind of ensures the bounds on all the various impls are right?
Also, this new type parameter is insta-stable, right? Is that a concern?
Insta-stable, yes. A concern? I don't know.
(Would it work to make the lang item an unstable
CustomBox<T, A>, and havetype Box<T> = CustomBox<T, Global>;for now?)
Unfortunately, the compiler desugars such aliases when printing errors. See #50882 (comment) (the part about the extra parameter doesn't apply anymore)
|
When is the next rust release? It would be cool if we waited long enough for the |
|
@Ericson2314 https://forge.rust-lang.org/ |
|
Oh right, beta. [@mati865 I couldn't help myself but https://xkcd.com/1179/, my broken American brain can pull itself together for the ISO standard, but flails around confronted with anything else consistent as it may be.] |
This turns `Box<T>` into `Box<T, A = Global>`, with a `A: Alloc` bound for impls. Ideally, inherent methods like `Box::new` would be applied to `Box<T, A: Alloc + Default>`, but as of now, that would be backwards incompatible because it would require type annotations in places where they currently aren't required. `impl FromIterator` is not covered because it relies on `Vec`, which would need allocator awareness. `DispatchFromDyn` is left out or being generic over `A` because there is no bound that would make it work currently. `FnBox` is left out because it's related to `DispatchFromDyn`.
|
beta already has what it takes to build without |
|
@glandium I'd thought for bootstrapping purposes we'd need to wait another cycle for it to hit stable. But, glad to be wrong in any event! |
|
Pinging @glandium from triage - this PR has sat idle for more than a month. |
|
Pinging @glandium from triage - giving this one more week before closing for inactivity |
Ping from triage: @glandium @SimonSapin, is this still blocked by the above comment? If so, this may be labeled as |
SimonSapin
added
the
S-blocked
|
Yes it is. Added the label. |
hdhoang
removed
the
S-waiting-on-author
|
Should another pub fn into_raw_from(b: Box<T, A>) -> (*mut T, A) {
let (u, a) = Box::into_raw_non_null(b);
(u.as_ptr(), a)
}
pub fn into_raw_non_null_from(b: Box<T, A>) -> (NonNull<T>, A) {
let (u, a) = Box::into_unique_from(b);
(u.into(), a)
}
pub fn into_raw_unique_from(b: Box<T, A>) -> (Unique<T>, A) {
let mut unique = b.0;
let allocator = b.1;
mem::forget(b);
unsafe { (Unique::new_unchecked(unique.as_mut() as *mut T), allocator) }
}This way it's possible to round-trip from box <-> pointer with an allocator. |
|
@ObsidianMinor Sadly it's not possible that easy as However a while ago I had a solution for this problem, but I can't remember right now. Edit: If I remember corretly, it was some magic with |
|
from triaging, I would like to close this PR until the blocking discussion is resolved:
Thank you for your work, let's reopen and continue when we can make progress. |
hdhoang
added
S-blocked-closed
and removed
S-blocked
Support custom allocators in `Box` r? `@Amanieu` This pull request requires a crater run. ### Prior work: - rust-lang#71873 - rust-lang#58457 - [`alloc-wg`](https://github.com/TimDiekmann/alloc-wg)-crate Currently blocked on: - ~rust-lang#77118~ - ~rust-lang/chalk#615 (rust-lang#77515)~
|
In case anyone subscribed here and want an update: #77187 just got merged 🚀 |
jyn514
added
S-blocked
This turns
Box<T>intoBox<T, A = Global>, with aA: Allocboundfor impls.
Ideally, inherent methods like
Box::newwould be applied toBox<T, A: Alloc + Default>, but as of now, that would be backwardsincompatible because it would require type annotations in places where
they currently aren't required.
impl FromIteratoris not covered because it relies onVec, whichwould need allocator awareness.
DispatchFromDynis left out or being generic overAbecause thereis no bound that would make it work currently.
FnBoxis left outbecause it's related to
DispatchFromDyn.