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

saethlin · 2023-02-26T01:07:51Z

Only one of these uses is tested for in the rustc-perf benchmark suite. The impact there on compile time is somewhat dramatic, but I am inclined to make this change as a simplification to the library and wait for people to complain if it explodes their compilation time. I think in the absence of data or reports from users about what code paths really matter, if we are optimizing for compilation time, it's hard to argue against using #[rustc_box] everywhere we currently call Box::new.

saethlin · 2023-02-26T01:08:01Z

@bors try @rust-timer queue

bors · 2023-02-26T01:08:10Z

⌛ Trying commit 589cb6e35f32b2da7c206f5cd54aeff261bb2c7b with merge e3337189d45e02992e5cd2be28ad35470f90f10c...

bors · 2023-02-26T03:38:13Z

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

rust-timer · 2023-02-26T05:21:54Z

Finished benchmarking commit (e3337189d45e02992e5cd2be28ad35470f90f10c): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	37.8%	[3.0%, 85.4%]	12
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.2%	[-0.2%, -0.2%]	1
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	3.5%	[3.4%, 3.7%]	2
Regressions ❌ (secondary)	42.5%	[3.3%, 62.8%]	10
Improvements ✅ (primary)	-2.9%	[-2.9%, -2.9%]	1
Improvements ✅ (secondary)	-2.6%	[-2.6%, -2.6%]	1
All ❌✅ (primary)	1.4%	[-2.9%, 3.7%]	3

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	70.3%	[30.0%, 111.1%]	8
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

saethlin · 2023-02-26T23:16:57Z

@bors try @rust-timer queue

bors · 2023-02-26T23:17:05Z

⌛ Trying commit 34198eef64f920b0a0c2b72302765972c5586930 with merge 56d7174232da03c5cf3a5e13f90d40c09121fbdf...

bors · 2023-02-27T02:04:18Z

☀️ Try build successful - checks-actions
Build commit: 56d7174232da03c5cf3a5e13f90d40c09121fbdf (56d7174232da03c5cf3a5e13f90d40c09121fbdf)

rust-timer · 2023-02-27T06:12:13Z

