Add OnPurgeMemory() to MemoryCoordinatorClient

Add OnPurgeMemory() to MemoryCoordinatorClient

Before this CL, MemoryCoordinatorClient has only one callback called
OnMemoryStateChange(). Clients try to free up memory when state change
happens (e.g. NORMAL -> SUSPENDED). This may not be a good strategy on
some platforms because we may touch compressed pages (see [1] for details).

This CL add another callback called OnPurgeMemory() to separate logic
for purging existing memory from memory state changes. This way we can
build flexible strategies for handling memory pressure.

[1] https://groups.google.com/a/chromium.org/forum/?utm_medium=email&utm_source=footer#!msg/project-trim/s96xSirL2Hs/18uq1zfHEgAJ

BUG=684287
Review-Url: https://codereview.chromium.org/2655083003
Cr-Commit-Position: refs/heads/master@{#446951}
Committed: https://chromium.googlesource.com/chromium/src/+/a7f5cbbe7f704ea74d57b7413a7b10b7a1b68750

[bashi, 2017-01-26 01:55:32]

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

[commit-bot: I haz the power, 2017-01-26 01:56:29]

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

[bashi, 2017-01-26 01:57:51]

bashi@chromium.org changed reviewers:
+ chrisha@chromium.org, dcheng@chromium.org, haraken@chromium.org

[bashi, 2017-01-26 01:57:51]

PTAL?

In follow-up CLs I'll move existing purging logic in clients to OnPurgeMemory().
(like https://codereview.chromium.org/2648323005/)

[commit-bot: I haz the power, 2017-01-26 05:03:19]

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

[commit-bot: I haz the power, 2017-01-26 05:03:20]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[haraken, 2017-01-26 05:53:12]

LGTM

https://codereview.chromium.org/2655083003/diff/1/base/memory/memory_coordina...
File base/memory/memory_coordinator_client.h (right):

https://codereview.chromium.org/2655083003/diff/1/base/memory/memory_coordina...
base/memory/memory_coordinator_client.h:60: //  * THROTTLED: Use smaller limits
for memory allocations and caches.

THROTTLED: Use smaller limits for memory future allocations and caches. You
don't need to take any action on existing allocations (they are taken care of by
OnPurgeMemory()).

https://codereview.chromium.org/2655083003/diff/1/base/memory/memory_coordina...
base/memory/memory_coordinator_client.h:62: //               because releasing
resources will be done by OnPurgeMemory().

SUSPENDED: Use much smaller limits for future memory allocations and caches.

Actually maybe we should drop the SUSPENDED state until we figure out what we
should do there. (You don't need to do that in this CL.)

https://codereview.chromium.org/2655083003/diff/1/base/memory/memory_coordina...
base/memory/memory_coordinator_client.h:68: // (e.g. parsed resource that can be
reloaded from disk).

This is not a good example. We cannot easily purge on-memory resources because
we don't have an API to lock an original resource in the disk cache...

[bashi, 2017-01-26 07:09:58]

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

[bashi, 2017-01-26 07:10:01]

https://codereview.chromium.org/2655083003/diff/1/base/memory/memory_coordina...
File base/memory/memory_coordinator_client.h (right):

https://codereview.chromium.org/2655083003/diff/1/base/memory/memory_coordina...
base/memory/memory_coordinator_client.h:60: //  * THROTTLED: Use smaller limits
for memory allocations and caches.
On 2017/01/26 05:53:12, haraken wrote:
> 
> THROTTLED: Use smaller limits for memory future allocations and caches. You
> don't need to take any action on existing allocations (they are taken care of
by
> OnPurgeMemory()).

Done.

https://codereview.chromium.org/2655083003/diff/1/base/memory/memory_coordina...
base/memory/memory_coordinator_client.h:62: //               because releasing
resources will be done by OnPurgeMemory().
On 2017/01/26 05:53:12, haraken wrote:
> 
> SUSPENDED: Use much smaller limits for future memory allocations and caches.
Done.
> 
> Actually maybe we should drop the SUSPENDED state until we figure out what we
> should do there. (You don't need to do that in this CL.)

https://codereview.chromium.org/2655083003/diff/1/base/memory/memory_coordina...
base/memory/memory_coordinator_client.h:68: // (e.g. parsed resource that can be
reloaded from disk).
On 2017/01/26 05:53:12, haraken wrote:
> 
> This is not a good example. We cannot easily purge on-memory resources because
> we don't have an API to lock an original resource in the disk cache...

Removed.

[commit-bot: I haz the power, 2017-01-26 07:10:07]

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

[dcheng, 2017-01-26 08:45:32]

https://codereview.chromium.org/2655083003/diff/1/base/memory/memory_coordina...
File base/memory/memory_coordinator_client.h (right):

https://codereview.chromium.org/2655083003/diff/1/base/memory/memory_coordina...
base/memory/memory_coordinator_client.h:62: //               because releasing
resources will be done by OnPurgeMemory().
Does that mean that going into SUSPENDED will call both this callback and
OnPurgeMemory?

[commit-bot: I haz the power, 2017-01-26 09:00:33]

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

[commit-bot: I haz the power, 2017-01-26 09:00:34]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[chrisha, 2017-01-26 22:17:35]

lgtm!

