win: clock implementation

win: clock implementation

R=mark@chromium.org
BUG=crashpad:1
Committed: https://chromium.googlesource.com/crashpad/crashpad/+/db6492e15405e5bfa4fc99097dd866544b19a0ee

[scottmg, 2014-12-16 22:29:13]

Patchset #1 (id:1) has been deleted

[scottmg, 2014-12-16 22:30:51]

Patchset #2 (id:40001) has been deleted

[scottmg, 2014-12-16 22:30:53]

Patchset #1 (id:20001) has been deleted

[scottmg, 2014-12-16 22:32:36]

Patchset #1 (id:60001) has been deleted

[scottmg, 2014-12-16 22:33:34]

Patchset #1 (id:80001) has been deleted

[scottmg, 2014-12-16 22:37:22]

Patchset #1 (id:100001) has been deleted

[Mark Mentovai, 2014-12-16 23:06:54]

https://codereview.chromium.org/807973002/diff/120001/util/misc/clock.h
File util/misc/clock.h (right):

https://codereview.chromium.org/807973002/diff/120001/util/misc/clock.h#newco...
util/misc/clock.h:41: //! being interrupted by a signal. On Windows,
timeBeginPeriod is not adjusted.
nanosleep is a POSIXism, so that sentence should say so.

`timeBeginPeriod()`? Also, I didn’t know what that did without looking it up,
and its name doesn’t make it obvious, so maybe tack on something like “to set
the timer resolution,” if that’s even correct.

https://codereview.chromium.org/807973002/diff/120001/util/misc/clock_win.cc
File util/misc/clock_win.cc (right):

https://codereview.chromium.org/807973002/diff/120001/util/misc/clock_win.cc#...
util/misc/clock_win.cc:25: QueryPerformanceFrequency(&frequency);
PCHECK() or DPCHECK() this and QueryPerformanceCounter()?

https://codereview.chromium.org/807973002/diff/120001/util/misc/clock_win.cc#...
util/misc/clock_win.cc:25: QueryPerformanceFrequency(&frequency);
Cache this like we do in clock_mac.cc? (TimebaseInternal() and Timebase().) The
documentation says it’s cacheable.

https://codereview.chromium.org/807973002/diff/120001/util/misc/clock_win.cc#...
util/misc/clock_win.cc:30: time.QuadPart *= 1000000000;
1E9 or kNanosecondsPerSecond = 1E9?

https://codereview.chromium.org/807973002/diff/120001/util/misc/clock_win.cc#...
util/misc/clock_win.cc:30: time.QuadPart *= 1000000000;
Fear of overflow: what kind of number is frequency.QuadPart normally? Even if
it’s on the order of 1M (I’d consider that reasonable), overflow becomes a
concern at only 2.5 hours (2.5 * 60 * 60 * 1E6 * 1E9 ≅ 2^63).

In Chrome, base/time_win.cc does a little dance to deal with the potential for
overflow, and I suspect that the same is necessary here.

Actually, if overflow is likely at as little as 2.5 hours of uptime, I wouldn’t
even bother with time_win.cc’s fast path. I’d stick with the simplicity of the
pessimistic but safe approach each time.

https://codereview.chromium.org/807973002/diff/120001/util/misc/clock_win.cc#...
util/misc/clock_win.cc:40: const uint64_t kNanosecondsPerMillisecond = 1000000;
1E6? Counting more than four identical characters in a row is hard.

https://codereview.chromium.org/807973002/diff/120001/util/misc/clock_win.cc#...
util/misc/clock_win.cc:41: ::Sleep(static_cast<DWORD>(nanoseconds /
kNanosecondsPerMillisecond));
The :: isn’t necessary, unless they’re so strongly ingrained in Windows
programming that you wouldn’t recognize Sleep() as a win32 call without them.

[scottmg, 2014-12-17 01:01:28]

https://codereview.chromium.org/807973002/diff/120001/util/misc/clock.h
File util/misc/clock.h (right):

https://codereview.chromium.org/807973002/diff/120001/util/misc/clock.h#newco...
util/misc/clock.h:41: //! being interrupted by a signal. On Windows,
timeBeginPeriod is not adjusted.
On 2014/12/16 23:06:53, Mark Mentovai wrote:
> nanosleep is a POSIXism, so that sentence should say so.
> 
> `timeBeginPeriod()`? Also, I didn’t know what that did without looking it up,
> and its name doesn’t make it obvious, so maybe tack on something like “to set
> the timer resolution,” if that’s even correct.

Done.

https://codereview.chromium.org/807973002/diff/120001/util/misc/clock_win.cc
File util/misc/clock_win.cc (right):

https://codereview.chromium.org/807973002/diff/120001/util/misc/clock_win.cc#...
util/misc/clock_win.cc:25: QueryPerformanceFrequency(&frequency);
On 2014/12/16 23:06:54, Mark Mentovai wrote:
> PCHECK() or DPCHECK() this and QueryPerformanceCounter()?

