Fix the Chrome OS ResourceReporter

Fix the Chrome OS ResourceReporter

The currently reported samples are noisy and useless. We can't extract
any useful info from them. This CL is an attempt to fix that by doing
the following:

- Report only intensive cpu (>70%) and memory (>60% of phy memory per cpu) users.
- Listen to the task manager only once, get the full data and stop it once there's
  something to report.
- Use system time instead of time ticks to accurately measure the time
  between reporting samples, so that it makes sense between reboots.
- Update the unit tests.

BUG=649486
TEST=unit_tests --gtest_filter=ResourceReporterTest.*

Committed: https://crrev.com/5b2b3d9f1582f69b1dac375a73881271ed7b96c5
Cr-Commit-Position: refs/heads/master@{#424855}

[afakhry, 2016-09-26 21:40:22]

Description was changed from

==========
Fix the Chrome OS ResourceReporter

The currently reported samples are noisy and useless. We can't extract
any useful info from them. This CL is an attempt to fix that by doing
the following:

- Report only intensive cpu and memory users.
- TODO: add more.

BUG=649486
TEST=unit_tests --gtest_filter=ResourceReporterTest.*
==========

to

==========
Fix the Chrome OS ResourceReporter

The currently reported samples are noisy and useless. We can't extract
any useful info from them. This CL is an attempt to fix that by doing
the following:

- Report only intensive cpu (>70%) and memory (>800MB) users.
- Listen to the task manager only once, get the full data and stop it.
- Update the unit tests.

BUG=649486
TEST=unit_tests --gtest_filter=ResourceReporterTest.*
==========

[afakhry, 2016-09-26 21:40:35]

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

[commit-bot: I haz the power, 2016-09-26 21:41:13]

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-09-26 22:38:36]

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

[commit-bot: I haz the power, 2016-09-26 22:38:36]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[afakhry, 2016-09-27 00:16:27]

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

[commit-bot: I haz the power, 2016-09-27 00:17:05]

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-09-27 01:13:27]

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

[commit-bot: I haz the power, 2016-09-27 01:13:28]

Dry run: This issue passed the CQ dry run.

[afakhry, 2016-09-29 00:46:57]

Description was changed from

==========
Fix the Chrome OS ResourceReporter

The currently reported samples are noisy and useless. We can't extract
any useful info from them. This CL is an attempt to fix that by doing
the following:

- Report only intensive cpu (>70%) and memory (>800MB) users.
- Listen to the task manager only once, get the full data and stop it.
- Update the unit tests.

BUG=649486
TEST=unit_tests --gtest_filter=ResourceReporterTest.*
==========

to

==========
Fix the Chrome OS ResourceReporter

The currently reported samples are noisy and useless. We can't extract
any useful info from them. This CL is an attempt to fix that by doing
the following:

- Report only intensive cpu (>70%) and memory (>800MB) users.
- Listen to the task manager only once, get the full data and stop it.
- Use system time instead of time ticks to accurately measure the time
  between reporting samples, so that it makes sense between reboots.
- Update the unit tests.

BUG=649486
TEST=unit_tests --gtest_filter=ResourceReporterTest.*
==========

[afakhry, 2016-09-29 00:55:37]

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

[commit-bot: I haz the power, 2016-09-29 00:56:01]

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

[afakhry, 2016-09-29 01:26:28]

afakhry@chromium.org changed reviewers:
+ nick@chromium.org

[afakhry, 2016-09-29 01:26:29]

Nick, could you please take a look at this CL? Thanks!

[commit-bot: I haz the power, 2016-09-29 01:53:57]

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

[commit-bot: I haz the power, 2016-09-29 01:53:58]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[afakhry, 2016-09-29 16:59:23]

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

[commit-bot: I haz the power, 2016-09-29 16:59:46]

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-09-29 17:37:42]

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

[commit-bot: I haz the power, 2016-09-29 17:37:43]

