diff mbox

implementation of std::thread::hardware_concurrency()

Message ID CAMPTgK0dyW0uP03vmOJ4N53vk0NwbFOgirGFqwz7wD2SFZhE_A@mail.gmail.com
State New
Headers show

Commit Message

niXman Nov. 1, 2011, 11:33 a.m. UTC 
Rechecked.





2011/11/1 Paolo Carlini <pcarlini@gmail.com>:
> Hi,
>
>> This is patch is implement the std::thread::hardware_concurrency().
>> Tested on pthreads-win32/winpthreads on windows OS, and on Linux/FreeBSD.
>
> Please send library patches to the library mailing list too. Also, always parch mainline first: actually in the latter the function is alread implemented, maybe something is missing for win32, please check, rediff, and resend.
>
> Thanks
> Paolo

Comments

Paolo Carlini Nov. 1, 2011, 11:47 a.m. UTC | #1 
On 11/01/2011 12:33 PM, niXman wrote:
> Rechecked.
Stylistically, you are missing a lot of spaces around the operators, eg:

     return (count > 0) ? count : 0;

also, patches are always submitted with a ChangeLog entry.

Do you have already a Copyright assignment in place? I'm asking in 
general, for your future submissions, this specific patch probably would 
be small enough to not require it.

Paolo.
Marc Glisse Nov. 1, 2011, 11:54 a.m. UTC | #2 
On Tue, 1 Nov 2011, niXman wrote:

