Architecture for cross-process memory notification suppressing

Architecture for cross-process memory notification suppressing

This patch adds IPC architecture for suppressing memory pressure
notifications in all processes:

              BROWSER PROCESS                    CHILD PROCESSES

         MemoryPressureListener::           MemoryPressureListener::
        SetNotificationsSuppressed         SetNotificationsSuppressed
        (existing static method*)          (existing static method*)
                     ^                                  ^
                     |                                  |
       +--------------------------+                     |
       | MemoryPressureController |                     |
  +..> |        (singleton)       |                     |
  :    +--------------------------+                     |
  :                  |                                  |
  :                  V                                  |
  :    +--------------------------+       +--------------------------+
  :    |    MemoryMessageFilter   | <===> | ChildMemoryMessageFilter |
  :    |    (per child process)   |  IPC  |        (singleton)       |
  :    +--------------------------+       +--------------------------+
  :
  :
  +.. Memory.setPressureNotificationsSuppressed
              (proposed DevTools API**)

*)  The required functionality for individual processes was added in:
    https://codereview.chromium.org/1312163003.
**) The new DevTools API will be added in the following 3-sided patch:
    https://codereview.chromium.org/1336363002,
    https://codereview.chromium.org/1311343007, and
    https://codereview.chromium.org/1342833004.

This patch adds new message filters on both sides (MemoryMessageFilter
in the browser process and ChildMemoryMessageFilter in the child
process) because we anticipate more functionality to be added to
MemoryPressureController in the near future (methods for simulating
memory pressure signals and, more importantly, propagating memory
pressure signals to all processes on desktop Chrome). Encapsulating the
relevant IPC communication in dedicated message filters is arguably
better than keeping augmenting (and having duplicate code in)
BrowserChildProcessHostImpl, RenderProcessHostImpl, and
ChildThreadImpl.

