OOM handling changes #50880
OOM handling changes #50880
Conversation
|
r? @bluss (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
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 |
src/libstd/alloc.rs
Outdated
| #[doc(inline)] pub use alloc_system::System; | ||
| #[doc(inline)] pub use core::alloc::*; | ||
|
|
||
| use panicking::HOOK_LOCK; |
There was a problem hiding this comment.
Is there any particular reason we're reusing this lock?
There was a problem hiding this comment.
I saw no particular reason not to reuse it.
There was a problem hiding this comment.
OOM hooks and panic hooks are totally unrelated concepts :P Let's avoid the overlap.
src/libstd/alloc.rs
Outdated
| /// The OOM hook is invoked when an infallible memory allocation fails. | ||
| /// The default hook prints a message to standard error and aborts the | ||
| /// execution, but this behavior can be customized with the [`set_hook`] | ||
| /// and [`take_hook`] functions. |
There was a problem hiding this comment.
The hook currently isn't allowed to unwind IIRC. Probably worth documenting.
There was a problem hiding this comment.
Isn’t it really? Why?
We have accepted an RFC for oom=panic for the use case of handling OOM with catch_unwind: #48043, https://github.com/rust-lang/rfcs/blob/master/text/2116-alloc-me-maybe.md#oompanic
There was a problem hiding this comment.
It's definitely intended to work, but there's a comment over here claiming we die in MIR transforms if you try to actually do it: https://github.com/rust-lang/rust/blob/master/src/liballoc/alloc.rs#L104-L121
There was a problem hiding this comment.
I don't actually know why Box still needs a special lang item to allocate though - maybe we can kill off exchange_alloc entirely at this point?
There was a problem hiding this comment.
Maybe box foo syntax needs a lang item? It’s how Box::new is defined.
There was a problem hiding this comment.
Ah yeah, that'd be it. Placement new's getting removed though so we should be able to switch Box::new over to just use the global allocator like other collections do.
There was a problem hiding this comment.
The hook currently isn't allowed to unwind IIRC. Probably worth documenting.
If the hook unwinding crashes the compiler, but the intent is for it to be allowed per the oom=panic RFC, it probably makes sense to leave any comment about unwinding out of the documentation.
src/libstd/alloc.rs
Outdated
| } | ||
|
|
||
| fn default_hook(_: Layout) -> ! { | ||
| rtabort!("memory allocation failed") |
There was a problem hiding this comment.
Might as well update this to something like memory allocation of {} bytes failed.
|
cc @gankro for the RawVec changes |
|
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 |
|
So interestingly, unwinding does work: #![feature(allocator_api)]
fn main() {
std::alloc::set_hook(Box::new(|layout| { panic!("oom {}", layout.size()) }));
let result = std::panic::catch_unwind(|| {
let v = Vec::<u8>::with_capacity(100000000000000);
println!("{:p}", &v[..]);
});
println!("{:?}", result);
}On my machine, with rustc built with the patches here, this prints the following: |
|
Ah sweet, seems like it may just be an out of date comment then! |
|
☔ The latest upstream changes (presumably #51041) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Did you mean to change the |
|
Dammit, I keep doing this on rebase. |
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.
|
Have a look at #30801 which previously implemented OOM handler hooks (which then got removed later, somehow). Two things stand out in particular:
|
|
The hooks were removed in #42727 without an explanation... About About the |
|
Should I restore the function names that were used before? |
As I said, I'm not adding the call to rtabort. It's already there. I'm merely moving it. If you feel strongly about this, please file a separate issue. |
|
@bors r+ Looks good! |
|
📌 Commit a4d899b has been approved by |
bors
added
S-waiting-on-bors
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
|
☀️ Test successful - status-appveyor, status-travis |
Bug 1458161 added a rust OOM handler based on an unstable API that was removed in 1.27, replaced with something that didn't allow to get the failed allocation size. Latest 1.28 nightly (2018-06-13) has rust-lang/rust#50880, rust-lang/rust#51264 and rust-lang/rust#51241 merged, which allow to hook the OOM handler and get the failed allocation size again. Because this is still an unstable API, we explicitly depend on strict versions of rustc. We also explicitly error out if automation builds end up using a rustc version that doesn't allow us to get the allocation size for rust OOM, because we don't want that to happen without knowing. --HG-- extra : rebase_source : 6c097151046d088cf51f4755dd69bde97bb8bd8b
Bug 1458161 added a rust OOM handler based on an unstable API that was removed in 1.27, replaced with something that didn't allow to get the failed allocation size. Latest 1.28 nightly (2018-06-13) has rust-lang/rust#50880, rust-lang/rust#51264 and rust-lang/rust#51241 merged, which allow to hook the OOM handler and get the failed allocation size again. Because this is still an unstable API, we explicitly depend on strict versions of rustc. We also explicitly error out if automation builds end up using a rustc version that doesn't allow us to get the allocation size for rust OOM, because we don't want that to happen without knowing. UltraBlame original commit: 5182bca90d0609f182d5a7b6b48ed2ffbbce32c2
Bug 1458161 added a rust OOM handler based on an unstable API that was removed in 1.27, replaced with something that didn't allow to get the failed allocation size. Latest 1.28 nightly (2018-06-13) has rust-lang/rust#50880, rust-lang/rust#51264 and rust-lang/rust#51241 merged, which allow to hook the OOM handler and get the failed allocation size again. Because this is still an unstable API, we explicitly depend on strict versions of rustc. We also explicitly error out if automation builds end up using a rustc version that doesn't allow us to get the allocation size for rust OOM, because we don't want that to happen without knowing. UltraBlame original commit: 5182bca90d0609f182d5a7b6b48ed2ffbbce32c2
Bug 1458161 added a rust OOM handler based on an unstable API that was removed in 1.27, replaced with something that didn't allow to get the failed allocation size. Latest 1.28 nightly (2018-06-13) has rust-lang/rust#50880, rust-lang/rust#51264 and rust-lang/rust#51241 merged, which allow to hook the OOM handler and get the failed allocation size again. Because this is still an unstable API, we explicitly depend on strict versions of rustc. We also explicitly error out if automation builds end up using a rustc version that doesn't allow us to get the allocation size for rust OOM, because we don't want that to happen without knowing. UltraBlame original commit: 5182bca90d0609f182d5a7b6b48ed2ffbbce32c2
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_oomwith theLayout. 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:
featureto put the hook setting functions behind. (and surprisingly, the compile went through without any annotation on the functions)Cc: @SimonSapin, @sfackler