Fix compiler panic with a large number of threads

[practicalrs]

Hi,

This PR is an attempt to fix the problem described here #117638 using the solution suggested in this comment #117638 (comment)

Best regards,
Michal

[rustbot]

r? @jieyouxu

rustbot has assigned @jieyouxu.
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

[practicalrs]

CC @matthiaskrgr @nnethercote

[jieyouxu]

r? @SparrowLii since this is probably parallel frontend stuff

[SparrowLii]

That makes sense, thanks!
@bors r+ rollup

[bors]

📌 Commit 818099c has been approved by SparrowLii

It is now in the queue for this repository.

[fmease]

Please squash into one commit, this only touches 3 lines.
@bors r-

[practicalrs]

@fmease All good now?

[fmease]

Yes, thanks! @bors r=SparrowLii

[bors]

📌 Commit ef30dab has been approved by SparrowLii

It is now in the queue for this repository.

[klensy]

Please no?

This should have some warning when max value actually reached.

[practicalrs]

Please no?

This should have some warning when max value actually reached.

Hmm... maybe?

How to emit a warning for such a thing in the compiler?

[klensy]

Probably parsing can be left as it is, and actual max value checked here

rust/compiler/rustc_session/src/session.rs

Lines 819 to 821 in 298c746

	pub fn threads(&self) -> usize {
	self.opts.unstable_opts.threads
	}

and emit_warn here (or early_warn)? Probably someone will suggest correct method.

[fmease]

@bors r-

[practicalrs]

Probably parsing can be left as it is, and actual max value checked here

I would add this warning on parsing.

I think I just need to revert to the first version and add this warning in else.

@fmease @SparrowLii What do you think?

[klensy]

Better place:

rust/compiler/rustc_session/src/config.rs

Lines 2463 to 2465 in 298c746

	if unstable_opts.threads == 0 {
	early_dcx.early_fatal("value for threads must be a positive non-zero integer");
	}

[practicalrs]

Warning will lie for rustc -Zhtreads=0 if pc actually have > 256 cores.

So. Should I use std::thread::available_parallelism() to determinate the proper value?

[klensy]

Maybe apply min value after that match in parse_threads?

[practicalrs]

@klensy

Maybe this makes more sense?

[klensy]

More like

pub fn parse_threads(slot: &mut usize, v: Option<&str>) -> bool {
    let ret = match v.and_then(|s| s.parse().ok()) {
        Some(0) => {
            *slot = std::thread::available_parallelism().map_or(1, NonZero::<usize>::get);
            true
        }
        Some(i) => {
            *slot = i;
            true
        }
        None => false,
    };
    // We want to cap the number of threads here to avoid large numbers like 999999 and compiler panics.
    // This solution was suggested here https://github.com/rust-lang/rust/issues/117638#issuecomment-1800925067
    *slot = slot.clone().min(MAX_THREADS_CAP);
    ret
}

and left things in config.rs as in previous try with simple warning.

[practicalrs]

@klensy Done.

[klensy]

@rustbot review

for official reviewer

[SparrowLii]

@bors r+ rollup

[bors]

📌 Commit 7591eb6 has been approved by SparrowLii

It is now in the queue for this repository.

[bjoernager]

Wouldn't we want to be able to use more threads on future machines? I feel having this limit hard-coded could pose a problem, or maybe I'm missing something?

[practicalrs]

Wouldn't we want to be able to use more threads on future machines? I feel having this limit hard-coded could pose a problem, or maybe I'm missing something?

When you use it like -Zthreads=0, then you should have a value from std::thread::available_parallelism()

In one iteration of the PR I also tried to use it for Some(i) match, but @klensy suggested a different solution.

[klensy]

The way it was implemented, value was taken from available_parallelism() but still warned that it was capped at 256, even if it actually don't.

[practicalrs]

The way it was implemented, value was taken from available_parallelism() but still warned that it was capped at 256, even if it actually don't.

I tested it on my machine and it was capped properly to 6.

[practicalrs]

@SparrowLii @matthiaskrgr Can #117638 be closed now?