Non-deterministic overflow in rustc-rayon-core causing CI failures

[hkratz]

It seems that enabling overflow checks (#89776) uncovered non-deterministic overflows in rustc-rayon-core causing the build failures of #90042 and #90222:

• https://github.com/rust-lang-ci/rust/runs/3987259810?check_suite_focus=true#step:25:26550
• https://github.com/rust-lang-ci/rust/runs/3987471878?check_suite_focus=true#step:25:21315

Overflow location:
https://github.com/rust-lang/rustc-rayon/blob/c8ec88d8a2236d1fe19d65a4ab38834f76d256b7/rayon-core/src/sleep/mod.rs#L330

This code is not in upstream rayon but just in our fork, introduced here:
rust-lang/rustc-rayon@27911f7#diff-cde9d726ca2f32c319420be6c61d2e57ad9daff7f85a5f8f7be25474a15f9c4dR328

[hkratz]

@rustbot label +T-compiler +A-parallel-queries

[hkratz]

Interestingly enough the errors only appeared on Windows, in rustdoc and in builds which don't have the parallel queries enabled. So I guess Rustdocs use of rustc-rayon triggers it?

rust/src/librustdoc/docfs.rs

Lines 66 to 77 in d1d8145

	if !self.sync_only && cfg!(windows) {
	// A possible future enhancement after more detailed profiling would
	// be to create the file sync so errors are reported eagerly.
	let sender = self.errors.clone().expect("can't write after closing");
	rayon::spawn(move || {
	fs::write(&path, contents).unwrap_or_else(|e| {
	sender.send(format!("\"{}\": {}", path.display(), e)).unwrap_or_else(|_| {
	panic!("failed to send error on \"{}\"", path.display())
	})
	});
	});
	} else {

[camelid]

cc @jyn514 since IIRC you were looking into the DocFS code at some point recently

[jyn514]

I don't have any insight here, I think this just happens to be the only part of the compiler using rayon when cfg(parallel_compiler) is disabled.

[Mark-Simulacrum]

Hm... I would suggest that rustdoc probably wants to use mainline rayon here, rather than rustc's fork of it, since I/O code likely doesn't want to interop with jobserver and such. I don't know whether that will fix this particular issue, but it seems like a good idea.

[jyn514]

@Mark-Simulacrum what jobserver are you referring to here? I don't know what the differences between mainline rayon and rustc's fork are.

[Mark-Simulacrum]

The fork is primarily intended to interface with the jobserver that Cargo creates (though I don't know that rustdoc ever configures it as such) to ensure that rayon's worker threads are not using more than the allocated share of CPU resources. This is the same jobserver that manages e.g. LLVM parallelism within each rustc compilation, for example, to ensure there's at most "ncpu" threads running at the same time.

For I/O heavy work like this, I'd expect that you don't want that - your threads are mostly blocked on I/O completion which is not CPU-heavy for the most part.

[the8472]

Hm... I would suggest that rustdoc probably wants to use mainline rayon here, rather than rustc's fork of it, since I/O code likely doesn't want to interop with jobserver and such. I don't know whether that will fix this particular issue, but it seems like a good idea.

Is that really the right solution? It seems like the overflow checks have found a bug in rustc_rayon which was exposed by the use in rustdoc. Removing the use in rustdoc does not remove the fix additions in the custom rayon version.

Isn't the goal of adding overflow checks to uncover latent bugs and then fix them rather than finding new ways to ignore them?

[Mark-Simulacrum]

We should still try to fix the bug, absolutely, and investigate whether it manifests in upstream rayon as well. However, I don't think that changes the assessment that relying on upstream rayon is likely better than the fork in the long run for rustdoc's use case.

[camelid]

Is that really the right solution? It seems like the overflow checks have found a bug in rustc_rayon which was exposed by the use in rustdoc. Removing the use in rustdoc does not remove the fix additions in the custom rayon version.

Yeah, we switched to mainline rayon mostly for other reasons. That's why I left this issue open :)

[saethlin]

@hkratz Please paste the contents of links in issues instead of just providing a URL. Those links to CI logs now dead, so the only hint now about how to reproduce this is

on Windows, in rustdoc