Add Mac memory pressure statistic reporting and consolidate platform code

Per offline discussion, leaving "ChromeOS.MemoryPressureLevel" code in place for ChromeOS while history builds up in "Memory.PressureLevel"

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

[lgrey, 2016-10-20 19:11:29]

Description was changed from

==========
Per offline discussion, leaving "ChromeOS.MemoryPressureLevel" code in place for
ChromeOS while history builds up in "Memory.PressureLevel"

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

to

==========
Per offline discussion, leaving "ChromeOS.MemoryPressureLevel" code in place for
ChromeOS while history builds up in "Memory.PressureLevel"

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

[shrike, 2016-10-20 21:04:30]

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

[shrike, 2016-10-20 21:04:31]

https://codereview.chromium.org/2434103003/diff/1/base/memory/memory_pressure...
File base/memory/memory_pressure_monitor_mac.cc (right):

https://codereview.chromium.org/2434103003/diff/1/base/memory/memory_pressure...
base/memory/memory_pressure_monitor_mac.cc:44:
base::Bind(&MemoryPressureListener::NotifyMemoryPressure)),
I think MemoryPressureListener::NotifyMemoryPressure() still needs to be static?
I haven't used Bind() that much, but I'm not seeing from this declaration how it
can call MemoryPressureListener::NotifyMemoryPressure() on this particular
instance.

https://codereview.chromium.org/2434103003/diff/1/base/memory/memory_pressure...
base/memory/memory_pressure_monitor_mac.cc:80: for (int i = 0; i <
last_level_seconds; i++) {
This is unfortunate, because last_level_seconds could be quite large. I wonder
if the UMA folks might have a better way of doing this (someone like
asvitkine@).

[lgrey, 2016-10-21 14:15:04]

lgrey@chromium.org changed reviewers:
+ asvitkine@chromium.org

[lgrey, 2016-10-21 14:15:04]

https://codereview.chromium.org/2434103003/diff/1/base/memory/memory_pressure...
File base/memory/memory_pressure_monitor_mac.cc (right):

https://codereview.chromium.org/2434103003/diff/1/base/memory/memory_pressure...
base/memory/memory_pressure_monitor_mac.cc:44:
base::Bind(&MemoryPressureListener::NotifyMemoryPressure)),
On 2016/10/20 21:04:31, shrike wrote:
> I think MemoryPressureListener::NotifyMemoryPressure() still needs to be
static?
> I haven't used Bind() that much, but I'm not seeing from this declaration how
it
> can call MemoryPressureListener::NotifyMemoryPressure() on this particular
> instance.

This threw me for a loop for a bit, then I realized: the method I changed from
static is "MemoryPressureMonitor::NotifyMemoryPressureChanged" which is called
directly inside the event handler vs.
"MemoryPressureListener::NotifyMemoryPressure" which is defined elsewhere. 

I mixed these two up earlier when I was working on this as well; would it be
worth it to rename "NotifyMemoryPressureChanged" so they're more distinct?

https://codereview.chromium.org/2434103003/diff/1/base/memory/memory_pressure...
base/memory/memory_pressure_monitor_mac.cc:80: for (int i = 0; i <
last_level_seconds; i++) {
On 2016/10/20 21:04:30, shrike wrote:
> This is unfortunate, because last_level_seconds could be quite large. I wonder
> if the UMA folks might have a better way of doing this (someone like
> asvitkine@).

asvitkine@ what do you think?

For context, this particular stat is an enumeration, but it's counting
cumulative seconds in a given memory pressure state. Windows and ChromeOS poll
every second, but Mac gets notifications on change, hence this weird workaround.
Is there any way to put more than one incidence into an enumeration bucket in a
single call?

Another possibility is we could decouple the state change notifications from
reporting with a separate timer just to report the current state.

[Alexei Svitkine (slow), 2016-10-21 14:53:35]

asvitkine@chromium.org changed reviewers:
+ chrisha@chromium.org, georgesak@chromium.org

[Alexei Svitkine (slow), 2016-10-21 14:53:36]

+georgesak +chrisha to review memory pressure stuff since they've worked in this
space before on other platforms

[chrisha, 2016-10-21 18:18:58]

This is largely fine by me. We already talked about this work in an email
thread.

Other comments inline.

https://codereview.chromium.org/2434103003/diff/1/base/memory/memory_pressure...
File base/memory/memory_pressure_monitor_mac.cc (right):

https://codereview.chromium.org/2434103003/diff/1/base/memory/memory_pressure...
base/memory/memory_pressure_monitor_mac.cc:44:
base::Bind(&MemoryPressureListener::NotifyMemoryPressure)),
On 2016/10/21 14:15:04, lgrey wrote:
> On 2016/10/20 21:04:31, shrike wrote:
> > I think MemoryPressureListener::NotifyMemoryPressure() still needs to be
> static?
> > I haven't used Bind() that much, but I'm not seeing from this declaration
how
> it
> > can call MemoryPressureListener::NotifyMemoryPressure() on this particular
> > instance.
> 
> This threw me for a loop for a bit, then I realized: the method I changed from
> static is "MemoryPressureMonitor::NotifyMemoryPressureChanged" which is called
> directly inside the event handler vs.
> "MemoryPressureListener::NotifyMemoryPressure" which is defined elsewhere. 
> 
> I mixed these two up earlier when I was working on this as well; would it be
> worth it to rename "NotifyMemoryPressureChanged" so they're more distinct?

