Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Detect number of CPU cores if nproc is installed #5477

Merged

Conversation

AlbertVeli
Copy link
Contributor

If nproc is not installed, use the old default 4.

I updated configure.ac and ran autotools to regenerate configure but only kept the nproc changes in
configure. Some other things were also changed but I removed the unrelated changes.

Maybe configure should be regenerated in total from configure.ac to be sure everything works as
intended. Which version of autoconf? I used 2.71 and the current was generated with 2.69.

@solardiz
Copy link
Member

Oh wow. I'm sorry I didn't comment yesterday. I feel that nproc is currently too obscure. Is it a very recent addition maybe? Where is it available at all? I've checked a few systems and none have it. Maybe we should also support grep -c ^processor /proc/cpuinfo. And I think AC_CHECK_TOOL is too heavy (produces too much shell code) for tools that we only use by configure itself. I'd hard-code maybe ~10 lines of shell code to check for these two ways of detecting CPU count and using one of them. This would also side-step the issue of autoconf version for a clean update - it'd be the exact same piece inserted into configure.ac and configure.

Also, we don't add more commits to a PR to fix-up errors in other commits in the same PR. We amend and force-push instead. And we prefix commit titles with subsystem name e.g. "configure: Detect number of logical CPUs, suggest it in make -j". The rest of the commit message shouldn't include discussion that wouldn't be relevant after the PR is merged.

Thank you!

@AlbertVeli AlbertVeli force-pushed the 5473-detect_number_of_cores branch from ddaac72 to 061b520 Compare May 16, 2024 06:49
@AlbertVeli
Copy link
Contributor Author

Thanks. I squashed the two commits, changed the commit message and force pushed. But it still uses nproc, I will try out the /proc method instead. Although that will only work on Linux. Maybe it's possible to find a way that works on macOS too. uname -s will return Linux or Darwin. If it's something else maybe just go with the default 4.

@solardiz
Copy link
Member

Thanks. I'm still unhappy about the use of AC_CHECK_TOOL, which I find too heavy for a tool only needed at configure time. But let's merge this anyway, and perhaps revise later.

@solardiz solardiz merged commit 2ccc0f1 into openwall:bleeding-jumbo May 16, 2024
31 of 32 checks passed
@claudioandre-br
Copy link
Member

claudioandre-br commented May 16, 2024

Two small observations about the PR in case any comments about this appear in the future.

It respects the value of NUM_OMP_THREADS:

$ OMP_NUM_THREADS=70 nproc
70

# E.g., inside CircleCI you'll see (if you don't set OMP_NUM_THREADS):
Configure finished.  Now "make -s clean && make -sj36" to compile.

The check occurs and appears after all other checks.

Target CPU ......................................... x86_64 AVX2, 64-bit LE
Fuzzing test ....................................... yes
[...]
Experimental code (default disabled) ............... no
ZTEX USB-FPGA module 1.15y support ................. no

checking for nproc... nproc
Install missing libraries to get any needed features that were omitted.

Configure finished.  Now "make -s clean && make -sj3" to compile.

@solardiz
Copy link
Member

I feel that nproc is currently too obscure. Is it a very recent addition maybe? Where is it available at all? I've checked a few systems and none have it.

I was wrong - it is indeed present on some of my systems. I don't know how I missed it before.

@solardiz
Copy link
Member

I think AC_CHECK_TOOL is too heavy (produces too much shell code) for tools that we only use by configure itself. I'd hard-code maybe ~10 lines of shell code to check for these two ways of detecting CPU count and using one of them. This would also side-step the issue of autoconf version for a clean update - it'd be the exact same piece inserted into configure.ac and configure.

I've implemented this (turned out to be just 3 lines) and am testing it now.

@solardiz
Copy link
Member

I wonder if we should also use this detected vCPU count in our CI setups. Maybe have configure create a shell script / one-liner with make -s clean && make -sj4 in it (with the correct number substituted) so that we could just run it?

@solardiz
Copy link
Member

I wonder if we should also use this detected vCPU count in our CI setups.

Actually, we already have similar logic in there, in .ci/run-build-and-tests.sh:

nproc="$(nproc)" || nproc=1
j="-j$nproc"

if [ $nproc -gt 2 ]; then
	export OMP_NUM_THREADS=2
fi

cd src
time ./configure $*
time make $j
time make $j check

@claudioandre-br
Copy link
Member

I'm afraid GitHub is offering 4 vCPUs to be used and the script is limiting "too much" at runtime. For no reason.

@solardiz
Copy link
Member

I'm afraid GitHub is offering 4 vCPUs to be used and the script is limiting "too much" at runtime. For no reason.

What do you mean? When nproc is present, the script will use all vCPUs for build. It does limit to 2 threads for OpenMP testing, but I think that's fine. It'd probably take almost twice longer to test with 4 OpenMP threads, because those 4 are likely just hardware threads in 2 physical cores.

@claudioandre-br
Copy link
Member

What do you mean?

It does limit to 2 threads for OpenMP testing

but I think that's fine.

Okay, I just didn't see any reason to spare hardware.

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.

3 participants