Skip to content
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 fallible Box, Arc, and Rc allocator APIs #80310

Merged
merged 7 commits into from
Jan 1, 2021

Conversation

Manishearth
Copy link
Member

cc #48043

It was suggested in #48043 (comment) that Box::try_* follows the spirit of RFC 2116. This PR is an attempt to add the relevant APIs, tied to the same feature gate. Happy to make any changes or turn this into an RFC if necessary.

cc @rust-lang/wg-allocators

@rust-highfive
Copy link
Collaborator

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 22, 2020
/// ```
#[unstable(feature = "allocator_api", issue = "32838")]
// #[unstable(feature = "new_uninit", issue = "63291")]
pub fn try_new_uninit_in(alloc: A) -> Result<Box<mem::MaybeUninit<T>, A>, AllocError> {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unclear if I should also be adding these

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no objections

// #[unstable(feature = "new_uninit", issue = "63291")]
pub fn try_new_uninit_in(alloc: A) -> Result<Box<mem::MaybeUninit<T>, A>, AllocError> {
let layout = Layout::new::<mem::MaybeUninit<T>>();
let ptr = alloc.allocate(layout)?.cast();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not update new_uninit_in to be try_new_uninit_in().unwrap_or_else(), but can if folks prefer. I wasn't sure if it would have performance issues.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the current implementation of new_uninit_in already does an unwrap_or_else on failed allocations, I think it'd result in the same performance. If you want, you can still do a perf CI run to make sure though?

@rust-log-analyzer
Copy link
Collaborator

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
configure: rust.channel         := nightly
configure: rust.debug-assertions := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
configure: 
---
skip untracked path cpu-usage.csv during rustfmt invocations
skip untracked path src/doc/book/ during rustfmt invocations
skip untracked path src/doc/rust-by-example/ during rustfmt invocations
skip untracked path src/llvm-project/ during rustfmt invocations
Diff in /checkout/library/alloc/src/boxed.rs at line 153:
 use core::ptr::{self, Unique};
 use core::task::{Context, Poll};
 
-use crate::alloc::{handle_alloc_error, Allocator, AllocError, Global, Layout};
Running `"/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/rustfmt" "--config-path" "/checkout" "--edition" "2018" "--unstable-features" "--skip-children" "--check" "/checkout/library/alloc/src/boxed.rs"` failed.
If you're running `tidy`, try again with `--bless`. Or, if you just want to format code, run `./x.py fmt` instead.
+use crate::alloc::{handle_alloc_error, AllocError, Allocator, Global, Layout};
 use crate::borrow::Cow;
 use crate::raw_vec::RawVec;
 use crate::str::from_boxed_utf8_unchecked;
Diff in /checkout/library/alloc/src/boxed.rs at line 404:
     // #[unstable(feature = "new_uninit", issue = "63291")]
     pub fn try_new_zeroed_in(alloc: A) -> Result<Box<mem::MaybeUninit<T>, A>, AllocError> {
         let layout = Layout::new::<mem::MaybeUninit<T>>();
-        let ptr =
-            alloc.allocate_zeroed(layout)?.cast();
+        let ptr = alloc.allocate_zeroed(layout)?.cast();
         unsafe { Ok(Box::from_raw_in(ptr.as_ptr(), alloc)) }
 
failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test --stage 2 src/tools/tidy
Build completed unsuccessfully in 0:00:21

@ggriffiniii
Copy link

Drive by comment since I'm looking at making similar changes for Rc/Arc. It seems like a try_new method (one that attempts an allocation from the global allocator and returns a Result on failure) would be a natural extension, but is missing from this change. Was that an intentional design choice for some reason?

@Manishearth
Copy link
Member Author

No, I'm considering adding those actually, I just wasn't sure what the libs team would want.

@Manishearth Manishearth changed the title Add fallible box allocator APIs Add fallible Box, Arc, and Rc allocator APIs Dec 31, 2020
@Manishearth
Copy link
Member Author

I've added APIs for Arc and Rc. I have only modified _new() APIs, I have not done anything for .pin() or .make_mut() (or even new_cyclic())

@Manishearth
Copy link
Member Author

@kennytm I've made further changes, if you'd like to review. I think this is ready to land if the @rust-lang/libs is okay with this going through without an RFC (it is feature gated, and @SimonSapin indicated an RFC is probably not necessary so probably no big deal).

///
/// The function `mem_to_rcbox` is called with the data pointer
/// and must return back a (potentially fat)-pointer for the `RcBox<T>`.
unsafe fn try_allocate_for_layout(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you make allocate_for_layout reuse try_allocate_for_layout(...).unwrap_or_else(...)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh ok. too bad the layout still needs to be recomputed.

library/alloc/src/sync.rs Show resolved Hide resolved
@kennytm
Copy link
Member

kennytm commented Dec 31, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Dec 31, 2020

📌 Commit 8f3cb7d has been approved by kennytm

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 31, 2020
///
/// [zeroed]: mem::MaybeUninit::zeroed
#[unstable(feature = "allocator_api", issue = "32838")]
// #[unstable(feature = "new_uninit", issue = "63291")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be #[inline]

/// # Ok::<(), std::alloc::AllocError>(())
/// ```
#[unstable(feature = "allocator_api", issue = "32838")]
// #[unstable(feature = "new_uninit", issue = "63291")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be #[inline]

/// ```
/// #![feature(allocator_api, new_uninit)]
///
///
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
///

/// # Ok::<(), std::alloc::AllocError>(())
/// ```
///
/// [zeroed]: ../../std/mem/union.MaybeUninit.html#method.zeroed
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// [zeroed]: ../../std/mem/union.MaybeUninit.html#method.zeroed
/// [zeroed]: mem::MaybeUninit::zeroed

@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jan 1, 2021
@bors
Copy link
Contributor

bors commented Jan 1, 2021

⌛ Trying commit 375e7c5 with merge 233a249a8282ce5b42fc70c2cce6537e0743dbf7...

@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@bors
Copy link
Contributor

bors commented Jan 1, 2021

☀️ Try build successful - checks-actions
Build commit: 233a249a8282ce5b42fc70c2cce6537e0743dbf7 (233a249a8282ce5b42fc70c2cce6537e0743dbf7)

@rust-timer
Copy link
Collaborator

Queued 233a249a8282ce5b42fc70c2cce6537e0743dbf7 with parent 44e3daf, future comparison URL.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 1, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (233a249a8282ce5b42fc70c2cce6537e0743dbf7): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jan 1, 2021
@Manishearth
Copy link
Member Author

@bors rollup- r=kennytm

@bors
Copy link
Contributor

bors commented Jan 1, 2021

📌 Commit 375e7c5 has been approved by kennytm

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 1, 2021
@bors
Copy link
Contributor

bors commented Jan 1, 2021

⌛ Testing commit 375e7c5 with merge 18d27b2...

@bors
Copy link
Contributor

bors commented Jan 1, 2021

☀️ Test successful - checks-actions
Approved by: kennytm
Pushing 18d27b2 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 1, 2021
@bors bors merged commit 18d27b2 into rust-lang:master Jan 1, 2021
@rustbot rustbot added this to the 1.51.0 milestone Jan 1, 2021
@Manishearth Manishearth deleted the box-try-alloc branch January 1, 2021 17:15
@Manishearth Manishearth added the relnotes Marks issues that should be documented in the release notes of the next release. label Jan 2, 2021
@Manishearth
Copy link
Member Author

Marking as relnotes. This is nightly-only, and I'm not sure if we typically relnotes nightly features, but the part of the userbase particularly excited about this feature typically is already on nightly, so it might be worth announcing anyway.

@jonas-schievink jonas-schievink removed the relnotes Marks issues that should be documented in the release notes of the next release. label Feb 5, 2021
@jonas-schievink
Copy link
Contributor

Removing relnotes – the release notes are written and published for stable releases only, so it does not make sense to include nightly features there.

@Manishearth
Copy link
Member Author

@jonas-schievink ah, i wish there were a way to flag that something should be included in the blog post

@jonas-schievink
Copy link
Contributor

I don't recall us every mentioning unstable feature in the release blog post. You could write a dedicated blog post for this feature though!

@Manishearth
Copy link
Member Author

Hmm, good point. It seems minor enough that I probably won't bother, though.

bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 11, 2021
fix incorrect Box::from_raw_in doctest

Now that Miri can run doctests, I ran it on liballoc, and found exactly one problem: this test creates a `Box<u8>` to deallocate a 4-byte allocation!

Introduced by rust-lang#80310 so r? `@Manishearth` `@kennytm`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.