Make HistoryBackend a client of memory coordinator

Make HistoryBackend a client of memory coordinator

When memory coordinator is enabled, HistoryBackend becomes a
client of memory coordinator instead of installing
MemoryPressureListener.

Also MemoryCoordinator becomes a singleton so that any component
can access MemoryCoordinator to listen to memory events.

BUG=639700
Committed: https://crrev.com/953291ad42525b286dbf60be49cecd001c98b8e6
Cr-Commit-Position: refs/heads/master@{#415141}

[bashi, 2016-08-22 00:27:23]

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

[commit-bot: I haz the power, 2016-08-22 00:27:35]

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-08-22 01:34:39]

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

[commit-bot: I haz the power, 2016-08-22 01:34:40]

Dry run: This issue passed the CQ dry run.

[bashi, 2016-08-23 00:24:22]

Description was changed from

==========
Make HistoryBackend a client of memory coordinator

When memory coordinator is enabled, HistoryBackend becomes a
client of memory coordinator instead of installing
MemoryPressureListener.

This CL also adds a static getter for MemoryCoordinator so that
any component can access the memory coordinator.

BUG=39700
==========

to

==========
Make HistoryBackend a client of memory coordinator

When memory coordinator is enabled, HistoryBackend becomes a
client of memory coordinator instead of installing
MemoryPressureListener.

Also MemoryCoordinator becomes a singleton so that any component
which can access to MemoryCoordinator to listen memory events.

BUG=39700
==========

[bashi, 2016-08-23 00:25:09]

Description was changed from

==========
Make HistoryBackend a client of memory coordinator

When memory coordinator is enabled, HistoryBackend becomes a
client of memory coordinator instead of installing
MemoryPressureListener.

Also MemoryCoordinator becomes a singleton so that any component
which can access to MemoryCoordinator to listen memory events.

BUG=39700
==========

to

==========
Make HistoryBackend a client of memory coordinator

When memory coordinator is enabled, HistoryBackend becomes a
client of memory coordinator instead of installing
MemoryPressureListener.

Also MemoryCoordinator becomes a singleton so that any component
which can access to MemoryCoordinator to listen memory events.

BUG=639700
==========

[bashi, 2016-08-23 00:27:11]

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

[bashi, 2016-08-23 00:27:12]

PTAL

This CL depends on https://codereview.chromium.org/2265893002. If this looks
good to you, I'll ask history owners' review.

[haraken, 2016-08-23 00:34:38]

non-owner LGTM

[bashi, 2016-08-23 00:35:47]

Description was changed from

==========
Make HistoryBackend a client of memory coordinator

When memory coordinator is enabled, HistoryBackend becomes a
client of memory coordinator instead of installing
MemoryPressureListener.

Also MemoryCoordinator becomes a singleton so that any component
which can access to MemoryCoordinator to listen memory events.

BUG=639700
==========

to

==========
Make HistoryBackend a client of memory coordinator

When memory coordinator is enabled, HistoryBackend becomes a
client of memory coordinator instead of installing
MemoryPressureListener.

Also MemoryCoordinator becomes a singleton so that any component
can access to MemoryCoordinator to listen memory events.

BUG=639700
==========

[bashi, 2016-08-23 00:36:26]

Description was changed from

==========
Make HistoryBackend a client of memory coordinator

When memory coordinator is enabled, HistoryBackend becomes a
client of memory coordinator instead of installing
MemoryPressureListener.

Also MemoryCoordinator becomes a singleton so that any component
can access to MemoryCoordinator to listen memory events.

BUG=639700
==========

to

==========
Make HistoryBackend a client of memory coordinator

When memory coordinator is enabled, HistoryBackend becomes a
client of memory coordinator instead of installing
MemoryPressureListener.

Also MemoryCoordinator becomes a singleton so that any component
can access MemoryCoordinator to listen to memory events.

BUG=639700
==========

[bashi, 2016-08-23 14:04:38]

bashi@chromium.org changed reviewers:
+ jam@chromium.org, sky@chromium.org

[bashi, 2016-08-23 14:04:38]

sky@: could you review components/history ?
jam@: could you review content/ ?

[sky, 2016-08-23 17:20:08]

https://codereview.chromium.org/2261073002/diff/20001/components/history/core...
File components/history/core/browser/history_backend.cc (right):

