available_parallelism: Add documentation for why we don't look at ulimit
#144188
Conversation
|
rustbot has assigned @Mark-Simulacrum. Use |
rustbot
added
S-waiting-on-review
| /// - It does not attempt to take `ulimit` into account. If there is a limit set on the number of | ||
| /// threads, `available_parallelism` cannot know how much of that limit a Rust program should | ||
| /// take, or know in a reliable and race-free way how much of that limit is already taken. |
There was a problem hiding this comment.
🤔 If we did check ulimit, and made a guess based on it, getting it wrong and exceeding the value that is correct would just get the process killed anyways, right?
There was a problem hiding this comment.
I'd expect an error returned from thread::spawn, not getting killed (well, from the spawn builder, I think thread spawn would just panic). But, yes, I think that's right.
There was a problem hiding this comment.
I think the distinction is deeper. ulimit restricts a different but related resource.
available_parallelism is essentially about how much cpu-time we get, not threads. It's just that users then turn around and use that to calculate the number of threads to make use of that cpu-time and oversubscription will be managed via sharing. This is quite obvious once you have multiple threadpools in a rust program independently using available_parallelism to size themselves (e.g. rayon + tokio).
ulimit is how many threads one may have globally, in total, even idle ones, and no oversubscription allowed.
Intended and existing usage of available_parallelism can't handle that discrepancy. It's not like runtimes check available_parallelism every time before they spawn a thread.
There was a problem hiding this comment.
Right, that makes sense. So we might as well encounter whatever fate awaits us when we exceed the ulimit.
There was a problem hiding this comment.
@joshtriplett I think this revision is probably fine as-is if Mark likes it.
But would it make more sense to jump off the8472's remark and note that the only truly "race-free" way (because it must be enforced by OS-level concurrency limits) to determine whether spawning a thread is possible within the OS-defined limits is to try to actually spawn the thread? Otherwise it's a TOCTOU problem, so it's true it simply does not make any attempt to guess. And then underscore that the value returned is not a guarantee that you can successfully spawn that many threads, merely that spawning that many threads may be useful.
There was a problem hiding this comment.
I was trying to say that I don't think not checking ulimit is really a limitation of the current implementation. It's out of scope.
We're not going to check if spawning a thread would run into memory or vma exhaustion either.
Maybe it'd make sense to list this in a more general "what this method isn't" clarification paragraph.
There was a problem hiding this comment.
right, I probably didn't capture what you said correctly since it's a Nuance.
mostly I am agreeing in the direction of "what you said is correct, and in that sense this doesn't need to be a Linux-specific note".
rustbot
removed
the
S-waiting-on-review
rustbot
added
the
S-waiting-on-review
|
@bors r+ rollup |
bors
added
S-waiting-on-bors
Rollup of 12 pull requests Successful merges: - #142205 (Mark `slice::swap_with_slice` unstably const) - #144188 (`available_parallelism`: Add documentation for why we don't look at `ulimit`) - #144322 (Add lint against dangling pointers from local variables) - #144497 (tests: Add test for basic line-by-line stepping in a debugger) - #144559 (Enable extract-insert-dyn.rs test on RISC-V (riscv64)) - #144667 (`AlignmentEnum` should just be `repr(usize)` now) - #144706 (Do not give function allocations alignment in consteval and Miri.) - #144746 (resolve: Cleanups and micro-optimizations to extern prelude) - #144785 (Regression test for LLVM error with unsupported expression in static initializer for const pointer in array on macOS.) - #144811 (Stylize `*-lynxos178-*` target maintainer handle to make it easier to copy/paste) - #144848 (For "stage 1" ui-fulldeps, use the stage 1 compiler to query target info) - #144853 (Remove unnecessary `rust_` prefixes) Failed merges: - #144794 (Port `#[coroutine]` to the new attribute system) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #144188 - joshtriplett:available-parallelism, r=Mark-Simulacrum `available_parallelism`: Add documentation for why we don't look at `ulimit`
|
Bors is confused. @bors r- |
bors
added
S-waiting-on-author
Rollup of 12 pull requests Successful merges: - rust-lang/rust#142205 (Mark `slice::swap_with_slice` unstably const) - rust-lang/rust#144188 (`available_parallelism`: Add documentation for why we don't look at `ulimit`) - rust-lang/rust#144322 (Add lint against dangling pointers from local variables) - rust-lang/rust#144497 (tests: Add test for basic line-by-line stepping in a debugger) - rust-lang/rust#144559 (Enable extract-insert-dyn.rs test on RISC-V (riscv64)) - rust-lang/rust#144667 (`AlignmentEnum` should just be `repr(usize)` now) - rust-lang/rust#144706 (Do not give function allocations alignment in consteval and Miri.) - rust-lang/rust#144746 (resolve: Cleanups and micro-optimizations to extern prelude) - rust-lang/rust#144785 (Regression test for LLVM error with unsupported expression in static initializer for const pointer in array on macOS.) - rust-lang/rust#144811 (Stylize `*-lynxos178-*` target maintainer handle to make it easier to copy/paste) - rust-lang/rust#144848 (For "stage 1" ui-fulldeps, use the stage 1 compiler to query target info) - rust-lang/rust#144853 (Remove unnecessary `rust_` prefixes) Failed merges: - rust-lang/rust#144794 (Port `#[coroutine]` to the new attribute system) r? `@ghost` `@rustbot` modify labels: rollup
…, r=Mark-Simulacrum `available_parallelism`: Add documentation for why we don't look at `ulimit`
No description provided.