Reduce default 'large array' threshold

[GnomedDev]

As-is this threshold is 512kb, but as #9449 points out this is way too high for most people to consider sensible (why would you want to copy 256kb of data around on the stack or duplicate it via const) and didn't get any discussion when originally added. This PR reduces it the threshold to 1kb, which is higher than the issue says ("a few cpu words") but helps out for actual codebases.

While reducing this, I found that large_stack_arrays was triggering for statically promoted arrays in constants/statics, so I also fixed that up as seen in the difference to array_size_threshold.stderr.

Closes #9449.

changelog: [large_stack_arrays]: No longer triggers in static/const context
changelog: [large_const_arrays]: Changed the default of [array-size-threshold] from 512kb to 16kb

[rustbot]

r? @xFrednet

rustbot has assigned @xFrednet.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[workingjubilee]

I would like to suggest a default of 64KiB, 16KiB, or 4KiB instead. This is because those are precisely the allocation granularity for certain memory mappings and anyone who is copying data structures of those size around is prooobably doing it on purpose? Doubly so if they are actually aligned to that value.

Also there are architectures where values in the range 2KiB to 64KiB are in fact possible sizes for single registers.

[GnomedDev]

large_stack_arrays is pedantic, so there is an expectation of some false lints, which is what you are saying could happen with people using large locals on purpose for good reasons, so I don't think reducing the value helps for that.

large_const_arrays is perf, but I don't think this affects this as you can easily swap to static and explicitly copy into a local for that case.

What do you think? I agree that the 1kib number may be a little low, but the specific context here makes me think this should be kept as in this PR.

[workingjubilee]

Hmm. Other architectural trivia that you may wish to consider: 32KiB, 16KiB or 8KiB as a limit also might make sense because many CPUs, even 32-bit ones, have around 32KiB as their L1 data cache size. That's true for x86 CPUs since the Pentium M, and the ever-popular-to-use-as-reference Raspberry Pi has it at 16KiB for the first one up to 64KiB on recent ones. So anything larger than 16KiB can't be copied without hitting the L2 cache on a lot of CPUs.

Basically: copies of a few KiB are still relatively cheap on most application processors, and even very low-power application processors like the Pi are still massively beefier than an embedded MCU in this regard. Usually if something is only applicable to a few projects it's a restriction lint, and aiui the value for this lint can be altered. So even if pedantic, it still seems sensible to try to put it at a default value where most people would feel concerned.

In particular, the stdlib uses around 8KiB as a value for all its on-stack arrays, too, for most targets.

It might even be worth considering a target-specific default?

[workingjubilee]

fwiw I think it's obvious we should drop it below 512KB that's just kinda "what"

[GnomedDev]

I'm good with the a 16kib default, that's a number that makes my brain happy. I'll swap that over in a bit.

@rustbot author

[GnomedDev]

Just bumped it up to 16kib @workingjubilee

@rustbot ready

[xFrednet]

The changes look good to me. 16kb sounds reasonable, if people need a lower value, they can still change the default. It would be cool to have configuration templates, for example for embedded development. But that's an idea and a totally different topic :D

Thank you for the changes! And also thank you @workingjubilee for the input.

---

Roses are red,
Violets are blue,
Both less than 512kb
But the lint triggers now

[bors]

📌 Commit 995eb95 has been approved by xFrednet

It is now in the queue for this repository.

[bors]

⌛ Testing commit 995eb95 with merge 2d2e8cc...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: xFrednet
Pushing 2d2e8cc to master...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: xFrednet
Pushing 2d2e8cc to master...

[bors]

👀 Test was successful, but fast-forwarding failed: 422 Changes must be made through a pull request.

[flip1995]

This change breaks the Clippy sync when testing the parallel compiler: rust-lang/rust#131892

IIUC: With this being a static, it must implement Sync, but since Span doesn't implement Sync this also doesn't implement sync.

[GnomedDev]

Okay, for now we can revert back to const and add an allow if it fires. Longer term we should look at why Span isn't Sync.

[flip1995]

Done in rust-lang/rust@cf918d5