Move OS_WIN specific logic in MessagePumpDefault::Run into waitable_event_win.cc

base/synchronization/waitable_event_win.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/synchronization/waitable_event.h"
 
 #include <windows.h>
 #include <stddef.h>
 
+#include <algorithm>
 #include <utility>
 
 #include "base/debug/activity_tracker.h"
 #include "base/logging.h"
 #include "base/numerics/safe_conversions.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/time/time.h"
 
 namespace base {
 
@@ skipping 17 matching lines @@
 
 void WaitableEvent::Reset() {
   ResetEvent(handle_.Get());
 }
 
 void WaitableEvent::Signal() {
   SetEvent(handle_.Get());
 }
 
 bool WaitableEvent::IsSignaled() {
-  return TimedWait(TimeDelta());
+  DWORD result = WaitForSingleObject(handle_.Get(), 0);
+  switch (result) {
+    case WAIT_OBJECT_0:
+      return true;
+    case WAIT_TIMEOUT:
+      return false;
+  }
+
+  // It is most unexpected that this should ever fail.  Help consumers learn
+  // about it if it should ever fail.
+  NOTREACHED() << "WaitForSingleObject failed";
+  return false;
 }
 
 void WaitableEvent::Wait() {
   // Record the event that this thread is blocking upon (for hang diagnosis).
   base::debug::ScopedEventWaitActivity event_activity(this);
 
   base::ThreadRestrictions::AssertWaitAllowed();
   DWORD result = WaitForSingleObject(handle_.Get(), INFINITE);
   // It is most unexpected that this should ever fail.  Help consumers learn
   // about it if it should ever fail.
   DCHECK_EQ(WAIT_OBJECT_0, result) << "WaitForSingleObject failed";
 }
 
 bool WaitableEvent::TimedWait(const TimeDelta& max_time) {

[danakj, 2016/10/21 22:01:22]

nit: can you name timedeltas with _delta suffix and timeticks as _time suffix?
That would help reading this a lot.

[stanisc, 2016/11/11 03:50:48]

Done.

+  DCHECK_GE(max_time, TimeDelta());
+  if (max_time.is_zero())
+    return IsSignaled();
+
   // Record the event that this thread is blocking upon (for hang diagnosis).
   base::debug::ScopedEventWaitActivity event_activity(this);
+  base::ThreadRestrictions::AssertWaitAllowed();

[danakj, 2016/10/21 22:01:22]

can you put assertions first? (just above event_activity)

[stanisc, 2016/11/11 03:50:48]

Done.

 
-  DCHECK_GE(max_time, TimeDelta());
-  if (!max_time.is_zero())
-    base::ThreadRestrictions::AssertWaitAllowed();
+  constexpr TimeDelta min_delay = TimeDelta::FromMilliseconds(1);
+  TimeTicks target_time = TimeTicks::Now() + max_time;

[danakj, 2016/10/21 22:01:22]

It's possible this was a bad suggestion because it requires an extra Now() call
since we don't have delayed_work_time_ here which we are basically recreating.
WDYT is this ok?

[stanisc, 2016/10/21 23:05:25]

Yeah, that is unfortunate and redundant that the caller code might be
calculating the delay by subtracting Now() from a deadline (like the message
pump code does), and this code has to add Now() back to recreate the deadline.

The posix version does the same thing. I might be good idea to add a TimedWait
overload that takes an actual deadline (TimeTicks) rather than a delay. But I
don't know how many call sites would benefit from that. 

And I've just realized that this code will overflow with TimeDelay::Max() (or
other nearly max delay). So this needs more work to handle a potential overflow.
The posix version has the same issue by the way.

[danakj, 2016/10/21 23:15:26]

I was wondering about an overload also, this seems worth doing then for our
tight event loops, esp to benefit all the platforms at once. TimedWaitUntil()?

Thanks for noticing the overflow also. Maybe we make the event loops use another
override here, and then fix the overflow for both windows and posix in another
CL?

+  TimeDelta delay = max_time;
 
-  // Truncate the timeout to milliseconds. The API specifies that this method
-  // can return in less than |max_time| (when returning false), as the argument
-  // is the maximum time that a caller is willing to wait.
-  DWORD timeout = saturated_cast<DWORD>(max_time.InMilliseconds());
-
-  DWORD result = WaitForSingleObject(handle_.Get(), timeout);
-  switch (result) {
-    case WAIT_OBJECT_0:
-      return true;
-    case WAIT_TIMEOUT:
-      return false;
-  }
-  // It is most unexpected that this should ever fail.  Help consumers learn
-  // about it if it should ever fail.
-  NOTREACHED() << "WaitForSingleObject failed";
+  do {
+    // On Windows, waiting for less than 1 ms results in WaitForSingleObject
+    // returning promptly which may result in the caller code spinning.
+    // We need to ensure that we specify at least the minimally possible 1 ms
+    // delay unless the initial timeout was exactly zero.
+    delay = std::max(delay, min_delay);

[danakj, 2016/10/21 22:01:22]

I think you can just write TimeDelta::FromMilliseconds(1) here instead of
|min_delay|, since this is where the comment explaining it is, and the method is
constexpr so it's not like we'll execute code every time. Otherwise this comment
is explaining a constant defined up above which is more awkward.

[stanisc, 2016/11/11 03:50:48]

Done.

+    // Truncate the timeout to milliseconds.
+    DWORD timeout = saturated_cast<DWORD>(delay.InMilliseconds());
+    DWORD result = WaitForSingleObject(handle_.Get(), timeout);
+    switch (result) {
+      case WAIT_OBJECT_0:
+        return true;
+      case WAIT_TIMEOUT:
+        // TimedWait can time out earlier than the specified |timeout| on
+        // Windows. To make this consistent with the posix implementation we
+        // should guarantee that TimedWait doesn't return earlier than the
+        // specified |max_time| and wait again for the remaining time.
+        delay = target_time - TimeTicks::Now();
+        break;
+      default:
+        // It is most unexpected that this should ever fail.  Help consumers
+        // learn about it if it should ever fail.
+        NOTREACHED() << "WaitForSingleObject failed";

[danakj, 2016/10/21 22:01:22]

This falls into the NOTREACHED+return antipattern. Another way to write this
would be

DWORD result = WaitForSingleObject(...);
DCHECK(result == WAIT_OBJECT_0 || result == WAIT_TIMEOUT) << result;
switch (result) {
  case WAIT_OBJECT_0:
    ...
  case WAIT_TIMEOUT:
    ...
}

and then just don't handle other cases (we'd call WaitForSingleObject again).
Would that be okay?

Similar comment for IsSignaled, but smaller even:

DCHECK(result == WAIT_OBJECT_0 || result == WAIT_TIMEOUT) << result;
return result == WAIT_OBJECT_0;

[stanisc, 2016/11/11 03:50:48]

Good suggestion. Fixed.

+        return false;
+    }
+  } while (delay > TimeDelta());
   return false;
 }
 
 // static
 size_t WaitableEvent::WaitMany(WaitableEvent** events, size_t count) {
   DCHECK(count) << "Cannot wait on no events";
 
   // Record an event (the first) that this thread is blocking upon.
   base::debug::ScopedEventWaitActivity event_activity(events[0]);
 
@@ skipping 13 matching lines @@
                              INFINITE);  // no timeout
   if (result >= WAIT_OBJECT_0 + count) {
     DPLOG(FATAL) << "WaitForMultipleObjects failed";
     return 0;
   }
 
   return result - WAIT_OBJECT_0;
 }
 
 }  // namespace base