-
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
Add GlobalAlloc trait + tweaks for initial stabilization #49669
Conversation
Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. 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 |
I moved this |
I’ve added an exception in tidy. |
src/liballoc/raw_vec.rs
Outdated
@@ -756,7 +753,7 @@ mod tests { | |||
// before allocation attempts start failing. | |||
struct BoundedAlloc { fuel: usize } | |||
unsafe impl Alloc for BoundedAlloc { | |||
unsafe fn alloc(&mut self, layout: Layout) -> Result<*mut u8, AllocErr> { | |||
unsafe fn alloc(&mut self, layout: Layout) -> Result<NonNull<u8>, AllocErr> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have NonNull<u8>
leftovers in this file.
src/liballoc/rc.rs
Outdated
@@ -737,7 +737,7 @@ impl<T: Clone> RcFromSlice<T> for Rc<[T]> { | |||
// In the event of a panic, elements that have been written | |||
// into the new RcBox will be dropped, then the memory freed. | |||
struct Guard<T> { | |||
mem: *mut u8, | |||
mem: NonNull<u8>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could use NonNull<Void>
here too.
src/liballoc/Cargo.toml
Outdated
@@ -9,6 +9,7 @@ path = "lib.rs" | |||
|
|||
[dependencies] | |||
core = { path = "../libcore" } | |||
libc = { path = "../rustc/libc_shim" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
liballoc should not depend on libc. This will break code that runs in kernel-like environments where you have a global memory allocator but no libc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if libc was only a dependency for cfg(any(unix, target_os = "redox"))
? It’s only used there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer if liballoc was completely OS-independent. I am currently using Rust to make Linux binaries which do not link to libc (just using raw syscalls).
This is why, when I originally implemented printing a message on OOM (#30801), I allowed the OOM to be dynamically set at runtime. It would default to intrinsics::abort
, but would be overridden by libstd on startup to a handler that prints a message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you’re saying that a oom
method should be added to the GlobalAlloc
trait so that #[global_allocator]
can forward it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking of adding back a dynamic OOM handler, but your idea is better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does dynamic handler mean, if not the same as in my previous comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never mind, I see later in the thread that you mention set_oom_handler
.
src/libcore/alloc.rs
Outdated
} | ||
} | ||
|
||
/// The `CannotReallocInPlace` error is used when `grow_in_place` or | ||
/// The `CannotgInPlace` error is used when `grow_in_place` or |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, thanks.
Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. 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 |
I don't understand the reasoning behind this. Isn't |
@Amanieu no one will be using the |
Well if it’s the only thing available on Stable, people (at least some library authors) will definitely use |
These tests already regressed at an intermediate commit for this PR and rust-lang/llvm#110 fixed them at the time, but now they regressed again and I don’t know why.
|
What is the allocator going to be doing in the oom method? I don't really see why e.g. jemalloc would handle that differently than the system allocator. |
A little too big for me to handle 😉 |
You are right that jemalloc would handle this the same way as the system allocator. However in a There are two ways to support this functionality:
Both of these will work for |
But if you want to customize that behavior then sticking it on |
Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. 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 |
@sfackler You could use the same argument to say that the |
Yeah, Alloc::oom doesn't make much sense to me either. |
Add GlobalAlloc trait + tweaks for initial stabilization This is the outcome of discussion at the Rust All Hands in Berlin. The high-level goal is stabilizing sooner rather than later the ability to [change the global allocator](#27389), as well as allocating memory without abusing `Vec::with_capacity` + `mem::forget`. Since we’re not ready to settle every detail of the `Alloc` trait for the purpose of collections that are generic over the allocator type (for example the possibility of a separate trait for deallocation only, and what that would look like exactly), we propose introducing separately **a new `GlobalAlloc` trait**, for use with the `#[global_allocator]` attribute. We also propose a number of changes to existing APIs. They are batched in this one PR in order to minimize disruption to Nightly users. The plan for initial stabilization is detailed in the tracking issue #49668. CC @rust-lang/libs, @glandium ## Immediate breaking changes to unstable features * For pointers to allocated memory, change the pointed type from `u8` to `Opaque`, a new public [extern type](#43467). Since extern types are not `Sized`, `<*mut _>::offset` cannot be used without first casting to another pointer type. (We hope that extern types can also be stabilized soon.) * In the `Alloc` trait, change these pointers to `ptr::NonNull` and change the `AllocErr` type to a zero-size struct. This makes return types `Result<ptr::NonNull<Opaque>, AllocErr>` be pointer-sized. * Instead of a new `Layout`, `realloc` takes only a new size (in addition to the pointer and old `Layout`). Changing the alignment is not supported with `realloc`. * Change the return type of `Layout::from_size_align` from `Option<Self>` to `Result<Self, LayoutErr>`, with `LayoutErr` a new opaque struct. * A `static` item registered as the global allocator with the `#[global_allocator]` **must now implement the new `GlobalAlloc` trait** instead of `Alloc`. ## Eventually-breaking changes to unstable features, with a deprecation period * Rename the respective `heap` modules to `alloc` in the `core`, `alloc`, and `std` crates. (Yes, this does mean that `::alloc::alloc::Alloc::alloc` is a valid path to a trait method if you have `exetrn crate alloc;`) * Rename the the `Heap` type to `Global`, since it is the entry point for what’s registered with `#[global_allocator]`. Old names remain available for now, as deprecated `pub use` reexports. ## Backward-compatible changes * Add a new [extern type](#43467) `Opaque`, for use in pointers to allocated memory. * Add a new `GlobalAlloc` trait shown below. Unlike `Alloc`, it uses bare `*mut Opaque` without `NonNull` or `Result`. NULL in return values indicates an error (of unspecified nature). This is easier to implement on top of `malloc`-like APIs. * Add impls of `GlobalAlloc` for both the `Global` and `System` types, in addition to existing impls of `Alloc`. This enables calling `GlobalAlloc` methods on the stable channel before `Alloc` is stable. Implementing two traits with identical method names can make some calls ambiguous, but most code is expected to have no more than one of the two traits in scope. Erroneous code like `use std::alloc::Global; #[global_allocator] static A: Global = Global;` (where `Global` is defined to call itself, causing infinite recursion) is not statically prevented by the type system, but we count on it being hard enough to do accidentally and easy enough to diagnose. ```rust extern { pub type Opaque; } pub unsafe trait GlobalAlloc { unsafe fn alloc(&self, layout: Layout) -> *mut Opaque; unsafe fn dealloc(&self, ptr: *mut Opaque, layout: Layout); unsafe fn alloc_zeroed(&self, layout: Layout) -> *mut Opaque { // Default impl: self.alloc() and ptr::write_bytes() } unsafe fn realloc(&self, ptr: *mut Opaque, old_layout: Layout, new_size: usize) -> *mut Opaque { // Default impl: self.alloc() and ptr::copy_nonoverlapping() and self.dealloc() } fn oom(&self) -> ! { // intrinsics::abort } // More methods with default impls may be added in the future } ``` ## Bikeshed The tracking issue #49668 lists some open questions. If consensus is reached before this PR is merged, changes can be integrated.
☀️ Test successful - status-appveyor, status-travis |
Update jemallocator, fix for nightly-2018-04-15 CC https://github.com/alexcrichton/jemallocator/pull/40, rust-lang/rust#49669 <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20640) <!-- Reviewable:end -->
CC https://github.com/alexcrichton/jemallocator/pull/40, rust-lang/rust#49669 The new version of jemallocator requires a more recent jemalloc which doesn’t build on our current Android toolchain. Rather than figuring that out, duplicate ~70 lines from jemallocator and use the older jemalloc-sys directly.
CC https://github.com/alexcrichton/jemallocator/pull/40, rust-lang/rust#49669 The new version of jemallocator requires a more recent jemalloc which doesn’t build on our current Android toolchain. Rather than figuring that out, duplicate ~70 lines from jemallocator and use the older jemalloc-sys directly.
CC https://github.com/alexcrichton/jemallocator/pull/40, rust-lang/rust#49669 The new version of jemallocator requires a more recent jemalloc which doesn’t build on our current Android toolchain. Rather than figuring that out, duplicate ~70 lines from jemallocator and use the older jemalloc-sys directly.
CC https://github.com/alexcrichton/jemallocator/pull/40, rust-lang/rust#49669 The new version of jemallocator requires a more recent jemalloc which doesn’t build on our current Android toolchain. Rather than figuring that out, duplicate ~70 lines from jemallocator and use the older jemalloc-sys directly.
CC https://github.com/alexcrichton/jemallocator/pull/40, rust-lang/rust#49669 The new version of jemallocator requires a more recent jemalloc https://github.com/alexcrichton/jemallocator/pull/34 which doesn’t build on our current Android toolchain jemalloc/jemalloc#1175. To avoid blocking on figuring that out, duplicate ~70 lines from jemallocator and use the older jemalloc-sys directly.
Fork the jemallocator crate, fix for nightly-2018-04-15 CC https://github.com/alexcrichton/jemallocator/pull/40, rust-lang/rust#49669 The new version of jemallocator requires a more recent jemalloc https://github.com/alexcrichton/jemallocator/pull/34 which doesn’t build on our current Android toolchain jemalloc/jemalloc#1175. To avoid blocking on figuring that out, duplicate ~70 lines from jemallocator and use the older jemalloc-sys directly. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20641) <!-- Reviewable:end -->
So this removed the specializations of Why? And where should these be re-added? |
These still exist in https://crates.io/crates/jemallocator. The |
CC https://github.com/alexcrichton/jemallocator/pull/40, rust-lang/rust#49669 The new version of jemallocator requires a more recent jemalloc https://github.com/alexcrichton/jemallocator/pull/34 which doesn’t build on our current Android toolchain jemalloc/jemalloc#1175. To avoid blocking on figuring that out, duplicate ~70 lines from jemallocator and use the older jemalloc-sys directly.
This is the outcome of discussion at the Rust All Hands in Berlin. The high-level goal is stabilizing sooner rather than later the ability to change the global allocator, as well as allocating memory without abusing
Vec::with_capacity
+mem::forget
.Since we’re not ready to settle every detail of the
Alloc
trait for the purpose of collections that are generic over the allocator type (for example the possibility of a separate trait for deallocation only, and what that would look like exactly), we propose introducing separately a newGlobalAlloc
trait, for use with the#[global_allocator]
attribute.We also propose a number of changes to existing APIs. They are batched in this one PR in order to minimize disruption to Nightly users.
The plan for initial stabilization is detailed in the tracking issue #49668.
CC @rust-lang/libs, @glandium
Immediate breaking changes to unstable features
u8
toOpaque
, a new public extern type. Since extern types are notSized
,<*mut _>::offset
cannot be used without first casting to another pointer type. (We hope that extern types can also be stabilized soon.)Alloc
trait, change these pointers toptr::NonNull
and change theAllocErr
type to a zero-size struct. This makes return typesResult<ptr::NonNull<Opaque>, AllocErr>
be pointer-sized.Layout
,realloc
takes only a new size (in addition to the pointer and oldLayout
). Changing the alignment is not supported withrealloc
.Layout::from_size_align
fromOption<Self>
toResult<Self, LayoutErr>
, withLayoutErr
a new opaque struct.static
item registered as the global allocator with the#[global_allocator]
must now implement the newGlobalAlloc
trait instead ofAlloc
.Eventually-breaking changes to unstable features, with a deprecation period
heap
modules toalloc
in thecore
,alloc
, andstd
crates. (Yes, this does mean that::alloc::alloc::Alloc::alloc
is a valid path to a trait method if you haveexetrn crate alloc;
)Heap
type toGlobal
, since it is the entry point for what’s registered with#[global_allocator]
.Old names remain available for now, as deprecated
pub use
reexports.Backward-compatible changes
Opaque
, for use in pointers to allocated memory.GlobalAlloc
trait shown below. UnlikeAlloc
, it uses bare*mut Opaque
withoutNonNull
orResult
. NULL in return values indicates an error (of unspecified nature). This is easier to implement on top ofmalloc
-like APIs.GlobalAlloc
for both theGlobal
andSystem
types, in addition to existing impls ofAlloc
. This enables callingGlobalAlloc
methods on the stable channel beforeAlloc
is stable. Implementing two traits with identical method names can make some calls ambiguous, but most code is expected to have no more than one of the two traits in scope. Erroneous code likeuse std::alloc::Global; #[global_allocator] static A: Global = Global;
(whereGlobal
is defined to call itself, causing infinite recursion) is not statically prevented by the type system, but we count on it being hard enough to do accidentally and easy enough to diagnose.Bikeshed
The tracking issue #49668 lists some open questions. If consensus is reached before this PR is merged, changes can be integrated.