Finished benchmarking commit (56d7174232da03c5cf3a5e13f90d40c09121fbdf): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

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

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.2%	[2.2%, 2.2%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.8%	[-4.2%, -3.3%]	2
All ❌✅ (primary)	-	-	0

Cycles

This benchmark run did not return any relevant results for this metric.

library/alloc/src/macros.rs

saethlin · 2023-02-28T23:23:12Z

library/alloc/src/boxed.rs

-        (#[rustc_box]
-        Box::new(x))
-        .into()
+        Box::new(x).into()


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

saethlin · 2023-02-28T23:23:27Z

library/alloc/src/boxed.rs

    fn from(array: [T; N]) -> Box<[T]> {
-        #[rustc_box]
        Box::new(array)


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.

saethlin · 2023-02-28T23:23:54Z

library/alloc/src/vec/mod.rs

-        <[T]>::into_vec(
-            #[rustc_box]
-            Box::new(s),
-        )
+        <[T]>::into_vec(Box::new(s))


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.

saethlin · 2023-02-28T23:31:34Z

library/alloc/src/boxed.rs

    fn default() -> Self {
-        #[rustc_box]


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.

rustbot · 2023-02-28T23:43:57Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

thomcc · 2023-02-28T23:48:37Z

This looks fine to me. Thanks for working through it!

@bors r+

bors · 2023-02-28T23:48:39Z

📌 Commit 5448123 has been approved by thomcc

It is now in the queue for this repository.

bors · 2023-03-01T09:13:27Z

⌛ Testing commit 5448123 with merge 64165aa...

bors · 2023-03-01T12:16:20Z

☀️ Test successful - checks-actions
Approved by: thomcc
Pushing 64165aa to master...

rust-timer · 2023-03-01T13:34:38Z

Finished benchmarking commit (64165aa): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.6%	[-0.7%, -0.5%]	4
Improvements ✅ (secondary)	-0.3%	[-0.3%, -0.3%]	3
All ❌✅ (primary)	-0.6%	[-0.7%, -0.5%]	4

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.4%	[2.0%, 4.8%]	2
Improvements ✅ (primary)	-2.6%	[-3.9%, -1.3%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.6%	[-3.9%, -1.3%]	2

Cycles

This benchmark run did not return any relevant results for this metric.

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Feb 26, 2023

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 26, 2023

This comment has been minimized.

Sign in to view

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Feb 26, 2023

saethlin force-pushed the remove-library-rustc-box branch from 589cb6e to 34198ee Compare February 26, 2023 23:15

saethlin changed the title ~~Remove #[rustc_box] from everywhere but Box::new~~ Remove or document uses of #[rustc_box] in library Feb 26, 2023

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 26, 2023

This comment has been minimized.

Sign in to view

rustbot removed S-waiting-on-perf Status: Waiting on a perf run to be completed. perf-regression Performance regression. labels Feb 27, 2023

joboet reviewed Feb 27, 2023

View reviewed changes

library/alloc/src/macros.rs Show resolved Hide resolved

Remove or justify use of #[rustc_box]

5448123

saethlin force-pushed the remove-library-rustc-box branch from 34198ee to 5448123 Compare February 28, 2023 23:21

saethlin commented Feb 28, 2023

View reviewed changes

rustbot assigned thomcc Feb 28, 2023

saethlin marked this pull request as ready for review February 28, 2023 23:43

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 Feb 28, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 1, 2023

bors merged commit 64165aa into rust-lang:master Mar 1, 2023

rustbot added this to the 1.69.0 milestone Mar 1, 2023

This was referenced Mar 1, 2023

Remove box_syntax #108471

Merged

Restrict #[rustc_box] to Box::new calls #108516

Merged

saethlin deleted the remove-library-rustc-box branch March 15, 2023 00:33

est31 mentioned this pull request May 2, 2023

Moving #[rustc_box] to move_val_init intrinsic #110715

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

saethlin commented Feb 26, 2023 •

edited

Loading

saethlin commented Feb 26, 2023

This comment has been minimized.

bors commented Feb 26, 2023

This comment has been minimized.

bors commented Feb 26, 2023

This comment has been minimized.

rust-timer commented Feb 26, 2023

saethlin commented Feb 26, 2023

This comment has been minimized.

bors commented Feb 26, 2023

This comment has been minimized.

bors commented Feb 27, 2023

This comment has been minimized.

rust-timer commented Feb 27, 2023

saethlin Feb 28, 2023

saethlin Feb 28, 2023

saethlin Feb 28, 2023

saethlin Feb 28, 2023

rustbot commented Feb 28, 2023

thomcc commented Feb 28, 2023

bors commented Feb 28, 2023

bors commented Mar 1, 2023

bors commented Mar 1, 2023

rust-timer commented Mar 1, 2023

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

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

Conversation

saethlin commented Feb 26, 2023 • edited Loading

saethlin commented Feb 26, 2023

This comment has been minimized.

bors commented Feb 26, 2023

This comment has been minimized.

bors commented Feb 26, 2023

This comment has been minimized.

rust-timer commented Feb 26, 2023

Overall result: ❌ regressions - ACTION NEEDED

saethlin commented Feb 26, 2023

This comment has been minimized.

bors commented Feb 26, 2023

This comment has been minimized.

bors commented Feb 27, 2023

This comment has been minimized.

rust-timer commented Feb 27, 2023

Overall result: no relevant changes - no action needed

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

rustbot commented Feb 28, 2023

thomcc commented Feb 28, 2023

bors commented Feb 28, 2023

bors commented Mar 1, 2023

bors commented Mar 1, 2023

rust-timer commented Mar 1, 2023

Overall result: ✅ improvements - no action needed

saethlin commented Feb 26, 2023 •

edited

Loading