https://codereview.chromium.org/2655083003/diff/20001/base/memory/memory_coor...
File base/memory/memory_coordinator_client.h (right):

https://codereview.chromium.org/2655083003/diff/20001/base/memory/memory_coor...
base/memory/memory_coordinator_client.h:24: // well.
A comment that OnMemoryStateChange should only affect future allocations, and
not cause the component to actively free memory, but to wait for a separate
PurgeMemory call for that.

(We strongly anticipate that these calls will come in pairs, but under
conditions when the OS is already swapping the purge call will be deferred until
afterwards.)

[bashi, 2017-01-26 23:44:49]

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

[bashi, 2017-01-26 23:45:00]

https://codereview.chromium.org/2655083003/diff/1/base/memory/memory_coordina...
File base/memory/memory_coordinator_client.h (right):

https://codereview.chromium.org/2655083003/diff/1/base/memory/memory_coordina...
base/memory/memory_coordinator_client.h:62: //               because releasing
resources will be done by OnPurgeMemory().
On 2017/01/26 08:45:32, dcheng wrote:
> Does that mean that going into SUSPENDED will call both this callback and
> OnPurgeMemory?

Yes. Please refer line 42. That said, we may not call OnPurgeMemory() in some
cases because purging could touch swapped (compressed) pages. The motivation of
this changes is to separate throttling future allocations and suspending logic
from purging existing allocations.

https://codereview.chromium.org/2655083003/diff/20001/base/memory/memory_coor...
File base/memory/memory_coordinator_client.h (right):

https://codereview.chromium.org/2655083003/diff/20001/base/memory/memory_coor...
base/memory/memory_coordinator_client.h:24: // well.
On 2017/01/26 22:17:35, chrisha (slow) wrote:
> A comment that OnMemoryStateChange should only affect future allocations, and
> not cause the component to actively free memory, but to wait for a separate
> PurgeMemory call for that.
> 
> (We strongly anticipate that these calls will come in pairs, but under
> conditions when the OS is already swapping the purge call will be deferred
until
> afterwards.)

Done.

[commit-bot: I haz the power, 2017-01-26 23:46:00]

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, 2017-01-27 02:04:55]

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

[commit-bot: I haz the power, 2017-01-27 02:04:56]

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

[dcheng, 2017-01-27 09:19:08]

rs lgtm for base

I do feel like the guidelines for the different states could be clearer: it's
currently documented as "use smaller limits" and "use much smaller limits",
which are subjective.

I also wonder... should we explicitly recommend that people do *not* purge
memory in response to calls to OnMemoryStateChange() and only do it with
explicit OnPurgeMemory() calls?

[bashi, 2017-01-30 00:04:00]

On 2017/01/27 09:19:08, dcheng wrote:
> rs lgtm for base
> 
> I do feel like the guidelines for the different states could be clearer: it's
> currently documented as "use smaller limits" and "use much smaller limits",
> which are subjective.
Agreed. As haraken@ commented on #7, we aren't sure what we should do in
suspended state at this point. I'll update the description (or may remote that
state) when we stabilize the APIs.
> 
> I also wonder... should we explicitly recommend that people do *not* purge
> memory in response to calls to OnMemoryStateChange() and only do it with
> explicit OnPurgeMemory() calls?

Updated comments OnMemoryStateChange().

[bashi, 2017-01-30 00:04:17]

The CQ bit was checked by bashi@chromium.org

[bashi, 2017-01-30 00:04:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, chrisha@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2655083003/#ps60001
(title: "comment")

[commit-bot: I haz the power, 2017-01-30 00:04:39]

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-01-30 01:05:36]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1485734657204730,
"parent_rev": "7f6c6a0b64b8d4df8cb476d88bc582109c0d6bff", "commit_rev":
"a7f5cbbe7f704ea74d57b7413a7b10b7a1b68750"}

[commit-bot: I haz the power, 2017-01-30 01:07:13]

Description was changed from

==========
Add OnPurgeMemory() to MemoryCoordinatorClient

Before this CL, MemoryCoordinatorClient has only one callback called
OnMemoryStateChange(). Clients try to free up memory when state change
happens (e.g. NORMAL -> SUSPENDED). This may not be a good strategy on
some platforms because we may touch compressed pages (see [1] for details).

This CL add another callback called OnPurgeMemory() to separate logic
for purging existing memory from memory state changes. This way we can
build flexible strategies for handling memory pressure.

[1]
https://groups.google.com/a/chromium.org/forum/?utm_medium=email&utm_source=f...

BUG=684287
==========

to

==========
Add OnPurgeMemory() to MemoryCoordinatorClient

Before this CL, MemoryCoordinatorClient has only one callback called
OnMemoryStateChange(). Clients try to free up memory when state change
happens (e.g. NORMAL -> SUSPENDED). This may not be a good strategy on
some platforms because we may touch compressed pages (see [1] for details).

This CL add another callback called OnPurgeMemory() to separate logic
for purging existing memory from memory state changes. This way we can
build flexible strategies for handling memory pressure.

[1]
https://groups.google.com/a/chromium.org/forum/?utm_medium=email&utm_source=f...

BUG=684287

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

[commit-bot: I haz the power, 2017-01-30 01:07:15]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/a7f5cbbe7f704ea74d57b7413a7b...