Add fallible Box, Arc, and Rc allocator APIs #80310

Manishearth · 2020-12-22T21:50:22Z

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 · 2020-12-22T21:50:25Z

r? @kennytm

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

Manishearth · 2020-12-22T21:51:17Z

library/alloc/src/boxed.rs

+ /// ```
+ #[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> {


Unclear if I should also be adding these

I have no objections

Manishearth · 2020-12-22T21:52:24Z

library/alloc/src/boxed.rs

+ // #[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();


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.

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 · 2020-12-22T22:03:30Z

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 · 2020-12-29T17:20:10Z

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 · 2020-12-29T22:23:39Z

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

Manishearth · 2020-12-31T08:31:31Z

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 · 2020-12-31T08:42:01Z

@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).

kennytm · 2020-12-31T10:19:05Z

library/alloc/src/rc.rs

+ ///
+ /// 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(


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

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

library/alloc/src/sync.rs

kennytm · 2020-12-31T16:42:15Z

@bors r+

bors · 2020-12-31T16:42:16Z

📌 Commit 8f3cb7d has been approved by kennytm

TimDiekmann · 2020-12-31T16:43:40Z

library/alloc/src/boxed.rs

+ ///
+ /// [zeroed]: mem::MaybeUninit::zeroed
+ #[unstable(feature = "allocator_api", issue = "32838")]
+ // #[unstable(feature = "new_uninit", issue = "63291")]


This should be #[inline]

TimDiekmann · 2020-12-31T16:43:57Z

library/alloc/src/boxed.rs

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


This should be #[inline]

TimDiekmann · 2020-12-31T16:44:58Z

library/alloc/src/boxed.rs

+ /// ```
+ /// #![feature(allocator_api, new_uninit)]
+ ///
+ ///


Suggested change

///

TimDiekmann · 2020-12-31T16:45:52Z

library/alloc/src/sync.rs

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


Suggested change

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

/// [zeroed]: mem::MaybeUninit::zeroed

bors · 2021-01-01T00:59:46Z

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

rust-log-analyzer · 2021-01-01T01:02:50Z

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

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

bors · 2021-01-01T02:00:18Z

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

rust-timer · 2021-01-01T02:00:20Z

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

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

rust-timer · 2021-01-01T03:37:23Z

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

Manishearth · 2021-01-01T08:47:40Z

@bors rollup- r=kennytm

bors · 2021-01-01T08:47:42Z

📌 Commit 375e7c5 has been approved by kennytm

bors · 2021-01-01T10:29:49Z

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

bors · 2021-01-01T13:17:35Z

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

Manishearth · 2021-01-02T07:38:01Z

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 · 2021-02-05T21:40:16Z

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 · 2021-02-05T22:05:16Z

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