This patch represents the second step towards implementing 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/0b119f3392d6c6169bbb792347a04f34ce649156
Cr-Commit-Position: refs/heads/master@{#350169}

[petrcermak, 2015-09-08 13:41:08]

Patchset #1 (id:1) has been deleted

[petrcermak, 2015-09-08 13:44:59]

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

[petrcermak, 2015-09-08 13:45:00]

Hi, this is a prototype of the cross-process memory notification suppressing
architecture we discussed yesterday. Please have a look at it and let me know
what you think.

Thanks,
Petr

[Sami, 2015-09-08 18:06:56]

Looks pretty good. Same comment about naming as in the Blink patch.

https://codereview.chromium.org/1332583002/diff/20001/content/browser/devtool...
File content/browser/devtools/protocol/memory_handler.cc (right):

https://codereview.chromium.org/1332583002/diff/20001/content/browser/devtool...
content/browser/devtools/protocol/memory_handler.cc:22: return Response::OK();
Is there ever a need to wait for this to complete in all processes before
proceeding (e.g., benchmarks)?

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

https://codereview.chromium.org/1332583002/diff/20001/content/browser/memory/...
content/browser/memory/memory_message_filter.h:27: static void
SendSetNotificationsSuppressedToAllProcesses(bool suppressed);
I'm slightly queasy about making this a static method (b/c testing), especially
if we're going to add more stuff here. I wonder if we should have something like
TracingControllerImpl on top:

https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/tr...

WDYT? Of course, YAGNI applies and what you have here is nice and simple.

[petrcermak, 2015-09-11 10:43:50]

https://codereview.chromium.org/1332583002/diff/20001/content/browser/devtool...
File content/browser/devtools/protocol/memory_handler.cc (right):

https://codereview.chromium.org/1332583002/diff/20001/content/browser/devtool...
content/browser/devtools/protocol/memory_handler.cc:22: return Response::OK();
On 2015/09/08 18:06:56, Sami wrote:
> Is there ever a need to wait for this to complete in all processes before
> proceeding (e.g., benchmarks)?

Unfortunately, there is no way to get a callback when all memory pressure
listeners are finished (this would require a tremendous amount of plumbing
because it triggers a v8 GC, which in turn triggers a minor Blink GC, which then
schedules a major GC, ...), so even if we waited for all processes to handle the
IPC request, it wouldn't actually mean much. Therefore, I think it's better to
return immediately for simplicity and also performance (e.g. no need to store
and post tasks in the browser).

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

https://codereview.chromium.org/1332583002/diff/20001/content/browser/memory/...
content/browser/memory/memory_message_filter.h:27: static void
SendSetNotificationsSuppressedToAllProcesses(bool suppressed);
On 2015/09/08 18:06:56, Sami wrote:
> I'm slightly queasy about making this a static method (b/c testing),
especially
> if we're going to add more stuff here. I wonder if we should have something
like
> TracingControllerImpl on top:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/tr...
> 
> WDYT? Of course, YAGNI applies and what you have here is nice and simple.

Yeah, this is something that I thought about and I'm still not sure
what is the "right" thing to do here...

I totally agree that having a singleton object like MemoryController
would be better in many ways including testing. The only issue I see
with it is that it will create the following situation:

  Browser process:

    MemoryController (singleton)
      - suppresses memory pressure notifications in the process (*)
      - keeps a list of memory message filters for all child processes

    MemoryMessageFilter (per child process)
      - sends IPC messages to the child process

  Child processes:

    ChildMemoryMessageFilter (effectively singleton)
      - receives IPC messages from the browser process
      - suppresses memory pressure notifications in the process (**)

The problem is that the code for suppressing memory pressure
notifications is now triggered from two very different places:
the controller in the browser process (*) and the filter in the
child processes (**). The natural solution would be to have
controllers on both sides that have a common base class:

                       +------------------+
                       | MemoryController |
                       +------------------+
                                ^
                                |
               +----------------+----------------+
               |                                 |
  +-------------------------+       +-----------------------+
  | BrowserMemoryController |       | ChildMemoryController |
  +-------------------------+       +-----------------------+
               :                                 ^
               V                                 :
    +---------------------+        +--------------------------+
    | MemoryMessageFilter | =====> | ChildMemoryMessageFilter |
    +---------------------+  IPC   +--------------------------+

Maybe we should just go ahead with this to avoid refactors in the
future. WDYT?

[Sami, 2015-09-11 11:37:42]

https://codereview.chromium.org/1332583002/diff/20001/content/browser/devtool...
File content/browser/devtools/protocol/memory_handler.cc (right):

https://codereview.chromium.org/1332583002/diff/20001/content/browser/devtool...
content/browser/devtools/protocol/memory_handler.cc:22: return Response::OK();
On 2015/09/11 10:43:50, petrcermak wrote:
> On 2015/09/08 18:06:56, Sami wrote:
> > Is there ever a need to wait for this to complete in all processes before
> > proceeding (e.g., benchmarks)?
> 
> Unfortunately, there is no way to get a callback when all memory pressure
> listeners are finished (this would require a tremendous amount of plumbing
> because it triggers a v8 GC, which in turn triggers a minor Blink GC, which
then
> schedules a major GC, ...), so even if we waited for all processes to handle
the
> IPC request, it wouldn't actually mean much. Therefore, I think it's better to
> return immediately for simplicity and also performance (e.g. no need to store
> and post tasks in the browser).

Like we discussed, I only meant waiting until the 'inhibit' bit has propagated
to all processes. Activating tracing after toggling this bit will force
synchronization as a side effect, so this isn't strictly required.

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

https://codereview.chromium.org/1332583002/diff/20001/content/browser/memory/...
content/browser/memory/memory_message_filter.h:27: static void
SendSetNotificationsSuppressedToAllProcesses(bool suppressed);
On 2015/09/11 10:43:50, petrcermak wrote:
> On 2015/09/08 18:06:56, Sami wrote:
> > I'm slightly queasy about making this a static method (b/c testing),
> especially
> > if we're going to add more stuff here. I wonder if we should have something
> like
> > TracingControllerImpl on top:
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/tr...
> > 
> > WDYT? Of course, YAGNI applies and what you have here is nice and simple.
> 
> Yeah, this is something that I thought about and I'm still not sure
> what is the "right" thing to do here...
> 
> I totally agree that having a singleton object like MemoryController
> would be better in many ways including testing. The only issue I see
> with it is that it will create the following situation:
> 
>   Browser process:
> 
>     MemoryController (singleton)
>       - suppresses memory pressure notifications in the process (*)
>       - keeps a list of memory message filters for all child processes
> 
>     MemoryMessageFilter (per child process)
>       - sends IPC messages to the child process
> 
>   Child processes:
> 
>     ChildMemoryMessageFilter (effectively singleton)
>       - receives IPC messages from the browser process
>       - suppresses memory pressure notifications in the process (**)
> 
> The problem is that the code for suppressing memory pressure
> notifications is now triggered from two very different places:
> the controller in the browser process (*) and the filter in the
> child processes (**). The natural solution would be to have
> controllers on both sides that have a common base class:
> 
>                        +------------------+
>                        | MemoryController |
>                        +------------------+
>                                 ^
>                                 |
>                +----------------+----------------+
>                |                                 |
>   +-------------------------+       +-----------------------+
>   | BrowserMemoryController |       | ChildMemoryController |
>   +-------------------------+       +-----------------------+
>                :                                 ^
>                V                                 :
>     +---------------------+        +--------------------------+
>     | MemoryMessageFilter | =====> | ChildMemoryMessageFilter |
>     +---------------------+  IPC   +--------------------------+
> 
> Maybe we should just go ahead with this to avoid refactors in the
> future. WDYT?

As soon as you need ascii art in a code review comment you should discuss IRL --
which we did :)

