[Mac] Report statistics more regularly in Mac memory pressure monitor

[Mac] Report statistics more regularly in Mac memory pressure monitor

Currently, we only report UMA statistics on receiving a pressure
change event from the system. This means that we could be missing
many ticks of "normal" pressure from sessions that never encounter
pressure, skewing our stats. Other platforms don't have this issue
since they poll.

This change piggybacks statistic reporting on GetCurrentPressureLevel()
which is called regularly by RendererFrameManager::GetMaxNumberOfSavedFrames()
This also allows us to enable reporting for Sierra.

To make this work, GetCurrentPressureLevel is now declared non-const.
There don't appear to be any const MemoryPressureMonitors in the codebase
at the moment, so this should be safe.

BUG=655304
Committed: https://crrev.com/f518237b7651d1eeb115bffcc51f11ee0e9078cc
Cr-Commit-Position: refs/heads/master@{#434660}

[lgrey, 2016-11-11 20:49:04]

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

[commit-bot: I haz the power, 2016-11-11 20:49:28]

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-11-11 21:52:14]

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

[commit-bot: I haz the power, 2016-11-11 21:52:15]

Dry run: This issue passed the CQ dry run.

[lgrey, 2016-11-14 17:12:15]

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

[commit-bot: I haz the power, 2016-11-14 17:12:58]

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-11-14 18:03:57]

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

[commit-bot: I haz the power, 2016-11-14 18:03:58]

Dry run: This issue passed the CQ dry run.

[lgrey, 2016-11-14 18:21:36]

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

[lgrey, 2016-11-14 18:21:36]

PTAL :)

[Mark Mentovai, 2016-11-14 19:12:16]

https://codereview.chromium.org/2495003004/diff/20001/base/memory/memory_pres...
File base/memory/memory_pressure_monitor_mac.cc (right):

https://codereview.chromium.org/2495003004/diff/20001/base/memory/memory_pres...
base/memory/memory_pressure_monitor_mac.cc:47:
last_statistic_report_(CFAbsoluteTimeGetCurrent()),
Is this an OK initial value? It’s not associated with a valid
last_pressure_level_.

https://codereview.chromium.org/2495003004/diff/20001/base/memory/memory_pres...
base/memory/memory_pressure_monitor_mac.cc:103: modf(since_last_report +
reporting_error_, &ticks_to_report);
Good practice is to not use modf(), but instead use std::modf(). This is tricky:
they’re different. modf() is the C function that works on doubles. std::modf()
is the C++11 function that has overloads for different floating-point types.
Using the std:: version keeps precision constant throughout a pipeline of
floating-point operations without requiring anyone to remember whether
CFTimeInterval is a float or a double.

This file already #include <cmath>, so std::modf() is available.

This comment is true of a lot of the <math.h> stuff: use <cmath> instead, but be
sure to use std::.

[lgrey, 2016-11-14 19:23:52]

https://codereview.chromium.org/2495003004/diff/20001/base/memory/memory_pres...
File base/memory/memory_pressure_monitor_mac.cc (right):

https://codereview.chromium.org/2495003004/diff/20001/base/memory/memory_pres...
base/memory/memory_pressure_monitor_mac.cc:47:
last_statistic_report_(CFAbsoluteTimeGetCurrent()),
On 2016/11/14 19:12:15, Mark Mentovai wrote:
> Is this an OK initial value? It’s not associated with a valid
> last_pressure_level_.

I think so. Worst case scenario is the application is launched in warn/critical
and we report an extra tick of normal.

https://codereview.chromium.org/2495003004/diff/20001/base/memory/memory_pres...
base/memory/memory_pressure_monitor_mac.cc:103: modf(since_last_report +
reporting_error_, &ticks_to_report);
On 2016/11/14 19:12:15, Mark Mentovai wrote:
> Good practice is to not use modf(), but instead use std::modf(). This is
tricky:
> they’re different. modf() is the C function that works on doubles. std::modf()
> is the C++11 function that has overloads for different floating-point types.
> Using the std:: version keeps precision constant throughout a pipeline of
> floating-point operations without requiring anyone to remember whether
> CFTimeInterval is a float or a double.
> 
> This file already #include <cmath>, so std::modf() is available.
> 
> This comment is true of a lot of the <math.h> stuff: use <cmath> instead, but
be
> sure to use std::.

Done.

[lgrey, 2016-11-18 15:32:22]

Ping :)

[Mark Mentovai, 2016-11-18 15:36:35]

No need to wait that long to ping! LGTM.

[lgrey, 2016-11-18 17:14:54]

I'm very embarrassed to say I didn't actually make the ticks 5 seconds, so:
updated. 

Now where's that shame cube emoji?

[Mark Mentovai, 2016-11-18 17:26:11]

LGTM

[lgrey, 2016-11-18 17:42:58]

Thanks, Mark!

Jayson, PTAL and let me know if this works for you.

[shrike, 2016-11-18 21:56:43]

lgtm

[lgrey, 2016-11-18 22:03:28]

On 2016/11/18 21:56:43, shrike wrote:
> lgtm

Thanks!

+ afakhry@ for ChromeOS resource reporter and halliwell@ for Chromecast

[lgrey, 2016-11-18 22:03:45]

lgrey@chromium.org changed reviewers:
+ afakhry@chromium.org, halliwell@chromium.org

[afakhry, 2016-11-18 22:32:04]

resource_reporter_unittest.cc lgtm

[halliwell, 2016-11-18 23:32:42]

On 2016/11/18 22:32:04, afakhry wrote:
> resource_reporter_unittest.cc lgtm

chromecast/ lgtm

[lgrey, 2016-11-28 15:04:30]

The CQ bit was checked by lgrey@chromium.org

[commit-bot: I haz the power, 2016-11-28 15:04:44]

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-11-28 15:52:00]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1480345470066280,
"parent_rev": "b89b0434cce300e61f948ed0e6c3cd8f89d6dd4b", "commit_rev":
"d95bb2062440650c03b206e5891f67ead3dc1c71"}

[commit-bot: I haz the power, 2016-11-28 15:52:27]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-28 15:53:54]

Description was changed from

==========
[Mac] Report statistics more regularly in Mac memory pressure monitor

Currently, we only report UMA statistics on receiving a pressure
change event from the system. This means that we could be missing
many ticks of "normal" pressure from sessions that never encounter
pressure, skewing our stats. Other platforms don't have this issue
since they poll.

This change piggybacks statistic reporting on GetCurrentPressureLevel()
which is called regularly by RendererFrameManager::GetMaxNumberOfSavedFrames()
This also allows us to enable reporting for Sierra.

To make this work, GetCurrentPressureLevel is now declared non-const.
There don't appear to be any const MemoryPressureMonitors in the codebase
at the moment, so this should be safe.

BUG=655304
==========

to

==========
[Mac] Report statistics more regularly in Mac memory pressure monitor

Currently, we only report UMA statistics on receiving a pressure
change event from the system. This means that we could be missing
many ticks of "normal" pressure from sessions that never encounter
pressure, skewing our stats. Other platforms don't have this issue
since they poll.

This change piggybacks statistic reporting on GetCurrentPressureLevel()
which is called regularly by RendererFrameManager::GetMaxNumberOfSavedFrames()
This also allows us to enable reporting for Sierra.

To make this work, GetCurrentPressureLevel is now declared non-const.
There don't appear to be any const MemoryPressureMonitors in the codebase
at the moment, so this should be safe.

BUG=655304

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

[commit-bot: I haz the power, 2016-11-28 15:53:55]

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