Update default policy for memory coordinator

Update default policy for memory coordinator

For now, memory coordinator is going to focus on how to detect memory
pressure and when to ask TabManager to schedule tab discarding. To
make things simpler, remove all actions other than tab discarding from
the default policy.

BUG=696844
Review-Url: https://codereview.chromium.org/2943283002
Cr-Commit-Position: refs/heads/master@{#481390}
Committed: https://chromium.googlesource.com/chromium/src/+/c8af805863f40f75271df364e73ad02c2f6a18f2

[bashi, 2017-06-19 03:04:42]

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

[commit-bot: I haz the power, 2017-06-19 03:04:51]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bashi, 2017-06-19 03:20:30]

Description was changed from

==========
Update default policy for memory coordinator

BUG=696844
==========

to

==========
Update default policy for memory coordinator

For now, memory coordinator is going to focus on how to detect memory
pressure and when to ask TabManager to schedule tab discarding. To
make things simpler, remove all actions other than tab discarding from
the default policy.

BUG=696844
==========

[bashi, 2017-06-19 03:27:37]

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

[bashi, 2017-06-19 03:27:37]

PTAL

IIUC we will focus on "urgent tab killing" and we will put off other actions
like purging and allocation throttling.

[haraken, 2017-06-19 04:31:27]

Are you planning to ship MemoryCoordinator while keeping the existing
MemoryPressureListener? If yes, this CL makes sense.

Otherwise, I'm a bit afraid that MemoryCoordinator will regress the OOM rate etc
(because MemoryCoordinator just reacts to high memory pressure scenarios caused
by Chrome's excessive memory pressure.)

[bashi, 2017-06-19 04:37:24]

On 2017/06/19 04:31:27, haraken wrote:
> Are you planning to ship MemoryCoordinator while keeping the existing
> MemoryPressureListener? If yes, this CL makes sense.
> 
> Otherwise, I'm a bit afraid that MemoryCoordinator will regress the OOM rate
etc
> (because MemoryCoordinator just reacts to high memory pressure scenarios
caused
> by Chrome's excessive memory pressure.)

Yes, I'm planning to keep MemoryPressureListeners with MemoryCoordinator. The
only exception is TabManager. It won't be a MemoryPressureListener when mc is
enabled.

[commit-bot: I haz the power, 2017-06-19 04:38:14]

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

[commit-bot: I haz the power, 2017-06-19 04:38:15]

Dry run: This issue passed the CQ dry run.

[haraken, 2017-06-19 04:52:53]

On 2017/06/19 04:37:24, bashi wrote:
> On 2017/06/19 04:31:27, haraken wrote:
> > Are you planning to ship MemoryCoordinator while keeping the existing
> > MemoryPressureListener? If yes, this CL makes sense.
> > 
> > Otherwise, I'm a bit afraid that MemoryCoordinator will regress the OOM rate
> etc
> > (because MemoryCoordinator just reacts to high memory pressure scenarios
> caused
> > by Chrome's excessive memory pressure.)
> 
> Yes, I'm planning to keep MemoryPressureListeners with MemoryCoordinator. The
> only exception is TabManager. It won't be a MemoryPressureListener when mc is
> enabled.

LGTM but let's wait for Chris' approval.

Original plan:

1) Replace MPL with MC v0 so that it doesn't regress performance / memory. (<==
people have many concerns about how to implement the policy and the metrics.)
2) Support more functionalities in MC v1 (e.g., urgent tab killing, dynamic
svelte etc).

Current plan:

1) Keep MPL. Implement the urgent tab killing to MC. (<== this will give us some
insights about how to implement the policy and the metrics.)
2) Replace MPL with MC v0 so that it doesn't regress performance / memory.
3) Support more functionalities in MC v1 (e.g., dynamic svelte etc).

I really want to avoid going back and forth about the approach of replacing MPL
with MC. We should really decide on one approach and move things forward. It's
more important that we get to 2) as soon as possible.

[haraken, 2017-06-19 16:27:01]

