MessagePumpDefault can spin for up to 1 ms on Windows

base/message_loop/message_pump_default.cc

Index: base/message_loop/message_pump_default.cc
diff --git a/base/message_loop/message_pump_default.cc b/base/message_loop/message_pump_default.cc
index 0fac8b3c9c9adbe7597b59096a30ba2a9ad7e935..d7dbaa7f2738b5b36deff73d9eafeb151c775342 100644
--- a/base/message_loop/message_pump_default.cc
+++ b/base/message_loop/message_pump_default.cc
@@ -4,6 +4,8 @@
 
 #include "base/message_loop/message_pump_default.h"
 
+#include <algorithm>
+
 #include "base/logging.h"
 #include "base/threading/thread_restrictions.h"
 #include "build/build_config.h"
@@ -54,7 +56,25 @@ void MessagePumpDefault::Run(Delegate* delegate) {
     } else {
       TimeDelta delay = delayed_work_time_ - TimeTicks::Now();
       if (delay > TimeDelta()) {
+#if defined(OS_WIN)

[danakj, 2016/06/22 22:26:55]

I am wondering about your choice to put this change here in an #ifdef of in
waitable_event_win.cc. Should this not apply to other uses of the method?

I see TimedWait is allowed to return early if it returns false, so maybe not
that part? But the min_delay seems more general.

Any thoughts?

[vmiura, 2016/06/22 23:25:28]

Yeah, would it make sense to use max_time.InMillisecondsRoundedUp() in
WaitableEvent::TimedWait()?

[stanisc, 2016/06/22 23:39:43]

I considered that but wanted to minimize the impact. I agree the fix inside
TimedWait would be cleaner. I'll try that and run through the lab to see if
there are any issues.

[brucedawson, 2016/06/22 23:41:51]

That would require evaluating all uses of TimedWait() to make sure that rounding
up is appropriate. This is the only place where we have seen the problematic
spinning so this is the minimum change to fix the spinning.

InMillisecondsRoundedUp() does sound like an interesting alternate way of
implementing this.

[danakj, 2016/06/22 23:52:59]

Hm, I guess sometimes people would want to not wait for 1ms when their delay is
less. Can this comment be more explicit that this may make us wait too long, but
we're doing this to ensure chrome goes idle at least once when there is some
delay present?

[stanisc, 2016/06/23 00:20:33]

The behavior of timed wait depends on platform. On Windows 7 the wait times out
earlier almost always, but on Windows 8 or Windows 10 it very often takes an
extra millisecond to time out. If we round up unconditionally then we might end
up waiting up to 2 extra milliseconds. I think the better approach is to round
down, and then if the wait times out early do another short wait. The only
exception would be a sub-millisecond wait where we want to always round up.
This kind of logic is easy to implement in the message pump - that's what my
change essentially does.

+        // crbug.com/487724: on Windows, waiting for less than 1 ms results in
+        // returning from TimedWait promptly and spinning the loop.
+        const TimeDelta min_delay = TimeDelta::FromMilliseconds(1);

[danakj, 2016/06/22 22:26:55]

nit: You could write constexpr instead for even betterness.

[stanisc, 2016/06/23 21:18:50]

Done.

+        do {
+          delay = std::max(delay, min_delay);
+          if (event_.TimedWait(delay))
+            break;
+
+          // TimedWait can time out earlier than the specified |delay| on
+          // Windows. It doesn't make sense to run the outer loop in that case

[danakj, 2016/06/22 23:52:59]

What part of this is windows-specific, since TimedWait() can return false on any
platform? Do other platforms need to make a loop thru everything when it was
false/early return?

[brucedawson, 2016/06/23 00:01:12]

The Windows wait/sleep functions all take millisecond times in integers, thus
forcing the delay to be rounded down, and it is that rounding down that this
change is working around.

I don't think other platforms have this unfortunate behavior.

I would love to have nanosleep on Windows, but alas...

[danakj, 2016/06/23 00:02:55]

That makes the min_delay windows specific. But what if the delay was > 1
millisecond, but TimedWait returned false? That can happen anywhere, I think.

+          // because there isn't going to be any new work. It is less overhead
+          // go just go back to wait.

[danakj, 2016/06/22 22:26:55]

"to just go"

[stanisc, 2016/06/23 21:18:50]

Done.

+          // In practice this inner wait loop might have up to 3 iterations.
+          delay = delayed_work_time_ - TimeTicks::Now();

[danakj, 2016/06/22 22:26:55]

IIRC Now() is quite expensive on some windows systems, are those all deprecated?
Or are there cases this could be worse?

[stanisc, 2016/06/22 23:39:43]

Good question! If we don't do this inner loop with Now(), we'd go through the
outer loop which would still call Now() inside MessageLoop::DoDelayedWork
(assuming there is at least one future delayed task). In addition to that the
outer loop would go into MessageLoop::DoWork and attempt to reload work queue
(which involves acquiring a lock) and do a bit of other work. So there is more
work overall. If we fix the spinning issue above the difference might be small
enough to not care.

Also if we fix the minimum delay inside waitable_timer_win.cc as you suggested
above then perhaps no changes would be needed in this file at all.

[brucedawson, 2016/06/22 23:41:51]

I don't think Now() is ever particularly expensive? It's sometimes inaccurate.

RolloverProtectedNow just calls the tick function, which is cheap.

QPCNow has a variable cost depending on whether it uses rdtsc or the timer chip
(I believe) but I don't know that it is ever expensive enough to not make this
check appropriate.

The times when I've seen it on a profile have all been because we were spinning
in a loop calling it.

[danakj, 2016/06/22 23:52:59]

Interesting, I recall in compositor land we went to some work to try avoiding
calling Now() as much, due to perceived costs.

I see we would circle around and call Now() anyways so this is strictly better
tho, cool.

+        } while (delay > TimeDelta());
+#else
         event_.TimedWait(delay);
+#endif
       } else {
         // It looks like delayed_work_time_ indicates a time in the past, so we
         // need to call DoDelayedWork now.