Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(10)

Issue 1910063003: More accurate implementation of watched thread time for Gpu Watchdog. (Closed)

Created:
3 years, 5 months ago by stanisc
Modified:
3 years, 5 months ago
CC:
chromium-reviews, darin-cc_chromium.org, jam, piman+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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}

Patch Set 1 #

Total comments: 6

Patch Set 2 : Addressed CR feedback #

Total comments: 1

Patch Set 3 : Fixed comment #

Unified diffs Side-by-side diffs Delta from patch set Stats (+56 lines, -47 lines) Patch
M base/time/time.h View 1 2 2 chunks +10 lines, -3 lines 0 comments Download
M base/time/time_win.cc View 1 4 chunks +9 lines, -13 lines 0 comments Download
M content/gpu/gpu_watchdog_thread.h View 1 2 chunks +2 lines, -2 lines 0 comments Download
M content/gpu/gpu_watchdog_thread.cc View 1 2 chunks +35 lines, -29 lines 0 comments Download

Messages

Total messages: 16 (4 generated)
stanisc
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 ...
3 years, 5 months ago (2016-04-21 19:43:10 UTC) #2
Mark Mentovai
TimeTicks::Now()’s a pretty terrible name, but you’re not inventing it. No objections to the new ...
3 years, 5 months ago (2016-04-21 20:27:56 UTC) #3
jbauman
lgtm, though we'll want to check the number of hang reports after this is checked ...
3 years, 5 months ago (2016-04-21 20:59:53 UTC) #4
fdoray
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#newcode531 base/time/time_win.cc:531: GetQueryThreadCycleTimeFunction()(thread_handle.platform_handle(), We can call ::QueryThreadCycleTime() directly ...
3 years, 5 months ago (2016-04-21 21:08:38 UTC) #5
stanisc
On 2016/04/21 20:27:56, Mark Mentovai wrote: > TimeTicks::Now()’s a pretty terrible name, but you’re not ...
3 years, 5 months ago (2016-04-21 21:10:48 UTC) #6
Mark Mentovai
On 2016/04/21 21:10:48, stanisc wrote: > On 2016/04/21 20:27:56, Mark Mentovai wrote: > > TimeTicks::Now()’s ...
3 years, 5 months ago (2016-04-21 21:13:33 UTC) #7
stanisc
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#newcode531 base/time/time_win.cc:531: GetQueryThreadCycleTimeFunction()(thread_handle.platform_handle(), On 2016/04/21 21:08:37, fdoray wrote: ...
3 years, 5 months ago (2016-04-22 01:11:46 UTC) #8
Mark Mentovai
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#newcode762 base/time/time.h:762: // Please use Now() or GetForThread ...
3 years, 5 months ago (2016-04-22 01:14:47 UTC) #9
fdoray
lgtm
3 years, 5 months ago (2016-04-22 13:04:50 UTC) #10
commit-bot: I haz the power
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
3 years, 5 months ago (2016-04-22 18:53:46 UTC) #13
commit-bot: I haz the power
Committed patchset #3 (id:40001)
3 years, 5 months ago (2016-04-22 19:37:37 UTC) #14
commit-bot: I haz the power
3 years, 5 months ago (2016-04-22 19:51:47 UTC) #16
Message was sent while issue was closed.
Patchset 3 (id:??) landed as
https://crrev.com/f5173e2286a6735cdefeda91c9dc5359dc4b23af
Cr-Commit-Position: refs/heads/master@{#389210}

Powered by Google App Engine
This is Rietveld 408576698