Yeah, the rename might help. Maybe "OnMemoryPressureChanged"?

https://codereview.chromium.org/2434103003/diff/1/base/memory/memory_pressure...
base/memory/memory_pressure_monitor_mac.cc:80: for (int i = 0; i <
last_level_seconds; i++) {
On 2016/10/21 14:15:04, lgrey wrote:
> On 2016/10/20 21:04:30, shrike wrote:
> > This is unfortunate, because last_level_seconds could be quite large. I
wonder
> > if the UMA folks might have a better way of doing this (someone like
> > asvitkine@).
> 
> asvitkine@ what do you think?
> 
> For context, this particular stat is an enumeration, but it's counting
> cumulative seconds in a given memory pressure state. Windows and ChromeOS poll
> every second, but Mac gets notifications on change, hence this weird
workaround.
> Is there any way to put more than one incidence into an enumeration bucket in
a
> single call?
> 
> Another possibility is we could decouple the state change notifications from
> reporting with a separate timer just to report the current state.

We had talked about switching to polling on Mac, as shrike@ had found that the
messages could sometimes be delayed by a long period of time (ie, up to a
minute).

Do we just want to move to a polling solution to be uniform? (Also, IIRC the
polling is currently at 5 seconds?)

(This increases idle cost, but we have some fun new timer coalescing stuff on
the way that will mitigate this.)

[lgrey, 2016-10-21 18:39:35]

https://codereview.chromium.org/2434103003/diff/1/base/memory/memory_pressure...
File base/memory/memory_pressure_monitor_mac.cc (right):

https://codereview.chromium.org/2434103003/diff/1/base/memory/memory_pressure...
base/memory/memory_pressure_monitor_mac.cc:44:
base::Bind(&MemoryPressureListener::NotifyMemoryPressure)),
On 2016/10/21 18:18:58, chrisha (slow) wrote:
> On 2016/10/21 14:15:04, lgrey wrote:
> > On 2016/10/20 21:04:31, shrike wrote:
> > > I think MemoryPressureListener::NotifyMemoryPressure() still needs to be
> > static?
> > > I haven't used Bind() that much, but I'm not seeing from this declaration
> how
> > it
> > > can call MemoryPressureListener::NotifyMemoryPressure() on this particular
> > > instance.
> > 
> > This threw me for a loop for a bit, then I realized: the method I changed
from
> > static is "MemoryPressureMonitor::NotifyMemoryPressureChanged" which is
called
> > directly inside the event handler vs.
> > "MemoryPressureListener::NotifyMemoryPressure" which is defined elsewhere. 
> > 
> > I mixed these two up earlier when I was working on this as well; would it be
> > worth it to rename "NotifyMemoryPressureChanged" so they're more distinct?
> 
> Yeah, the rename might help. Maybe "OnMemoryPressureChanged"?

Done.