On 2017/06/19 04:52:53, haraken wrote:
> On 2017/06/19 04:37:24, bashi wrote:
> > On 2017/06/19 04:31:27, haraken wrote:
> > > Are you planning to ship MemoryCoordinator while keeping the existing
> > > MemoryPressureListener? If yes, this CL makes sense.
> > > 
> > > Otherwise, I'm a bit afraid that MemoryCoordinator will regress the OOM
rate
> > etc
> > > (because MemoryCoordinator just reacts to high memory pressure scenarios
> > caused
> > > by Chrome's excessive memory pressure.)
> > 
> > Yes, I'm planning to keep MemoryPressureListeners with MemoryCoordinator.
The
> > only exception is TabManager. It won't be a MemoryPressureListener when mc
is
> > enabled.
> 
> LGTM but let's wait for Chris' approval.
> 
> Original plan:
> 
> 1) Replace MPL with MC v0 so that it doesn't regress performance / memory.
(<==
> people have many concerns about how to implement the policy and the metrics.)
> 2) Support more functionalities in MC v1 (e.g., urgent tab killing, dynamic
> svelte etc).
> 
> Current plan:
> 
> 1) Keep MPL. Implement the urgent tab killing to MC. (<== this will give us
some
> insights about how to implement the policy and the metrics.)
> 2) Replace MPL with MC v0 so that it doesn't regress performance / memory.
> 3) Support more functionalities in MC v1 (e.g., dynamic svelte etc).
> 
> I really want to avoid going back and forth about the approach of replacing
MPL
> with MC. We should really decide on one approach and move things forward. It's
> more important that we get to 2) as soon as possible.

As I sent an email to chrome-memory@, IIUC MemoryPressureListener is not working
on renderers at all (I think this is a serious bug in theory although I don't
find any problematic regression in memory related UMAs)... So there would be no
need to keep MPL with MC.

[bashi, 2017-06-20 00:10:54]

On 2017/06/19 16:27:01, haraken wrote:
> On 2017/06/19 04:52:53, haraken wrote:
> > On 2017/06/19 04:37:24, bashi wrote:
> > > On 2017/06/19 04:31:27, haraken wrote:
> > > > Are you planning to ship MemoryCoordinator while keeping the existing
> > > > MemoryPressureListener? If yes, this CL makes sense.
> > > > 
> > > > Otherwise, I'm a bit afraid that MemoryCoordinator will regress the OOM
> rate
> > > etc
> > > > (because MemoryCoordinator just reacts to high memory pressure scenarios
> > > caused
> > > > by Chrome's excessive memory pressure.)
> > > 
> > > Yes, I'm planning to keep MemoryPressureListeners with MemoryCoordinator.
> The
> > > only exception is TabManager. It won't be a MemoryPressureListener when mc
> is
> > > enabled.
> > 
> > LGTM but let's wait for Chris' approval.
> > 
> > Original plan:
> > 
> > 1) Replace MPL with MC v0 so that it doesn't regress performance / memory.
> (<==
> > people have many concerns about how to implement the policy and the
metrics.)
> > 2) Support more functionalities in MC v1 (e.g., urgent tab killing, dynamic
> > svelte etc).
> > 
> > Current plan:
> > 
> > 1) Keep MPL. Implement the urgent tab killing to MC. (<== this will give us
> some
> > insights about how to implement the policy and the metrics.)
> > 2) Replace MPL with MC v0 so that it doesn't regress performance / memory.
> > 3) Support more functionalities in MC v1 (e.g., dynamic svelte etc).
> > 
> > I really want to avoid going back and forth about the approach of replacing
> MPL
> > with MC. We should really decide on one approach and move things forward.
It's
> > more important that we get to 2) as soon as possible.
> 
> As I sent an email to chrome-memory@, IIUC MemoryPressureListener is not
working
> on renderers at all (I think this is a serious bug in theory although I don't
> find any problematic regression in memory related UMAs)... So there would be
no
> need to keep MPL with MC.

I don't think that's true.
- MPLs in browser still be working
- Renderers listens OS's signal on Android

I wrote the doc about that and I described above many times. Also, I'm not sure
not sending signals to renderer is a bug -- we stopped sending signals
intentionally as Brett described.

[haraken, 2017-06-20 01:32:09]

