Move purging from OnMemoryState to OnPurgeMemory() in cc/

Move purging from OnMemoryState to OnPurgeMemory() in cc/

We are going to separate memory purging logic from memory state changes.
This CL moves purging from OnMemoryState() to OnPurgeMemory() in cc/.

BUG=684287
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2664703002
Cr-Commit-Position: refs/heads/master@{#449484}
Committed: https://chromium.googlesource.com/chromium/src/+/c841b76ac7d8465283eb13e87d05388004a6f6bf

[bashi, 2017-01-30 02:04:05]

Description was changed from

==========
Move purging from OnMemoryState to OnPurgeMemory() in cc/

We are going to separate memory purging logic from memory state changes.
This CL moves purging from OnMemoryState() to OnPurgeMemory() in cc/.

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

to

==========
Move purging from OnMemoryState to OnPurgeMemory() in cc/

We are going to separate memory purging logic from memory state changes.
This CL moves purging from OnMemoryState() to OnPurgeMemory() in cc/.

BUG=684287
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[bashi, 2017-01-30 02:05:43]

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

[commit-bot: I haz the power, 2017-01-30 02:05:55]

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-30 02:06:39]

bashi@chromium.org changed reviewers:
+ tasak@google.com

[bashi, 2017-01-30 02:06:39]

tasak@: could you check this CL doesn't break your purge+suspend experiment?

[tasak, 2017-01-30 02:17:10]

lgtm

[bashi, 2017-01-30 02:44:20]

bashi@chromium.org changed reviewers:
+ ericrk@chromium.org

[bashi, 2017-01-30 02:44:20]

ericrk@: PTAL?

[commit-bot: I haz the power, 2017-01-30 03:00:51]

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

[commit-bot: I haz the power, 2017-01-30 03:00:52]

Dry run: This issue passed the CQ dry run.

[ericrk, 2017-01-30 15:58:56]

Just so I can better understand what is changing - will there still be a
"suspended" state? Or will Purge just be called without a memory state change?

https://codereview.chromium.org/2664703002/diff/1/cc/tiles/gpu_image_decode_c...
File cc/tiles/gpu_image_decode_cache.cc (right):

https://codereview.chromium.org/2664703002/diff/1/cc/tiles/gpu_image_decode_c...
cc/tiles/gpu_image_decode_cache.cc:1277: if (state ==
base::MemoryState::SUSPENDED)
The previous logic handled THROTTLED as well, with different limits. Is it
correct to remove that at the moment?

https://codereview.chromium.org/2664703002/diff/1/cc/tiles/software_image_dec...
File cc/tiles/software_image_decode_cache.cc (right):

https://codereview.chromium.org/2664703002/diff/1/cc/tiles/software_image_dec...
cc/tiles/software_image_decode_cache.cc:1171: OnPurgeMemory();
This also removes the ability to handle THROTTLED state, is this intentional? I
think we just want "ReduceCacheUsage" here.

[bashi, 2017-01-31 19:30:56]

Thank you for review!