https://codereview.chromium.org/2434103003/diff/1/base/memory/memory_pressure...
base/memory/memory_pressure_monitor_mac.cc:80: for (int i = 0; i <
last_level_seconds; i++) {
On 2016/10/21 18:18:58, chrisha (slow) wrote:
> On 2016/10/21 14:15:04, lgrey wrote:
> > On 2016/10/20 21:04:30, shrike wrote:
> > > This is unfortunate, because last_level_seconds could be quite large. I
> wonder
> > > if the UMA folks might have a better way of doing this (someone like
> > > asvitkine@).
> > 
> > asvitkine@ what do you think?
> > 
> > For context, this particular stat is an enumeration, but it's counting
> > cumulative seconds in a given memory pressure state. Windows and ChromeOS
poll
> > every second, but Mac gets notifications on change, hence this weird
> workaround.
> > Is there any way to put more than one incidence into an enumeration bucket
in
> a
> > single call?
> > 
> > Another possibility is we could decouple the state change notifications from
> > reporting with a separate timer just to report the current state.
> 
> We had talked about switching to polling on Mac, as shrike@ had found that the
> messages could sometimes be delayed by a long period of time (ie, up to a
> minute).
> 
> Do we just want to move to a polling solution to be uniform? (Also, IIRC the
> polling is currently at 5 seconds?)
> 
> (This increases idle cost, but we have some fun new timer coalescing stuff on
> the way that will mitigate this.)

My mistake, it looks like Chrome polls once a second and Windows polls every 5
seconds. Is the right move to standardize everything at 5 (for
Memory.PressureLevel at least)?

[shrike, 2016-10-21 20:44:20]

On 2016/10/21 18:18:58, chrisha (slow) wrote:
> Do we just want to move to a polling solution to be uniform? (Also, IIRC the
> polling is currently at 5 seconds?)
> 
> (This increases idle cost, but we have some fun new timer coalescing stuff on
> the way that will mitigate this.)

I'm wondering if this is similar to my proposed memory pressure polling code
where I piggyback off of other timer events to avoid waking up separately to
check memory pressure? If so, then polling here might not be terrible.

Short of that. I have the same question as lgrey@: is there any way to put more
than one incidence into an enumeration bucket in a single call? That would allow
us to keep the current notification-based system and capture memory pressure
status as if we're polling (or at least do that without the for loop).

[lgrey, 2016-11-03 19:49:14]

PTAL

All platforms now report in units of 5 seconds, and Mac reports cumulative ticks
in a single call.

[shrike, 2016-11-03 21:15:22]

lgtm

[lgrey, 2016-11-04 16:15:23]

lgrey@chromium.org changed reviewers:
+ skuhne@google.com

[lgrey, 2016-11-04 16:15:23]

Stefan, PTAL for ChromeOS

[lgrey, 2016-11-04 16:15:49]

lgrey@chromium.org changed reviewers:
+ skuhne@chromium.org
- skuhne@google.com

[Mr4D (OOO till 08-26), 2016-11-04 16:51:31]

lgtm. Thanks for doing that!

[lgrey, 2016-11-04 18:12:48]

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

[commit-bot: I haz the power, 2016-11-04 18:13: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-11-04 20:04:28]

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

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

Dry run: This issue passed the CQ dry run.

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

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

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

Mark PTAL for OWNERS

[Mark Mentovai, 2016-11-04 21:53:01]

LGTM

[Alexei Svitkine (slow), 2016-11-04 22:00:04]

asvitkine@chromium.org changed reviewers:
- asvitkine@chromium.org

[lgrey, 2016-11-07 14:57:05]

The CQ bit was checked by lgrey@chromium.org

[lgrey, 2016-11-07 14:57:06]

The patchset sent to the CQ was uploaded after l-g-t-m from shrike@chromium.org,
skuhne@chromium.org
Link to the patchset: https://codereview.chromium.org/2434103003/#ps80001
(title: "Add radar ID")

[commit-bot: I haz the power, 2016-11-07 14:57:14]

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-07 16:34:14]

Description was changed from

==========
Per offline discussion, leaving "ChromeOS.MemoryPressureLevel" code in place for
ChromeOS while history builds up in "Memory.PressureLevel"

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

to

==========
Per offline discussion, leaving "ChromeOS.MemoryPressureLevel" code in place for
ChromeOS while history builds up in "Memory.PressureLevel"

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

[commit-bot: I haz the power, 2016-11-07 16:34:15]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-07 16:42:59]

Description was changed from

==========
Per offline discussion, leaving "ChromeOS.MemoryPressureLevel" code in place for
ChromeOS while history builds up in "Memory.PressureLevel"

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

to

==========
Per offline discussion, leaving "ChromeOS.MemoryPressureLevel" code in place for
ChromeOS while history builds up in "Memory.PressureLevel"

BUG=655304

Committed: https://crrev.com/1c1ec069eb9f7b0ffc61e17e00d5ac2c5657cbc2
Cr-Commit-Position: refs/heads/master@{#430292}
==========

[commit-bot: I haz the power, 2016-11-07 16:43:01]

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