They can't fail on XP or later, so I prefer not to.

https://codereview.chromium.org/807973002/diff/120001/util/misc/clock_win.cc#...
util/misc/clock_win.cc:25: QueryPerformanceFrequency(&frequency);
On 2014/12/16 23:06:54, Mark Mentovai wrote:
> Cache this like we do in clock_mac.cc? (TimebaseInternal() and Timebase().)
The
> documentation says it’s cacheable.

Done.

https://codereview.chromium.org/807973002/diff/120001/util/misc/clock_win.cc#...
util/misc/clock_win.cc:30: time.QuadPart *= 1000000000;
On 2014/12/16 23:06:54, Mark Mentovai wrote:
> Fear of overflow: what kind of number is frequency.QuadPart normally? Even if
> it’s on the order of 1M (I’d consider that reasonable), overflow becomes a
> concern at only 2.5 hours (2.5 * 60 * 60 * 1E6 * 1E9 ≅ 2^63).
> 
> In Chrome, base/time_win.cc does a little dance to deal with the potential for
> overflow, and I suspect that the same is necessary here.
> 
> Actually, if overflow is likely at as little as 2.5 hours of uptime, I
wouldn’t
> even bother with time_win.cc’s fast path. I’d stick with the simplicity of the
> pessimistic but safe approach each time.

Done.

https://codereview.chromium.org/807973002/diff/120001/util/misc/clock_win.cc#...
util/misc/clock_win.cc:30: time.QuadPart *= 1000000000;
On 2014/12/16 23:06:54, Mark Mentovai wrote:
> 1E9 or kNanosecondsPerSecond = 1E9?

Done.

https://codereview.chromium.org/807973002/diff/120001/util/misc/clock_win.cc#...
util/misc/clock_win.cc:40: const uint64_t kNanosecondsPerMillisecond = 1000000;
On 2014/12/16 23:06:54, Mark Mentovai wrote:
> 1E6? Counting more than four identical characters in a row is hard.

Done.

https://codereview.chromium.org/807973002/diff/120001/util/misc/clock_win.cc#...
util/misc/clock_win.cc:41: ::Sleep(static_cast<DWORD>(nanoseconds /
kNanosecondsPerMillisecond));
On 2014/12/16 23:06:54, Mark Mentovai wrote:
> The :: isn’t necessary, unless they’re so strongly ingrained in Windows
> programming that you wouldn’t recognize Sleep() as a win32 call without them.

Done.

[Mark Mentovai, 2014-12-17 01:12:23]

LGTM

https://codereview.chromium.org/807973002/diff/140001/util/misc/clock_win.cc
File util/misc/clock_win.cc (right):

https://codereview.chromium.org/807973002/diff/140001/util/misc/clock_win.cc#...
util/misc/clock_win.cc:51: // without implementing a busy loop.
Probably should do the loop until ClockMonotonicNanoseconds() is at least at its
original value + nanoseconds, but it’s not important now if you don’t want to do
that. This function is unused everywhere except for some very Mac-specific code
that will may soon be declared dead.

[scottmg, 2014-12-17 01:57:40]

Patchset #3 (id:160001) has been deleted

[scottmg, 2014-12-17 01:58:11]

https://codereview.chromium.org/807973002/diff/140001/util/misc/clock_win.cc
File util/misc/clock_win.cc (right):

https://codereview.chromium.org/807973002/diff/140001/util/misc/clock_win.cc#...
util/misc/clock_win.cc:51: // without implementing a busy loop.
On 2014/12/17 01:12:23, Mark Mentovai wrote:
> Probably should do the loop until ClockMonotonicNanoseconds() is at least at
its
> original value + nanoseconds, but it’s not important now if you don’t want to
do
> that. This function is unused everywhere except for some very Mac-specific
code
> that will may soon be declared dead.

Done. I look forward to deleting it!

[Mark Mentovai, 2014-12-17 05:15:57]

https://codereview.chromium.org/807973002/diff/180001/util/misc/clock_win.cc
File util/misc/clock_win.cc (right):

https://codereview.chromium.org/807973002/diff/180001/util/misc/clock_win.cc#...
util/misc/clock_win.cc:58: Sleep(1);
Hmm, I had something slightly different in mind, just one Sleep() call in the
function, inside the loop body, which would have looked like

  uint64_t remaining_ms = nanoseconds / kNanosecondsPerMillisecond;
  uint64_t now_ms = ClockMonotonicNanoseconds() / kNanosecondsPerMillisecond;
  const uint64_t deadline_ms = now_ms + remaining_ms;
  do {
    Sleep(static_cast<DWORD>(std::min(remaining_ms,
                                      std::numeric_limits<DWORD>::max()));
    now_ms = ClockMonotonicNanoseconds() / kNanosecondsPerMillisecond;
    remaining_ms = deadline_ms - now_ms;
  } while (now_ms < deadline_ms);

so that remainder sleeps try to make up as much of the remainder as possible. I
didn’t really care so much about sub-millisecond resolution, which would be
futile with this API.

But the way you wrote this now, it looks like maybe I misunderstood the comment
you had here before. I took “can sleep for less than requested” to mean that it
could be interrupted way earlier than the deadline, as in a POSIX EINTR
(“interrupted system call”).

If you were really just referring to truncation when converting nanoseconds to
milliseconds, then I don’t think that it’s worth trying to correct, and we
should just get rid of this loop and go back to the single Sleep().

Sorry if I misunderstood and it caused a little runaround. You can feel free to
push back if it sounds like I’m asking for anything weird.

[Mark Mentovai, 2014-12-17 05:46:22]

(if that’s the case, patch set 2 still LGTM, maybe with the comment tweaked to
indicate that you meant that the short sleep would be caused by truncation in
division.)

[scottmg, 2014-12-17 06:03:00]

On 2014/12/17 05:15:57, Mark Mentovai wrote:
> https://codereview.chromium.org/807973002/diff/180001/util/misc/clock_win.cc
> File util/misc/clock_win.cc (right):
> 
>
https://codereview.chromium.org/807973002/diff/180001/util/misc/clock_win.cc#...
> util/misc/clock_win.cc:58: Sleep(1);
> Hmm, I had something slightly different in mind, just one Sleep() call in the
> function, inside the loop body, which would have looked like
> 
>   uint64_t remaining_ms = nanoseconds / kNanosecondsPerMillisecond;
>   uint64_t now_ms = ClockMonotonicNanoseconds() / kNanosecondsPerMillisecond;
>   const uint64_t deadline_ms = now_ms + remaining_ms;
>   do {
>     Sleep(static_cast<DWORD>(std::min(remaining_ms,
>                                       std::numeric_limits<DWORD>::max()));
>     now_ms = ClockMonotonicNanoseconds() / kNanosecondsPerMillisecond;
>     remaining_ms = deadline_ms - now_ms;
>   } while (now_ms < deadline_ms);
> 
> so that remainder sleeps try to make up as much of the remainder as possible.
I
> didn’t really care so much about sub-millisecond resolution, which would be
> futile with this API.
> 
> But the way you wrote this now, it looks like maybe I misunderstood the
comment
> you had here before. I took “can sleep for less than requested” to mean that
it
> could be interrupted way earlier than the deadline, as in a POSIX EINTR
> (“interrupted system call”).
> 
> If you were really just referring to truncation when converting nanoseconds to
> milliseconds, then I don’t think that it’s worth trying to correct, and we
> should just get rid of this loop and go back to the single Sleep().

No EINTR on Windows. You have to use something like
http://msdn.microsoft.com/en-us/library/windows/desktop/ms683242(v=vs.85).aspx
and send back the information in an application-defined way.

It's actually a third option, a leaky API specification:

"""
This function causes a thread to relinquish the remainder of its time slice and
become unrunnable for an interval based on the value of dwMilliseconds. The
system clock "ticks" at a constant rate. If dwMilliseconds is less than the
resolution of the system clock, the thread may sleep for less than the specified
length of time. If dwMilliseconds is greater than one tick but less than two,
the wait can be anywhere between one and two ticks, and so on. 
"""

Before Win8, by default the tick is 16ms, so for Sleep(N) of N < 16, you might
get less than the requested time. The Sleep(1) is intended to wait until the
next tick which will then be at least a tick later, and so will be larger than N
that was less than tick size. That is, it could be an 'if' rather than a
'while', and would have the same effect, but that feels odd.

Anyway, I think the right thing to do is probably just delete the function and
leave it unimplemented on Windows for now. A correct implementation is subtle,
not especially useful, and as we're not using it anyway, it's probably not worth
spending time on. Sound OK?

> 
> Sorry if I misunderstood and it caused a little runaround. You can feel free
to
> push back if it sounds like I’m asking for anything weird.

[Mark Mentovai, 2014-12-17 13:47:53]

Yup, that’s cool. LGTM to that.

[scottmg, 2014-12-17 18:21:19]

Patchset #4 (id:200001) has been deleted

[scottmg, 2014-12-17 18:22:01]

On 2014/12/17 13:47:53, Mark Mentovai wrote:
> Yup, that’s cool. LGTM to that.

Done.

[Mark Mentovai, 2014-12-17 18:41:18]

LGTM. Ship it!

[scottmg, 2014-12-17 18:45:49]

Committed patchset #4 (id:220001) manually as
db6492e15405e5bfa4fc99097dd866544b19a0ee (presubmit successful).