Task manager: Get physical memory efficiently for all processes from SharedSampler

Task manager: Get physical memory efficiently for all processes from SharedSampler.

This leverages recently introduced SharedSampler to get
Physical Memory (Memory column) efficiently for all
processes in one go. This applies only to Windows
implementation. Two other memory columns which aren't
shown by default are still mapped to the old, more
expensive implementation.

BUG=618862
Committed: https://crrev.com/a8d8427d83905a9fc64242dde7b21f1a8eda839c
Cr-Commit-Position: refs/heads/master@{#415066}

[stanisc, 2016-08-13 00:47:56]

The CQ bit was checked by stanisc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-13 00:48:27]

Dry run: 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, 2016-08-13 02:14:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-13 02:14:17]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[stanisc, 2016-08-25 01:07:08]

The CQ bit was checked by stanisc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-25 01:07:50]

Dry run: 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, 2016-08-25 02:08:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-25 02:08:15]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[stanisc, 2016-08-25 18:50:01]

The CQ bit was checked by stanisc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-25 18:51:11]

Dry run: 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, 2016-08-25 20:41:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-25 20:41:51]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[stanisc, 2016-08-25 22:59:09]

The CQ bit was checked by stanisc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-25 22:59:55]

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

[stanisc, 2016-08-25 23:50:19]

Description was changed from

==========
Initial implementation of getting Physical memory efficiently in TaskManager on
Windows

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

to

==========
Task manager: Get physical memory efficiently for all processes from
SharedSampler.

This leverages recently introduced SharedSampler to get
Physical Memory (Memory column) efficiently for all
processes in one go. This applies only to Windows
implementation. Two other memory columns which aren't shown by default are still
mapped to the old, more
expensive implementation.

BUG=618862
==========

[stanisc, 2016-08-25 23:51:01]

Description was changed from

==========
Task manager: Get physical memory efficiently for all processes from
SharedSampler.

This leverages recently introduced SharedSampler to get
Physical Memory (Memory column) efficiently for all
processes in one go. This applies only to Windows
implementation. Two other memory columns which aren't shown by default are still
mapped to the old, more
expensive implementation.

BUG=618862
==========

to

==========
Task manager: Get physical memory efficiently for all processes from
SharedSampler.

This leverages recently introduced SharedSampler to get
Physical Memory (Memory column) efficiently for all
processes in one go. This applies only to Windows
implementation. Two other memory columns which aren't
shown by default are still mapped to the old, more
expensive implementation.

BUG=618862
==========

[stanisc, 2016-08-25 23:51:46]

stanisc@chromium.org changed reviewers:
+ brucedawson@chromium.org, nick@chromium.org

[stanisc, 2016-08-25 23:51:47]

PTAL!

[commit-bot: I haz the power, 2016-08-26 00:22:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-26 00:22:26]

Dry run: This issue passed the CQ dry run.

[brucedawson, 2016-08-26 01:02:11]

https://codereview.chromium.org/2238403003/diff/60001/chrome/browser/task_man...
File chrome/browser/task_manager/sampling/shared_sampler_unittest_win.cc
(right):

https://codereview.chromium.org/2238403003/diff/60001/chrome/browser/task_man...
chrome/browser/task_manager/sampling/shared_sampler_unittest_win.cc:28: :
expected_refresh_type_(0), finished_refresh_type_(0),
We are now allowed to initialize member variables where they are declared
instead of in the constructor. I think it makes it clearer. Thoughts?

See "Non-Static Class Member Initializers" in https://chromium-cpp.appspot.com/

https://codereview.chromium.org/2238403003/diff/60001/chrome/browser/task_man...
chrome/browser/task_manager/sampling/shared_sampler_unittest_win.cc:119:
EXPECT_LT(0, idle_wakeups_per_second());
If we except a greater than zero rate now then why aren't we using EXPECT_GT
(with the arguments swapped). It's good to minimize the semantic distance
between the code and comments.

See later comment also.

https://codereview.chromium.org/2238403003/diff/60001/chrome/browser/task_man...
chrome/browser/task_manager/sampling/shared_sampler_unittest_win.cc:132:
std::unique_ptr<uint8_t[]> memory_block(new uint8_t[allocated_size]);
On further reflection...

This could be simplified to:

    std::vector<uint8_t> memory_block(allocated_size);

