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

Remove or document uses of #[rustc_box] in library #108476

Merged
merged 1 commit into from
Mar 1, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions library/alloc/src/boxed.rs
Original file line number Diff line number Diff line change
Expand Up @@ -283,9 +283,7 @@ impl<T> Box<T> {
#[must_use]
#[inline(always)]
pub fn pin(x: T) -> Pin<Box<T>> {
(#[rustc_box]
Box::new(x))
.into()
Box::new(x).into()
Copy link
Member Author

Choose a reason for hiding this comment

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

In an unoptimized build, T already needs to be on the stack, so this change shouldn't inhibit any in-place heap initialization.

}

/// Allocates memory on the heap then places `x` into it,
Expand Down Expand Up @@ -1242,8 +1240,8 @@ unsafe impl<#[may_dangle] T: ?Sized, A: Allocator> Drop for Box<T, A> {
#[stable(feature = "rust1", since = "1.0.0")]
impl<T: Default> Default for Box<T> {
/// Creates a `Box<T>`, with the `Default` value for T.
#[inline]
fn default() -> Self {
#[rustc_box]
Copy link
Member Author

Choose a reason for hiding this comment

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

The "T is already a function argument" justification doesn't apply here.

But what does matter is that T is the return value for the T::default() call. So this use is slightly more exciting than the other cases.

Initially, I was concerned that this might require #[rustc_box]. But in a totally unoptimized build, you still cannot Box::default a T which is more than half your stack size: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=e4322542645a0afe6c2644511b838e5c

One possible complaint is that this halves the size of the T that can be defaulted, because now we have to pass it through two function calls. That is true. But MIR optimizations can also halve the possible size of T, if T::default() is inlined into Box::default. We don't do MIR inlining currently at -Copt-level=0, but my point is that anyone relying on the size of a T they can default with this function is already lined up for trouble outside of this PR.

Box::new(T::default())
}
}
Expand All @@ -1252,6 +1250,7 @@ impl<T: Default> Default for Box<T> {
#[stable(feature = "rust1", since = "1.0.0")]
#[rustc_const_unstable(feature = "const_default_impls", issue = "87864")]
impl<T> const Default for Box<[T]> {
#[inline]
fn default() -> Self {
let ptr: Unique<[T]> = Unique::<[T; 0]>::dangling();
Box(ptr, Global)
Expand All @@ -1262,6 +1261,7 @@ impl<T> const Default for Box<[T]> {
#[stable(feature = "default_box_extra", since = "1.17.0")]
#[rustc_const_unstable(feature = "const_default_impls", issue = "87864")]
impl const Default for Box<str> {
#[inline]
fn default() -> Self {
// SAFETY: This is the same as `Unique::cast<U>` but with an unsized `U = str`.
let ptr: Unique<str> = unsafe {
Expand Down Expand Up @@ -1616,7 +1616,6 @@ impl<T, const N: usize> From<[T; N]> for Box<[T]> {
/// println!("{boxed:?}");
/// ```
fn from(array: [T; N]) -> Box<[T]> {
#[rustc_box]
Box::new(array)
Comment on lines 1618 to 1619
Copy link
Member Author

Choose a reason for hiding this comment

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

Same as Box::pin, in an unoptimized build, T already needs to be on the stack, so this change shouldn't inhibit any in-place heap initialization.

}
}
Expand Down
2 changes: 2 additions & 0 deletions library/alloc/src/macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,8 @@ macro_rules! vec {
);
($($x:expr),+ $(,)?) => (
$crate::__rust_force_expr!(<[_]>::into_vec(
// This rustc_box is not required, but it produces a dramatic improvement in compile
// time when constructing arrays with many elements.
#[rustc_box]
$crate::boxed::Box::new([$($x),+])
saethlin marked this conversation as resolved.
Show resolved Hide resolved
))
Expand Down
5 changes: 1 addition & 4 deletions library/alloc/src/vec/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3131,10 +3131,7 @@ impl<T, const N: usize> From<[T; N]> for Vec<T> {
/// ```
#[cfg(not(test))]
fn from(s: [T; N]) -> Vec<T> {
<[T]>::into_vec(
#[rustc_box]
Box::new(s),
)
<[T]>::into_vec(Box::new(s))
Copy link
Member Author

Choose a reason for hiding this comment

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

Same as other cases; in an unoptimized build, T already needs to be on the stack, so this change shouldn't inhibit any in-place heap initialization.

}

#[cfg(test)]
Expand Down