-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Tracking issue for the GlobalAlloc trait and related APIs #49668
Comments
I was initially confused by the name |
Relevant discussions pre-PR:
|
The OSX system allocator supports that. Jemalloc does too, optionally. Glibc malloc and Windows heap allocator don't. |
The fact that |
Has the |
Naming bikesheds: Given that the attribute is Also, I am not very happy with |
If Agreed that |
I would personally keep the name "alloc" for traits and methods, the
I don't think we should implement |
The |
Yeah, |
I don't think My rule of thumb for |
As Alex said, |
That should only require |
Why |
|
Every time I see this trait name I get it confused with |
@retep998 What do you think would be a better name for the trait? |
[Reposting in the right tracking issue this time...] So, the name currently decided on by @SimonSapin for the allocated memory type seems to be Opaque. I concur that Void is a dubious name, but I don't think Opaque is great, mainly due to its complete lack of descriptiveness. I propose one of the following:
|
I would add |
|
The |
I'm OK with global functions, with the understanding that they will be deprecated once |
I’ve previously argued that if we have a set of free functions that have the exact same signature as a trait, they might as well be a trait impl. But it is a good point that functions would be easier to deprecate, so I’m fine with that as well. |
Right now we can't deprecate trait impls AFAIK: #39935 |
As discussed in rust-lang#49668 (comment) and subsequent, there are use-cases where the OOM handler needs to know the size of the allocation that failed. The alignment might also be a cause for allocation failure, so providing it as well can be useful.
OOM handling changes As discussed in #49668 (comment) and subsequent. This does have codegen implications. Even without the hooks, and with a handler that ignores the arguments, the compiler doesn't eliminate calling `rust_oom` with the `Layout`. Even if it managed to eliminate that, with the hooks, I don't know if the compiler would be able to figure out it can skip it if the hook is never set. A couple implementation notes: - I went with explicit enums rather than bools because it makes it clearer in callers what is being requested. - I didn't know what `feature` to put the hook setting functions behind. (and surprisingly, the compile went through without any annotation on the functions) - There's probably some bikeshedding to do on the naming. Cc: @SimonSapin, @sfackler
AFAICT this is not exactly how it is implemented in neither libc++ [0] nor libstdc++ [1]. When a size of zero is requested, the size is as you mention changed to one, but then, a real memory allocation for this new size is performed with the requested alignment, and a unique pointer to the address is returned. That is, for allocations of zero size, C++ calls the system allocator, potentially performing a context switch to the kernel, etc.
C++ does not have zero-sized types: all types are at least 1 byte in size. For example, if you create an array of length zero of an empty type If your language does not have a concept of zero-sized types, zero-sized allocations do not really make much sense, and I argue that the real reason nobody is complaining about these is because nobody is triggering calls to Many C++ developers have, however, complained about the lack of zero-sized types in the language, and ZST are already part of the C++20 standard and opt-in in some situations, but none that could trigger I think a better example than C++
Which if the implementation decides to return something different than
So AFAICT for zero-sized allocations a compliant C11 implementation returns a pointer that cannot be dereferenced but can be freed. There are no requirements on the pointers to be unique. Since There is a potentially interesting presentation about the topic [2] discussing it in the context of security vulnerabilities, but these probably don't apply to Rust and I haven't gone through it all yet. [0] https://github.com/llvm-mirror/libcxx/blob/master/src/new.cpp#L71 |
I would like to clarify that the point I'm trying to make: The overhead of the extra zero-check in the common case (non-zero length allocation) is tiny to the point of being irrelevant. As such, we should aim to provide an API with fewer footguns, especially considering that ZSTs are much more common in Rust. By the way, I would like to add another data point for how allocation APIs handle zero sizes. jemalloc's internal API ( [1] http://jemalloc.net/jemalloc.3.html
|
The libs team discussed this thread today today and consensus was to:
/// #[global_allocator] can only be applied to a `static` item that implements this trait
pub unsafe trait GlobalAlloc {
unsafe fn alloc(&self, layout: Layout) -> *mut u8;
unsafe fn dealloc(&self, ptr: *mut u8, layout: Layout);
unsafe fn alloc_zeroed(&self, layout: Layout) -> *mut u8 {
// Default impl: self.alloc() and ptr::write_bytes()
}
unsafe fn realloc(&self, ptr: *mut u8, layout: Layout, new_size: usize) -> *mut u8 {
// Default impl: self.alloc() and ptr::copy_nonoverlapping() and self.dealloc()
}
// More methods with default impls may be added in the future
}
#[derive(Debug, Copy, Clone, PartialEq, Eq)]
pub struct Layout { /* private */ }
impl Layout {
pub fn from_size_align(size: usize: align: usize) -> Result<Self, LayoutError> {…}
pub unsafe fn from_size_align_unchecked(size: usize: align: usize) -> Self {…}
pub fn size(&self) -> usize {…}
pub fn align(&self) -> usize {…}
pub fn new<T>() -> Self {…}
pub fn for_value<T: ?Sized>(t: &T) -> Self {…}
}
#[derive(Copy, Clone, Debug)]
pub struct LayoutError { /* private */ }
/// Forwards to the `static` item registered
/// with `#[global_allocator]` if any, or the operating system’s default.
unsafe fn alloc(&self, layout: Layout) -> *mut u8 {…}
unsafe fn dealloc(&self, ptr: *mut u8, layout: Layout) {…}
unsafe fn alloc_zeroed(&self, layout: Layout) -> *mut u8 {…}
unsafe fn realloc(&self, ptr: *mut u8, layout: Layout, new_size: usize) -> *mut u8 {…}
/// The operating system’s default allocator.
pub struct System;
impl GlobalAlloc for System {…} Update: also: pub fn oom(layout: Layout) -> ! {…} |
The C99 Rationale V5.10 document has rationale on why
It is not 100% clear to me what this rationale means, but it appears to mean that they decided to allow About C++, I've asked on stackoverflow, and s Bo Persson pointed to C++'s standard 9th defect report:
So it seems that in the original draft |
If the |
@gnzlbg The use case of intermediate possibly-zero-size |
Basically, the trait API looks ok, but what is missing there is the comments expressing the function requirements, semantics, and guarantees. |
Right, this still needs lots of docs to write out the API contracts. Returning null indicates an error or failure. Maybe the OS is out of memory, maybe the given Calling |
Thanks for typing this up @SimonSapin! That all looks accurate and good to me |
I'm not too happy with the lack of As a side note, should we add a |
I don't think a global_ prefix is all that necessary. It's a free function so it has to be talking to the global allocator, right? There aren't any other options. |
Or a |
Stabilization PR: #51241 (finally!) |
Stabilize GlobalAlloc and #[global_allocator] This PR implements the changes discussed in #49668 (comment) Fixes #49668 Fixes #27389 This does not change the default global allocator: #36963
As discussed in rust-lang/rust#49668 (comment) and subsequent, there are use-cases where the OOM handler needs to know the size of the allocation that failed. The alignment might also be a cause for allocation failure, so providing it as well can be useful.
PR #49669 adds a
GlobalAlloc
trait, separate fromAlloc
. This issue track the stabilization of this trait and some related APIs, to provide the ability to change the global allocator, as well as allocating memory without abusingVec::with_capacity
+mem::forget
.Defined in or reexported from the
std::alloc
module:Update: below is the API proposed when this issue was first opened. The one being stabilized is at #49668 (comment).
CC @rust-lang/libs, @glandium
Unresolved questions or otherwise blocking
Global
type.GlobalAllocator
?Name of theRenamed toVoid
type. Lower-casevoid
would match C (our*mut void
is very close to C’svoid*
type), but violates the convension of camel-case for non-primitive types. ButVoid
in camel-case is already the name of an empty enum (not an extern type like here) in a popular crate. (An extern type is desirable so that<*mut _>::offset
cannot be called without first casting to another pointer type.) Maybe call itOpaque
?Unknown
?Opaque
.TakingLayout
by reference, or making itCopy
Alloc methods should take layout by reference #48458.Copy
: Implement Copy for std::alloc::Layout #50109Not to be required by this trait since glibc and Windows do not support this.GlobalAlloc::owns
: Alloc: Add owns method #44302 proposes making it required for allocators to be able to tell if it “owns” a given pointer.Settle semantics of layout "fit" and other safety conditions.Without anusable_size
method (for now), the layout passed to dealloc must be the same as was passed to alloc. (Same as withAlloc::usable_size
’s default impl returning(layout.size(), layout.size())
.)AnReplace {Alloc,GlobalAlloc}::oom with a lang item. #50144oom
method is part ofGlobalAlloc
so that implementation-specific printing to stderr does not need to be part of liballoc and instead goes through#[global_allocator]
, but this does not really belong in theGlobalAlloc
trait. Should another mechanism like#[global_allocator]
be added to have pluggable OOM handling?Not proposed (yet) for stabilization
Alloc
traitLayout
methodsAllocErr
typeWe’re not ready to settle on a design for collections generic over the allocation type. Discussion of this use case should continue on the tracking issue for the
Alloc
trait: #32838.The text was updated successfully, but these errors were encountered: