Set default move_size_limit to 4kB for large_assignments lint on nightly

[Enselic]

Only enable it by default in the nightly compiler. The limit can be changed on a per-crate basis with:

#![feature(large_assignments)]
#![move_size_limit = "8192"]

or with

-Zmove-size-limit=8192

Does the "enable the lint by default with a 4k threshold" step in #83518.

Tasks for this PR:

• Figure out how to write a test that tests that the stable compiler still has a default move_size_limit of 0. I suspect it is easy once you know the trick, but I haven't been able to figure out the trick yet.
• Handle perf regression.

Tasks after this PR:

• I found a bug where the lint is duplicated unnecessarily when generic instantiations are involved. See FIXME in the test. We should file an issue about it so we don't forget it. Or at least write a row in the tracking issue.
• Improve diagnostics so that ./x test library/alloc clearly says from where a large move into e.g. black_box() is made (when 39a42c3 is temporarily reverted)

r? @oli-obk who is E-mentor.

[rustbot]

These commits modify compiler targets.
(See the Target Tier Policy.)

[oli-obk]

Is this actually reported while compiling libcore? Or is this from a use of Cell::new with large types?

[Enselic]

Good point. It is the latter. Should be fixed now. Unfortunately the diagnostics does not tell from where the move is, so it was a bit tricky to figure out. For the black_box() case the diagnostic looks like this:

error: moving 65536 bytes
   --> /home/martin/src/rust/library/core/src/hint.rs:293:5
    |
293 |     crate::intrinsics::black_box(dummy)
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ value moved from here
    |
    = note: The current maximum size is 4096, but it can be customized with the move_size_limit attribute: `#![move_size_limit = "..."]`
    = note: `-D large-assignments` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(large_assignments)]`

error: moving 131072 bytes
   --> /home/martin/src/rust/library/core/src/hint.rs:293:5
    |
293 |     crate::intrinsics::black_box(dummy)
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ value moved from here
    |
    = note: The current maximum size is 4096, but it can be customized with the move_size_limit attribute: `#![move_size_limit = "..."]`

error: could not compile `alloc` (bench "collectionsbenches") due to 2 previous errors

I added a task that we should add to the tracking issue that we need to improve diagnostics.

[oli-obk]

Please change this in a separate commit so I can see the changes from the code changes separately from the test changes

[Enselic]

Absolutely. I have now split this PR up into these three commits:

• 2505a7d Add regression tests for new default move_size_limit = 4096
• a9cb350 Set default move_size_limit to 4kB for large_assignments lint on nightly
• 39a42c3 Mark #[allow(large_assignments)] for existing large moves

[oli-obk]

• I found a bug where the lint is duplicated unnecessarily when generic instantiations are involved. See FIXME in the test. We should file an issue about it so we don't forget it. Or at least write a row in the tracking issue.

oof yea, this is an issue. Maybe we should only emit the lint on items in the local crate.

[oli-obk]

@bors try @rust-timer queue

[bors]

⌛ Trying commit 84f8c26 with merge 9f87deb...

[bors]

☀️ Try build successful - checks-actions
Build commit: 9f87deb (9f87deb35baf9dcf79860829669d1204a0cd9f2a)

[bors]

☀️ Try build successful - checks-actions
Build commit: 9f87deb (9f87deb35baf9dcf79860829669d1204a0cd9f2a)

[Enselic]

Maybe we should only emit the lint on items in the local crate.

Just want to make sure I understand: In what way would that help? Here is an example where the lint is duplicated using only items from the local crate:

struct NotBox<const N: usize> {
    _data: [u8; N],
}

impl<const N: usize> NotBox<N> {
    fn new(data: [u8; N]) -> Self {
        Self { _data: data } // NOTE: Duplicated here three times: warning: moving 5000/6000/7000 bytes 
    }
}

fn main() {
    let _ = NotBox::new([0u8; 5000]);
    let _ = NotBox::new([0u8; 6000]);
    let _ = NotBox::new([0u8; 7000]);
}

[...]
warning: moving 5000 bytes
 --> /home/martin/src/bin/src/main.rs:7:9
  |
7 |         Self { _data: data }
  |         ^^^^^^^^^^^^^^^^^^^^ value moved from here
  |
  = note: The current maximum size is 4096, but it can be customized with the move_size_limit attribute: `#![move_size_limit = "..."]`

warning: moving 6000 bytes
 --> /home/martin/src/bin/src/main.rs:7:9
  |
7 |         Self { _data: data }
  |         ^^^^^^^^^^^^^^^^^^^^ value moved from here
  |
  = note: The current maximum size is 4096, but it can be customized with the move_size_limit attribute: `#![move_size_limit = "..."]`

warning: moving 7000 bytes
 --> /home/martin/src/bin/src/main.rs:7:9
  |
7 |         Self { _data: data }
  |         ^^^^^^^^^^^^^^^^^^^^ value moved from here
  |
  = note: The current maximum size is 4096, but it can be customized with the move_size_limit attribute: `#![move_size_limit = "..."]`
[...]

[rust-timer]

Finished benchmarking commit (9f87deb): 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)	3.0%	[0.4%, 7.2%]	49
Regressions ❌ (secondary)	3.9%	[0.8%, 9.0%]	16
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	3.0%	[0.4%, 7.2%]	49

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)	2.3%	[1.3%, 4.8%]	31
Regressions ❌ (secondary)	3.5%	[2.2%, 8.4%]	14
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.5%	[-1.5%, -1.5%]	1
All ❌✅ (primary)	2.3%	[1.3%, 4.8%]	31

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)	4.6%	[1.5%, 9.7%]	45
Regressions ❌ (secondary)	5.1%	[1.0%, 11.7%]	16
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	4.6%	[1.5%, 9.7%]	45

Binary size

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.1%	[0.0%, 0.2%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.1%	[0.0%, 0.2%]	2

Bootstrap: 630.224s -> 630.254s (0.00%)
Artifact size: 317.56 MiB -> 317.68 MiB (0.04%)

[Enselic]

The perf run does not support my claim of the compiler seemed "very slow". I now realized stage1 -> stage2 must be slow simply because I have enabled some debug options for the compiler. So it was a false alarm from me. That said, there is still a noticeable regression in performance, as you can see.

[oli-obk]

Just want to make sure I understand: In what way would that help? Here is an example where the lint is duplicated using only items from the local crate:

Mostly because you can't do anything about lints reported in your dependencies. You can't disable lints emitted in generic functions in libcore if they are only emitted there because of something you did locally.

Wrt the perf issue, it does look like it's all these new layout_of calls. I guess one thing that can be done is to first check whether the lint is enabled at all before doing any work to check if it should get emitted.

[Enselic]

Might take a while until this is redo for re-review, so:

@rustbot author

[bors]

☔ The latest upstream changes (presumably #116054) made this pull request unmergeable. Please resolve the merge conflicts.

[Enselic]

I think we're quite far off from being able to merge this PR, so I will close this PR for now. I plan on re-opening it and resume work on this once the timing is right. See the tracking issue for newly added tasks we probably should do before enabling large_assignments by default.