Add architecture for simulating memory pressure notifications in all processes

Add architecture for simulating memory pressure notifications in all processes

This patch augments the architecture for cross-process memory
notification suppressing (https://codereview.chromium.org/1361963002/)
with methods and IPC messages for simulating notifications.

The required architecture for simulating memory pressure notifications
within a single process was added in
https://codereview.chromium.org/1370713002/.

This patch is a part of the plan to implement a DevTools API for
suppressing and simulating memory pressure signals in Chrome. The main
use case for this feature is to enforce consistent conditions across
memory measurements. See https://goo.gl/cZFdH3 for more details.

BUG=516776
Committed: https://crrev.com/bdfa9fad1377fac771a6e2ddf169adae989799be
Cr-Commit-Position: refs/heads/master@{#351623}

[petrcermak, 2015-09-25 15:21:30]

Patchset #2 (id:20001) has been deleted

[petrcermak, 2015-09-25 15:21:35]

Patchset #1 (id:1) has been deleted

[petrcermak, 2015-09-25 15:22:50]

petrcermak@chromium.org changed reviewers:
+ nasko@chromium.org, primiano@chromium.org

[petrcermak, 2015-09-25 15:22:50]

Hi,

Please this patch.

Thanks,
Petr

[petrcermak, 2015-09-25 15:24:37]

+cc rmcilroy

[petrcermak, 2015-09-25 16:31:18]

+cc chrisha

[chrisha, 2015-09-25 17:04:50]

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

[chrisha, 2015-09-25 17:04:51]

Approach lgtm (I don't know enough about IPC implementation to comment on it
specifically.)

Maybe worth mentioning to reviewers that this is on top of
https://codereview.chromium.org/1370713002/ ?

[Primiano Tucci (use gerrit), 2015-09-28 12:47:32]

Thanks! Just some comments.
Other than those LGTM (remember to add owners for _messages and general content)

https://codereview.chromium.org/1362233003/diff/40001/content/browser/memory/...
File content/browser/memory/memory_message_filter.h (right):

https://codereview.chromium.org/1362233003/diff/40001/content/browser/memory/...
content/browser/memory/memory_message_filter.h:15: 
are you doing this just to avoid wrapping the line below?
This will cause to alias content::MemoryPressureLevel to base::... to whoever
includes this header. IMHO this becomes harder to read (you have to follow the
indirections) vs. just using the full base type

https://codereview.chromium.org/1362233003/diff/40001/content/browser/memory/...
File content/browser/memory/memory_pressure_controller.cc (right):

https://codereview.chromium.org/1362233003/diff/40001/content/browser/memory/...
content/browser/memory/memory_pressure_controller.cc:78: // controller is a
leaky singleton (i.e. it is never deleted).
the i.e. part in  parenthesis is really superfluos.

https://codereview.chromium.org/1362233003/diff/40001/content/browser/memory/...
content/browser/memory/memory_pressure_controller.cc:91: for
(MemoryMessageFilterSet::iterator it = memory_message_filters_.begin();
you could use a one-line C++11 style foreach here
for (scoped_refptr<MemoryMessageFilter>& mf : memory_message_filters_)

(the & ref avoids to temporary copy the scoped_refptr in the scope of the for)

https://codereview.chromium.org/1362233003/diff/40001/content/browser/memory/...
content/browser/memory/memory_pressure_controller.cc:93:
it->get()->SendSimulatePressureNotification(level);
..and that will avoid you to use the ->get()
What an awesome saving of bytes :)

https://codereview.chromium.org/1362233003/diff/40001/content/browser/memory/...
File content/browser/memory/memory_pressure_controller.h (right):

https://codereview.chromium.org/1362233003/diff/40001/content/browser/memory/...
content/browser/memory/memory_pressure_controller.h:20: using
MemoryMessageFilterSet = std::set<scoped_refptr<MemoryMessageFilter>>;
ditto here. ALso, this is now duplicating the alias.
At very least move the using MemoryMessageFilterSet back where the typedef was.

https://codereview.chromium.org/1362233003/diff/40001/content/browser/memory/...
File content/browser/memory/memory_pressure_controller_browsertest.cc (right):

https://codereview.chromium.org/1362233003/diff/40001/content/browser/memory/...
content/browser/memory/memory_pressure_controller_browsertest.cc:18: const auto
MEMORY_PRESSURE_LEVEL_MODERATE =
you could move these two lines in the test that needs them

https://codereview.chromium.org/1362233003/diff/40001/content/browser/memory/...
content/browser/memory/memory_pressure_controller_browsertest.cc:175:
RunAllPendingInMessageLoop();  // Force the listener to run.
I'd reword the coomment to  "Wait for the listener"
This is not really "forcing"

https://codereview.chromium.org/1362233003/diff/40001/content/child/memory/ch...
File content/child/memory/child_memory_message_filter.h (right):

https://codereview.chromium.org/1362233003/diff/40001/content/child/memory/ch...
content/child/memory/child_memory_message_filter.h:13: using MemoryPressureLevel
= base::MemoryPressureListener::MemoryPressureLevel;
ditto

[petrcermak, 2015-09-28 15:39:33]

Thanks for your comments.

Petr

https://codereview.chromium.org/1362233003/diff/40001/content/browser/memory/...
File content/browser/memory/memory_message_filter.h (right):

https://codereview.chromium.org/1362233003/diff/40001/content/browser/memory/...
content/browser/memory/memory_message_filter.h:15: 
On 2015/09/28 12:47:31, Primiano Tucci wrote:
> are you doing this just to avoid wrapping the line below?
> This will cause to alias content::MemoryPressureLevel to base::... to whoever
> includes this header. IMHO this becomes harder to read (you have to follow the
> indirections) vs. just using the full base type

Done.

https://codereview.chromium.org/1362233003/diff/40001/content/browser/memory/...
File content/browser/memory/memory_pressure_controller.cc (right):

https://codereview.chromium.org/1362233003/diff/40001/content/browser/memory/...
content/browser/memory/memory_pressure_controller.cc:78: // controller is a
leaky singleton (i.e. it is never deleted).
On 2015/09/28 12:47:31, Primiano Tucci wrote:
> the i.e. part in  parenthesis is really superfluos.

Done (removed in both methods).

https://codereview.chromium.org/1362233003/diff/40001/content/browser/memory/...
content/browser/memory/memory_pressure_controller.cc:91: for
(MemoryMessageFilterSet::iterator it = memory_message_filters_.begin();
On 2015/09/28 12:47:31, Primiano Tucci wrote:
> you could use a one-line C++11 style foreach here
> for (scoped_refptr<MemoryMessageFilter>& mf : memory_message_filters_)
> 
> (the & ref avoids to temporary copy the scoped_refptr in the scope of the for)

Done (in both methods).

https://codereview.chromium.org/1362233003/diff/40001/content/browser/memory/...
content/browser/memory/memory_pressure_controller.cc:93:
it->get()->SendSimulatePressureNotification(level);
On 2015/09/28 12:47:32, Primiano Tucci wrote:
> ..and that will avoid you to use the ->get()
> What an awesome saving of bytes :)

Done.

https://codereview.chromium.org/1362233003/diff/40001/content/browser/memory/...
File content/browser/memory/memory_pressure_controller.h (right):

https://codereview.chromium.org/1362233003/diff/40001/content/browser/memory/...
content/browser/memory/memory_pressure_controller.h:20: using
MemoryMessageFilterSet = std::set<scoped_refptr<MemoryMessageFilter>>;
On 2015/09/28 12:47:32, Primiano Tucci wrote:
> ditto here. ALso, this is now duplicating the alias.
> At very least move the using MemoryMessageFilterSet back where the typedef
was.

Done.

https://codereview.chromium.org/1362233003/diff/40001/content/browser/memory/...
File content/browser/memory/memory_pressure_controller_browsertest.cc (right):

https://codereview.chromium.org/1362233003/diff/40001/content/browser/memory/...
content/browser/memory/memory_pressure_controller_browsertest.cc:18: const auto
MEMORY_PRESSURE_LEVEL_MODERATE =
On 2015/09/28 12:47:32, Primiano Tucci wrote:
> you could move these two lines in the test that needs them

Done.

https://codereview.chromium.org/1362233003/diff/40001/content/browser/memory/...
content/browser/memory/memory_pressure_controller_browsertest.cc:175:
RunAllPendingInMessageLoop();  // Force the listener to run.
On 2015/09/28 12:47:32, Primiano Tucci wrote:
> I'd reword the coomment to  "Wait for the listener"
> This is not really "forcing" 

Done.

https://codereview.chromium.org/1362233003/diff/40001/content/child/memory/ch...
File content/child/memory/child_memory_message_filter.h (right):

https://codereview.chromium.org/1362233003/diff/40001/content/child/memory/ch...
content/child/memory/child_memory_message_filter.h:13: using MemoryPressureLevel
= base::MemoryPressureListener::MemoryPressureLevel;
On 2015/09/28 12:47:32, Primiano Tucci wrote:
> ditto

Done.

[nasko, 2015-09-30 16:44:58]

LGTM with a few nits.

https://codereview.chromium.org/1362233003/diff/60001/content/browser/memory/...
File content/browser/memory/memory_pressure_controller.cc (right):

https://codereview.chromium.org/1362233003/diff/60001/content/browser/memory/...
content/browser/memory/memory_pressure_controller.cc:66: for (const
scoped_refptr<MemoryMessageFilter>& mf : memory_message_filters_)
nit: s/mf/filter/

https://codereview.chromium.org/1362233003/diff/60001/content/browser/memory/...
content/browser/memory/memory_pressure_controller.cc:89: for (const
scoped_refptr<MemoryMessageFilter>& mf : memory_message_filters_)
nit: s/mf/filter/

https://codereview.chromium.org/1362233003/diff/60001/content/child/memory/ch...
File content/child/memory/child_memory_message_filter.cc (right):

https://codereview.chromium.org/1362233003/diff/60001/content/child/memory/ch...
content/child/memory/child_memory_message_filter.cc:35: // Simulate a memory
pressure notification in the child process.
Redundant comment. It is pretty clear by the actual code what is happening.

https://codereview.chromium.org/1362233003/diff/60001/content/common/memory_m...
File content/common/memory_messages.h (right):

https://codereview.chromium.org/1362233003/diff/60001/content/common/memory_m...
content/common/memory_messages.h:20: using MemoryPressureLevel =
base::MemoryPressureListener::MemoryPressureLevel;
This line seems useless, since all usage of MemoryPressureLevel are prefixed
with the full qualifier.

[petrcermak, 2015-09-30 18:18:37]

Thanks for your comments.

Petr

https://codereview.chromium.org/1362233003/diff/60001/content/browser/memory/...
File content/browser/memory/memory_pressure_controller.cc (right):

https://codereview.chromium.org/1362233003/diff/60001/content/browser/memory/...
content/browser/memory/memory_pressure_controller.cc:66: for (const
scoped_refptr<MemoryMessageFilter>& mf : memory_message_filters_)
On 2015/09/30 16:44:58, nasko (slow to review) wrote:
> nit: s/mf/filter/

Done.

https://codereview.chromium.org/1362233003/diff/60001/content/browser/memory/...
content/browser/memory/memory_pressure_controller.cc:89: for (const
scoped_refptr<MemoryMessageFilter>& mf : memory_message_filters_)
On 2015/09/30 16:44:58, nasko (slow to review) wrote:
> nit: s/mf/filter/

Done.

https://codereview.chromium.org/1362233003/diff/60001/content/child/memory/ch...
File content/child/memory/child_memory_message_filter.cc (right):

https://codereview.chromium.org/1362233003/diff/60001/content/child/memory/ch...
content/child/memory/child_memory_message_filter.cc:35: // Simulate a memory
pressure notification in the child process.
On 2015/09/30 16:44:58, nasko (slow to review) wrote:
> Redundant comment. It is pretty clear by the actual code what is happening.

Done.

https://codereview.chromium.org/1362233003/diff/60001/content/common/memory_m...
File content/common/memory_messages.h (right):

https://codereview.chromium.org/1362233003/diff/60001/content/common/memory_m...
content/common/memory_messages.h:20: using MemoryPressureLevel =
base::MemoryPressureListener::MemoryPressureLevel;
On 2015/09/30 16:44:58, nasko (slow to review) wrote:
> This line seems useless, since all usage of MemoryPressureLevel are prefixed
> with the full qualifier.

Done.

[petrcermak, 2015-09-30 18:18:41]

The CQ bit was checked by petrcermak@chromium.org

[petrcermak, 2015-09-30 18:18:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, chrisha@chromium.org, nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1362233003/#ps80001
(title: "Address nasko's comments")

[commit-bot: I haz the power, 2015-09-30 18:22:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1362233003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1362233003/80001

[commit-bot: I haz the power, 2015-09-30 20:05:34]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2015-09-30 20:06:19]

Patchset 3 (id:??) landed as
https://crrev.com/bdfa9fad1377fac771a6e2ddf169adae989799be
Cr-Commit-Position: refs/heads/master@{#351623}