Added support for negative --jobs parameter, counting backwards from max CPUs. #9221

orlp · 2021-03-01T16:10:00Z

Closes #9217.
Closes #9219.

…max CPUs.

rust-highfive · 2021-03-01T16:10:03Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

alexcrichton · 2021-03-02T21:01:40Z

Thanks for the PR! This seems like a neat little feature which doesn't really have a ton of impact on Cargo itself in terms of overall design. One thing I'm curious about, though, is if you're aware of any precedent for this? We try to mirror the -j semantics of build tools like ninja/make, but I'm not familiar with any other build tools that have this sort of interpretation of the -j flag.

orlp · 2021-03-02T21:31:25Z

@alexcrichton

I know ninja defined -j 0 as 'unlimited parallelism', but doesn't support -j < 0. I'm not personally aware of any other tools with a similar parameter that support a negative number.

alexcrichton · 2021-03-03T15:14:28Z

Ah ok thanks for the clarification. Again this seems like a nice feature, but I'm a bit wary blazing the trail here when very established other build systems haven't implemented something like this.

We discussed this at the Cargo meeting yesterday and one worry was that -j -1 can be ambiguous if you don't already know what it means. We weren't entirely sure how to smooth over that, but it's also somewhat of a minor concern. One question we did have, did you envision this being passed on the CLI or through configuration more frequently? We could presumably make a more verbose syntax in .cargo/config.toml with inline tables and such rather than just a simple integer.

orlp · 2021-03-03T19:17:41Z

@alexcrichton I envision both being used, honestly. I don't know which would be more frequent. I am honestly not that familiar with .cargo/config.toml, so I'll refrain from commenting on how its config should look like.

One worry was that -j -1 can be ambiguous if you don't already know what it means

Yeah I would prefer it if there was at least a mention of it in the help.

-j, --jobs <N>                   Number of parallel jobs, defaults to # of CPUs

I tried but couldn't think of a good short unambiguous description that fits in ~70 cols (the longest line in the options currently).

Either way, even if someone assumed that -j -1 would mean 'all processors' (the only other reasonable interpretation IMO), they're only off by 1 at the worst, so really not the end of the world.

alexcrichton · 2021-03-05T22:22:42Z

src/cargo/core/compiler/build_config.rs

