[Mac] Return cached pressure level in GetCurrentPressureLevel().

[Mac] Return cached pressure level in GetCurrentPressureLevel().

MemoryPressureMonitor clients call GetCurrentPressureLevel(), sometimes
frequently, to get the current memory pressure reading. Up until
recently the cached memory pressure level could be very stale, so we
updated it on every call to GetCurrentPressureLevel(). Now that the
memory pressure level is updated regularly, we can just return the
cached value (and avoid the extra syscalls).

R=lgrey@chromium.org,mark@chromium.org
BUG=701408
Review-Url: https://codereview.chromium.org/2743253006
Cr-Commit-Position: refs/heads/master@{#456890}
Committed: https://chromium.googlesource.com/chromium/src/+/f53f979ccc20eac06865022a612910ae510e796c

[shrike, 2017-03-14 17:17:25]

PTAL

[lgrey, 2017-03-14 17:23:02]

LGTM (but note that I'm not yet a committer)

[shrike, 2017-03-14 21:11:45]

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

[shrike, 2017-03-14 21:11:46]

PTAL (for owners approval)

[Mark Mentovai, 2017-03-14 21:40:17]

LGTM

[shrike, 2017-03-14 22:37:03]

The CQ bit was checked by shrike@chromium.org

[shrike, 2017-03-14 22:37:47]

Description was changed from

==========
[Mac] Return cached pressure level in GetCurrentPressureLevel().

MemoryPressureMonitor clients call GetCurrentPressureLevel(), sometimes
frequently, to get the current memory pressure reading. Up until
recently the cached memory pressure level could be very stale, so we
updated it on every call to GetCurrentPressureLevel(). Now that the
memory pressure level is updated regularly, we can just return the
cached value (and avoid the extra syscalls).

R=lgrey@chromium.org
BUG=701408
==========

to

==========
[Mac] Return cached pressure level in GetCurrentPressureLevel().

MemoryPressureMonitor clients call GetCurrentPressureLevel(), sometimes
frequently, to get the current memory pressure reading. Up until
recently the cached memory pressure level could be very stale, so we
updated it on every call to GetCurrentPressureLevel(). Now that the
memory pressure level is updated regularly, we can just return the
cached value (and avoid the extra syscalls).

R=lgrey@chromium.org,mark@chromium.org
BUG=701408
==========

[commit-bot: I haz the power, 2017-03-14 22:37:51]

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-03-14 23:31:39]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1489531023579360, "parent_rev":
"52dceecdb0a9da763a6f62d0c5b010843692d489", "commit_rev":
"f53f979ccc20eac06865022a612910ae510e796c"}

[commit-bot: I haz the power, 2017-03-14 23:35:06]

Description was changed from

==========
[Mac] Return cached pressure level in GetCurrentPressureLevel().

MemoryPressureMonitor clients call GetCurrentPressureLevel(), sometimes
frequently, to get the current memory pressure reading. Up until
recently the cached memory pressure level could be very stale, so we
updated it on every call to GetCurrentPressureLevel(). Now that the
memory pressure level is updated regularly, we can just return the
cached value (and avoid the extra syscalls).

R=lgrey@chromium.org,mark@chromium.org
BUG=701408
==========

to

==========
[Mac] Return cached pressure level in GetCurrentPressureLevel().

MemoryPressureMonitor clients call GetCurrentPressureLevel(), sometimes
frequently, to get the current memory pressure reading. Up until
recently the cached memory pressure level could be very stale, so we
updated it on every call to GetCurrentPressureLevel(). Now that the
memory pressure level is updated regularly, we can just return the
cached value (and avoid the extra syscalls).

R=lgrey@chromium.org,mark@chromium.org
BUG=701408

Review-Url: https://codereview.chromium.org/2743253006
Cr-Commit-Position: refs/heads/master@{#456890}
Committed:
https://chromium.googlesource.com/chromium/src/+/f53f979ccc20eac06865022a6129...
==========

[commit-bot: I haz the power, 2017-03-14 23:35:06]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/f53f979ccc20eac06865022a6129...

[brettw, 2017-05-10 20:46:50]

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

[brettw, 2017-05-10 20:46:50]

This change introduces a race condition and I think is incorrect.

The last_pressure_level_ is driven only off of a 5 second timer that is
independent of the system notifications of memory pressure.

So if we get a system memory pressure notification before the UMA timer has
updated last_pressure_level_, checking the current memory pressure from a memory
pressure notification handler (which is what we do for tab pressure
notifications) will report "no pressure" and it will do nothing.

This is easily reproducible using the "memory_pressure" tool on Mac. It could be
the UMA timer beats the system notification most of the time in practice since
the premise of the polling seems to be that the system notification is super
delayed, something that's not the case when simulating pressure events. But we
should still fix this.

[shrike, 2017-05-10 20:55:47]

On 2017/05/10 20:46:50, brettw (behind--catching up) wrote:
> This change introduces a race condition and I think is incorrect.
> 
> The last_pressure_level_ is driven only off of a 5 second timer that is
> independent of the system notifications of memory pressure.
> 
> So if we get a system memory pressure notification before the UMA timer has
> updated last_pressure_level_, checking the current memory pressure from a
memory
> pressure notification handler (which is what we do for tab pressure
> notifications) will report "no pressure" and it will do nothing.
> 
> This is easily reproducible using the "memory_pressure" tool on Mac. It could
be
> the UMA timer beats the system notification most of the time in practice since
> the premise of the polling seems to be that the system notification is super
> delayed, something that's not the case when simulating pressure events. But we
> should still fix this.

OK, I will prepare a cl for that.

[brettw, 2017-05-10 21:00:34]

On 2017/05/10 20:55:47, shrike wrote:
> On 2017/05/10 20:46:50, brettw (behind--catching up) wrote:
> > This change introduces a race condition and I think is incorrect.
> > 
> > The last_pressure_level_ is driven only off of a 5 second timer that is
> > independent of the system notifications of memory pressure.
> > 
> > So if we get a system memory pressure notification before the UMA timer has
> > updated last_pressure_level_, checking the current memory pressure from a
> memory
> > pressure notification handler (which is what we do for tab pressure
> > notifications) will report "no pressure" and it will do nothing.
> > 
> > This is easily reproducible using the "memory_pressure" tool on Mac. It
could
> be
> > the UMA timer beats the system notification most of the time in practice
since
> > the premise of the polling seems to be that the system notification is super
> > delayed, something that's not the case when simulating pressure events. But
we
> > should still fix this.
> 
> OK, I will prepare a cl for that.

I *think* all this might need is to update the cached value from the OS
notification.

[shrike, 2017-05-10 21:06:31]

On 2017/05/10 21:00:34, brettw (behind--catching up) wrote:
> > OK, I will prepare a cl for that.
> 
> I *think* all this might need is to update the cached value from the OS
> notification.

Yes, that was my thought as well.