Add comments explaining Task Manager idle wakeups computation.

Add comments explaining Task Manager idle wakeups computation.

Adds some comments that would have been helpful to me as I traced the code to understand it. I think they will be helpful to future readers of the code.

BUG=687424
Review-Url: https://codereview.chromium.org/2667023004
Cr-Commit-Position: refs/heads/master@{#447890}
Committed: https://chromium.googlesource.com/chromium/src/+/36fa3dee1690ef5561ee86c653fc9b56fcba73e1

[shrike, 2017-02-01 02:10:07]

Description was changed from

==========
Remove error in Task Manager idle wakeups computation.

Removes the extra "time_delta / 2" term in calculating idle wakeups
per second on Mac and Linux. This term appears to be a mistake as it
has different units than wakeups_delta_for_ms, the term to which it's
being added.

BUG=687424
==========

to

==========
Remove error in Task Manager idle wakeups computation.

Removes the extra "time_delta / 2" term in calculating idle wakeups
per second on Mac and Linux. This term appears to be a mistake as it
has different units than wakeups_delta_for_ms, the term to which it's
being added.

BUG=687424
==========

[shrike, 2017-02-01 02:10:07]

shrike@chromium.org changed reviewers:
+ brucedawson@chromium.org

[shrike, 2017-02-01 02:10:57]

brucedawson@ - PTAL (for a sanity check).

[brucedawson, 2017-02-02 00:55:29]

I'm pretty sure it's correct as is. I've used this old trick too many times I
think.

https://codereview.chromium.org/2667023004/diff/20001/base/process/process_me...
File base/process/process_metrics.cc (left):

https://codereview.chromium.org/2667023004/diff/20001/base/process/process_me...
base/process/process_metrics.cc:89: return (wakeups_delta_for_ms + time_delta /
2) / time_delta;
This term is presumably (see comment on line 87) to round the value to nearest.
Otherwise the result is truncated, leading to systemic bias.

Adding the divisor divided by two is equivalent to going:

    (int)(((double)wakeups_delta_for_ms / time_delta) + 0.5)

but adding time_delta / 2 avoids converting to floating-point and back.

It's easiest to think of it as being equivalent (modulo computer arithmetic
quirks) to:

wakeups_delta_for_ms / time_delta + (time_delta / 2) / time_delta

[brucedawson, 2017-02-02 00:55:56]

BTW, stanisc@ probably knows the code best.

[shrike, 2017-02-02 16:09:48]

Description was changed from

==========
Remove error in Task Manager idle wakeups computation.

Removes the extra "time_delta / 2" term in calculating idle wakeups
per second on Mac and Linux. This term appears to be a mistake as it
has different units than wakeups_delta_for_ms, the term to which it's
being added.

BUG=687424
==========

to

==========
Add comments explaining Task Manager idle wakeups computation.

Adds some comments that would have been helpful to me as I traced the code to
understand it. I think they will be helpful to future readers of the code.

BUG=687424
==========

[shrike, 2017-02-02 16:09:48]

shrike@chromium.org changed reviewers:
+ mark@chromium.org

[shrike, 2017-02-02 16:12:50]

mark@ - PTAL

https://codereview.chromium.org/2667023004/diff/20001/base/process/process_me...
File base/process/process_metrics.cc (left):

https://codereview.chromium.org/2667023004/diff/20001/base/process/process_me...
base/process/process_metrics.cc:89: return (wakeups_delta_for_ms + time_delta /
2) / time_delta;
On 2017/02/02 00:55:29, brucedawson (slow) wrote:
> This term is presumably (see comment on line 87) to round the value to
nearest.
> Otherwise the result is truncated, leading to systemic bias.
> 
> Adding the divisor divided by two is equivalent to going:
> 
>     (int)(((double)wakeups_delta_for_ms / time_delta) + 0.5)
> 
> but adding time_delta / 2 avoids converting to floating-point and back.
> 
> It's easiest to think of it as being equivalent (modulo computer arithmetic
> quirks) to:
> 
> wakeups_delta_for_ms / time_delta + (time_delta / 2) / time_delta

Thank you brucedawson@. I saw the comment about rounding but didn't understand
it. This is a clever approach to adding 1/2, but I don't think it's clear, and I
dislike that unit-wise it doesn't make sense. I'm going to expand the comment.

[brucedawson, 2017-02-02 19:52:03]

lgtm

https://codereview.chromium.org/2667023004/diff/40001/base/process/process_me...
File base/process/process_metrics.cc (right):

https://codereview.chromium.org/2667023004/diff/40001/base/process/process_me...
base/process/process_metrics.cc:89: // dropping down into floating point).
It's funny how the set of idioms that are obvious and not needing explanation
varies between programmers. I agree that this one is non-obvious to enough
readers to justify a more specific comment.

https://codereview.chromium.org/2667023004/diff/40001/base/process/process_me...
File base/process/process_metrics_mac.cc (right):

https://codereview.chromium.org/2667023004/diff/40001/base/process/process_me...
base/process/process_metrics_mac.cc:337: // power usage. See the powermetrics
man page for more info.
That's pretty cool. I wish we had that information on Windows as well.

[Mark Mentovai, 2017-02-02 21:55:12]

LGTM

[shrike, 2017-02-02 22:29:55]

The CQ bit was checked by shrike@chromium.org

[commit-bot: I haz the power, 2017-02-02 22:30:55]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-03 00:05:39]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1486074595832880,
"parent_rev": "c61a3771648a29fb70430d87e2cd5b932388680f", "commit_rev":
"36fa3dee1690ef5561ee86c653fc9b56fcba73e1"}

[commit-bot: I haz the power, 2017-02-03 00:06:15]

Description was changed from

==========
Add comments explaining Task Manager idle wakeups computation.

Adds some comments that would have been helpful to me as I traced the code to
understand it. I think they will be helpful to future readers of the code.

BUG=687424
==========

to

==========
Add comments explaining Task Manager idle wakeups computation.

Adds some comments that would have been helpful to me as I traced the code to
understand it. I think they will be helpful to future readers of the code.

BUG=687424

Review-Url: https://codereview.chromium.org/2667023004
Cr-Commit-Position: refs/heads/master@{#447890}
Committed:
https://chromium.googlesource.com/chromium/src/+/36fa3dee1690ef5561ee86c653fc...
==========

[commit-bot: I haz the power, 2017-02-03 00:06:16]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/36fa3dee1690ef5561ee86c653fc...