-        let jobs = jobs.or(cfg.jobs).unwrap_or(::num_cpus::get() as u32);
+        let jobs = match jobs.or(cfg.jobs) {
+            None => ::num_cpus::get() as u32,
+            Some(j) if j < 0 => (::num_cpus::get() as i32 + j).max(1) as u32,


I think I'd personally prefer that this return an error if the result is less than one rather than clamping to one

@alexcrichton I think it's better if not, suppose that a build script contains -j -2 but is then run on a dual-core machine, now your build breaks. Also, -j 1000 doesn't give an error either.

Hm this is something I'm worried about though in that it's sort of hidden behavior. Using -j100 when you have 2 cpus just means "spawn up to 100 things at the same time" which is always possible, you're basically asking the kernel to do tons of scheduling for you. If the request ends up being "spawn at most -N" tasks I'm not sure Cargo can reasonbly handle that and I would prefer we be conservative and return an error.

If in the future, though, the script case is common enough and the error is just getting in everyone's way we could change it to clamp to 1

alexcrichton · 2021-03-05T22:24:05Z

Ok this seems reasonable enough to me to go ahead and merge. We'll be trailblazers here in that there's not really any precedent for this, but I don't think that's the end of the world.

@rfcbot fcp merge

rfcbot · 2021-03-05T22:24:06Z

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

docs (Added support for negative --jobs parameter, counting backwards from max CPUs. #9221 (comment))

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

ehuss · 2021-03-08T16:13:44Z

@rfcbot concern docs

This needs some documentation updates. Can you make the following updates?

https://github.com/rust-lang/cargo/blob/master/src/doc/man/includes/options-jobs.md
- And then go to src/doc and run ./build-man.sh to rebuild the man pages (docs here)
I think a mention in the config docs would also be good.

orlp · 2021-03-08T16:20:07Z

@ehuss The config docs I have no problem with, I can do that. I did try to think of a way to specify the command line doc but I could not find a concise wording I was happy with. My concern is that the command line docs are generally very terse and the best description I could come up with was still significantly longer than all others.

So suggestions are definitely welcome.

ehuss · 2021-03-10T15:41:27Z

I wouldn't worry too much about the clap CLI help string. Maybe something like (negative is relative to max), but I would also be fine with not adding anything. Up to you!

bors · 2021-04-16T22:44:53Z

☔ The latest upstream changes (presumably #9367) made this pull request unmergeable. Please resolve the merge conflicts.

ehuss · 2021-07-01T00:54:07Z

Ping @orlp, are you still interested in moving forward with this? I think the documentation was the only thing that needed updating.

dtolnay · 2021-10-09T23:01:14Z

How feasible would it be to accept a very basic expression grammar for -j? As in -j cpus-2 or -j 'cpus / 2', or possibly -j n-1 / -j N-1. I think I would find this more appealing and self explanatory for readers than a negative number.

ehuss · 2021-12-14T17:52:03Z

I'm going to close due to inactivity. We are still interested in some solution here. Feel free to reopen if you have a chance to work on it again.

Support for negative --jobs parameter, counting backwards from max CPUs Fixes #9217. Continuation of #9221.

Added support for negative --jobs parameter, counting backwards from …

07cc897

…max CPUs.

rust-highfive assigned alexcrichton Mar 1, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 1, 2021

alexcrichton reviewed Mar 5, 2021

View reviewed changes

alexcrichton added the T-cargo Team: Cargo label Mar 5, 2021

rfcbot added proposed-final-comment-period An FCP proposal has started, but not yet signed off. disposition-merge FCP with intent to merge labels Mar 5, 2021

Eh2406 mentioned this pull request Mar 10, 2021

Introduce 'nice' value under cargo.toml -> [build] #9250

Open

ehuss added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 20, 2021

ehuss closed this Dec 14, 2021

rfcbot removed proposed-final-comment-period An FCP proposal has started, but not yet signed off. disposition-merge FCP with intent to merge labels Dec 14, 2021

yerke mentioned this pull request Jul 10, 2022

Support for negative --jobs parameter, counting backwards from max CPUs #10844

Merged

epage mentioned this pull request Jul 11, 2022

Allow a negative number of jobs to count from maximum number of cores #9217

Closed

bors added a commit that referenced this pull request Aug 1, 2022

Auto merge of #10844 - yerke:yerke/negative_jobs, r=ehuss

223e84a

Support for negative --jobs parameter, counting backwards from max CPUs Fixes #9217. Continuation of #9221.

weihanglo mentioned this pull request May 27, 2023

Support a "default" option for build.jobs #11816

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for negative --jobs parameter, counting backwards from max CPUs. #9221

Added support for negative --jobs parameter, counting backwards from max CPUs. #9221

orlp commented Mar 1, 2021 •

edited by ehuss

Loading

rust-highfive commented Mar 1, 2021

alexcrichton commented Mar 2, 2021

orlp commented Mar 2, 2021

alexcrichton commented Mar 3, 2021

orlp commented Mar 3, 2021 •

edited

Loading

alexcrichton Mar 5, 2021

orlp Mar 5, 2021 •

edited

Loading

alexcrichton Mar 8, 2021

alexcrichton commented Mar 5, 2021

rfcbot commented Mar 5, 2021 •

edited

Loading

ehuss commented Mar 8, 2021

orlp commented Mar 8, 2021

ehuss commented Mar 10, 2021

bors commented Apr 16, 2021

ehuss commented Jul 1, 2021

dtolnay commented Oct 9, 2021

ehuss commented Dec 14, 2021

Added support for negative --jobs parameter, counting backwards from max CPUs. #9221

Added support for negative --jobs parameter, counting backwards from max CPUs. #9221

Conversation

orlp commented Mar 1, 2021 • edited by ehuss Loading

rust-highfive commented Mar 1, 2021

alexcrichton commented Mar 2, 2021

orlp commented Mar 2, 2021

alexcrichton commented Mar 3, 2021

orlp commented Mar 3, 2021 • edited Loading

alexcrichton Mar 5, 2021

Choose a reason for hiding this comment

orlp Mar 5, 2021 • edited Loading

Choose a reason for hiding this comment

alexcrichton Mar 8, 2021

Choose a reason for hiding this comment

alexcrichton commented Mar 5, 2021

rfcbot commented Mar 5, 2021 • edited Loading

ehuss commented Mar 8, 2021

orlp commented Mar 8, 2021

ehuss commented Mar 10, 2021

bors commented Apr 16, 2021

ehuss commented Jul 1, 2021

dtolnay commented Oct 9, 2021

ehuss commented Dec 14, 2021

orlp commented Mar 1, 2021 •

edited by ehuss

Loading

orlp commented Mar 3, 2021 •

edited

Loading

orlp Mar 5, 2021 •

edited

Loading

rfcbot commented Mar 5, 2021 •

edited

Loading