Stabilize dyn subset of Allocator as core::alloc::Alloc#157286
Stabilize dyn subset of Allocator as core::alloc::Alloc#157286jmillikin wants to merge 4 commits into
dyn subset of Allocator as core::alloc::Alloc#157286Conversation
|
These commits modify the If this was unintentional then you should revert the changes before this PR is merged. |
rustbot
added
S-waiting-on-review
|
Example of unsoundness in existing code caused by the fn allocate_zeroed(&self, layout: Layout) -> Result<NonNull<[u8]>, AllocError> {
let ptr = self.allocate(layout)?;
if layout.size() > 0 {
unsafe {
ptr.as_mut_ptr().write_bytes(0, layout.size());
}
}
Ok(ptr)
}The expectation that |
jmillikin
force-pushed
the
stabilize-alloc-subset
branch
2 times, most recently
from
8e19eb8 to
0dc6d4f
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
jmillikin
force-pushed
the
stabilize-alloc-subset
branch
from
1394323 to
c047de3
Compare
|
This PR modifies |
jmillikin
force-pushed
the
stabilize-alloc-subset
branch
2 times, most recently
from
15faebd to
4b90609
Compare
|
These commits modify the If this was unintentional then you should revert the changes before this PR is merged. |
jmillikin
force-pushed
the
stabilize-alloc-subset
branch
from
385c4cf to
161846f
Compare
rustbot
added
the
T-bootstrap
|
Does this stabilization allow using a type that implements If so, how do you deal with #157089? |
|
The remaining CI failures are related to using a non-ustream source for I'm hesitant to do major surgery on the build just to get that temporary workaround working better. If this seems like a reasonable way to stabilize allocation then I'll send a separate PR to set up |
|
Are you aware of the existing ongoing stabilization effort and the other stabilization PR? |
This PR does not allow an #[stable]
pub trait Alloc { /* memory allocation */ }
#[unstable]
pub trait Allocator: Alloc { /* magic marker for ongoing stdlib collection support for custom allocation */ } |
Yes, I commented on them several times and have also been discussing the topic in Zulip. Hence this PR. edit: |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
I'm not sure having the dyn-compatible form of The design I am currently most in favour of would be something to the effect of: trait Allocator {
// example dyn-incompatible item
const MIN_ALIGN: usize;
fn allocate(...) -> Result<...>;
}
trait DynAllocator {
// same methods as Allocator, different names to prevent name resolution issues
fn dyn_allocate(...) -> Result<...>;
}
impl<A: DynAllocator> Allocator for A {
// conservative defaults for items
const MIN_ALIGN: usize = 1;
fn allocate(...) -> Result<...> {
self.dyn_allocate(...)
}
}that way, a |
|
Even if an allocator offers flags or richer error conditions, the ability to implement Thinking about the allocator API in terms of carving out known required features and then mapping them to types, The necessity of For this PR I somewhat arbitrarily made pub trait Alloc { /* memory allocation, dyn-compatible */ }
// an allocator is a handle to an `Alloc` instance
//
// safety: a clone's get() returns the same underlying `Alloc`
// as the original
pub unsafe trait Allocator {
type Alloc: Alloc;
fn alloc_get(&self) -> &Self::Alloc;
fn alloc_clone(&self) -> Self;
}
impl<'a, A: Alloc> Allocator for &'a A { ... }
impl<A: Alloc> Allocator for Rc<A> { ... }
impl<A: Alloc> Allocator for Arc<A> { ... }In that design it's not possible to create pub struct TCMalloc;
impl Alloc for TCMalloc { ... }
impl Allocator for TCMalloc {
type Alloc = Self;
fn alloc_get(&self) -> &Self::Alloc { self }
fn alloc_clone(&self) -> Self { TCMalloc }
}
pub struct MyAlloc(Arc<InnerState>);
impl Alloc for MyAlloc { ... }
impl Allocator for MyAlloc {
type Alloc = Self;
fn alloc_get(&self) -> &Self::Alloc { self }
fn alloc_clone(&self) -> Self { MyAlloc(self.0.clone()) }
}The |
|
The reason to not have this be a supertrait is my proposed blanket impl. If we split the dyn-compatible and dyn-incompatible traits entirely, the dyn-incompatible one can have a blanket impl with conservative values in terms of the dyn-compatible trait and everyone can always require said dyn-incompatible trait. Otherwise, it would be impossible to express the semantics of "we want the associated constants/types if they're available", since it would require specialisation |
|
The blanket instance would mean you couldn't have a type impl both trait Allocator {
// alignments get rounded up to this value
const MIN_ALIGN: usize;
fn allocate(...) -> Result<...>;
}
impl<A: DynAllocator> Allocator for A {
// `dyn DynAllocator` is assumed to not round alignments
const MIN_ALIGN: usize = 1;
fn allocate(...) -> Result<...> {
self.dyn_allocate(...)
}
}
struct SizeClassAlloc;
impl SizeClassAlloc {
const MIN_ALIGN: usize = 8; // smallest size class is 8 bytes
}
impl DynAllocator for SizeClassAlloc {
fn dyn_allocate(&self, layout: Layout) -> Result<NonNull<u8>, AllocError> {
let align = layout.align().max(Self::MIN_ALIGN);
// this is fine, over-aligning is always permitted
}
}
// conflicting implementation of trait `Allocator` for type `SizeClassAlloc`
impl Allocator for SizeClassAlloc {
const MIN_SIZE: usize = 8;
fn allocate(&self, layout: Layout) -> Result<NonNull<u8>, AllocError> {
todo!()
}
} |
|
Yep, it would mean newtyping. That is a notable issue, but I think it's Less Bad than making downstream users not want to use the dyn-incompatible subtrait and possibly optimise around it |
I'm not sure what this means, or what situation you're trying to guard against. Can you explain more about what misuse you're forseeing? Downstream libraries that need Downstream libraries that need |
7 participants
Stabilize an MVP subset of the unstable
Allocatortrait ascore::alloc::Alloc. This new trait provides memory allocation only (it can be considered a trait version ofalloc/realloc/dealloc), and does not require the shared-identityCopy/CloneofAllocator.The
Allocatortrait remains as a marker trait for the safety properties required by the standard container types, such asBox, and is not being stabilized in this commit. It is expected that future work onallocator_apiwill expand theAllocatortrait with new safety properties or functionality (which may bedyn-incompatible).As part of the split, functions of
Allocwere adjusted to returnNonNull<u8>instead ofNonNull<[u8]>. This change is intended to better match the actual usage of allocation APIs, make the API more resistant to accidental unsoundness, and relax the expectation of slice length elision via inlining.The
Allocator::allocate_at_leastfunction is added for compatibility withhashbrown, which depends on the existing unstableallocator_apisignature when being built as part of the standard library. That wrinkle will need to be resolved via an updatedhashbrownbefore this can be merged.cc @rust-lang/libs @rust-lang/libs-api @rust-lang/opsem
r? libs