Sorry for lack of explanations. I should have provided the context. We still not
sure we keep suspended state in the future (c.f.
https://codereview.chromium.org/2655083003/#msg21), but basically we are going
to separate purging logic from memory state transitions so that we can handle
"future allocations" and "existing allocations" separately. For example, we may
call OnPurgeMemory() just before OnStateChange() on some platforms, but may not
on other platforms, depending on whether ZRAM exists on the platform. I updated
the design doc[1] and comments on MemoryCoordinatorClient[2].

[1]
https://docs.google.com/document/d/1rGeUzhfzIKwmBzUcqUEwxEpLQQRq3PulRuHwFBlVI...
[2]
https://cs.chromium.org/chromium/src/base/memory/memory_coordinator_client.h?...

https://codereview.chromium.org/2664703002/diff/1/cc/tiles/gpu_image_decode_c...
File cc/tiles/gpu_image_decode_cache.cc (right):

https://codereview.chromium.org/2664703002/diff/1/cc/tiles/gpu_image_decode_c...
cc/tiles/gpu_image_decode_cache.cc:1277: if (state ==
base::MemoryState::SUSPENDED)
On 2017/01/30 15:58:56, ericrk wrote:
> The previous logic handled THROTTLED as well, with different limits. Is it
> correct to remove that at the moment?

The purpose of this CL is to separate purging cache from memory state change
without breaking purge+suspend experiment. Purge+suspend experiment only uses
SUSPENDED state so not handling THROTTLED will be fine.

https://codereview.chromium.org/2664703002/diff/1/cc/tiles/software_image_dec...
File cc/tiles/software_image_decode_cache.cc (right):

https://codereview.chromium.org/2664703002/diff/1/cc/tiles/software_image_dec...
cc/tiles/software_image_decode_cache.cc:1171: OnPurgeMemory();
On 2017/01/30 15:58:56, ericrk wrote:
> This also removes the ability to handle THROTTLED state, is this intentional?
I
> think we just want "ReduceCacheUsage" here.

Yes, it's intentional. I'm going to move ReduceCacheUsage() call from
OnStateChange() to OnPurgeMemory(). We need to keep purging memory here because
currently purge+suspend experiment depends on this behavior. When I complete
updating all clients (this CL is the first one), and purge+suspend experiment
I'll remove this lines.

[ericrk, 2017-02-05 06:26:22]

lgtm

Thanks for explaining. This makes sense with the current explanations. THROTTLED
might be a bit broken right now, but I'm ok re-visiting that later, as it sounds
like THROTTLED is currently unused.

[bashi, 2017-02-07 00:58:59]

I've submitted https://codereview.chromium.org/2669323002/ and now we call both
OnMemoryState(SUSPENDED) and OnPurgeMemory() when suspending so clients no
longer have to call OnPurgeMemory() in OnMemoryState(). I've updated this CL.

ericrk@: could you take another look?

https://codereview.chromium.org/2664703002/diff/20001/cc/tiles/gpu_image_deco...
File cc/tiles/gpu_image_decode_cache.cc (right):

https://codereview.chromium.org/2664703002/diff/20001/cc/tiles/gpu_image_deco...
cc/tiles/gpu_image_decode_cache.cc:1280: EnsureCapacity(0);
It seems EnsureCapacity() changes its behavior based on |memory_state_|. Should
we change EnsureCapacity() instead of temporary changing |memory_state_|? Or
should we add a method that clears |perspstent_cache_|?

[ericrk, 2017-02-07 19:20:14]

https://codereview.chromium.org/2664703002/diff/20001/cc/tiles/gpu_image_deco...
File cc/tiles/gpu_image_decode_cache.cc (right):

https://codereview.chromium.org/2664703002/diff/20001/cc/tiles/gpu_image_deco...
cc/tiles/gpu_image_decode_cache.cc:1280: EnsureCapacity(0);
On 2017/02/07 00:58:59, bashi wrote:
> It seems EnsureCapacity() changes its behavior based on |memory_state_|.
Should
> we change EnsureCapacity() instead of temporary changing |memory_state_|? Or
> should we add a method that clears |perspstent_cache_|?

Does the coordinator call OnMemoryStateChange(SUSPENDED) before or after calling
Purge? If before, it seems like this is unnecessary. If after, it seems possible
to race where we Purge, the cache allocates new memory, then we set to suspended
and that memory is never freed?

[bashi, 2017-02-09 00:46:18]

https://codereview.chromium.org/2664703002/diff/20001/cc/tiles/gpu_image_deco...
File cc/tiles/gpu_image_decode_cache.cc (right):

https://codereview.chromium.org/2664703002/diff/20001/cc/tiles/gpu_image_deco...
cc/tiles/gpu_image_decode_cache.cc:1280: EnsureCapacity(0);
On 2017/02/07 19:20:14, ericrk wrote:
> On 2017/02/07 00:58:59, bashi wrote:
> > It seems EnsureCapacity() changes its behavior based on |memory_state_|.
> Should
> > we change EnsureCapacity() instead of temporary changing |memory_state_|? Or
> > should we add a method that clears |perspstent_cache_|?
> 
> Does the coordinator call OnMemoryStateChange(SUSPENDED) before or after
calling
> Purge? If before, it seems like this is unnecessary. If after, it seems
possible
> to race where we Purge, the cache allocates new memory, then we set to
suspended
> and that memory is never freed?

Thanks. I wasn't aware of the race. Current order is OnPurgeMemory() ->
OnMemoryStateChange(). I'll change the order.

That said our current plan is that OnPurgeMemory() should free up memory
regardless of memory state. We would call OnPurgeMemory() even in NORMAL state
to avoid the OS starting page swapping. Maybe I should modify EnsureCapacity()
to make it state independent but I'm not sure it's a right way. Probably revisit
later?

[ericrk, 2017-02-09 18:36:30]

still LGTM

https://codereview.chromium.org/2664703002/diff/20001/cc/tiles/gpu_image_deco...
File cc/tiles/gpu_image_decode_cache.cc (right):

https://codereview.chromium.org/2664703002/diff/20001/cc/tiles/gpu_image_deco...
cc/tiles/gpu_image_decode_cache.cc:1280: EnsureCapacity(0);
On 2017/02/09 00:46:18, bashi wrote:
> On 2017/02/07 19:20:14, ericrk wrote:
> > On 2017/02/07 00:58:59, bashi wrote:
> > > It seems EnsureCapacity() changes its behavior based on |memory_state_|.
> > Should
> > > we change EnsureCapacity() instead of temporary changing |memory_state_|?
Or
> > > should we add a method that clears |perspstent_cache_|?
> > 
> > Does the coordinator call OnMemoryStateChange(SUSPENDED) before or after
> calling
> > Purge? If before, it seems like this is unnecessary. If after, it seems
> possible
> > to race where we Purge, the cache allocates new memory, then we set to
> suspended
> > and that memory is never freed?
> 
> Thanks. I wasn't aware of the race. Current order is OnPurgeMemory() ->
> OnMemoryStateChange(). I'll change the order.
> 
> That said our current plan is that OnPurgeMemory() should free up memory
> regardless of memory state. We would call OnPurgeMemory() even in NORMAL state
> to avoid the OS starting page swapping. Maybe I should modify EnsureCapacity()
> to make it state independent but I'm not sure it's a right way. Probably
revisit
> later?

I'm good re-visitng later - once we get to enabling THROTTLED, we'll need some
tweaks anyway.

[bashi, 2017-02-09 23:00:14]

On 2017/02/09 18:36:30, ericrk wrote:
> still LGTM
> 
>
https://codereview.chromium.org/2664703002/diff/20001/cc/tiles/gpu_image_deco...
> File cc/tiles/gpu_image_decode_cache.cc (right):
> 
>
https://codereview.chromium.org/2664703002/diff/20001/cc/tiles/gpu_image_deco...
> cc/tiles/gpu_image_decode_cache.cc:1280: EnsureCapacity(0);
> On 2017/02/09 00:46:18, bashi wrote:
> > On 2017/02/07 19:20:14, ericrk wrote:
> > > On 2017/02/07 00:58:59, bashi wrote:
> > > > It seems EnsureCapacity() changes its behavior based on |memory_state_|.
> > > Should
> > > > we change EnsureCapacity() instead of temporary changing
|memory_state_|?
> Or
> > > > should we add a method that clears |perspstent_cache_|?
> > > 
> > > Does the coordinator call OnMemoryStateChange(SUSPENDED) before or after
> > calling
> > > Purge? If before, it seems like this is unnecessary. If after, it seems
> > possible
> > > to race where we Purge, the cache allocates new memory, then we set to
> > suspended
> > > and that memory is never freed?
> > 
> > Thanks. I wasn't aware of the race. Current order is OnPurgeMemory() ->
> > OnMemoryStateChange(). I'll change the order.
> > 
> > That said our current plan is that OnPurgeMemory() should free up memory
> > regardless of memory state. We would call OnPurgeMemory() even in NORMAL
state
> > to avoid the OS starting page swapping. Maybe I should modify
EnsureCapacity()
> > to make it state independent but I'm not sure it's a right way. Probably
> revisit
> > later?
> 
> I'm good re-visitng later - once we get to enabling THROTTLED, we'll need some
> tweaks anyway.

Thanks!

[bashi, 2017-02-09 23:00:45]

The CQ bit was checked by bashi@chromium.org

[bashi, 2017-02-09 23:00:46]

The patchset sent to the CQ was uploaded after l-g-t-m from tasak@google.com
Link to the patchset: https://codereview.chromium.org/2664703002/#ps20001
(title: "Don't call OnPurgeMemory() in OnStateChange()")

[commit-bot: I haz the power, 2017-02-09 23:01:31]

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-02-10 00:15:53]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1486681245831000,
"parent_rev": "4e5980d01e338036fca2cb16021cff79619ab4a8", "commit_rev":
"c841b76ac7d8465283eb13e87d05388004a6f6bf"}

[commit-bot: I haz the power, 2017-02-10 00:16:33]

Description was changed from

==========
Move purging from OnMemoryState to OnPurgeMemory() in cc/

We are going to separate memory purging logic from memory state changes.
This CL moves purging from OnMemoryState() to OnPurgeMemory() in cc/.

BUG=684287
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Move purging from OnMemoryState to OnPurgeMemory() in cc/

We are going to separate memory purging logic from memory state changes.
This CL moves purging from OnMemoryState() to OnPurgeMemory() in cc/.

BUG=684287
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

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

[commit-bot: I haz the power, 2017-02-10 00:16:34]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/c841b76ac7d8465283eb13e87d05...