Dry run: This issue passed the CQ dry run.

[ncarter (slow), 2016-09-29 21:32:12]

Glad to see you're respinning this.

lgtm

https://codereview.chromium.org/2363223002/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/resource_reporter/resource_reporter.cc (right):

https://codereview.chromium.org/2363223002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.cc:68: constexpr
base::TimeDelta kMinimumTimeBetweenReports =
constexpr + TimeDelta is an amazing combo.

https://codereview.chromium.org/2363223002/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/resource_reporter/resource_reporter_unittest.cc
(right):

https://codereview.chromium.org/2363223002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter_unittest.cc:141:
content::TestBrowserThreadBundle thread_bundle_;
The TestBrowserThreadBundle usually works best when it's the first data member
of a class (i.e. that would let DummyTaskManager interact with browserthreads in
its ctor/dtor)

[afakhry, 2016-10-04 18:17:28]

Description was changed from

==========
Fix the Chrome OS ResourceReporter

The currently reported samples are noisy and useless. We can't extract
any useful info from them. This CL is an attempt to fix that by doing
the following:

- Report only intensive cpu (>70%) and memory (>800MB) users.
- Listen to the task manager only once, get the full data and stop it.
- Use system time instead of time ticks to accurately measure the time
  between reporting samples, so that it makes sense between reboots.
- Update the unit tests.

BUG=649486
TEST=unit_tests --gtest_filter=ResourceReporterTest.*
==========

to

==========
Fix the Chrome OS ResourceReporter

The currently reported samples are noisy and useless. We can't extract
any useful info from them. This CL is an attempt to fix that by doing
the following:

- Report only intensive cpu (>70%) and memory (>60% of phy memory per cpu)
users.
- Listen to the task manager only once, get the full data and stop it once
there's
  something to report.
- Use system time instead of time ticks to accurately measure the time
  between reporting samples, so that it makes sense between reboots.
- Update the unit tests.

BUG=649486
TEST=unit_tests --gtest_filter=ResourceReporterTest.*
==========

[afakhry, 2016-10-04 18:20:01]

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

[commit-bot: I haz the power, 2016-10-04 18:20:21]

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

[afakhry, 2016-10-04 18:28:58]

Nick, here's what I found.
- Hard coded memory usage threshold is a bad idea! It should be device
dependent. I found that 60% of physical memory per CPU is a good value.

- When we are under critical memory pressure, we're notified once per second!
It's a bad idea to stop observing and then reobserving the task manager when we
have no violators to report (and hence no rappor sampling time will be stored).
  -- So I made it that we keep listening to the TM as long as memory pressure
level remains critical AND we have nothing to report yet (i.e. next refresh
might give us something to report).

I also updated the unit tests to be much more accurate.

https://codereview.chromium.org/2363223002/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/resource_reporter/resource_reporter.cc (right):

https://codereview.chromium.org/2363223002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.cc:68: constexpr
base::TimeDelta kMinimumTimeBetweenReports =
On 2016/09/29 21:32:11, ncarter wrote:
> constexpr + TimeDelta is an amazing combo.

Absolutely!

https://codereview.chromium.org/2363223002/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/resource_reporter/resource_reporter_unittest.cc
(right):

https://codereview.chromium.org/2363223002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter_unittest.cc:141:
content::TestBrowserThreadBundle thread_bundle_;
On 2016/09/29 21:32:12, ncarter wrote:
> The TestBrowserThreadBundle usually works best when it's the first data member
> of a class (i.e. that would let DummyTaskManager interact with browserthreads
in
> its ctor/dtor)

Done.

[commit-bot: I haz the power, 2016-10-04 19:08:19]

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

[commit-bot: I haz the power, 2016-10-04 19:08:20]

Dry run: This issue passed the CQ dry run.

[ncarter (slow), 2016-10-07 19:04:15]

https://codereview.chromium.org/2363223002/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/resource_reporter/resource_reporter.h (right):