On 2017/06/20 00:10:54, bashi wrote:
> On 2017/06/19 16:27:01, haraken wrote:
> > On 2017/06/19 04:52:53, haraken wrote:
> > > On 2017/06/19 04:37:24, bashi wrote:
> > > > On 2017/06/19 04:31:27, haraken wrote:
> > > > > Are you planning to ship MemoryCoordinator while keeping the existing
> > > > > MemoryPressureListener? If yes, this CL makes sense.
> > > > > 
> > > > > Otherwise, I'm a bit afraid that MemoryCoordinator will regress the
OOM
> > rate
> > > > etc
> > > > > (because MemoryCoordinator just reacts to high memory pressure
scenarios
> > > > caused
> > > > > by Chrome's excessive memory pressure.)
> > > > 
> > > > Yes, I'm planning to keep MemoryPressureListeners with
MemoryCoordinator.
> > The
> > > > only exception is TabManager. It won't be a MemoryPressureListener when
mc
> > is
> > > > enabled.
> > > 
> > > LGTM but let's wait for Chris' approval.
> > > 
> > > Original plan:
> > > 
> > > 1) Replace MPL with MC v0 so that it doesn't regress performance / memory.
> > (<==
> > > people have many concerns about how to implement the policy and the
> metrics.)
> > > 2) Support more functionalities in MC v1 (e.g., urgent tab killing,
dynamic
> > > svelte etc).
> > > 
> > > Current plan:
> > > 
> > > 1) Keep MPL. Implement the urgent tab killing to MC. (<== this will give
us
> > some
> > > insights about how to implement the policy and the metrics.)
> > > 2) Replace MPL with MC v0 so that it doesn't regress performance / memory.
> > > 3) Support more functionalities in MC v1 (e.g., dynamic svelte etc).
> > > 
> > > I really want to avoid going back and forth about the approach of
replacing
> > MPL
> > > with MC. We should really decide on one approach and move things forward.
> It's
> > > more important that we get to 2) as soon as possible.
> > 
> > As I sent an email to chrome-memory@, IIUC MemoryPressureListener is not
> working
> > on renderers at all (I think this is a serious bug in theory although I
don't
> > find any problematic regression in memory related UMAs)... So there would be
> no
> > need to keep MPL with MC.
> 
> I don't think that's true.
> - MPLs in browser still be working
> - Renderers listens OS's signal on Android

Ah, I was missing the Android bit. Makes sense.

> I wrote the doc about that and I described above many times. Also, I'm not
sure
> not sending signals to renderer is a bug -- we stopped sending signals
> intentionally as Brett described.

Right. That change makes sense in terms of stopping sending memory pressures
after OS started thrashing / swapping. However, that change doesn't make sense
in terms of V8 and Oilpan because they are designed assuming that the memory
pressure is working. The OOM-rate regression on ChromeCast was caused by this. I
wanted to make sure that this is not causing other problems in the real world.
As far as I look at memory UMAs, there's no observable regression or
improvement.

[bashi, 2017-06-21 23:42:06]

Thank you for review.

Let me submit this CL. In the future we want to re-enable purging and/or
allocation throttling with MC but for now I'd like to remove them to focus on
urgent tab killing.

[bashi, 2017-06-21 23:42:13]

The CQ bit was checked by bashi@chromium.org

[commit-bot: I haz the power, 2017-06-21 23:42:33]

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-06-22 01:15:21]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1498088533416570, "parent_rev":
"c5bfa5e20bf9e0d983a6538d66ea36c7a98f2d72", "commit_rev":
"c8af805863f40f75271df364e73ad02c2f6a18f2"}

[commit-bot: I haz the power, 2017-06-22 01:15:31]

Description was changed from

==========
Update default policy for memory coordinator

For now, memory coordinator is going to focus on how to detect memory
pressure and when to ask TabManager to schedule tab discarding. To
make things simpler, remove all actions other than tab discarding from
the default policy.

BUG=696844
==========

to

==========
Update default policy for memory coordinator

For now, memory coordinator is going to focus on how to detect memory
pressure and when to ask TabManager to schedule tab discarding. To
make things simpler, remove all actions other than tab discarding from
the default policy.

BUG=696844

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

[commit-bot: I haz the power, 2017-06-22 01:15:33]

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