In one of your previous changes it was a performance nuisance that vector
initialized its data to zero. In this case it is exactly what we want, and the
memset could be avoided. Or, just ignore this comment and leave it as is. In the
unit test which this CL deletes the working set was tested with vector, FWIW.

https://codereview.chromium.org/2238403003/diff/60001/chrome/browser/task_man...
chrome/browser/task_manager/sampling/shared_sampler_unittest_win.cc:142:
EXPECT_LE(initial_value + allocated_size, physical_bytes());
I find this hard to read with EXPECT_LE. Have you considered swapping the
arguments and using EXPECT_GE?

Or maybe it's just me.

https://codereview.chromium.org/2238403003/diff/60001/chrome/browser/task_man...
File chrome/browser/task_manager/sampling/shared_sampler_win.cc (right):

https://codereview.chromium.org/2238403003/diff/60001/chrome/browser/task_man...
chrome/browser/task_manager/sampling/shared_sampler_win.cc:484:
process_data.physical_bytes =
It seems unfortunate that this is almost directly after the big comment block
talking about context switches. Maybe move the process_data declaration and this
statement to before the comment block?

https://codereview.chromium.org/2238403003/diff/60001/chrome/browser/win/priv...
File chrome/browser/win/private_working_set_snapshot_unittest.cc (left):

https://codereview.chromium.org/2238403003/diff/60001/chrome/browser/win/priv...
chrome/browser/win/private_working_set_snapshot_unittest.cc:55:
std::vector<char> big_memory(alloc_size);
Hey, here's an example of how to increase the working set using vector! :-)

https://codereview.chromium.org/2238403003/diff/60001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (left):

https://codereview.chromium.org/2238403003/diff/60001/chrome/chrome_browser.g...
chrome/chrome_browser.gypi:695: 'browser/win/private_working_set_snapshot.cc',
It makes me sad to see my code being deleted, but it's the right thing to do.

[stanisc, 2016-08-26 18:53:50]

The CQ bit was checked by stanisc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-26 18:54:51]

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

[stanisc, 2016-08-26 20:47:56]

Addressed Bruce's feedback

https://codereview.chromium.org/2238403003/diff/60001/chrome/browser/task_man...
File chrome/browser/task_manager/sampling/shared_sampler_unittest_win.cc
(right):

https://codereview.chromium.org/2238403003/diff/60001/chrome/browser/task_man...
chrome/browser/task_manager/sampling/shared_sampler_unittest_win.cc:28: :
expected_refresh_type_(0), finished_refresh_type_(0),
On 2016/08/26 01:02:10, brucedawson wrote:
> We are now allowed to initialize member variables where they are declared
> instead of in the constructor. I think it makes it clearer. Thoughts?
> 
> See "Non-Static Class Member Initializers" in
https://chromium-cpp.appspot.com/

Yes, I know we can do this now with C++ 11. Initially I initialized a couple of
integer fields inline but later thought that it would be better to initialize
all fields in one place. Anyway, I've changed this the way you suggested.

https://codereview.chromium.org/2238403003/diff/60001/chrome/browser/task_man...
chrome/browser/task_manager/sampling/shared_sampler_unittest_win.cc:119:
EXPECT_LT(0, idle_wakeups_per_second());
On 2016/08/26 01:02:10, brucedawson wrote:
> If we except a greater than zero rate now then why aren't we using EXPECT_GT
> (with the arguments swapped). It's good to minimize the semantic distance
> between the code and comments.
> 
> See later comment also.

The only reason I've done it this way is to keep it similar to EXPECT_EQ
statements where constants are almost always in the first place. I have no
problem changing this the way you suggested.

https://codereview.chromium.org/2238403003/diff/60001/chrome/browser/task_man...
chrome/browser/task_manager/sampling/shared_sampler_unittest_win.cc:132:
std::unique_ptr<uint8_t[]> memory_block(new uint8_t[allocated_size]);
On 2016/08/26 01:02:10, brucedawson wrote:
> On further reflection...
> 
> This could be simplified to:
> 
>     std::vector<uint8_t> memory_block(allocated_size);
> 
> In one of your previous changes it was a performance nuisance that vector
> initialized its data to zero. In this case it is exactly what we want, and the
> memset could be avoided. Or, just ignore this comment and leave it as is. In
the
> unit test which this CL deletes the working set was tested with vector, FWIW.