jonas-schievink · 2021-02-05T22:08:28Z

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 · 2021-02-06T02:12:05Z

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

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`

rust-highfive assigned kennytm Dec 22, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 22, 2020

Manishearth force-pushed the box-try-alloc branch from 8a6e339 to 8ee2191 Compare December 22, 2020 21:50

Manishearth mentioned this pull request Dec 22, 2020

Tracking issue for try_reserve: RFC 2116 fallible collection allocation #48043

Open

5 tasks

Manishearth commented Dec 22, 2020

View reviewed changes

the8472 mentioned this pull request Dec 22, 2020

Publicly expose the allocated representation of Rc and Arc #80029

Closed

Manishearth force-pushed the box-try-alloc branch from 8ee2191 to 2f2e9c9 Compare December 23, 2020 04:59

kennytm approved these changes Dec 23, 2020

View reviewed changes

Manishearth added 3 commits December 31, 2020 08:14

Add fallible box allocator APIs (Box::try_new_*_in())

d116f48

Add fallible box APIs (Box::try_new_*)

dd2c6c3

Add fallible Rc APIs (Rc::try_new_*)

973fa8e

Manishearth changed the title ~~Add fallible box allocator APIs~~ Add fallible Box, Arc, and Rc allocator APIs Dec 31, 2020

Manishearth force-pushed the box-try-alloc branch from 2f2e9c9 to 2c1111f Compare December 31, 2020 08:30

Add fallible Arc APIs (Arc::try_new_*)

bb15fa1

Manishearth force-pushed the box-try-alloc branch from 2c1111f to bb15fa1 Compare December 31, 2020 08:32

Reuse Box::try_new_*_in() in Box::new_*_in()

589aa8e

kennytm reviewed Dec 31, 2020

View reviewed changes

Make [A]Rc::allocate_for_layout() use try_allocate_for_layout()

8f3cb7d

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

TimDiekmann suggested changes Dec 31, 2020

View reviewed changes

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

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 1, 2021

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

bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 1, 2021

bors merged commit 18d27b2 into rust-lang:master Jan 1, 2021

rustbot added this to the 1.51.0 milestone Jan 1, 2021

Manishearth deleted the box-try-alloc branch January 1, 2021 17:15

Manishearth added the relnotes Marks issues that should be documented in the release notes of the next release. label Jan 2, 2021

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

RalfJung mentioned this pull request Apr 10, 2021

fix incorrect Box::from_raw_in doctest #84053

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add fallible Box, Arc, and Rc allocator APIs #80310

Add fallible Box, Arc, and Rc allocator APIs #80310

Manishearth commented Dec 22, 2020

rust-highfive commented Dec 22, 2020

Manishearth Dec 22, 2020

Wodann Dec 24, 2020

Manishearth Dec 22, 2020

Wodann Dec 24, 2020

rust-log-analyzer commented Dec 22, 2020

ggriffiniii commented Dec 29, 2020

Manishearth commented Dec 29, 2020

Manishearth commented Dec 31, 2020

Manishearth commented Dec 31, 2020

kennytm Dec 31, 2020

Manishearth Dec 31, 2020

kennytm Dec 31, 2020

kennytm commented Dec 31, 2020

bors commented Dec 31, 2020

TimDiekmann Dec 31, 2020

TimDiekmann Dec 31, 2020

TimDiekmann Dec 31, 2020

TimDiekmann Dec 31, 2020

bors commented Jan 1, 2021

rust-log-analyzer commented Jan 1, 2021

bors commented Jan 1, 2021

rust-timer commented Jan 1, 2021

rust-timer commented Jan 1, 2021

Manishearth commented Jan 1, 2021

bors commented Jan 1, 2021

bors commented Jan 1, 2021

bors commented Jan 1, 2021

Manishearth commented Jan 2, 2021

jonas-schievink commented Feb 5, 2021

Manishearth commented Feb 5, 2021

jonas-schievink commented Feb 5, 2021

Manishearth commented Feb 6, 2021

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

Add fallible Box, Arc, and Rc allocator APIs #80310

Add fallible Box, Arc, and Rc allocator APIs #80310

Conversation

Manishearth commented Dec 22, 2020

rust-highfive commented Dec 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rust-log-analyzer commented Dec 22, 2020

ggriffiniii commented Dec 29, 2020

Manishearth commented Dec 29, 2020

Manishearth commented Dec 31, 2020

Manishearth commented Dec 31, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kennytm commented Dec 31, 2020

bors commented Dec 31, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Jan 1, 2021

rust-log-analyzer commented Jan 1, 2021

bors commented Jan 1, 2021

rust-timer commented Jan 1, 2021

rust-timer commented Jan 1, 2021

Manishearth commented Jan 1, 2021

bors commented Jan 1, 2021

bors commented Jan 1, 2021

bors commented Jan 1, 2021

Manishearth commented Jan 2, 2021

jonas-schievink commented Feb 5, 2021

Manishearth commented Feb 5, 2021

jonas-schievink commented Feb 5, 2021

Manishearth commented Feb 6, 2021