Skip running all lints if --cap-lints allow is used

[bjorn3]

They won't be able to emit any warnings or errors anyway.

cc #74704 and #106983

[rustbot]

r? @eholk

(rustbot has picked a reviewer for you, use r? to override)

[bjorn3]

@bors try @rust-timer queue

[bors]

⌛ Trying commit db97f09 with merge 30b22310fe398f1c51ae455ed5fa45eac79c2f38...

[matthiaskrgr]

This will not break something like first setting all lints to allow to then setting one particular lint to warn to only have findings of that particular lint, right?

[bjorn3]

No, this only affects --cap-lints which is used for non-local dependencies by cargo and overrides all lint level definitions in the source file. It seems that the --force-warn CLI arg takes precedent over --cap-lints, so I will need to run all lints anyway if --force-warn is used, but I figured I did first do a perf run before trying to implement that.

Edit: I think something like sess.opts.lint_opts.any(|(_, level)| matches!(level, Level::ForceWarn(_))) would work to check if --force-warn is used.

[bors]

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

[lqd]

I also went down a similar route recently (including only doing the optimization when no --force-warn options were passed), and overall it seemed this would not be enough without some restructuring of lints and passes, for the reasons below.

Let me know if my analysis was incorrect at the time.

They won't be able to emit any warnings or errors anyway.

Some lints are required to be emitted even on dependencies, independently of --force-warn and of --cap-lints=allow, so this is subtly wrong IMO: some future-incompatibility lints could be FutureReleaseErrorReportNow and they'd be ignored if we took this approach of early-returning on cap-lints allow. And since some lints can be multiplexed with other lints in arbitrary passes, we may need to restructure and statically prevent some lints from being ignored when cargo is allowing lints, while allowing/ignoring the others in general.

In the end, the time save was also very small, in addition to only being applicable when building deps, so I didn't pursue this much further. IIRC only a small number of rustc-perf benchmarks had dependencies with "significant" linting time (I can find that data if you want), and it was at best a few hundred ms save in an otherwise slow multi-second builds.

IIUC this experiment is also changing the default to get something out of a perf run, otherwise no --cap-lints=allow would be applied there, but even that is going to be unrealistic as only the crates depending on the profiled ones would see lints being turned off in the real world ? (including our bin benchmarks)

[bjorn3]

Some lints are required to be emitted even on dependencies, independently of --force-warn and of --cap-lints=allow, so this is subtly wrong IMO: some future-incompatibility lints could be FutureReleaseErrorReportNow and they'd be ignored if we took this approach of early-returning on cap-lints allow.

Right, didn't think of that.

In the end, the time save was also very small, in addition to only being applicable when building deps, so I didn't pursue this much further. IIRC only a small number of rustc-perf benchmarks had dependencies with "significant" linting time (I can find that data if you want), and it was at best a few hundred ms save in an otherwise slow multi-second builds.

Eyeballing -Zself-profile for libcore I estimate the save to be ~5%. Given that most changes I make to cg_clif require recompiling the standard library, this could save several seconds out of the ~40s I have to wait when making a change. But let's see what perf will tell us.

IIUC this experiment is also changing the default to get something out of a perf run, otherwise no --cap-lints=allow would be applied there, but even that is going to be unrealistic as only the crates depending on the profiled ones would see lints being turned off in the real world ? (including our bin benchmarks)

Correct. This change won't help with most incremental rebuilds, but it should help with from scratch builds.

[Kobzol]

I actually wonder if e.g. for binary benchmarks, we profile only the final/leaf bin crate, or if we also profile its (local) crate dependencies in the same workspace that it depends on 🤔

[rust-timer]

Finished benchmarking commit (30b22310fe398f1c51ae455ed5fa45eac79c2f38): comparison URL.

Overall result: ✅ improvements - 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 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)	-4.6%	[-13.4%, -0.2%]	209
Improvements ✅ (secondary)	-9.6%	[-98.9%, -0.4%]	204
All ❌✅ (primary)	-4.6%	[-13.4%, -0.2%]	209

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)	1.2%	[1.2%, 1.2%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-4.3%	[-12.3%, -0.9%]	168
Improvements ✅ (secondary)	-9.0%	[-65.7%, -1.1%]	138
All ❌✅ (primary)	-4.2%	[-12.3%, 1.2%]	169

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)	2.1%	[2.1%, 2.1%]	2
Improvements ✅ (primary)	-6.0%	[-14.2%, -1.0%]	163
Improvements ✅ (secondary)	-13.0%	[-98.6%, -2.1%]	153
All ❌✅ (primary)	-6.0%	[-14.2%, -1.0%]	163

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

Bootstrap: 653.19s -> 647.282s (-0.90%)

[lqd]

I think this may have removed more work than expected: possibly the need to typeck some items via the unused items lints, and CTFE allocation validation which is confusingly triggered by a lint. But maybe you wanted to focus at bootstrap times for crates depending on libcore ?

[apiraino]

I think @rustbot author

[bjorn3]

I don't intent on working on this again in the near future.