content: Add heartbeat trace for CategorizedWorkerPool.

content: Add heartbeat trace for CategorizedWorkerPool.

When CategorizedWorkerPoolThread is waiting for more tasks, it looks
like it is hung as it starts working only after the related condition
variable is signaled. Adding heartbeat trace can help detect whether
thread is live or not. The category for trace is purposefully kept
different "heartbeat", so as to capture traces for long duration.

BUG=618262

[prashant.n, 2016-06-07 09:56:54]

prashant.n@samsung.com changed reviewers:
+ danakj@chromium.org, jam@chromium.org, reveman@chromium.org,
robliao@chromium.org

[prashant.n, 2016-06-07 09:56:55]

ptal.

[robliao, 2016-06-07 14:44:19]

Description was changed from

==========
content: Add heatbeat trace for CategorizedWorkerPool.

When CategorizedWorkerPoolThread is waiting for more tasks, it looks
like it is hung as it starts working only after the related condition
variable is signaled. Adding heartbeat trace can help detect whether
thread is live or not. The category for trace is purposefully kept
different "hearbeat", so as to capture traces for long duration.

BUG=
==========

to

==========
content: Add heatbeat trace for CategorizedWorkerPool.

When CategorizedWorkerPoolThread is waiting for more tasks, it looks
like it is hung as it starts working only after the related condition
variable is signaled. Adding heartbeat trace can help detect whether
thread is live or not. The category for trace is purposefully kept
different "heartbeat", so as to capture traces for long duration.

BUG=
==========

[robliao, 2016-06-07 14:44:47]

Description was changed from

==========
content: Add heatbeat trace for CategorizedWorkerPool.

When CategorizedWorkerPoolThread is waiting for more tasks, it looks
like it is hung as it starts working only after the related condition
variable is signaled. Adding heartbeat trace can help detect whether
thread is live or not. The category for trace is purposefully kept
different "heartbeat", so as to capture traces for long duration.

BUG=
==========

to

==========
content: Add heartbeat trace for CategorizedWorkerPool.

When CategorizedWorkerPoolThread is waiting for more tasks, it looks
like it is hung as it starts working only after the related condition
variable is signaled. Adding heartbeat trace can help detect whether
thread is live or not. The category for trace is purposefully kept
different "heartbeat", so as to capture traces for long duration.

BUG=
==========

[robliao, 2016-06-07 15:40:30]

https://codereview.chromium.org/2047433004/diff/1/base/synchronization/condit...
File base/synchronization/condition_variable.h (right):

https://codereview.chromium.org/2047433004/diff/1/base/synchronization/condit...
base/synchronization/condition_variable.h:97: bool TimedWait(const TimeDelta&
max_time);
Add a unit test to check this.

https://codereview.chromium.org/2047433004/diff/1/base/synchronization/condit...
File base/synchronization/condition_variable_win.cc (right):

https://codereview.chromium.org/2047433004/diff/1/base/synchronization/condit...
base/synchronization/condition_variable_win.cc:31: bool timed_out = false;
Minimal scope: Move to right before the if check below.

https://codereview.chromium.org/2047433004/diff/1/base/synchronization/condit...
base/synchronization/condition_variable_win.cc:39: timed_out = GetLastError() ==
WAIT_TIMEOUT;
Looks like your changelist uncovered a longstanding bug here. The comparison
should be GetLastError() == ERROR_TIMEOUT.

https://codereview.chromium.org/2047433004/diff/1/base/synchronization/condit...
base/synchronization/condition_variable_win.cc:40: DCHECK(max_time !=
TimeDelta::FromMilliseconds(INFINITE) || !timed_out);
This DCHECK will pass all cases where max_time is not set to infinite, missing
real failures where max_time is not INFINITE.

There's not a real need to check that we receive a timeout with INFINITE as its
specified by the Windows platform that INFINITE waits will not timeout.

As a result, the original DCHECK can still stand, and equivalently here
DCHECK(timed_out).

https://codereview.chromium.org/2047433004/diff/1/content/renderer/categorize...
File content/renderer/categorized_worker_pool.cc (right):

