Message ID | CAMPTgK0dyW0uP03vmOJ4N53vk0NwbFOgirGFqwz7wD2SFZhE_A@mail.gmail.com |
---|---|
State | New |
Headers | show |
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.
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.
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.
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. >
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.
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.
Is there a reason you used "hw.ncpu" not the constant HW_NCPU ?
> 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.
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. >
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.
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
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.
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; }
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.
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.
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 --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