https://codereview.chromium.org/2363223002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.h:172:
task_manager::TaskManagerInterface* task_manager_for_testing_;
A for_testing_ data member should be a last resort, since it'll result in
codeblocks in the .cc file that are unreachable in unittests. Two ways I see to
switch this to dependency injection:

(1) is to pass the TaskManagerInterface in the ctor. That's awkward to
impossible with a singleton, so I'd suggest instead:

(2) Pass a TaskManagerInterface* to StartMonitoring.

Then this member can be, like, task_manager_to_observe_, with a comment. Then
ChromeBrowserMainPartsChromeos::PreMainMessageLoopRun() just gets the real TM
singleton instance, and passes it on.

[ncarter (slow), 2016-10-07 19:04:16]

https://codereview.chromium.org/2363223002/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/resource_reporter/resource_reporter.h (right):

https://codereview.chromium.org/2363223002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.h:172:
task_manager::TaskManagerInterface* task_manager_for_testing_;
A for_testing_ data member should be a last resort, since it'll result in
codeblocks in the .cc file that are unreachable in unittests. Two ways I see to
switch this to dependency injection:

(1) is to pass the TaskManagerInterface in the ctor. That's awkward to
impossible with a singleton, so I'd suggest instead:

(2) Pass a TaskManagerInterface* to StartMonitoring.

Then this member can be, like, task_manager_to_observe_, with a comment. Then
ChromeBrowserMainPartsChromeos::PreMainMessageLoopRun() just gets the real TM
singleton instance, and passes it on.

[afakhry, 2016-10-12 16:37:16]

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

[commit-bot: I haz the power, 2016-10-12 16:37:33]

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

[afakhry, 2016-10-12 16:37:36]

https://codereview.chromium.org/2363223002/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/resource_reporter/resource_reporter.h (right):

https://codereview.chromium.org/2363223002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.h:172:
task_manager::TaskManagerInterface* task_manager_for_testing_;
On 2016/10/07 19:04:16, ncarter wrote:
> A for_testing_ data member should be a last resort, since it'll result in
> codeblocks in the .cc file that are unreachable in unittests. Two ways I see
to
> switch this to dependency injection:
> 
> (1) is to pass the TaskManagerInterface in the ctor. That's awkward to
> impossible with a singleton, so I'd suggest instead:
> 
> (2) Pass a TaskManagerInterface* to StartMonitoring.
> 
> Then this member can be, like, task_manager_to_observe_, with a comment. Then
> ChromeBrowserMainPartsChromeos::PreMainMessageLoopRun() just gets the real TM
> singleton instance, and passes it on.

That's a great suggestion actually! Done.
Thanks!

[ncarter (slow), 2016-10-12 16:56:10]

lgtm

[afakhry, 2016-10-12 17:11:50]

afakhry@chromium.org changed reviewers:
+ gab@chromium.org, stevenjb@chromium.org

[afakhry, 2016-10-12 17:11:50]

gab@chromium.org: Please review changes in 
  >> browser_prefs.cc

stevenjb@chromium.org: Please review changes in 
  >> chrome_browser_main_chromeos.cc

[afakhry, 2016-10-12 17:12:41]

afakhry@chromium.org changed reviewers:
- gab@chromium.org, stevenjb@chromium.org

[afakhry, 2016-10-12 17:14:33]

afakhry@chromium.org changed reviewers:
+ derat@chromium.org, pam@chromium.org

[afakhry, 2016-10-12 17:14:34]

stevenjb and gab are OOO.

pam@chromium.org: Please review changes in 
  >> browser_prefs.cc

derat@chromium.org: Please review changes in 
  >> chrome_browser_main_chromeos.cc

[commit-bot: I haz the power, 2016-10-12 17:21:30]

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

[commit-bot: I haz the power, 2016-10-12 17:21:31]

Dry run: This issue passed the CQ dry run.

