PR libstdc++/80538 Only call sleep for non-zero values

Message ID 20181011163751.GA8203@redhat.com
State New
Headers show
Series
  • PR libstdc++/80538 Only call sleep for non-zero values
Related show

Commit Message

Jonathan Wakely Oct. 11, 2018, 4:37 p.m.
Avoid a system call when no sleep is required. Sleep in a loop (actually
two loops) to handle interruption by signals.

	PR libstdc++/80538
	* src/c++11/thread.cc (this_thread::__sleep_for)
	[_GLIBCXX_HAVE_SLEEP]: Only call sleep for non-zero values.
	Loop while sleep call is interrupted and until steady_clock
	shows requested duration has elapsed.
	(!_GLIBCXX_HAVE_USLEEP]: Use the _GLIBCXX_HAVE_SLEEP code path, but
	avoiding the usleep call.
	* testsuite/30_threads/this_thread/60421.cc: Test repeated
	signal interruptions.

Tested x86_64-linux (by manually fudging the configure macros to test
the !_GLIBCXX_USE_NANOSLEEP paths).

Committed to trunk.
commit 7cad0b3cbb85a78dce40535f897ba27886469da9
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Fri Apr 28 17:43:25 2017 +0100

    PR libstdc++/80538 Only call sleep for non-zero values
    
    Avoid a system call when no sleep is required. Sleep in a loop (actually
    two loops) to handle interruption by signals.
    
            PR libstdc++/80538
            * src/c++11/thread.cc (this_thread::__sleep_for)
            [_GLIBCXX_HAVE_SLEEP]: Only call sleep for non-zero values.
            Loop while sleep call is interrupted and until steady_clock
            shows requested duration has elapsed.
            (!_GLIBCXX_HAVE_USLEEP]: Use the _GLIBCXX_HAVE_SLEEP code path, but
            avoiding the usleep call.
            * testsuite/30_threads/this_thread/60421.cc: Test repeated
            signal interruptions.

Comments

Jonathan Wakely Oct. 11, 2018, 9:36 p.m. | #1
On 11/10/18 17:37 +0100, Jonathan Wakely wrote:
>Avoid a system call when no sleep is required. Sleep in a loop (actually
>two loops) to handle interruption by signals.
>
>	PR libstdc++/80538
>	* src/c++11/thread.cc (this_thread::__sleep_for)
>	[_GLIBCXX_HAVE_SLEEP]: Only call sleep for non-zero values.
>	Loop while sleep call is interrupted and until steady_clock
>	shows requested duration has elapsed.
>	(!_GLIBCXX_HAVE_USLEEP]: Use the _GLIBCXX_HAVE_SLEEP code path, but
>	avoiding the usleep call.
>	* testsuite/30_threads/this_thread/60421.cc: Test repeated
>	signal interruptions.
>
>Tested x86_64-linux (by manually fudging the configure macros to test
>the !_GLIBCXX_USE_NANOSLEEP paths).
>
>Committed to trunk.
>
>

>commit 7cad0b3cbb85a78dce40535f897ba27886469da9
>Author: Jonathan Wakely <jwakely@redhat.com>
>Date:   Fri Apr 28 17:43:25 2017 +0100
>
>    PR libstdc++/80538 Only call sleep for non-zero values
>
>    Avoid a system call when no sleep is required. Sleep in a loop (actually
>    two loops) to handle interruption by signals.
>
>            PR libstdc++/80538
>            * src/c++11/thread.cc (this_thread::__sleep_for)
>            [_GLIBCXX_HAVE_SLEEP]: Only call sleep for non-zero values.
>            Loop while sleep call is interrupted and until steady_clock
>            shows requested duration has elapsed.
>            (!_GLIBCXX_HAVE_USLEEP]: Use the _GLIBCXX_HAVE_SLEEP code path, but
>            avoiding the usleep call.
>            * testsuite/30_threads/this_thread/60421.cc: Test repeated
>            signal interruptions.
>
>diff --git a/libstdc++-v3/src/c++11/thread.cc b/libstdc++-v3/src/c++11/thread.cc
>index c62cb71bf99..564eae6f166 100644
>--- a/libstdc++-v3/src/c++11/thread.cc
>+++ b/libstdc++-v3/src/c++11/thread.cc
>@@ -194,18 +194,35 @@ namespace this_thread
>     while (::nanosleep(&__ts, &__ts) == -1 && errno == EINTR)
>       { }
> #elif defined(_GLIBCXX_HAVE_SLEEP)
>-# ifdef _GLIBCXX_HAVE_USLEEP
>-    ::sleep(__s.count());
>-    if (__ns.count() > 0)
>+    const auto target = chrono::steady_clock::now() + __s + __ns;
>+    while (true)
>       {
>-        long __us = __ns.count() / 1000;
>-        if (__us == 0)
>-          __us = 1;
>-        ::usleep(__us);
>-      }
>+	unsigned secs = __s.count();
>+	if (__ns.count() > 0)
>+	  {
>+# ifdef _GLIBCXX_HAVE_USLEEP
>+	    long us = __ns.count() / 1000;
>+	    if (us == 0)
>+	      us = 1;
>+	    ::usleep(us);
> # else
>-    ::sleep(__s.count() + (__ns.count() >= 1000000));
>+	    if (__ns.count() > 1000000 || secs == 0)
>+	      ++secs; // No sub-second sleep function, so round up.
> # endif

I wonder if this would be worthwhile:

--- a/libstdc++-v3/src/c++11/thread.cc
+++ b/libstdc++-v3/src/c++11/thread.cc
@@ -208,6 +208,8 @@ namespace this_thread
 # else
            if (__ns.count() > 1000000 || secs == 0)
              ++secs; // No sub-second sleep function, so round up.
+           else
+             __gthread_yield();
 # endif
          }