> diff --git a/libstdc++-v3/src/thread.cc b/libstdc++-v3/src/thread.cc
> index 09e7fc5..6feda4d 100644
> --- a/libstdc++-v3/src/thread.cc
> +++ b/libstdc++-v3/src/thread.cc
> @@ -112,10 +112,20 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>   unsigned int
>   thread::hardware_concurrency() noexcept
>   {
> -    int __n = _GLIBCXX_NPROCS;
> -    if (__n < 0)
> -      __n = 0;
> -    return __n;
> +    int count=0;
> +#if defined(PTW32_VERSION) || \
> +   (defined(__MINGW64_VERSION_MAJOR) && defined(_POSIX_THREADS)) || \
> +   defined(__hpux)
> +    count=pthread_num_processors_np();
> +#elif defined(__APPLE__) || defined(__FreeBSD__)
> +    size_t size=sizeof(count);
> +    sysctlbyname("hw.ncpu", &count, &size, NULL, 0);
> +#elif defined(_SC_NPROCESSORS_ONLN)
> +    count=sysconf(_SC_NPROCESSORS_ONLN);
> +#elif defined(_GLIBCXX_USE_GET_NPROCS)
> +    count=_GLIBCXX_NPROCS;
> +#endif
> +    return (count>0)?count:0;

Er, the macro _GLIBCXX_NPROCS already handles the case 
sysconf(_SC_NPROCESSORS_ONLN). It looks like you actually want to remove 
the macro _GLIBCXX_NPROCS completely.
Jonathan Wakely Nov. 1, 2011, 3:18 p.m. UTC | #3 
On 1 November 2011 11:54, Marc Glisse wrote:
> On Tue, 1 Nov 2011, niXman wrote:
>
>> diff --git a/libstdc++-v3/src/thread.cc b/libstdc++-v3/src/thread.cc
>> index 09e7fc5..6feda4d 100644
>> --- a/libstdc++-v3/src/thread.cc
>> +++ b/libstdc++-v3/src/thread.cc
>> @@ -112,10 +112,20 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>>  unsigned int
>>  thread::hardware_concurrency() noexcept
>>  {
>> -    int __n = _GLIBCXX_NPROCS;
>> -    if (__n < 0)
>> -      __n = 0;
>> -    return __n;
>> +    int count=0;
>> +#if defined(PTW32_VERSION) || \
>> +   (defined(__MINGW64_VERSION_MAJOR) && defined(_POSIX_THREADS)) || \
>> +   defined(__hpux)
>> +    count=pthread_num_processors_np();
>> +#elif defined(__APPLE__) || defined(__FreeBSD__)
>> +    size_t size=sizeof(count);
>> +    sysctlbyname("hw.ncpu", &count, &size, NULL, 0);
>> +#elif defined(_SC_NPROCESSORS_ONLN)
>> +    count=sysconf(_SC_NPROCESSORS_ONLN);
>> +#elif defined(_GLIBCXX_USE_GET_NPROCS)
>> +    count=_GLIBCXX_NPROCS;
>> +#endif
>> +    return (count>0)?count:0;
>
> Er, the macro _GLIBCXX_NPROCS already handles the case
> sysconf(_SC_NPROCESSORS_ONLN). It looks like you actually want to remove the
> macro _GLIBCXX_NPROCS completely.

Right, I already handled the case of using sysconf.  I'm going to veto
this patch in its current form - please check how it works now before
changing this code.

_GLIBCXX_NPROCS should be made to call pthread_num_processors_np() for
mingw or HPUX.
niXman Nov. 1, 2011, 3:40 p.m. UTC | #4 
With what exactly do you don't accept this patch?


2011/11/1 Jonathan Wakely <jwakely.gcc@gmail.com>:
> On 1 November 2011 11:54, Marc Glisse wrote:
>> On Tue, 1 Nov 2011, niXman wrote:
>>
>>> diff --git a/libstdc++-v3/src/thread.cc b/libstdc++-v3/src/thread.cc
>>> index 09e7fc5..6feda4d 100644
>>> --- a/libstdc++-v3/src/thread.cc
>>> +++ b/libstdc++-v3/src/thread.cc
>>> @@ -112,10 +112,20 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>>>  unsigned int
>>>  thread::hardware_concurrency() noexcept
>>>  {
>>> -    int __n = _GLIBCXX_NPROCS;
>>> -    if (__n < 0)
>>> -      __n = 0;
>>> -    return __n;
>>> +    int count=0;
>>> +#if defined(PTW32_VERSION) || \
>>> +   (defined(__MINGW64_VERSION_MAJOR) && defined(_POSIX_THREADS)) || \
>>> +   defined(__hpux)
>>> +    count=pthread_num_processors_np();
>>> +#elif defined(__APPLE__) || defined(__FreeBSD__)
>>> +    size_t size=sizeof(count);
>>> +    sysctlbyname("hw.ncpu", &count, &size, NULL, 0);
>>> +#elif defined(_SC_NPROCESSORS_ONLN)
>>> +    count=sysconf(_SC_NPROCESSORS_ONLN);
>>> +#elif defined(_GLIBCXX_USE_GET_NPROCS)
>>> +    count=_GLIBCXX_NPROCS;
>>> +#endif
>>> +    return (count>0)?count:0;
>>
>> Er, the macro _GLIBCXX_NPROCS already handles the case
>> sysconf(_SC_NPROCESSORS_ONLN). It looks like you actually want to remove the
>> macro _GLIBCXX_NPROCS completely.
>
> Right, I already handled the case of using sysconf.  I'm going to veto
> this patch in its current form - please check how it works now before
> changing this code.
>
> _GLIBCXX_NPROCS should be made to call pthread_num_processors_np() for
> mingw or HPUX.
>
Jonathan Wakely Nov. 1, 2011, 3:47 p.m. UTC | #5 
I've put gcc-patches@ back in the CC list and removed gcc@


On 1 November 2011 15:35, niXman wrote:
>> Er, the macro _GLIBCXX_NPROCS already handles
>> the case sysconf(_SC_NPROCESSORS_ONLN).
>> It looks like you actually want to remove the macro
>> _GLIBCXX_NPROCS completely.
>
> Fixed.

No, this still isn't acceptable.

I do not want to see preprocessor tests like

+#elif defined(__APPLE__) || defined(__FreeBSD__)

in the body of get_thread::hardware_concurrency(), the configure
script should determine what is available on the platform and set an
appropriate macro.

Look at the definition of _GLIBCXX_NPROCS and adjust that to do

#define _GLIBCXX_NPROCS pthread_num_processors_np()

for the relevant platforms.

For the platforms using sysctlbyname there could be an inline function
that calls it, and _GLIBCXX_NPROCS could be defined to call that, so
that thread::hardware_concurrency() can still be defined as it is
today.

Please read the code you're changing and understand how it works today
before making changes.
Jonathan Wakely Nov. 1, 2011, 3:52 p.m. UTC | #6 
On 1 November 2011 15:47, Jonathan Wakely wrote:
> I've put gcc-patches@ back in the CC list and removed gcc@

Oops, *now* I've removed gcc@


> On 1 November 2011 15:35, niXman wrote:
>>> Er, the macro _GLIBCXX_NPROCS already handles
>>> the case sysconf(_SC_NPROCESSORS_ONLN).
>>> It looks like you actually want to remove the macro
>>> _GLIBCXX_NPROCS completely.
>>
>> Fixed.
>
> No, this still isn't acceptable.
>
> I do not want to see preprocessor tests like
>
> +#elif defined(__APPLE__) || defined(__FreeBSD__)
>
> in the body of get_thread::hardware_concurrency(), the configure
> script should determine what is available on the platform and set an
> appropriate macro.
>
> Look at the definition of _GLIBCXX_NPROCS and adjust that to do
>
> #define _GLIBCXX_NPROCS pthread_num_processors_np()
>
> for the relevant platforms.
>
> For the platforms using sysctlbyname there could be an inline function
> that calls it, and _GLIBCXX_NPROCS could be defined to call that, so
> that thread::hardware_concurrency() can still be defined as it is
> today.
>
> Please read the code you're changing and understand how it works today
> before making changes.
>

Also, you should include <sys/sysctl.h> before calling sysctlbyname.

What header is required for pthread_num_processors_np?  If it is not
<pthread.h> then that must be included explicitly too.
Jonathan Wakely Nov. 1, 2011, 3:55 p.m. UTC | #7 
Is there a reason you used "hw.ncpu" not the constant HW_NCPU ?
niXman Nov. 1, 2011, 3:57 p.m. UTC | #8 
> What header is required for pthread_num_processors_np?
pthread.h

> Also, you should include <sys/sysctl.h> before calling sysctlbyname.
On the right - yes.
sysctlbyname() implicitly included in some header files.
niXman Nov. 1, 2011, 3:57 p.m. UTC | #9 
Ok. I correct it.

2011/11/1 Jonathan Wakely <jwakely.gcc@gmail.com>:
> I've put gcc-patches@ back in the CC list and removed gcc@
>
>
> On 1 November 2011 15:35, niXman wrote:
>>> Er, the macro _GLIBCXX_NPROCS already handles
>>> the case sysconf(_SC_NPROCESSORS_ONLN).
>>> It looks like you actually want to remove the macro
>>> _GLIBCXX_NPROCS completely.
>>
>> Fixed.
>
> No, this still isn't acceptable.
>
> I do not want to see preprocessor tests like
>
> +#elif defined(__APPLE__) || defined(__FreeBSD__)
>
> in the body of get_thread::hardware_concurrency(), the configure
> script should determine what is available on the platform and set an
> appropriate macro.
>
> Look at the definition of _GLIBCXX_NPROCS and adjust that to do
>
> #define _GLIBCXX_NPROCS pthread_num_processors_np()
>
> for the relevant platforms.
>
> For the platforms using sysctlbyname there could be an inline function
> that calls it, and _GLIBCXX_NPROCS could be defined to call that, so
> that thread::hardware_concurrency() can still be defined as it is
> today.
>
> Please read the code you're changing and understand how it works today
> before making changes.
>
Jonathan Wakely Nov. 1, 2011, 4:01 p.m. UTC | #10 
On 1 November 2011 15:57, niXman wrote:
>> What header is required for pthread_num_processors_np?
> pthread.h

OK.

This assumes that Pthreads is the only abstraction available on __hpux
(i.e. that if _GLIBCXX_HAS_GTHREADS is true then we have already
included <pthread.h>):

+#if defined(PTW32_VERSION) || \
+   (defined(__MINGW64_VERSION_MAJOR) && defined(_POSIX_THREADS)) || \
+   defined(__hpux)

Is that assumption safe?


>> Also, you should include <sys/sysctl.h> before calling sysctlbyname.
> On the right - yes.
> sysctlbyname() implicitly included in some header files.

The manual page says it requires <sys/sysctl.h> so please do that,
otherwise a future version of darwin or freebsd might stop implicitly
including it and break the code.
Jonathan Wakely Nov. 1, 2011, 4:13 p.m. UTC | #11 
On 1 November 2011 16:01, Jonathan Wakely wrote:
> On 1 November 2011 15:57, niXman wrote:
>>> What header is required for pthread_num_processors_np?
>> pthread.h
>
> OK.
>
> This assumes that Pthreads is the only abstraction available on __hpux
> (i.e. that if _GLIBCXX_HAS_GTHREADS is true then we have already
> included <pthread.h>):
>
> +#if defined(PTW32_VERSION) || \
> +   (defined(__MINGW64_VERSION_MAJOR) && defined(_POSIX_THREADS)) || \
> +   defined(__hpux)
>
> Is that assumption safe?

OK, gthr-dec.h includes <pthread.h> so I think it is safe.

Do all supported versions of Pthreads-win32, mingw64 and HPUX define
pthread_num_processors_np() in <pthread.h>?  They might not, which is
why there should be a configure test checking for the availability of
that function, which sets a macro such as
_GLIBCXX_USE_PTHREAD_NUM_PROCESSORS_NP, which is then checked in
src/thread.cc
Mike Stump Nov. 1, 2011, 5:06 p.m. UTC | #12 
On Nov 1, 2011, at 8:55 AM, Jonathan Wakely wrote:
> Is there a reason you used "hw.ncpu" not the constant HW_NCPU ?

I suspect on some systems, this would be a runtime value....  so, no fixed constant could ever work.
Jonathan Wakely Nov. 1, 2011, 5:13 p.m. UTC | #13 
On 1 November 2011 17:06, Mike Stump wrote:
> On Nov 1, 2011, at 8:55 AM, Jonathan Wakely wrote:
>> Is there a reason you used "hw.ncpu" not the constant HW_NCPU ?
>
> I suspect on some systems, this would be a runtime value....  so, no fixed constant could ever work.

It's a constant for identifying the sysctl, not a constant for the
number of processors e.g. (untested)

  int mib[] = { CTL_HW, HW_NCPU };
  if (!sysctl(mib, 2, &count, &size, NULL, 0))

The Mac OS X man page says "the sysctl() function runs in about a
third the time as the same request made via the sysctlbyname()
function."

My preferred solution (which would be consistent with the existing
code, and additionally support NetBSD, OpenBSD and Irix) would be to
add autoconf tests for the required functionality, then:

#if defined(_GLIBCXX_USE_GET_NPROCS)
# include <sys/sysinfo.h>
# define _GLIBCXX_NPROCS get_nprocs()
#elif defined(_GLIBCXX_USE_SC_NPROCESSORS_ONLN)
# include <unistd.h>
# define _GLIBCXX_NPROCS sysconf(_SC_NPROCESSORS_ONLN)
#elif defined(_GLIBCXX_USE_SC_NPROC_ONLN)
# include <unistd.h>
# define _GLIBCXX_NPROCS sysconf(_SC_NPROC_ONLN)
#elif defined(_GLIBCXX_USE_PTHREADS_NUM_PROCESSORS_NP)
# define _GLIBCXX_NPROCS pthread_num_processors_np()
#elif defined(_GLIBCXX_USE_SYSCTLBYNAME_HW_NCPU)
# include <sys/sysctl.h>
static inline int get_nprocs()
{
  int count;
  size_t size = sizeof(count);
  int mib[] = { CTL_HW, HW_NCPU };
  if (!sysctl(mib, 2, &count, &size, NULL, 0))
    return count;
  return 0;
}
# define _GLIBCXX_NPROCS get_nprocs()
#else
# define _GLIBCXX_NPROCS 0
#endif

...

  unsigned int
  thread::hardware_concurrency() noexcept
  {
    int __n = _GLIBCXX_NPROCS;
    if (__n < 0)
      __n = 0;
    return __n;
  }
Mike Stump Nov. 1, 2011, 5:47 p.m. UTC | #14 
On Nov 1, 2011, at 10:13 AM, Jonathan Wakely wrote:
>> I suspect on some systems, this would be a runtime value....  so, no fixed constant could ever work.
> 
> It's a constant for identifying the sysctl, not a constant for the
> number of processors e.g. (untested)

Ah, never mind, ignore me.
Jonathan Wakely Nov. 7, 2011, 9:08 a.m. UTC | #15 
I'm testing my suggestion on a netbsd machine, I'd be grateful for
darwin testing once I've committed it, which I expect to be later
today.
niXman Nov. 7, 2011, 9:13 a.m. UTC | #16 
I am currently working on a patch for OpenBSD.I wrote a some autoconf
tests for mingw/*BSD.
2011/11/7 Jonathan Wakely <jwakely.gcc@gmail.com>:
> I'm testing my suggestion on a netbsd machine, I'd be grateful for
> darwin testing once I've committed it, which I expect to be later
> today.
>
diff mbox

Patch

diff --git a/libstdc++-v3/src/thread.cc b/libstdc++-v3/src/thread.cc
index 09e7fc5..6feda4d 100644
--- a/libstdc++-v3/src/thread.cc
+++ b/libstdc++-v3/src/thread.cc
@@ -112,10 +112,20 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
   unsigned int
   thread::hardware_concurrency() noexcept
   {
-    int __n = _GLIBCXX_NPROCS;
-    if (__n < 0)
-      __n = 0;
-    return __n;
+    int count=0;
+#if defined(PTW32_VERSION) || \
+   (defined(__MINGW64_VERSION_MAJOR) && defined(_POSIX_THREADS)) || \
+   defined(__hpux)
+    count=pthread_num_processors_np();
+#elif defined(__APPLE__) || defined(__FreeBSD__)
+    size_t size=sizeof(count);
+    sysctlbyname("hw.ncpu", &count, &size, NULL, 0);
+#elif defined(_SC_NPROCESSORS_ONLN)
+    count=sysconf(_SC_NPROCESSORS_ONLN);
+#elif defined(_GLIBCXX_USE_GET_NPROCS)
+    count=_GLIBCXX_NPROCS;
+#endif
+    return (count>0)?count:0;
   }

 _GLIBCXX_END_NAMESPACE_VERSION