Skip to content

nproc: simplify core count#12753

Open
oech3 wants to merge 1 commit into
uutils:mainfrom
oech3:nproc-1
Open

nproc: simplify core count#12753
oech3 wants to merge 1 commit into
uutils:mainfrom
oech3:nproc-1

Conversation

@oech3

@oech3 oech3 commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@oech3 oech3 marked this pull request as ready for review June 10, 2026 08:46
@codspeed-hq

codspeed-hq Bot commented Jun 10, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 147 untouched benchmarks
⏩ 222 skipped benchmarks1


Comparing oech3:nproc-1 (ca70c50) with main (4223537)

Open in CodSpeed

Footnotes

  1. 222 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@max-amb max-amb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this at risk of an underflow? num_cpus_all() returns a usize, so if we subtract a usize from a usize we risk underflow. It may be completely mitigated by the .max() call but the compiler doesn't like it:

fn main() {
    let x: usize = 5;
    dbg!((x-6).max(1));
}

fails to compile.

Could use a saturating_sub?

@oech3

oech3 commented Jun 10, 2026

Copy link
Copy Markdown
Contributor Author

Available on 64-bit only.

@max-amb

max-amb commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Ah yeah, could cast to a signed type, or I think the original is the only solution. wdyt? Maybe something more obvious is

    let initial_cores = std::cmp::min(limit, cores);
    let cores = if initial_cores <= ignore {
        1
    } else {
        initial_cores - ignore
    }

@oech3

This comment was marked as low quality.

@max-amb

max-amb commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

But will it not panic?

@oech3

oech3 commented Jun 10, 2026

Copy link
Copy Markdown
Contributor Author

.max(1) is clear enough for me

@max-amb

max-amb commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

When compiled in release it will wrap and we get the wrapped number as it is bigger? I'm not too familiar with this though.

https://doc.rust-lang.org/book/ch03-02-data-types.html#integer-overflow

It panics in debug mode

@oech3 oech3 force-pushed the nproc-1 branch 2 times, most recently from 9f93a38 to 4331c2a Compare June 10, 2026 11:40
@oech3

oech3 commented Jun 10, 2026

Copy link
Copy Markdown
Contributor Author

It seems working on 32bit too.

@github-actions

Copy link
Copy Markdown

GNU testsuite comparison:

Skip an intermittent issue tests/tail/symlink (fails in this run but passes in the 'main' branch)
Skip an intermittent issue tests/tail/tail-n0f (fails in this run but passes in the 'main' branch)
Skipping an intermittent issue tests/date/date-locale-hour (passes in this run but fails in the 'main' branch)
Skipping an intermittent issue tests/misc/tty-eof (passes in this run but fails in the 'main' branch)
Note: The gnu test tests/cut/cut-huge-range is now being skipped but was previously passing.

sylvestre added a commit to sylvestre/rust-clippy that referenced this pull request Jun 10, 2026
Add two `complexity` lints that detect a single-sided clamp written as an
`if`/`else` (or a guarding `if`) and suggest `Ord::max` / `Ord::min`:

    let _ = if a < b { b } else { a };  // -> a.max(b)
    if cores < b { cores = b; }         // -> cores = cores.max(b);

This is the sound, generalizable form of the manual clamp simplified by
hand in uutils/coreutils#12753 (`nproc`). Unlike `manual_clamp`, which
only fires when both a lower and an upper bound are applied, these catch
the common single-bound floor/ceiling case.

Restricted to `Ord` types so float `NaN` semantics are not changed, and
emitted as `MaybeIncorrect` since the branching form re-evaluates the
selected operand.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants