More accurate implementation of watched thread time for Gpu Watchdog.

More accurate implementation of watched thread time for Gpu Watchdog.

This change leverages an accurate implementation of ThreadTicks to
allow GPU Watchdog thread report watched thread specific CPU time
deltas to help with GPU hang crash dump analysis.

This includes a small refactoring of ThreadTicks class that adds an
overload of ThreadTicks::Now that accepts an arbitrary thread handle
as an argument. That is limited to Windows OS only but if necessary we
could extend it to other platforms.

BUG=596190
Committed: https://crrev.com/f5173e2286a6735cdefeda91c9dc5359dc4b23af
Cr-Commit-Position: refs/heads/master@{#389210}

[stanisc, 2016-04-21 19:43:09]

stanisc@chromium.org changed reviewers:
+ fdoray@chromium.org, jbauman@chromium.org, mark@chromium.org

[stanisc, 2016-04-21 19:43:10]

jbauman@chromium.org: Please review changes in
content/gpu/gpu_watchdog_thread.cc

mark@chromium.org: Please review changes in base/time

fdoray@chromium.org: added you as the original author of ThreadTicks

[Mark Mentovai, 2016-04-21 20:27:56]

TimeTicks::Now()’s a pretty terrible name, but you’re not inventing it. No
objections to the new code in base. LGTM.

[jbauman, 2016-04-21 20:59:53]

lgtm, though we'll want to check the number of hang reports after this is
checked in to make sure they don't change too much.

[fdoray, 2016-04-21 21:08:38]

lgtm with comments

https://codereview.chromium.org/1910063003/diff/1/base/time/time_win.cc
File base/time/time_win.cc (right):

https://codereview.chromium.org/1910063003/diff/1/base/time/time_win.cc#newco...
base/time/time_win.cc:531:
GetQueryThreadCycleTimeFunction()(thread_handle.platform_handle(),
We can call ::QueryThreadCycleTime() directly and remove
GetQueryThreadCycleTimeFunction() above now that we don't support XP.

https://codereview.chromium.org/1910063003/diff/1/content/gpu/gpu_watchdog_th...
File content/gpu/gpu_watchdog_thread.cc (right):

https://codereview.chromium.org/1910063003/diff/1/content/gpu/gpu_watchdog_th...
content/gpu/gpu_watchdog_thread.cc:247: base::TimeDelta time_since_arm =
GetWatchedThreadTime() - arm_cpu_time_;
ThreadTicks::Now() returns a null ThreadTicks before it is initialized. That
means that |time_since_arm| can be zero just because ThreadTicks is not
initialized yet.

You should do something like:

ThreadTicks now = ThreadTicks::Now();
TimeDelta time_since_arm = now - arm_cpu_time_;
if (use_thread_cpu_time_ && !now.is_null() && (time_since_arm < timeout_)) {

https://codereview.chromium.org/1910063003/diff/1/content/gpu/gpu_watchdog_th...
content/gpu/gpu_watchdog_thread.cc:429: base::ThreadTicks();
Ideally, you would store a ThreadTicks and compute deltas between ThreadTicks
here
https://code.google.com/p/chromium/codesearch#chromium/src/content/gpu/gpu_wa....
There is no guarantee that the absolute value of ThreadTicks is meaningful (I'm
pretty sure that it is meaningful, but it's not guaranteed by the API).

You could do ThreadTicks() + TimeDelta::FromMilliseconds(... below to convert
the values that you get from ::GetThreadTimes to ThreadTicks.

[stanisc, 2016-04-21 21:10:48]

On 2016/04/21 20:27:56, Mark Mentovai wrote:
> TimeTicks::Now()’s a pretty terrible name, but you’re not inventing it. No
> objections to the new code in base. LGTM.

Actually it looks like I can't overload Now() because the test code now fails to
resolve "&ThreadTicks::Now". Any suggestion for a better name? How about
GetForThread?

[Mark Mentovai, 2016-04-21 21:13:33]

On 2016/04/21 21:10:48, stanisc wrote:
> On 2016/04/21 20:27:56, Mark Mentovai wrote:
> > TimeTicks::Now()’s a pretty terrible name, but you’re not inventing it. No
> > objections to the new code in base. LGTM.
> 
> Actually it looks like I can't overload Now() because the test code now fails
to
> resolve "&ThreadTicks::Now". Any suggestion for a better name? How about
> GetForThread?

That’d work. Revise the (int64_t) constructor’s comment to suggest Now() or
GetForThread().

[stanisc, 2016-04-22 01:11:46]

Addressed CR feedback

https://codereview.chromium.org/1910063003/diff/1/base/time/time_win.cc
File base/time/time_win.cc (right):

https://codereview.chromium.org/1910063003/diff/1/base/time/time_win.cc#newco...
base/time/time_win.cc:531:
GetQueryThreadCycleTimeFunction()(thread_handle.platform_handle(),
On 2016/04/21 21:08:37, fdoray wrote:
> We can call ::QueryThreadCycleTime() directly and remove
> GetQueryThreadCycleTimeFunction() above now that we don't support XP.

OK, I've removed GetQueryThreadCycleTimeFunction().

https://codereview.chromium.org/1910063003/diff/1/content/gpu/gpu_watchdog_th...
File content/gpu/gpu_watchdog_thread.cc (right):

https://codereview.chromium.org/1910063003/diff/1/content/gpu/gpu_watchdog_th...
content/gpu/gpu_watchdog_thread.cc:247: base::TimeDelta time_since_arm =
GetWatchedThreadTime() - arm_cpu_time_;
On 2016/04/21 21:08:38, fdoray wrote:
> ThreadTicks::Now() returns a null ThreadTicks before it is initialized. That
> means that |time_since_arm| can be zero just because ThreadTicks is not
> initialized yet.
> 
> You should do something like:
> 
> ThreadTicks now = ThreadTicks::Now();
> TimeDelta time_since_arm = now - arm_cpu_time_;
> if (use_thread_cpu_time_ && !now.is_null() && (time_since_arm < timeout_)) {

I think it should be fine for GetWatchedThreadTime to return null ThreadTicks.
That is what the current implementation returns most of the time anyway. The
initialization of ThreadTicks is supposed to happen within 50 ms. The typical
interval for the watchdog check is 10 seconds. The first time OnCheck is called
is pretty much immediately after the thread starts so the returned null
ThreadTicks value would be very close to the actual thread time - close enough
for the needs of this code.
And having a !now.is_null() check would likely break the logic when the legacy
part of GetWatchedThreadTime is used.

I could probably add a conditional ThreadTicks::WaitUntilInitialized in the
GpuWatchdogThread::Init(). That would delay the thread startup by 50 ms. Would
that be a better option?

https://codereview.chromium.org/1910063003/diff/1/content/gpu/gpu_watchdog_th...
content/gpu/gpu_watchdog_thread.cc:429: base::ThreadTicks();
On 2016/04/21 21:08:37, fdoray wrote:
> Ideally, you would store a ThreadTicks and compute deltas between ThreadTicks
> here
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/gpu/gpu_wa....
> There is no guarantee that the absolute value of ThreadTicks is meaningful
(I'm
> pretty sure that it is meaningful, but it's not guaranteed by the API).
> 
> You could do ThreadTicks() + TimeDelta::FromMilliseconds(... below to convert
> the values that you get from ::GetThreadTimes to ThreadTicks.

Yes, it seems like a good idea for this method to return ThreadTicks rather than
TimeDelta. Done that.

[Mark Mentovai, 2016-04-22 01:14:47]

LGTM in base

https://codereview.chromium.org/1910063003/diff/20001/base/time/time.h
File base/time/time.h (right):

https://codereview.chromium.org/1910063003/diff/20001/base/time/time.h#newcod...
base/time/time.h:762: // Please use Now() or GetForThread to create a new
object. This is for
GetForThread() with parentheses to match how Now() is written.

[fdoray, 2016-04-22 13:04:50]

lgtm

[stanisc, 2016-04-22 18:52:54]

The CQ bit was checked by stanisc@chromium.org

[stanisc, 2016-04-22 18:52:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
jbauman@chromium.org, mark@chromium.org, fdoray@chromium.org
Link to the patchset: https://codereview.chromium.org/1910063003/#ps40001
(title: "Fixed comment")

[commit-bot: I haz the power, 2016-04-22 18:53:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1910063003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1910063003/40001

[commit-bot: I haz the power, 2016-04-22 19:37:37]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-22 19:51:46]

Description was changed from

==========
More accurate implementation of watched thread time for Gpu Watchdog.

This change leverages an accurate implementation of ThreadTicks to
allow GPU Watchdog thread report watched thread specific CPU time
deltas to help with GPU hang crash dump analysis.

This includes a small refactoring of ThreadTicks class that adds an
overload of ThreadTicks::Now that accepts an arbitrary thread handle
as an argument. That is limited to Windows OS only but if necessary we
could extend it to other platforms.

BUG=596190
==========

to

==========
More accurate implementation of watched thread time for Gpu Watchdog.

This change leverages an accurate implementation of ThreadTicks to
allow GPU Watchdog thread report watched thread specific CPU time
deltas to help with GPU hang crash dump analysis.

This includes a small refactoring of ThreadTicks class that adds an
overload of ThreadTicks::Now that accepts an arbitrary thread handle
as an argument. That is limited to Windows OS only but if necessary we
could extend it to other platforms.

BUG=596190

Committed: https://crrev.com/f5173e2286a6735cdefeda91c9dc5359dc4b23af
Cr-Commit-Position: refs/heads/master@{#389210}
==========

[commit-bot: I haz the power, 2016-04-22 19:51:47]

Patchset 3 (id:??) landed as
https://crrev.com/f5173e2286a6735cdefeda91c9dc5359dc4b23af
Cr-Commit-Position: refs/heads/master@{#389210}