Set jobs default based on number of available CPUs #21
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
kienanstewart
force-pushed
the
cli-max_nproc_by_default
branch
from
0793ce4
to
c83ecb1
Compare
|
I wasn't sure about that since I just copied the default of GNU make, that is one job by default. But I guess it makes sense at a higher level, and it remains configurable. Buildroot does the same: |
eepp
requested changes
Sep 25, 2023
README.adoc
Outdated
| @@ -318,6 +318,9 @@ used to create the virtual environment. | |||
|
|
|||
| `vlttng` passes its `--jobs` (`-j`) option as is to `make`. | |||
|
|
|||
| The default number of jobs is dynamic, based on the length of | |||
| `os.sched_getaffinity()` for the current process. | |||
There was a problem hiding this comment.
The default value of the `--jobs` option is the number of CPUs on your
system.
vlttng/vlttng_cli.py
Outdated
| @@ -79,6 +79,13 @@ def _parse_args(): | |||
| return args | |||
|
|
|||
|
|
|||
| def _jobs_default(): | |||
There was a problem hiding this comment.
Just set a default_jobs variable once at the beginning of _parse_args() instead of calling it twice.
vlttng/vlttng_cli.py
Outdated
| @@ -43,7 +43,7 @@ def _parse_args(): | |||
| help='ignore project PROJECT (may be repeated)') | |||
| ap.add_argument('-j', '--jobs', nargs='?', const=None, metavar='JOBS', | |||
| action='store', type=int, | |||
| default=1, help='number of make jobs to run simultaneously') | |||
| default=_jobs_default(), help='number of make jobs to run simultaneously. Dynamic default: {}'.format(_jobs_default())) | |||
There was a problem hiding this comment.
number of make jobs to run simultaneously instead of {}
I noticed `vlttng_quick.py` uses `multiprocessing.cpu_count()`, but decided to use `len(os.sched_getaffinity(0))` to avoid importing `multiprocessing` in to the `vlttng.py` script. Signed-off-by: Kienan Stewart <kstewart@efficios.com>
kienanstewart
force-pushed
the
cli-max_nproc_by_default
branch
from
c83ecb1
to
fd5919b
Compare
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.
When invoking vlttng, I think it's more helpful to users to use the available system resources by building with a higher level of parallelism by default when more CPUs are available.
vlttng_quick.pysuggests a similar approach to users by prompting with a default based on the available cpu count.I noticed
vlttng_quick.pyusesmultiprocessing.cpu_count(), but decided to uselen(os.sched_getaffinity(0))to avoid importingmultiprocessingin to thevlttng.pyscript.