That way for durations smaller than 1ms we give other threads a chance
to run, instead of sleeping for the __s seconds, looping back to this
test, and then sleeping an entire extra second.

But I'm assuming that systems with no usleep are probably rare, and
can live with rounding up to sleep for a full second.
Bernhard Reutner-Fischer Oct. 13, 2018, 7:48 a.m. | #2
On 11 October 2018 23:36:15 CEST, Jonathan Wakely <jwakely@redhat.com> wrote:

>But I'm assuming that systems with no usleep are probably rare, and
>can live with rounding up to sleep for a full second.

Well conforming implementations usually won't have usleep which was obscolencent in SUSv3 and removed from SUSv4.
As you certainly know we have clock_nanosleep / nanosleep nowadays.

thanks,
Jonathan Wakely Oct. 13, 2018, 10:56 a.m. | #3
On Sat, 13 Oct 2018 at 08:49, Bernhard Reutner-Fischer
<rep.dot.nop@gmail.com> wrote:
>
> On 11 October 2018 23:36:15 CEST, Jonathan Wakely <jwakely@redhat.com> wrote:
>
> >But I'm assuming that systems with no usleep are probably rare, and
> >can live with rounding up to sleep for a full second.
>
> Well conforming implementations usually won't have usleep which was obscolencent in SUSv3 and removed from SUSv4.
> As you certainly know we have clock_nanosleep / nanosleep nowadays.

Yes, of course. We don't even look for usleep unless nanosleep isn't
available, so I mean systems with no nanosleep *and* no usleep.
Bernhard Reutner-Fischer Oct. 13, 2018, 8:34 p.m. | #4
On 13 October 2018 12:56:14 CEST, Jonathan Wakely <jwakely.gcc@gmail.com> wrote:

>Yes, of course. We don't even look for usleep unless nanosleep isn't
>available, so I mean systems with no nanosleep *and* no usleep.

Right. Wasn't obvious to me from just the patch. Sorry for the noise..

Patch

diff --git a/libstdc++-v3/src/c++11/thread.cc b/libstdc++-v3/src/c++11/thread.cc
index c62cb71bf99..564eae6f166 100644
--- a/libstdc++-v3/src/c++11/thread.cc
+++ b/libstdc++-v3/src/c++11/thread.cc
@@ -194,18 +194,35 @@  namespace this_thread
     while (::nanosleep(&__ts, &__ts) == -1 && errno == EINTR)
       { }
 #elif defined(_GLIBCXX_HAVE_SLEEP)
-# ifdef _GLIBCXX_HAVE_USLEEP
-    ::sleep(__s.count());
-    if (__ns.count() > 0)
+    const auto target = chrono::steady_clock::now() + __s + __ns;
+    while (true)
       {
-        long __us = __ns.count() / 1000;
-        if (__us == 0)
-          __us = 1;
-        ::usleep(__us);
-      }
+	unsigned secs = __s.count();
+	if (__ns.count() > 0)
+	  {
+# ifdef _GLIBCXX_HAVE_USLEEP
+	    long us = __ns.count() / 1000;
+	    if (us == 0)
+	      us = 1;
+	    ::usleep(us);
 # else
-    ::sleep(__s.count() + (__ns.count() >= 1000000));
+	    if (__ns.count() > 1000000 || secs == 0)
+	      ++secs; // No sub-second sleep function, so round up.
 # endif
+	  }
+
+	if (secs > 0)
+	  {
+	    // Sleep in a loop to handle interruption by signals:
+	    while ((secs = ::sleep(secs)))
+	      { }
+	  }
+	const auto now = chrono::steady_clock::now();
+	if (now >= target)
+	  break;
+	__s = chrono::duration_cast<chrono::seconds>(target - now);
+	__ns = chrono::duration_cast<chrono::nanoseconds>(target - (now + __s));
+    }
 #elif defined(_GLIBCXX_HAVE_WIN32_SLEEP)
     unsigned long ms = __ns.count() / 1000000;
     if (__ns.count() > 0 && ms == 0)
diff --git a/libstdc++-v3/testsuite/30_threads/this_thread/60421.cc b/libstdc++-v3/testsuite/30_threads/this_thread/60421.cc
index e7d69a49fb6..cd8d2fdd6f3 100644
--- a/libstdc++-v3/testsuite/30_threads/this_thread/60421.cc
+++ b/libstdc++-v3/testsuite/30_threads/this_thread/60421.cc
@@ -53,10 +53,19 @@  test02()
     sleeping = true;
     std::this_thread::sleep_for(time);
     result = std::chrono::system_clock::now() >= (start + time);
+    sleeping = false;
   });
-  while (!sleeping) { }
-  std::this_thread::sleep_for(std::chrono::milliseconds(500));
-  pthread_kill(t.native_handle(), SIGUSR1);
+  while (!sleeping)
+  {
+    // Wait for the thread to start sleeping.
+  }
+  while (sleeping)
+  {
+    // The sleeping thread should finish eventually,
+    // even if continually interrupted after less than a second:
+    std::this_thread::sleep_for(std::chrono::milliseconds(500));
+    pthread_kill(t.native_handle(), SIGUSR1);
+  }
   t.join();
   VERIFY( result );
 }