https://codereview.chromium.org/2047433004/diff/1/content/renderer/categorize...
content/renderer/categorized_worker_pool.cc:20: const int
kHeartbeatIntervalInSeconds = 60;  // 1 minute
constexpr

https://codereview.chromium.org/2047433004/diff/1/content/renderer/categorize...
content/renderer/categorized_worker_pool.cc:254: while (true) {
while (has_ready_to_run_tasks_cv->TimedWait(timeout))

[prashant.n, 2016-06-07 15:49:54]

I'll modify the patch as suggested and will add a unittest.

https://codereview.chromium.org/2047433004/diff/1/base/synchronization/condit...
File base/synchronization/condition_variable_win.cc (right):

https://codereview.chromium.org/2047433004/diff/1/base/synchronization/condit...
base/synchronization/condition_variable_win.cc:39: timed_out = GetLastError() ==
WAIT_TIMEOUT;
On 2016/06/07 15:40:30, robliao wrote:
> Looks like your changelist uncovered a longstanding bug here. The comparison
> should be GetLastError() == ERROR_TIMEOUT.

I just copied earlier code, I'll make chnages suggested by you.

https://codereview.chromium.org/2047433004/diff/1/base/synchronization/condit...
base/synchronization/condition_variable_win.cc:40: DCHECK(max_time !=
TimeDelta::FromMilliseconds(INFINITE) || !timed_out);
On 2016/06/07 15:40:30, robliao wrote:
> This DCHECK will pass all cases where max_time is not set to infinite, missing
> real failures where max_time is not INFINITE.
> 
> There's not a real need to check that we receive a timeout with INFINITE as
its
> specified by the Windows platform that INFINITE waits will not timeout.
> 
> As a result, the original DCHECK can still stand, and equivalently here
> DCHECK(timed_out).

I'll correct this. May be I'll add two explicit dchecks with error messages.

[jam, 2016-06-07 22:16:01]

is there a bug for this? it's not clear to me that this is an actual problem we
need to solve

[prashant.n, 2016-06-08 03:08:04]

On 2016/06/07 22:16:01, jam wrote:
> is there a bug for this? it's not clear to me that this is an actual problem
we
> need to solve

Bug 617010. It shows renderer hang. If we introduce heartbeats then it would
help indentifying whether it is really hanging or waiting for more tasks to get
scheduled.

[prashant.n, 2016-06-08 03:08:37]

Description was changed from

==========
content: Add heartbeat trace for CategorizedWorkerPool.

When CategorizedWorkerPoolThread is waiting for more tasks, it looks
like it is hung as it starts working only after the related condition
variable is signaled. Adding heartbeat trace can help detect whether
thread is live or not. The category for trace is purposefully kept
different "heartbeat", so as to capture traces for long duration.

BUG=
==========

to

==========
content: Add heartbeat trace for CategorizedWorkerPool.

When CategorizedWorkerPoolThread is waiting for more tasks, it looks
like it is hung as it starts working only after the related condition
variable is signaled. Adding heartbeat trace can help detect whether
thread is live or not. The category for trace is purposefully kept
different "heartbeat", so as to capture traces for long duration.

BUG=617010
==========

[prashant.n, 2016-06-08 11:23:27]

Description was changed from

==========
content: Add heartbeat trace for CategorizedWorkerPool.

When CategorizedWorkerPoolThread is waiting for more tasks, it looks
like it is hung as it starts working only after the related condition
variable is signaled. Adding heartbeat trace can help detect whether
thread is live or not. The category for trace is purposefully kept
different "heartbeat", so as to capture traces for long duration.

BUG=617010
==========

to

==========
content: Add heartbeat trace for CategorizedWorkerPool.

When CategorizedWorkerPoolThread is waiting for more tasks, it looks
like it is hung as it starts working only after the related condition
variable is signaled. Adding heartbeat trace can help detect whether
thread is live or not. The category for trace is purposefully kept
different "heartbeat", so as to capture traces for long duration.

BUG=618262
==========

[prashant.n, 2016-06-08 13:37:57]

ptal.