[petrcermak, 2015-09-14 12:46:22]

Thanks for your comments. PTAL.

Petr

https://codereview.chromium.org/1332583002/diff/20001/content/browser/devtool...
File content/browser/devtools/protocol/memory_handler.cc (right):

https://codereview.chromium.org/1332583002/diff/20001/content/browser/devtool...
content/browser/devtools/protocol/memory_handler.cc:22: return Response::OK();
On 2015/09/11 11:37:42, Sami wrote:
> On 2015/09/11 10:43:50, petrcermak wrote:
> > On 2015/09/08 18:06:56, Sami wrote:
> > > Is there ever a need to wait for this to complete in all processes before
> > > proceeding (e.g., benchmarks)?
> > 
> > Unfortunately, there is no way to get a callback when all memory pressure
> > listeners are finished (this would require a tremendous amount of plumbing
> > because it triggers a v8 GC, which in turn triggers a minor Blink GC, which
> then
> > schedules a major GC, ...), so even if we waited for all processes to handle
> the
> > IPC request, it wouldn't actually mean much. Therefore, I think it's better
to
> > return immediately for simplicity and also performance (e.g. no need to
store
> > and post tasks in the browser).
> 
> Like we discussed, I only meant waiting until the 'inhibit' bit has propagated
> to all processes. Activating tracing after toggling this bit will force
> synchronization as a side effect, so this isn't strictly required.

I added ack messages from child processes to address this (even if it is not
strictly necessary).

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

https://codereview.chromium.org/1332583002/diff/20001/content/browser/memory/...
content/browser/memory/memory_message_filter.h:27: static void
SendSetNotificationsSuppressedToAllProcesses(bool suppressed);
On 2015/09/11 11:37:42, Sami wrote:
> On 2015/09/11 10:43:50, petrcermak wrote:
> > On 2015/09/08 18:06:56, Sami wrote:
> > > I'm slightly queasy about making this a static method (b/c testing),
> > especially
> > > if we're going to add more stuff here. I wonder if we should have
something
> > like
> > > TracingControllerImpl on top:
> > > 
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/tr...
> > > 
> > > WDYT? Of course, YAGNI applies and what you have here is nice and simple.
> > 
> > Yeah, this is something that I thought about and I'm still not sure
> > what is the "right" thing to do here...
> > 
> > I totally agree that having a singleton object like MemoryController
> > would be better in many ways including testing. The only issue I see
> > with it is that it will create the following situation:
> > 
> >   Browser process:
> > 
> >     MemoryController (singleton)
> >       - suppresses memory pressure notifications in the process (*)
> >       - keeps a list of memory message filters for all child processes
> > 
> >     MemoryMessageFilter (per child process)
> >       - sends IPC messages to the child process
> > 
> >   Child processes:
> > 
> >     ChildMemoryMessageFilter (effectively singleton)
> >       - receives IPC messages from the browser process
> >       - suppresses memory pressure notifications in the process (**)
> > 
> > The problem is that the code for suppressing memory pressure
> > notifications is now triggered from two very different places:
> > the controller in the browser process (*) and the filter in the
> > child processes (**). The natural solution would be to have
> > controllers on both sides that have a common base class:
> > 
> >                        +------------------+
> >                        | MemoryController |
> >                        +------------------+
> >                                 ^
> >                                 |
> >                +----------------+----------------+
> >                |                                 |
> >   +-------------------------+       +-----------------------+
> >   | BrowserMemoryController |       | ChildMemoryController |
> >   +-------------------------+       +-----------------------+
> >                :                                 ^
> >                V                                 :
> >     +---------------------+        +--------------------------+
> >     | MemoryMessageFilter | =====> | ChildMemoryMessageFilter |
> >     +---------------------+  IPC   +--------------------------+
> > 
> > Maybe we should just go ahead with this to avoid refactors in the
> > future. WDYT?
> 
> As soon as you need ascii art in a code review comment you should discuss IRL
--
> which we did :)

