Rollup of 8 pull requests #106228
Merged
Rollup of 8 pull requests #106228
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There seem to be some scenarios where `cpu.cfs_period_us` can contain `0` This causes a panic when calling `std::thread::available_parallelism()` as is done so from binaries built by `cargo test`, which was how the issue was discovered. I don't feel like `0` is a good value for `cpu.cfs_period_us`, but I also don't think applications should panic if this value is seen. This case is handled by other projects which read this information: - num_cpus: https://github.com/seanmonstar/num_cpus/blob/e437b9d9083d717692e35d917de8674a7987dd06/src/linux.rs#L207-L210 - ninja: https://github.com/ninja-build/ninja/pull/2174/files - dotnet: https://github.com/dotnet/runtime/blob/c4341d45acca3ea662cd8d71e7d71094450dd045/src/coreclr/pal/src/misc/cgroup.cpp#L481-L483 Before this change, this panic could be seen in environments setup as described above: ``` $ RUST_BACKTRACE=1 cargo test Finished test [unoptimized + debuginfo] target(s) in 3.55s Running unittests src/main.rs (target/debug/deps/x-9a42e145aca2934d) thread 'main' panicked at 'attempt to divide by zero', library/std/src/sys/unix/thread.rs:546:70 stack backtrace: 0: rust_begin_unwind 1: core::panicking::panic_fmt 2: core::panicking::panic 3: std::sys::unix::thread::cgroups::quota 4: std::sys::unix::thread::available_parallelism 5: std::thread::available_parallelism 6: test::helpers::concurrency::get_concurrency 7: test::console::run_tests_console 8: test::test_main 9: test::test_main_static 10: x::main at ./src/main.rs:1:1 11: core::ops::function::FnOnce::call_once at /tmp/rust-1.64-1.64.0-1/library/core/src/ops/function.rs:248:5 note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace. error: test failed, to rerun pass '--bin local-rabmq-amqpprox' ``` I've tested this change in an environment which has the bad setup and rebuilding the test executable against a fixed std library fixes the panic.
There are OSs that always return the lowest free value. The algorithm in `lazy_init` always avoids keys with the sentinel value. In affected OSs, this means that each call to `lazy_init` will always request two keys from the OS and returns/frees the first one (with sentinel value) immediately afterwards. By making the sentinel value configurable, affected OSs can use a different value than zero to prevent this performance issue.
Previously, we used spans. This was not good. Sometimes, the span of the token that failed to match may come from a position later in the file which has been transcribed into a token stream way earlier in the file. If precisely this token fails to match, we think that it was the best match because its span is so high, even though other arms might have gotten further in the token stream. We now try to properly use the location in the token stream.
As discussed on rust-lang#105688, this makes x fmt only format modified files
Makes it clear from catch_unwind docs that the panic hook will be called before the panic is caught.
Move `ReentrantMutex` to `std::sync` If I understand rust-lang#84187 correctly, `sys_common` should not contain platform-independent code, even if it is private.
…u-se available_parallelism: Gracefully handle zero value cfs_period_us There seem to be some scenarios where the cgroup cpu quota field `cpu.cfs_period_us` can contain `0`. This field is used to determine the "amount" of parallelism suggested by the function `std::thread::available_parallelism` A zero value of this field cause a panic when `available_parallelism()` is invoked. This issue was detected by the call from binaries built by `cargo test`. I really don't feel like `0` is a good value for `cpu.cfs_period_us`, but I also don't think applications should panic if this value is seen. This panic started happening with rust 1.64.0. This case is gracefully handled by other projects which read this information: [num_cpus](https://github.com/seanmonstar/num_cpus/blob/e437b9d9083d717692e35d917de8674a7987dd06/src/linux.rs#L207-L210), [ninja](https://github.com/ninja-build/ninja/pull/2174/files), [dotnet](https://github.com/dotnet/runtime/blob/c4341d45acca3ea662cd8d71e7d71094450dd045/src/coreclr/pal/src/misc/cgroup.cpp#L481-L483) Before this change, running `cargo test` in environments configured as described above would trigger this panic: ``` $ RUST_BACKTRACE=1 cargo test Finished test [unoptimized + debuginfo] target(s) in 3.55s Running unittests src/main.rs (target/debug/deps/x-9a42e145aca2934d) thread 'main' panicked at 'attempt to divide by zero', library/std/src/sys/unix/thread.rs:546:70 stack backtrace: 0: rust_begin_unwind 1: core::panicking::panic_fmt 2: core::panicking::panic 3: std::sys::unix::thread::cgroups::quota 4: std::sys::unix::thread::available_parallelism 5: std::thread::available_parallelism 6: test::helpers::concurrency::get_concurrency 7: test::console::run_tests_console 8: test::test_main 9: test::test_main_static 10: x::main at ./src/main.rs:1:1 11: core::ops::function::FnOnce::call_once at /tmp/rust-1.64-1.64.0-1/library/core/src/ops/function.rs:248:5 note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace. error: test failed, to rerun pass '--bin x' ``` I've tested this change in an environment which has the bad (questionable?) setup and rebuilding the test executable against a fixed std library fixes the panic.
…value, r=m-ou-se Make sentinel value configurable in `library/std/src/sys_common/thread_local_key.rs` This is an excerpt of a changeset for the QNX/Neutrino OS. To make the patch for QNX smaller and easier to review, I've extracted this change (which is OS independent). I would be surprised if no other OS is also affected. All this patch does is to define a `const` for a sentinel value instead of using it directly at several places. There are OSs that always return the lowest free value. The algorithm in `lazy_init` always avoids keys with the sentinel value. In affected OSs, this means that each call to `lazy_init` will always request two keys from the OS and returns/frees the first one (with sentinel value) immediately afterwards. By making the sentinel value configurable, affected OSs can use a different value than zero to prevent this performance issue. On QNX/Neutrino, it is planned to use a different sentinel value: ```rust // Define a sentinel value that is unlikely to be returned // as a TLS key (but it may be returned). #[cfg(not(target_os = "nto"))] const KEY_SENTVAL: usize = 0; // On QNX/Neutrino, 0 is always returned when currently not in use. // Using 0 would mean to always create two keys and remote the first // one (with value of 0) immediately afterwards. #[cfg(target_os = "nto")] const KEY_SENTVAL: usize = libc::PTHREAD_KEYS_MAX + 1; ``` It seems like no other OS defines `PTHREAD_KEYS_MAX` in Rusts libc, but `limits.h` on unix systems does.
…catch-unwind, r=m-ou-se Clarify `catch_unwind` docs about panic hooks Makes it clear from `catch_unwind` docs that the panic hook will be called before the panic is caught. Fixes rust-lang#105432
…ompiler-errors Properly calculate best failure in macro matching Previously, we used spans. This was not good. Sometimes, the span of the token that failed to match may come from a position later in the file which has been transcribed into a token stream way earlier in the file. If precisely this token fails to match, we think that it was the best match because its span is so high, even though other arms might have gotten further in the token stream. We now try to properly use the location in the token stream. This needs a little cleanup as the `best_failure` field is getting out of hand but it should be mostly good to go. I hope I didn't violate too many abstraction boundaries..
Format only modified files As discussed on rust-lang#105688, this makes x fmt only format modified files. Fixes rust-lang#105688
…omcc adjust message on non-unwinding panic "thread panicked while panicking" is just plain wrong in case this is a non-unwinding panic, such as - a panic out of a `nounwind` function - the sanity checks we have in `mem::uninitialized` and `mem::zeroed` - the optional debug assertion in various unsafe std library functions
…Mark-Simulacrum Iterator::find: link to Iterator::position in docs for discoverability
rustbot
added
T-bootstrap
|
@bors r+ rollup=never p=8 |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
📌 Perf builds for each rolled up PR: previous master: 270c94e484 In the case of a perf regression, run the following command for each PR you suspect might be the cause: |
This was referenced Dec 29, 2022
|
Finished benchmarking commit (9709a43): comparison URL. Overall result: ❌ regressions - ACTION NEEDEDNext Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis 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.
CyclesThis benchmark run did not return any relevant results for this metric. |
|
@rust-timer build c88ca476fdc92309cea647f6276356ca25b63477 |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (c88ca476fdc92309cea647f6276356ca25b63477): comparison URL. Overall result: ❌ regressions - ACTION NEEDEDBenchmarking 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 @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis 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.
CyclesResultsThis 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.
|
|
@Nilstrieb @compiler-errors FYI: the small performance regressions seem to be caused by #105570 (not sure why the summary is not showing but the perf run above is finished). |
|
This is indeed likely to be caused by my PR, probably because |
|
Opened #106416 which should get the perf back (and maybe even go beyond!) |
|
Indeed looks like the performance was gained back. Awesome stuff! @rustbot label perf-regression-triaged |
rustbot
added
the
perf-regression-triaged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Successful merges:
ReentrantMutextostd::sync#104402 (MoveReentrantMutextostd::sync)library/std/src/sys_common/thread_local_key.rs#105359 (Make sentinel value configurable inlibrary/std/src/sys_common/thread_local_key.rs)catch_unwinddocs about panic hooks #105497 (Clarifycatch_unwinddocs about panic hooks)Failed merges:
r? @ghost
@rustbot modify labels: rollup
Create a similar rollup