More accurate implementation of watched thread time for Gpu Watchdog.

content/gpu/gpu_watchdog_thread.cc

 // Copyright (c) 2012 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 "content/gpu/gpu_watchdog_thread.h"
 
 #include <errno.h>
 #include <stdint.h>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
 #include "base/compiler_specific.h"
 #include "base/debug/alias.h"
 #include "base/files/file_util.h"
 #include "base/location.h"
 #include "base/macros.h"
 #include "base/power_monitor/power_monitor.h"
 #include "base/process/process.h"
 #include "base/single_thread_task_runner.h"
+#include "base/threading/platform_thread.h"
 #include "build/build_config.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/common/result_codes.h"
 
 #if defined(OS_WIN)
 #include <windows.h>
 #endif
 
 namespace content {
 namespace {
@@ skipping 205 matching lines @@
 }
 
 // Use the --disable-gpu-watchdog command line switch to disable this.
 void GpuWatchdogThread::DeliberatelyTerminateToRecoverFromHang() {
   // Should not get here while the system is suspended.
   DCHECK(!suspended_);
 
 #if defined(OS_WIN)
   // Defer termination until a certain amount of CPU time has elapsed on the
   // watched thread.
   base::TimeDelta time_since_arm = GetWatchedThreadTime() - arm_cpu_time_;

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

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_)) {

[stanisc, 2016/04/22 01:11:45]

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?

   if (use_thread_cpu_time_ && (time_since_arm < timeout_)) {
     message_loop()->PostDelayedTask(
         FROM_HERE,
         base::Bind(
             &GpuWatchdogThread::DeliberatelyTerminateToRecoverFromHang,
             weak_factory_.GetWeakPtr()),
         timeout_ - time_since_arm);
     return;
   }
 #endif
@@ skipping 157 matching lines @@
   suspended_ = false;
   resume_time_ = base::Time::Now();
 
   // After resuming jump-start the watchdog again.
   armed_ = false;
   OnCheck(true);
 }
 
 #if defined(OS_WIN)
 base::TimeDelta GpuWatchdogThread::GetWatchedThreadTime() {
-  FILETIME creation_time;
-  FILETIME exit_time;
-  FILETIME user_time;
-  FILETIME kernel_time;
-  BOOL result = GetThreadTimes(watched_thread_handle_,
-                               &creation_time,
-                               &exit_time,
-                               &kernel_time,
-                               &user_time);
-  DCHECK(result);
+  if (base::ThreadTicks::IsSupported()) {
+    // Convert ThreadTicks::Now() to TimeDelta.
+    return base::ThreadTicks::Now(
+               base::PlatformThreadHandle(watched_thread_handle_)) -
+           base::ThreadTicks();

[fdoray, 2016/04/21 21:08:37]

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/22 01:11:45]

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

 
-  ULARGE_INTEGER user_time64;
-  user_time64.HighPart = user_time.dwHighDateTime;
-  user_time64.LowPart = user_time.dwLowDateTime;
+  } else {
+    // Use GetThreadTimes as a backup mechanism.
+    FILETIME creation_time;
+    FILETIME exit_time;
+    FILETIME user_time;
+    FILETIME kernel_time;
+    BOOL result = GetThreadTimes(watched_thread_handle_, &creation_time,
+                                 &exit_time, &kernel_time, &user_time);
+    DCHECK(result);
 
-  ULARGE_INTEGER kernel_time64;
-  kernel_time64.HighPart = kernel_time.dwHighDateTime;
-  kernel_time64.LowPart = kernel_time.dwLowDateTime;
+    ULARGE_INTEGER user_time64;
+    user_time64.HighPart = user_time.dwHighDateTime;
+    user_time64.LowPart = user_time.dwLowDateTime;
 
-  // Time is reported in units of 100 nanoseconds. Kernel and user time are
-  // summed to deal with to kinds of hangs. One is where the GPU process is
-  // stuck in user level, never calling into the kernel and kernel time is
-  // not increasing. The other is where either the kernel hangs and never
-  // returns to user level or where user level code
-  // calls into kernel level repeatedly, giving up its quanta before it is
-  // tracked, for example a loop that repeatedly Sleeps.
-  return base::TimeDelta::FromMilliseconds(static_cast<int64_t>(
-      (user_time64.QuadPart + kernel_time64.QuadPart) / 10000));
+    ULARGE_INTEGER kernel_time64;
+    kernel_time64.HighPart = kernel_time.dwHighDateTime;
+    kernel_time64.LowPart = kernel_time.dwLowDateTime;
+
+    // Time is reported in units of 100 nanoseconds. Kernel and user time are
+    // summed to deal with to kinds of hangs. One is where the GPU process is
+    // stuck in user level, never calling into the kernel and kernel time is
+    // not increasing. The other is where either the kernel hangs and never
+    // returns to user level or where user level code
+    // calls into kernel level repeatedly, giving up its quanta before it is
+    // tracked, for example a loop that repeatedly Sleeps.
+    return base::TimeDelta::FromMilliseconds(static_cast<int64_t>(
+        (user_time64.QuadPart + kernel_time64.QuadPart) / 10000));
+  }
 }
 #endif
 
 }  // namespace content