https://codereview.chromium.org/2261073002/diff/20001/components/history/core...
components/history/core/browser/history_backend.cc:259:
memory_pressure_listener_.reset(new base::MemoryPressureListener(
How come you couldn't change MemoryPressureListener directly rather than
creating what looks like a very similar api?

[jam, 2016-08-23 17:37:16]

https://codereview.chromium.org/2261073002/diff/20001/content/browser/browser...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/2261073002/diff/20001/content/browser/browser...
content/browser/browser_main_loop.cc:719:
memory_coordinator::MemoryCoordinator::GetInstance();
why is this needed? the constructor of MC doesn't do anything, so why not just
have it instantiated by whoever first uses it?

[bashi, 2016-08-24 23:33:16]

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

[bashi, 2016-08-24 23:33:28]

Thank you for review!

https://codereview.chromium.org/2261073002/diff/20001/components/history/core...
File components/history/core/browser/history_backend.cc (right):

https://codereview.chromium.org/2261073002/diff/20001/components/history/core...
components/history/core/browser/history_backend.cc:259:
memory_pressure_listener_.reset(new base::MemoryPressureListener(
On 2016/08/23 17:20:08, sky wrote:
> How come you couldn't change MemoryPressureListener directly rather than
> creating what looks like a very similar api?

chrisha@ and haraken@ might be able to explain the motivation better, but
MemoryPressureListener has a lot of problem[1] and we are going to deprecate
MemoryPressureListener and replace it with MemoryCoordinatorClient
incrementally. The difference between MPL and MCC is described in [2].

[1]
https://docs.google.com/document/d/1dkUXXmpJk7xBUeQM-olBpTHJ2MXamDgY_kjNrl9JX...
[2]
https://docs.google.com/document/d/1a69mMr7jI7qK0OfKNlrZ350xiXizVMCCe8orGX7K8...

https://codereview.chromium.org/2261073002/diff/20001/content/browser/browser...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/2261073002/diff/20001/content/browser/browser...
content/browser/browser_main_loop.cc:719:
memory_coordinator::MemoryCoordinator::GetInstance();
On 2016/08/23 17:37:16, jam wrote:
> why is this needed? the constructor of MC doesn't do anything, so why not just
> have it instantiated by whoever first uses it?

Done.

[commit-bot: I haz the power, 2016-08-24 23:34:26]

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

[sky, 2016-08-24 23:48:21]

https://codereview.chromium.org/2261073002/diff/40001/components/history/core...
File components/history/core/browser/history_backend.cc (right):

https://codereview.chromium.org/2261073002/diff/40001/components/history/core...
components/history/core/browser/history_backend.cc:257:
memory_coordinator::MemoryCoordinator::GetInstance()->RegisterClient(this);
Shouldn't you unregister in the destructor?

https://codereview.chromium.org/2261073002/diff/40001/components/history/core...
components/history/core/browser/history_backend.cc:760: void
HistoryBackend::OnMemoryStateChange(
Make position match that of header.

[bashi, 2016-08-24 23:53:10]

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

[commit-bot: I haz the power, 2016-08-24 23:54:44]

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

[bashi, 2016-08-24 23:57:53]

https://codereview.chromium.org/2261073002/diff/40001/components/history/core...
File components/history/core/browser/history_backend.cc (right):

https://codereview.chromium.org/2261073002/diff/40001/components/history/core...
components/history/core/browser/history_backend.cc:257:
memory_coordinator::MemoryCoordinator::GetInstance()->RegisterClient(this);
On 2016/08/24 23:48:21, sky wrote:
> Shouldn't you unregister in the destructor?

Yes. Done.

https://codereview.chromium.org/2261073002/diff/40001/components/history/core...
components/history/core/browser/history_backend.cc:760: void
HistoryBackend::OnMemoryStateChange(
On 2016/08/24 23:48:21, sky wrote:
> Make position match that of header.

Done.

[sky, 2016-08-25 00:02:03]

LGTM

[commit-bot: I haz the power, 2016-08-25 01:59:17]

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

[commit-bot: I haz the power, 2016-08-25 01:59:18]

Dry run: This issue passed the CQ dry run.

[bashi, 2016-08-25 23:06:07]

jam@: could you take another look?

[jam, 2016-08-28 23:38:02]

lgtm

[bashi, 2016-08-28 23:40:20]

The CQ bit was checked by bashi@chromium.org

[bashi, 2016-08-28 23:40:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, jam@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2261073002/#ps80001
(title: "rebase")

[commit-bot: I haz the power, 2016-08-28 23:40:26]

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-08-29 00:47:27]

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

[commit-bot: I haz the power, 2016-08-29 00:47:28]

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...)

[bashi, 2016-08-29 00:56:14]

The CQ bit was checked by bashi@chromium.org

[commit-bot: I haz the power, 2016-08-29 00:56:22]

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-08-29 02:08:12]

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

[commit-bot: I haz the power, 2016-08-29 02:08:13]

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...)

[bashi, 2016-08-29 04:49:19]

The CQ bit was checked by bashi@chromium.org

[commit-bot: I haz the power, 2016-08-29 04:49:40]

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-08-29 06:21:52]

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

[commit-bot: I haz the power, 2016-08-29 06:21:54]

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...)

[bashi, 2016-08-29 09:56:12]

The CQ bit was checked by bashi@chromium.org

[commit-bot: I haz the power, 2016-08-29 09:56:30]

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-08-29 10:58:47]

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