Good suggestion. Done.

https://codereview.chromium.org/2238403003/diff/60001/chrome/browser/task_man...
chrome/browser/task_manager/sampling/shared_sampler_unittest_win.cc:142:
EXPECT_LE(initial_value + allocated_size, physical_bytes());
On 2016/08/26 01:02:10, brucedawson wrote:
> I find this hard to read with EXPECT_LE. Have you considered swapping the
> arguments and using EXPECT_GE?
> 
> Or maybe it's just me.

Done.

https://codereview.chromium.org/2238403003/diff/60001/chrome/browser/win/priv...
File chrome/browser/win/private_working_set_snapshot_unittest.cc (left):

https://codereview.chromium.org/2238403003/diff/60001/chrome/browser/win/priv...
chrome/browser/win/private_working_set_snapshot_unittest.cc:55:
std::vector<char> big_memory(alloc_size);
On 2016/08/26 01:02:10, brucedawson wrote:
> Hey, here's an example of how to increase the working set using vector! :-)

I see.

[commit-bot: I haz the power, 2016-08-26 20:49:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-26 20:49:37]

Dry run: This issue passed the CQ dry run.

[brucedawson, 2016-08-26 21:47:28]

lgtm but I defer to Nick for the owner review.

https://codereview.chromium.org/2238403003/diff/60001/chrome/browser/task_man...
File chrome/browser/task_manager/sampling/shared_sampler_unittest_win.cc
(right):

https://codereview.chromium.org/2238403003/diff/60001/chrome/browser/task_man...
chrome/browser/task_manager/sampling/shared_sampler_unittest_win.cc:28: :
expected_refresh_type_(0), finished_refresh_type_(0),
It is possible to initialize all fields in the class definition and then
"re-initialize" some (blocking_pool_runner_) in the constructor. If this is done
in the initializer list then there is no cost - each element is only initialized
once.

Anyway, don't feel that I'm laying down the law, on this or other style issues -
these are judgment calls.

[ncarter (slow), 2016-08-29 16:32:58]

lgtm

[stanisc, 2016-08-29 18:13:04]

stanisc@chromium.org changed reviewers:
+ grt@chromium.org

[stanisc, 2016-08-29 18:13:05]

+grt@chromium.org for changes in chrome/browser/win

[grt (UTC plus 2), 2016-08-29 18:38:47]

On 2016/08/29 18:13:05, stanisc wrote:
> mailto:+grt@chromium.org for changes in chrome/browser/win

rubberstamp lgtm since the author of the deleted code has granted his lgtm.

[stanisc, 2016-08-29 19:32:16]

The CQ bit was checked by stanisc@chromium.org

[commit-bot: I haz the power, 2016-08-29 20:11:42]

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, 2016-08-30 03:59:10]

Description was changed from

==========
Task manager: Get physical memory efficiently for all processes from
SharedSampler.

This leverages recently introduced SharedSampler to get
Physical Memory (Memory column) efficiently for all
processes in one go. This applies only to Windows
implementation. Two other memory columns which aren't
shown by default are still mapped to the old, more
expensive implementation.

BUG=618862
==========

to

==========
Task manager: Get physical memory efficiently for all processes from
SharedSampler.

This leverages recently introduced SharedSampler to get
Physical Memory (Memory column) efficiently for all
processes in one go. This applies only to Windows
implementation. Two other memory columns which aren't
shown by default are still mapped to the old, more
expensive implementation.

BUG=618862
==========

[commit-bot: I haz the power, 2016-08-30 03:59:11]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-08-30 04:02:12]

Description was changed from

==========
Task manager: Get physical memory efficiently for all processes from
SharedSampler.

This leverages recently introduced SharedSampler to get
Physical Memory (Memory column) efficiently for all
processes in one go. This applies only to Windows
implementation. Two other memory columns which aren't
shown by default are still mapped to the old, more
expensive implementation.

BUG=618862
==========

to

==========
Task manager: Get physical memory efficiently for all processes from
SharedSampler.

This leverages recently introduced SharedSampler to get
Physical Memory (Memory column) efficiently for all
processes in one go. This applies only to Windows
implementation. Two other memory columns which aren't
shown by default are still mapped to the old, more
expensive implementation.

BUG=618862

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

[commit-bot: I haz the power, 2016-08-30 04:02:13]

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