[libcxx] Threading support: externalize sleep_for()

[rmaprath]

Summary

Different platforms implement the wait/sleep behaviour in very different ways. It makes sense to hoist this functionality into the threading API.

I also feel similarly about hardware_concurrecy() implementation. Any thoughts on that?

[joerg]

include/__threading_support

367 ↗

(On Diff #87395)

I don't think giga::num makes things any clear compared to just spelling it out.

593 ↗

(On Diff #87395)

Use (ns + 999999) so that the cast rounds up.

594 ↗

(On Diff #87395)

Why is ns == 0 supposed to sleep at all? In fact, the caller already ensures that can't happen?

[rmaprath]

include/__threading_support

593 ↗

(On Diff #87395)

So, this code was lifted from the sources as-is. I will do this change, I think it makes sense.

594 ↗

(On Diff #87395)

IIUC, this is trying to detect round-downs and then compensate for it. With the above suggestion, this should no longer be needed. Will get rid of it as well.

[EricWF]

LGTM. @joerg has raised some legitimate concerns about the Windows implementation. However that shouldn't hold this cleanup up, since the implementation is preexisting.

include/__threading_support

354 ↗

(On Diff #87395)

Reserved names are needed for s and ns ts and the like now that this is in a header.

[rmaprath]

Sorry for the delay, I've updated the patch with all the comments addressed.

@EricWF: Got one question before I commit: The sleep_for function in the dylib (thread.cpp) is now quite small. Is there a particular reason to keep it there? I was wondering if we should move that bit to the <thread> header altogether.

[EricWF]

In D29630#670873, @rmaprath wrote:

Sorry for the delay, I've updated the patch with all the comments addressed.

@EricWF: Got one question before I commit: The sleep_for function in the dylib (thread.cpp) is now quite small. Is there a particular reason to keep it there? I was wondering if we should move that bit to the <thread> header altogether.

Removing the definition in the dylib would be ABI breaking.

[rmaprath]

In D29630#670876, @EricWF wrote:

In D29630#670873, @rmaprath wrote:

Sorry for the delay, I've updated the patch with all the comments addressed.

@EricWF: Got one question before I commit: The sleep_for function in the dylib (thread.cpp) is now quite small. Is there a particular reason to keep it there? I was wondering if we should move that bit to the <thread> header altogether.

Removing the definition in the dylib would be ABI breaking.

Ah! of course.

Thanks.

[joerg]

One small issue left, otherwise LGTM.

include/__threading_support

187 ↗

(On Diff #87669)

Drop the name here.

libcxx/trunk/include/__threading_support

 // -*- C++ -*-
 //===----------------------------------------------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is dual licensed under the MIT and the University of Illinois Open
 // Source Licenses. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef _LIBCPP_THREADING_SUPPORT
 #define _LIBCPP_THREADING_SUPPORT
 
 #include <__config>
+#include <chrono>
+#include <errno.h>
 
 #ifndef _LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER
 #pragma GCC system_header
 #endif
 
 #if defined(_LIBCPP_HAS_THREAD_API_EXTERNAL)
 # include <__external_threading>
 #elif !defined(_LIBCPP_HAS_NO_THREADS)
 
 #if defined(_LIBCPP_HAS_THREAD_API_PTHREAD)
 # include <pthread.h>
 # include <sched.h>
 #elif defined(_LIBCPP_HAS_THREAD_API_WIN32)
 #include <Windows.h>
 #include <process.h>
 #include <fibersapi.h>
-
-#include <chrono>
 #endif
 
 #if defined(_LIBCPP_HAS_THREAD_LIBRARY_EXTERNAL) || \
     defined(_LIBCPP_BUILDING_THREAD_LIBRARY_EXTERNAL)
 #define _LIBCPP_THREAD_ABI_VISIBILITY _LIBCPP_FUNC_VIS
 #else
 #define _LIBCPP_THREAD_ABI_VISIBILITY inline _LIBCPP_INLINE_VISIBILITY
 #endif

 int __libcpp_thread_join(__libcpp_thread_t *__t);
 
 _LIBCPP_THREAD_ABI_VISIBILITY
 int __libcpp_thread_detach(__libcpp_thread_t *__t);
 
 _LIBCPP_THREAD_ABI_VISIBILITY
 void __libcpp_thread_yield();
 
+_LIBCPP_THREAD_ABI_VISIBILITY
+void __libcpp_thread_sleep_for(const chrono::nanoseconds& ns);
+
 // Thread local storage
 _LIBCPP_THREAD_ABI_VISIBILITY
 int __libcpp_tls_create(__libcpp_tls_key* __key,
                         void(_LIBCPP_TLS_DESTRUCTOR_CC* __at_exit)(void*));
 
 _LIBCPP_THREAD_ABI_VISIBILITY
 void *__libcpp_tls_get(__libcpp_tls_key __key);
 
int __libcpp_thread_detach(__libcpp_thread_t *__t)
   return pthread_detach(*__t);
 }
 
 void __libcpp_thread_yield()
 {
   sched_yield();
 }
 
+void __libcpp_thread_sleep_for(const chrono::nanoseconds& __ns)
+{
+   using namespace chrono;
+   seconds __s = duration_cast<seconds>(__ns);
+   timespec __ts;
+   typedef decltype(__ts.tv_sec) ts_sec;
+   _LIBCPP_CONSTEXPR ts_sec __ts_sec_max = numeric_limits<ts_sec>::max();
+
+   if (__s.count() < __ts_sec_max)
+   {
+     __ts.tv_sec = static_cast<ts_sec>(__s.count());
+     __ts.tv_nsec = static_cast<decltype(__ts.tv_nsec)>((__ns - __s).count());
+   }
+   else
+   {
+     __ts.tv_sec = __ts_sec_max;
+     __ts.tv_nsec = 999999999; // (10^9 - 1)
+   }
+
+   while (nanosleep(&__ts, &__ts) == -1 && errno == EINTR);
+}
+
 // Thread local storage
 int __libcpp_tls_create(__libcpp_tls_key *__key, void (*__at_exit)(void *))
 {
   return pthread_key_create(__key, __at_exit);
 }
 
 void *__libcpp_tls_get(__libcpp_tls_key __key)
 {
int __libcpp_thread_detach(__libcpp_thread_t *__t)
   return 0;
 }
 
 void __libcpp_thread_yield()
 {
   SwitchToThread();
 }
 
+void __libcpp_thread_sleep_for(const chrono::nanoseconds& __ns)
+{
+  using namespace chrono;
+  // round-up to the nearest milisecond
+  milliseconds __ms = duration_cast<milliseconds>(__ns + 999999);
+  Sleep(__ms.count());
+}
+
 // Thread Local Storage
 int __libcpp_tls_create(__libcpp_tls_key* __key,
                         void(_LIBCPP_TLS_DESTRUCTOR_CC* __at_exit)(void*))
 {
   *__key = FlsAlloc(__at_exit);
   if (*__key == FLS_OUT_OF_INDEXES)
     return GetLastError();
   return 0;

libcxx/trunk/src/thread.cpp

 }
 
 namespace this_thread
 {
 
 void
 sleep_for(const chrono::nanoseconds& ns)
 {
-    using namespace chrono;
-    if (ns > nanoseconds::zero())
+    if (ns > chrono::nanoseconds::zero())
     {
-#if defined(_LIBCPP_WIN32API)
-        milliseconds ms = duration_cast<milliseconds>(ns);
-        if (ms.count() == 0 || ns > duration_cast<nanoseconds>(ms))
-          ++ms;
-        Sleep(ms.count());
-#else
-        seconds s = duration_cast<seconds>(ns);
-        timespec ts;
-        typedef decltype(ts.tv_sec) ts_sec;
-        _LIBCPP_CONSTEXPR ts_sec ts_sec_max = numeric_limits<ts_sec>::max();
-        if (s.count() < ts_sec_max)
-        {
-            ts.tv_sec = static_cast<ts_sec>(s.count());
-            ts.tv_nsec = static_cast<decltype(ts.tv_nsec)>((ns-s).count());
-        }
-        else
-        {
-            ts.tv_sec = ts_sec_max;
-            ts.tv_nsec = giga::num - 1;
-        }
-
-        while (nanosleep(&ts, &ts) == -1 && errno == EINTR)
-            ;
-#endif
+        __libcpp_thread_sleep_for(ns);
     }
 }
 
 }  // this_thread
 
 __thread_specific_ptr<__thread_struct>&
 __thread_local_data()
 {