[commit-bot: I haz the power, 2016-08-29 10:58:48]

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...)

[bashi, 2016-08-29 22:55:44]

The CQ bit was checked by bashi@chromium.org

[commit-bot: I haz the power, 2016-08-29 23:17:21]

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

[chrisha, 2016-08-30 02:40:12]

+1 to the singleton pattern, I think this will make other cases like this much
easier.

A question regarding the TrimMemory mechanism. The intent is for the
OnMemoryStateChange to be a transition to a new state, and for the client to use
consistently less memory in that new state. TrimMemory seems like a "drop
everything on the floor at this very moment and then continue on like normal"
effort, which is part of the janky problem we're trying to address with MC vs
MPL. The trim is accomplished by dropping the cache size momentarily, and then
increasing it again [1]. Can we consider following the trim by setting a
persistent but lower cache size setting that will persist under the given
MemoryState? Maybe add a different helper function in src/sql/Connection?

A strawman idea: set to 1/2 on moderate, and 1/4 on critical?

[1]
https://cs.chromium.org/chromium/src/sql/connection.cc?sq=package:chromium&rc...

[bashi, 2016-08-30 03:01:42]

On 2016/08/30 02:40:12, chrisha (slow) wrote:
> +1 to the singleton pattern, I think this will make other cases like this much
> easier.
> 
> A question regarding the TrimMemory mechanism. The intent is for the
> OnMemoryStateChange to be a transition to a new state, and for the client to
use
> consistently less memory in that new state. TrimMemory seems like a "drop
> everything on the floor at this very moment and then continue on like normal"
> effort, which is part of the janky problem we're trying to address with MC vs
> MPL. The trim is accomplished by dropping the cache size momentarily, and then
> increasing it again [1]. Can we consider following the trim by setting a
> persistent but lower cache size setting that will persist under the given
> MemoryState? Maybe add a different helper function in src/sql/Connection?
> 
> A strawman idea: set to 1/2 on moderate, and 1/4 on critical?
> 
> [1]
>
https://cs.chromium.org/chromium/src/sql/connection.cc?sq=package:chromium&rc...

This sounds a great idea to me. Let me try to do that in a follow-up CL.

[commit-bot: I haz the power, 2016-08-30 05:13:54]

Description was changed from

==========
Make HistoryBackend a client of memory coordinator

When memory coordinator is enabled, HistoryBackend becomes a
client of memory coordinator instead of installing
MemoryPressureListener.

Also MemoryCoordinator becomes a singleton so that any component
can access MemoryCoordinator to listen to memory events.

BUG=639700
==========

to

==========
Make HistoryBackend a client of memory coordinator

When memory coordinator is enabled, HistoryBackend becomes a
client of memory coordinator instead of installing
MemoryPressureListener.

Also MemoryCoordinator becomes a singleton so that any component
can access MemoryCoordinator to listen to memory events.

BUG=639700
==========

[commit-bot: I haz the power, 2016-08-30 05:13:57]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-08-30 05:16:46]

Description was changed from

==========
Make HistoryBackend a client of memory coordinator

When memory coordinator is enabled, HistoryBackend becomes a
client of memory coordinator instead of installing
MemoryPressureListener.

Also MemoryCoordinator becomes a singleton so that any component
can access MemoryCoordinator to listen to memory events.

BUG=639700
==========

to

==========
Make HistoryBackend a client of memory coordinator

When memory coordinator is enabled, HistoryBackend becomes a
client of memory coordinator instead of installing
MemoryPressureListener.

Also MemoryCoordinator becomes a singleton so that any component
can access MemoryCoordinator to listen to memory events.

BUG=639700

Committed: https://crrev.com/953291ad42525b286dbf60be49cecd001c98b8e6
Cr-Commit-Position: refs/heads/master@{#415141}
==========

[commit-bot: I haz the power, 2016-08-30 05:16:47]

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