I replaced the static behavior with a MemoryPressureController singleton.

[Sami, 2015-09-14 17:02:22]

Thanks, I think looks like a great foundation to build on.

[Primiano Tucci (use gerrit), 2015-09-16 09:11:50]

Looks a great start and I love the ascii art wrapped at 72 cols.
Some major comments here:
- I am not sure I buy the need of the ACK + callback. You seem to always
unconditionally send the ack, so I wonder whether we really need that layer of
complexity. Which value does it add? Even if the suppression could fail (doesn't
seem so in the current design) can we do something useful? If not, we should
drop the ACK + callback and make this much simpler.
- Not sure why you post to the UI thread. You are guaranteed to receive all the
messages in the IO thread, and what you are doing here is just looping and
calling a super short method (SetNotificationsSuppressed).

Also, by having everything living in the IO thread you can now remove the
base::subtle atomic in SetNotificationsSuppressed() ;-)

More comments inline

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

https://codereview.chromium.org/1332583002/diff/60001/content/browser/memory/...
content/browser/memory/memory_message_filter.cc:41: void
MemoryMessageFilter::OnSetPressureNotificationsSuppressedAck() {
This is probably unneeded (See my comment on the top of the cl)

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

https://codereview.chromium.org/1332583002/diff/60001/content/browser/memory/...
content/browser/memory/memory_message_filter.h:11: namespace memory {
Probably this memory namespace is superfluous. At the end you have 3 classes
(which have all "Memory" in their names), doesn't really justify a namespace.
The other similar classes seem to just live in content.

https://codereview.chromium.org/1332583002/diff/60001/content/browser/memory/...
content/browser/memory/memory_message_filter.h:21: void OnFilterRemoved()
override;
Plz doublecheck but I think you really want here is OnChannelConnected /
OnChannelClosing.
I don't think you can send messages between OnFilterAdded and
OnCHannelConnected, and similarly the IPC::Sender* becomes null between
OnChannelClosing and OnFilterRemoved (so if you try to send a message then you
get a nullptr)

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

https://codereview.chromium.org/1332583002/diff/60001/content/browser/memory/...
content/browser/memory/memory_pressure_controller.cc:22: if
(!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
Hmm why do you want this to live on the UI thread? I think you are just making
your life more difficult because have to deal with races on destruction.
All these methods (OnFilter/OnChannel) are guaranteed to be called on the IO
thread.
I'd just remove the thread hopping logic and add a
DCHECK(!CurrentlyOn(BrowserThread::IO))

https://codereview.chromium.org/1332583002/diff/60001/content/browser/memory/...
content/browser/memory/memory_pressure_controller.cc:87:
base::LazyInstance<MemoryPressureController>::Leaky g_controller =
out of curiosity why don't you use  Singleton + LeazySingletonTraits here? You
should use LazyInstance only if you need more than one lazy instances (it's
mostly a deal with using heap vs static storage)

[petrcermak, 2015-09-16 18:14:26]

Thanks for your comments. PTAL.

Petr

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

https://codereview.chromium.org/1332583002/diff/60001/content/browser/memory/...
content/browser/memory/memory_message_filter.cc:41: void
MemoryMessageFilter::OnSetPressureNotificationsSuppressedAck() {
On 2015/09/16 09:11:49, Primiano Tucci wrote:
> This is probably unneeded (See my comment on the top of the cl)

As discussed offline, we will drop acks for the time being. The reason is that
there is currently no architecture to wait until memory pressure notifications
have been handled by all listeners. Even if we acked the request, the following
ordering of events would still be possible:


          BROWSER PROCESS                        CHILD PROCESS

                                          MemoryPressureListener::
                                        NotifyMemoryPressure(level) ===+
                                                                       |
                                                                       |
      MemoryPressureController::            IPC               PostTask |
  SetPressureNotificationsSuppressed -----------------+                |
    InAllProcesses(true, callback)                    :                |
                                                      :                |
                                                      V                |
                                           MemoryPressureListener::    |
                                      SetNotificationsSuppressed(true) |
                                                      :                |
                                                      :                |
           callback(true) <---------------------------+                |
                 :                       IPC (ACK)                     |
                 V                                                     |
     FALSE BELIEF THAT NO MEMORY                                       |
  PRESSURE HANDLING WILL OCCUR FROM                                    |
        THIS POINT ONWARDS                                             |
                                                                       |
                                          MemoryPressureListener:: <===+
                                                Notify(level)

Therefore, we will not support acks to avoid giving false
guarantees.

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

https://codereview.chromium.org/1332583002/diff/60001/content/browser/memory/...
content/browser/memory/memory_message_filter.h:11: namespace memory {
On 2015/09/16 09:11:50, Primiano Tucci wrote:
> Probably this memory namespace is superfluous. At the end you have 3 classes
> (which have all "Memory" in their names), doesn't really justify a namespace.
> The other similar classes seem to just live in content.

Done.

https://codereview.chromium.org/1332583002/diff/60001/content/browser/memory/...
content/browser/memory/memory_message_filter.h:21: void OnFilterRemoved()
override;
On 2015/09/16 09:11:50, Primiano Tucci wrote:
> Plz doublecheck but I think you really want here is OnChannelConnected /
> OnChannelClosing.
> I don't think you can send messages between OnFilterAdded and
> OnCHannelConnected, and similarly the IPC::Sender* becomes null between
> OnChannelClosing and OnFilterRemoved (so if you try to send a message then
you
> get a nullptr)

I've searched through the Chromium source code and found the following:

ADD FILTER

OnChannelConnected(int32 peer_id)
does not guarantee that the sender has already been set. Consequently, the
initial message to suppress memory pressure notifications in a newly created
process could be dropped.

OnFilterAdded(IPC::Sender* sender)
is the right option here because it is called immediately after the sender is
set:
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro....

REMOVE FILTER

OnChannelClosing()
is the right option because is called immediately after the sender is unset:
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro....

OnFilterRemoved()
would work as well because it is guaranteed to be called immediately after
OnChannelClosing. Moreover, BrowserMessageFilter::Send() checks if the
underlying sender is null (so we could override this method even if it were
after the sender was unset).

SUMMARY

OnFilterAdded(IPC::Sender* sender) and OnChannelClosing() are the right methods
to override here.

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

https://codereview.chromium.org/1332583002/diff/60001/content/browser/memory/...
content/browser/memory/memory_pressure_controller.cc:22: if
(!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
On 2015/09/16 09:11:50, Primiano Tucci wrote:
> Hmm why do you want this to live on the UI thread? I think you are just making
> your life more difficult because have to deal with races on destruction.
> All these methods (OnFilter/OnChannel) are guaranteed to be called on the IO
> thread.
> I'd just remove the thread hopping logic and add a
> DCHECK(!CurrentlyOn(BrowserThread::IO))

Done (changed back to IO).

https://codereview.chromium.org/1332583002/diff/60001/content/browser/memory/...
content/browser/memory/memory_pressure_controller.cc:87:
base::LazyInstance<MemoryPressureController>::Leaky g_controller =
On 2015/09/16 09:11:50, Primiano Tucci wrote:
> out of curiosity why don't you use  Singleton + LeazySingletonTraits here? You
> should use LazyInstance only if you need more than one lazy instances (it's
> mostly a deal with using heap vs static storage)

Done (there wasn't any particular reason).

[petrcermak, 2015-09-16 18:15:08]

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

[petrcermak, 2015-09-16 18:15:09]

nasko: Hi, could you please review this patch?

Thanks,
Petr

[nasko, 2015-09-16 21:18:37]

I have an overall architectural question - Why do we need these message filters
at all? It seems to me that we care to send an IPC to all child processes of the
browser, which is doable by iterating over all RenderProcessHost and
BrowserChildProcessHost objects (existing APIs). Also, we care about each of
those processes doing the right thing. For renderer processes, we have
RenderThread, which is an IPC listener already and adding one more handler there
is trivial. The same holds for other child processes, which have ChildThread
which is an IPC listener.

Overall it seems we can pull off the same functionality without the extra
objects being introduced by this CL, which also means less overhead.

https://codereview.chromium.org/1332583002/diff/100001/content/browser/memory...
File content/browser/memory/memory_message_filter.cc (right):

https://codereview.chromium.org/1332583002/diff/100001/content/browser/memory...
content/browser/memory/memory_message_filter.cc:26: return false;
Why override this method instead of using the default implementation, which is
identical?

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

https://codereview.chromium.org/1332583002/diff/100001/content/browser/memory...
content/browser/memory/memory_pressure_controller.cc:53: base::Unretained(this),
suppressed));
Please add a comment why base::Unretained(this) is safe to use here.

https://codereview.chromium.org/1332583002/diff/100001/content/child/memory/c...
File content/child/memory/child_memory_message_filter.h (right):

https://codereview.chromium.org/1332583002/diff/100001/content/child/memory/c...
content/child/memory/child_memory_message_filter.h:12: // This class sends and
receives memory messages on child processes.
I don't see any evidence of this class sending any messages to the browser
process. It seems like a receive only one.

https://codereview.chromium.org/1332583002/diff/100001/content/child/memory/c...
File content/child/memory/child_memory_message_filter_unittest.cc (right):

https://codereview.chromium.org/1332583002/diff/100001/content/child/memory/c...
content/child/memory/child_memory_message_filter_unittest.cc:12: namespace {
Why the anonymous namespace? Tests usually just live in the content:: namespace
itself.

https://codereview.chromium.org/1332583002/diff/100001/content/common/memory_...
File content/common/memory_messages.h (right):

https://codereview.chromium.org/1332583002/diff/100001/content/common/memory_...
content/common/memory_messages.h:4: 
Some comment describing what types of messages belong to this file.

[petrcermak, 2015-09-17 14:19:59]

Patchset #6 (id:120001) has been deleted

[petrcermak, 2015-09-17 15:24:53]

Thanks for all your comments. PTAL.

Regarding your architectural question, there are two reasons why I
added the message filters:

  1. We will be adding some more functionality into the filters in the
     near future (one more IPC message) and potentially lots of extra
     functionality in the medium-term (to propagate memory pressure
     signals to all processes on desktop Chrome) and long-term
     (cross-process memory scheduling). It seems better to encapsulate
     all of this in separate message filters rather than keep
     augmenting (and having duplicate code in)
     BrowserChildProcessHostImpl, RenderProcessHostImpl, and
     ChildThreadImpl.

  2. A very important requirement for the memory pressure controller is
     to be able to suppress/simulate memory pressure signals in newly
     created processes (this will happen all the time in benchmarks).
     Simply iterating over renderer and child processes won't therefore
     be enough. While BrowserChildProcessHostImpl provides
     a BrowserChildProcessObserver, which we could use to listen for
     newly created child processes, the same functionality doesn't seem
     to be available in RenderProcessHostImpl (RenderProcessHostObserver
     can only listen for exit events). The two options are therefore to
     (1) add a message filter like I did, or (2) directly call the
     controller. I think that the first option is better because it
     can be added in the same places like all other message filters.
     If we were to add direct controller calls instead, we would need to
     put them somewhere after the relevant channel is opened
     (extra dependency in code that's meant to be generic).

I would prefer to add the filters on both sides. However, if you think
it's too much overhead, then I would propose keeping MemoryMessageFilter
on the browser side and merging ChildMemoryMessageFilter into
ChildThreadImpl::OnMessageReceived(). WDYT?

Thanks,
Petr

https://codereview.chromium.org/1332583002/diff/100001/content/browser/memory...
File content/browser/memory/memory_message_filter.cc (right):

https://codereview.chromium.org/1332583002/diff/100001/content/browser/memory...
content/browser/memory/memory_message_filter.cc:26: return false;
On 2015/09/16 21:18:36, nasko (slow to review) wrote:
> Why override this method instead of using the default implementation, which
is
> identical?

This is necessary becasue the method is pure in BrowserMessageFilter
(https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...),
which MemoryMessageFilterSubclasses.

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

https://codereview.chromium.org/1332583002/diff/100001/content/browser/memory...
content/browser/memory/memory_pressure_controller.cc:53: base::Unretained(this),
suppressed));
On 2015/09/16 21:18:37, nasko (slow to review) wrote:
> Please add a comment why base::Unretained(this) is safe to use here.

Done.

https://codereview.chromium.org/1332583002/diff/100001/content/child/memory/c...
File content/child/memory/child_memory_message_filter.h (right):

https://codereview.chromium.org/1332583002/diff/100001/content/child/memory/c...
content/child/memory/child_memory_message_filter.h:12: // This class sends and
receives memory messages on child processes.
On 2015/09/16 21:18:37, nasko (slow to review) wrote:
> I don't see any evidence of this class sending any messages to the browser
> process. It seems like a receive only one.

Done.

https://codereview.chromium.org/1332583002/diff/100001/content/child/memory/c...
File content/child/memory/child_memory_message_filter_unittest.cc (right):

https://codereview.chromium.org/1332583002/diff/100001/content/child/memory/c...
content/child/memory/child_memory_message_filter_unittest.cc:12: namespace {
On 2015/09/16 21:18:37, nasko (slow to review) wrote:
> Why the anonymous namespace? Tests usually just live in the content::
namespace
> itself.

Done.

https://codereview.chromium.org/1332583002/diff/100001/content/common/memory_...
File content/common/memory_messages.h (right):

https://codereview.chromium.org/1332583002/diff/100001/content/common/memory_...
content/common/memory_messages.h:4: 
On 2015/09/16 21:18:37, nasko (slow to review) wrote:
> Some comment describing what types of messages belong to this file.

Done.

[nasko, 2015-09-17 16:53:03]

Please add a bit to the CL description saying that the message filter approach
is taken as you anticipate more functionality to be added to it in the near
future. This will make it easier to follow for people and save time trying to
read the full design doc. Once that's done, LGTM.

[Primiano Tucci (use gerrit), 2015-09-17 18:23:50]

LGTM with some comments. Many thanks for the nice commit message (I especially
appreciate the fact that is wrapped @ 72 cols)

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

https://codereview.chromium.org/1332583002/diff/140001/content/browser/memory...
content/browser/memory/memory_pressure_controller.cc:20:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
You can use just DCHECK_CURRENTLY_ON macro

https://codereview.chromium.org/1332583002/diff/140001/content/browser/memory...
content/browser/memory/memory_pressure_controller.cc:21:
DCHECK(memory_message_filters_.find(filter) == memory_message_filters_.end());
You could make this a bit faster on debug builds by avoiding this find() here
and looking at the bool in the return value of insert below

https://codereview.chromium.org/1332583002/diff/140001/content/browser/memory...
content/browser/memory/memory_pressure_controller.cc:34:
DCHECK(memory_message_filters_.find(filter) != memory_message_filters_.end());
just use count() here, should be slighly faster

https://codereview.chromium.org/1332583002/diff/140001/content/child/memory/c...
File content/child/memory/child_memory_message_filter_unittest.cc (right):

https://codereview.chromium.org/1332583002/diff/140001/content/child/memory/c...
content/child/memory/child_memory_message_filter_unittest.cc:5: #include
"content/child/memory/child_memory_message_filter.h"
To be honest the child side is so naive (receive message -> call method) that
I'm not sure this test is really worth it.
There is a higher chance the test can get broken than the production code
itself, given that the size of the test code is > code itself.

[petrcermak, 2015-09-18 10:19:11]

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

[petrcermak, 2015-09-18 10:19:12]

Thanks for all your comments.

chrisha: Hi, could you please have a quick look at this? I just want to make
sure it's consistent with your memory pressure handling refactoring.

Petr

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

https://codereview.chromium.org/1332583002/diff/140001/content/browser/memory...
content/browser/memory/memory_pressure_controller.cc:20:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
On 2015/09/17 18:23:49, Primiano Tucci wrote:
> You can use just DCHECK_CURRENTLY_ON macro

Done.

https://codereview.chromium.org/1332583002/diff/140001/content/browser/memory...
content/browser/memory/memory_pressure_controller.cc:21:
DCHECK(memory_message_filters_.find(filter) == memory_message_filters_.end());
On 2015/09/17 18:23:49, Primiano Tucci wrote:
> You could make this a bit faster on debug builds by avoiding this find() here
> and looking at the bool in the return value of insert below

Done.

https://codereview.chromium.org/1332583002/diff/140001/content/browser/memory...
content/browser/memory/memory_pressure_controller.cc:34:
DCHECK(memory_message_filters_.find(filter) != memory_message_filters_.end());
On 2015/09/17 18:23:49, Primiano Tucci wrote:
> just use count() here, should be slighly faster

I changed this to use the return value of std::set::erase (like you suggested in
OnMemoryMessageFilterAdded).

https://codereview.chromium.org/1332583002/diff/140001/content/child/memory/c...
File content/child/memory/child_memory_message_filter_unittest.cc (right):

https://codereview.chromium.org/1332583002/diff/140001/content/child/memory/c...
content/child/memory/child_memory_message_filter_unittest.cc:5: #include
"content/child/memory/child_memory_message_filter.h"
On 2015/09/17 18:23:49, Primiano Tucci wrote:
> To be honest the child side is so naive (receive message -> call method) that
> I'm not sure this test is really worth it.
> There is a higher chance the test can get broken than the production code
> itself, given that the size of the test code is > code itself.

I thought a lot about this and I guess it's fine to remove the test now (I can
always add it back later if the behavior of ChildMemoryMessageFilter becomes
non-trivial).

[petrcermak, 2015-09-18 15:45:05]

+cc rmcilroy

[chrisha, 2015-09-21 15:26:16]

> chrisha: Hi, could you please have a quick look at this? I just want to make
> sure it's consistent with your memory pressure handling refactoring.

The overall approach and design look sane to me. For now, I'm only refactoring
the *source* of the memory pressure signals (MemoryPressureMonitor), so there is
no conflict here (these will continue to dispatch messages via
MemoryPressureListener). However, as we discussed elsewhere, some code directly
calls MemoryPressureMonitor::GetCurrentPressureLevel (and more is going to), so
I think eventually we'll want to move the notification suppression / fake
notifications directly there. I can add that functionality in the refactor, and
we can plumb things together afterwards to meet in the middle?

The other option is to add a GetCurrentPressureLevel to MemoryPressureListener,
and make clients call that directly, keeping filtering logic in the Listener?

[petrcermak, 2015-09-22 08:56:47]

On 2015/09/21 15:26:16, chrisha wrote:
> > chrisha: Hi, could you please have a quick look at this? I just want to make
> > sure it's consistent with your memory pressure handling refactoring.
> 
> The overall approach and design look sane to me. For now, I'm only refactoring
> the *source* of the memory pressure signals (MemoryPressureMonitor), so there
is
> no conflict here (these will continue to dispatch messages via
> MemoryPressureListener). However, as we discussed elsewhere, some code
directly
> calls MemoryPressureMonitor::GetCurrentPressureLevel (and more is going to),
so
> I think eventually we'll want to move the notification suppression / fake
> notifications directly there. I can add that functionality in the refactor,
and
> we can plumb things together afterwards to meet in the middle?
> 
> The other option is to add a GetCurrentPressureLevel to
MemoryPressureListener,
> and make clients call that directly, keeping filtering logic in the Listener?

I think that it would be better to keep all the dispatching and filtering logic
in a single place. Maybe we could reuse the MemoryPressureController added in
this step for this (i.e. route all memory pressure notifications through it).

Thanks,
Petr

[petrcermak, 2015-09-22 09:12:41]

The CQ bit was checked by petrcermak@chromium.org

[petrcermak, 2015-09-22 09:12:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1332583002/#ps180001
(title: "Final rebase")

[commit-bot: I haz the power, 2015-09-22 09:12:53]

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

[commit-bot: I haz the power, 2015-09-22 09:40:24]

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

[commit-bot: I haz the power, 2015-09-22 09:40:25]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[petrcermak, 2015-09-22 13:37:42]

The CQ bit was checked by petrcermak@chromium.org

[petrcermak, 2015-09-22 13:37:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1332583002/#ps200001
(title: "Add CONTENT_EXPORT to be able to link tests in component builds")

[commit-bot: I haz the power, 2015-09-22 13:38:09]

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

[commit-bot: I haz the power, 2015-09-22 14:26:33]

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

[commit-bot: I haz the power, 2015-09-22 14:26:34]

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

[petrcermak, 2015-09-22 14:46:18]

The CQ bit was checked by petrcermak@chromium.org

[petrcermak, 2015-09-22 14:46:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1332583002/#ps240001
(title: "Add comment explaining the use of scoped_ptr in the browser test
matcher")

[commit-bot: I haz the power, 2015-09-22 14:46:42]

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

[commit-bot: I haz the power, 2015-09-22 16:25:54]

Committed patchset #11 (id:240001)

[commit-bot: I haz the power, 2015-09-22 16:26:38]

Patchset 11 (id:??) landed as
https://crrev.com/0b119f3392d6c6169bbb792347a04f34ce649156
Cr-Commit-Position: refs/heads/master@{#350169}

[huangs, 2015-09-22 16:53:59]

A revert of this CL (patchset #11 id:240001) has been created in
https://codereview.chromium.org/1359873002/ by huangs@chromium.org.

The reason for reverting is: This seems to cause failure in Cast Linux:
content_browsertests:
MemoryPressureControllerBrowserTest.SetPressureNotificationsSuppressedInAllProcesses.

[Primiano Tucci (use gerrit), 2015-09-22 16:58:43]

On 2015/09/22 16:53:59, huangs wrote:
> A revert of this CL (patchset #11 id:240001) has been created in
> https://codereview.chromium.org/1359873002/ by mailto:huangs@chromium.org.
> 
> The reason for reverting is: This seems to cause failure in Cast Linux:
> content_browsertests:
>
MemoryPressureControllerBrowserTest.SetPressureNotificationsSuppressedInAllProcesses.

Link to the failing bot? Isn't there coverage on the CQ for Cast?