[Daniel Erat, 2016-10-12 19:56:37]

rubber-stamp lgtm for chrome_browser_main_chromeos.cc

[Pam (message me for reviews), 2016-10-12 21:06:06]

On 2016/10/12 19:56:37, Daniel Erat wrote:
> rubber-stamp lgtm for chrome_browser_main_chromeos.cc

rubber-stamp LGTM for chrome/browser/prefs/browser_prefs.cc

[afakhry, 2016-10-12 21:08:59]

The CQ bit was checked by afakhry@chromium.org

[commit-bot: I haz the power, 2016-10-12 21:09:36]

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-10-12 21:16:12]

Description was changed from

==========
Fix the Chrome OS ResourceReporter

The currently reported samples are noisy and useless. We can't extract
any useful info from them. This CL is an attempt to fix that by doing
the following:

- Report only intensive cpu (>70%) and memory (>60% of phy memory per cpu)
users.
- Listen to the task manager only once, get the full data and stop it once
there's
  something to report.
- Use system time instead of time ticks to accurately measure the time
  between reporting samples, so that it makes sense between reboots.
- Update the unit tests.

BUG=649486
TEST=unit_tests --gtest_filter=ResourceReporterTest.*
==========

to

==========
Fix the Chrome OS ResourceReporter

The currently reported samples are noisy and useless. We can't extract
any useful info from them. This CL is an attempt to fix that by doing
the following:

- Report only intensive cpu (>70%) and memory (>60% of phy memory per cpu)
users.
- Listen to the task manager only once, get the full data and stop it once
there's
  something to report.
- Use system time instead of time ticks to accurately measure the time
  between reporting samples, so that it makes sense between reboots.
- Update the unit tests.

BUG=649486
TEST=unit_tests --gtest_filter=ResourceReporterTest.*
==========

[commit-bot: I haz the power, 2016-10-12 21:16:13]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-10-12 21:19:03]

Description was changed from

==========
Fix the Chrome OS ResourceReporter

The currently reported samples are noisy and useless. We can't extract
any useful info from them. This CL is an attempt to fix that by doing
the following:

- Report only intensive cpu (>70%) and memory (>60% of phy memory per cpu)
users.
- Listen to the task manager only once, get the full data and stop it once
there's
  something to report.
- Use system time instead of time ticks to accurately measure the time
  between reporting samples, so that it makes sense between reboots.
- Update the unit tests.

BUG=649486
TEST=unit_tests --gtest_filter=ResourceReporterTest.*
==========

to

==========
Fix the Chrome OS ResourceReporter

The currently reported samples are noisy and useless. We can't extract
any useful info from them. This CL is an attempt to fix that by doing
the following:

- Report only intensive cpu (>70%) and memory (>60% of phy memory per cpu)
users.
- Listen to the task manager only once, get the full data and stop it once
there's
  something to report.
- Use system time instead of time ticks to accurately measure the time
  between reporting samples, so that it makes sense between reboots.
- Update the unit tests.

BUG=649486
TEST=unit_tests --gtest_filter=ResourceReporterTest.*

Committed: https://crrev.com/5b2b3d9f1582f69b1dac375a73881271ed7b96c5
Cr-Commit-Position: refs/heads/master@{#424855}
==========

[commit-bot: I haz the power, 2016-10-12 21:19:04]

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

[gab, 2016-10-17 19:12:16]

gab@chromium.org changed reviewers:
+ gab@chromium.org

[gab, 2016-10-17 19:12:17]

c/b/prefs lgtm % comment

https://codereview.chromium.org/2363223002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/resource_reporter/resource_reporter.cc (right):

https://codereview.chromium.org/2363223002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.cc:64: constexpr
char kLastRapporReportTimeKey[] =
Prefs are usually defined in their owns pref_names.h files, e.g.:
https://cs.chromium.org/search/?q=file:pref_names%5C.h&sq=package:chromium&ty...