Re: [PATCH] support for accessing CPU/core count (processor-count)

[Omar Polo]

Arthur Miller <arthur.miller@live.com> writes:

> Omar Polo <op@omarpolo.com> writes:
>
>> Campbell Barton <ideasman42@gmail.com> writes:
>>
>>> Hi, this patch adds support for accessing the number of CPU's / cores
>>> on a system, matching CPython's multiprocessing.cpu_count() [0].
>>>
>>> I've only tested this for Linux, this includes code that should work
>>> on other platforms, although that would need to be double-checked of
>>> course.
>>> For reference I checked CPython / Blender & Stack-overflow [1]
>>
>> I can confirm it works on OpenBSD too, but needs tweaking.  Some
>> comments inline and attaching an updated patch
>>
>>> Accessing this information can be useful to automatically detect the
>>> number of jobs to run.
>>>
>>>
>>> [0]:
>>> https://docs.python.org/3/library/multiprocessing.html#multiprocessing.cpu_count
>>> [1]: https://stackoverflow.com/a/3006416/432509
>>>
>>> commit 7be53f10f3df3c3183cc97d6bbadb78ebb61e8d2
>>> Author: Campbell Barton <ideasman42@gmail.com>
>>> Date:   Sun Oct 10 10:16:47 2021 +1100
>>> 
>>>     Support accessing the number of CPU's.
>>> 
>>>     Add (processor-count) for accessing the number of cores/CPU's.
>>> 
>>> diff --git a/src/emacs.c b/src/emacs.c
>>> index 866e43fda9..26e2f6b1f2 100644
>>> --- a/src/emacs.c
>>> +++ b/src/emacs.c
>>> @@ -3156,6 +3156,38 @@ DEFUN ("daemon-initialized", Fdaemon_initialized, 
>>> Sdaemon_initialized, 0, 0, 0,
>>>    return Qt;
>>>  }
>>> 
>>> +DEFUN ("processor-count", Fprocessor_count, Sprocessor_count, 0, 0, 0,
>>> +       doc: /* Return the number of CPUs in the system.
>>> +
>>> +The value will always be above zero, 1 for unsupported systems.  */)
>>> +  (void)
>>> +{
>>> +  int nproc = -1;
>>> +#ifdef WINDOWSNT
>>> +  SYSTEM_INFO info;
>>> +  GetSystemInfo(&info);
>>> +  nproc = (int)info.dwNumberOfProcessors;
>>> +#elif defined (__APPLE__) || \
>>> +      defined (__OpenBSD__) || \
>>> +      defined (__FreeBSD__) || \
>>> +      defined (__NetBSD__) || \
>>> +      defined (__DragonFly__)
>>> +  int mib[2];
>>> +  size_t len;
>>> +
>>> +  mib[0] = CTL_HW;
>>
>> the #include <sys/sysctl.h> is missing.
>>
>>> +  mib[1] = HW_NCPU;
>>
>> at least on OpenBSD this should be HW_NCPUONLINE.
>>
>> OpenBSD disables hyperthreading by default so HW_NCPU is (almost) always
>> misleading.  For example, on my machine
>>
>>      % uname -a
>>      OpenBSD venera 7.0 GENERIC.MP#221 amd64
>>      % sysctl hw.ncpu
>>      hw.ncpu=8
>>      % sysctl hw.ncpuonline
>>      hw.ncpuonline=4
>>
>> and this has been the case for a while already (I mean, a couple of
>> years if not more.)
>>
>> I don't have access to other BSDs other than OpenBSD, but judging from
>> the manpages online.
>>
>>  - NetBSD has hw.ncpuonline
>>  - DragonFly and FreeBSD have only hw.ncpu
>>  - apple I don't know how to check.  man.apple.com doesn't seem to
>>    exists ^^"
>>
>>> +  len = sizeof(nproc);
>>> +  sysctl(mib, 2, &nproc, &len, nullptr, 0);
>>                                   ^^^^^^^
>> shouldn't this be NULL?
>>
>>> +#elif defined (__hpux)
>>> +  nproc = mpctl(MPC_GETNUMSPUS, NULL, NULL);
>>> +#elif defined (_SC_NPROCESSORS_ONLN)
>>> +  nproc = (int)sysconf(_SC_NPROCESSORS_ONLN);
>>> +#endif
>>> +
>>> +  return make_fixnum (MAX(nproc, 1));
>>
>> emacs.c:3188:23: warning: implicit declaration of function 'MAX' is invalid 
>> in C99 [-Wimplicit-function-declaration]
>>
>> I'm attaching an updated patch.  Note that I'm not sure if including
>> sys/types.h and sys/sysctl.h breaks the build on some OS.
>>
>> if someone knows how to reduce the number of #ifdefs I'll be glad :)
>>
>>> +}
>>> +
>>
>> Cheers,
>>
>> Omar Polo
>>
>> From 5fe26a73abc60eae7dd2528e3854f2f379bcb4c7 Mon Sep 17 00:00:00 2001
>> From: Campbell Barton <ideasman42@gmail.com>
>> Date: Sun, 10 Oct 2021 08:21:01 +0000
>> Subject: [PATCH] add function to access the number of CPU
>>
>> * emacs.c (Fprocessor_count): add processor-count function to access
>>   the number of CPU
>> ---
>>  src/emacs.c | 35 +++++++++++++++++++++++++++++++++++
>>  1 file changed, 35 insertions(+)
>>
>> diff --git a/src/emacs.c b/src/emacs.c
>> index 866e43fda9..5bd10a7586 100644
>> --- a/src/emacs.c
>> +++ b/src/emacs.c
>> @@ -25,6 +25,8 @@ along with GNU Emacs.  If not, see 
>> <https://www.gnu.org/licenses/>.  */
>>  #include <fcntl.h>
>>  #include <stdlib.h>
>>  
>> +#include <sys/types.h>
>> +#include <sys/sysctl.h>
>>  #include <sys/file.h>
>>  #include <sys/stat.h>
>>  #include <unistd.h>
>> @@ -3156,6 +3158,38 @@ from the parent process and its tty file descriptors. 
>>  */)
>>    return Qt;
>>  }
>>  
>> +DEFUN ("processor-count", Fprocessor_count, Sprocessor_count, 0, 0, 0,
>> +       doc: /* Return the number of CPUs in the system.
>> +
>> +The value will always be above zero, 1 for unsupported systems.  */)
>> +  (void)
>> +{
>> +  int nproc = -1;
>> +#ifdef WINDOWSNT
>> +  SYSTEM_INFO info;
>> +  GetSystemInfo(&info);
>> +  nproc = (int)info.dwNumberOfProcessors;
>> +#elif defined (HW_NCPU) || defined (HW_NCPUONLINE)
>> +  int mib[2];
>> +  size_t len;
>> +
>> +  mib[0] = CTL_HW;
>> +#ifdef HW_NCPUONLINE
>> +  mib[1] = HW_NCPUONLINE;
>> +#else
>> +  mib[1] = HW_NCPU;
>> +#endif
>> +  len = sizeof(nproc);
>> +  sysctl(mib, 2, &nproc, &len, NULL, 0);
>> +#elif defined (__hpux)
>> +  nproc = mpctl(MPC_GETNUMSPUS, NULL, NULL);
>> +#elif defined (_SC_NPROCESSORS_ONLN)
>> +  nproc = (int)sysconf(_SC_NPROCESSORS_ONLN);
>> +#endif
>> +
>> +  return make_fixnum (nproc > 1 ? nproc : 1);
>> +}
>> +
>>  void
>>  syms_of_emacs (void)
>>  {
>> @@ -3176,6 +3210,7 @@ syms_of_emacs (void)
>>    defsubr (&Sinvocation_directory);
>>    defsubr (&Sdaemonp);
>>    defsubr (&Sdaemon_initialized);
>> +  defsubr (&Sprocessor_count);
>>  
>>    DEFVAR_LISP ("command-line-args", Vcommand_line_args,
>>             doc: /* Args passed by shell to Emacs, as a list of strings.
>
> What is wrong on just reading this from /cat/cpuinfo on GNU/Linux?
>
> Windows has "wmic cpu get NumberOfCores,NumberOfLogicalProcessors" which works
> on cmd prompt, which means executable from elisp as a process. I dont know for
> apple and *bsds.
>
> It is just text that can be parsed directly with elisp, so it is easier to
> maintain does not need to be compiled etc. I don't see cpu queries as a
> performance crucial query. Just a thought.

You're right

        syscstl -n hw.ncpuonline

would do it.  I guess that a rationale for doing this in C would be to
make it easier on the elisp side to use it.  We can try to support every
OS here instead of leaving the burden on the elisp package authors.

(the same thing applies to proced.  You could implement it by parsing ps
output I guess, but there are elisp API that calls into C for that.)

But I don't know, I don't really have strong opinions.  I've read a bit
of code that didn't look right and tried to improve it :)