Migrate ChromeOS to base::MemoryPressureMonitor

Migrate ChromeOS to base::MemoryPressureMonitor

BUG=463546
Committed: https://crrev.com/6d5c49f04dc84c1a8472fe3e2357dda3d80ab6f9
Cr-Commit-Position: refs/heads/master@{#325253}

[dmichael (off chromium), 2015-03-31 17:00:31]

Patchset #5 (id:80001) has been deleted

[dmichael (off chromium), 2015-03-31 17:00:35]

Patchset #4 (id:60001) has been deleted

[dmichael (off chromium), 2015-03-31 17:36:05]

dmichael@chromium.org changed reviewers:
+ chrisha@chromium.org, skuhne@chromium.org

[chrisha, 2015-03-31 17:53:14]

There's a lot going on in this CL that could maybe have been broken down into
smaller CLs for ease of review?
(ie: defining the new interface, moving thresholds to switches, porting the
existing observer to the new monitor interface, moving creation logic from
browser_main to browser_process_platform_part, etc).

The new interface stuff looks fine, but there's a lot of moving things around in
ChromeOS that I can't comment on. I'll leave that to Stefan.

[Mr4D (OOO till 08-26), 2015-03-31 21:12:17]

Hmmm. Looking at this I have a few questions (as pointed out in my comments):

  ChromeOS has several products which either use only apps / only content / .. -
as such the instantiation moved in the end into content (even though that would
have not caught the "Chrome only" case. => As such the availability as well as
the instantiation were problematical.

  => I really like the idea to instantiate the Monitor with the first listener
creation since no matter which version you look at - it will always work.
Furthermore it also addresses the task problem since each task would create the
Monitor and would work independently.

  => As such I think that "base" is the best location for this, since that is
available to every component.

I am fully open to discussions - but I would like to understand why we want to
do it this way, since it will require special handling for special cases. :)

https://codereview.chromium.org/1045433002/diff/120001/chrome/browser/browser...
File chrome/browser/browser_process_platform_part_chromeos.cc (right):

https://codereview.chromium.org/1045433002/diff/120001/chrome/browser/browser...
chrome/browser/browser_process_platform_part_chromeos.cc:100: if
(!oom_priority_manager_.get()) {
I suppose this is temporary?

Also - I am not sure that this will work. I remember vaguely that I moved it to
content in order to have it accessible for various target situations.
+ there were several efforts going on to separate content from chrome and vice
versa (e.g. for AppShell, etc.). This was the reason for moving the observer
into base, since that is available for all.

Looking into processes it would be even more beneficial to have them in base.

https://codereview.chromium.org/1045433002/diff/120001/chrome/browser/browser...
chrome/browser/browser_process_platform_part_chromeos.cc:101: if
(chromeos::switches::MemoryPressureHandlingEnabled()) {
What was the reason for not instantiating it with the first
MemoryPressureListener creation?

[dmichael (off chromium), 2015-03-31 21:14:32]

On 2015/03/31 17:53:14, chrisha wrote:
> There's a lot going on in this CL that could maybe have been broken down into
> smaller CLs for ease of review?
> (ie: defining the new interface,
OK, split to https://codereview.chromium.org/1048983006/

My thinking was to put it all together to (a) help demonstrate the motivation
and (b) make sure the design actually works. But it's enough to have this CL
with everything for reference.

> moving thresholds to switches, porting the
> existing observer to the new monitor interface, moving creation logic from
> browser_main to browser_process_platform_part, etc).
That stuff may be in one CL; a lot of it is just shoveling code around to keep
the dependency graph sane.

> 
> The new interface stuff looks fine, but there's a lot of moving things around
in
> ChromeOS that I can't comment on. I'll leave that to Stefan.

[dmichael (off chromium), 2015-04-08 21:18:04]

OK, I think this is ready for review now. The base stuff is landed, so now this
is mostly just:
1) A name change
2) A slight simplification now that we can use
base::MemoryPressureMonitor::Get()

[chrisha, 2015-04-08 21:38:16]

Looking good! Especially like the fact that we get rid of the various pieces of
GetMemoryPressureObserver plumbing in browser and content.

Some comments...

https://codereview.chromium.org/1045433002/diff/210001/base/chromeos/memory_p...
File base/chromeos/memory_pressure_monitor_chromeos.h (right):

https://codereview.chromium.org/1045433002/diff/210001/base/chromeos/memory_p...
base/chromeos/memory_pressure_monitor_chromeos.h:26: class BASE_EXPORT
MemoryPressureMonitorChromeOS {
Shouldn't this inherit from base::MemoryPressure?

https://codereview.chromium.org/1045433002/diff/210001/base/chromeos/memory_p...
base/chromeos/memory_pressure_monitor_chromeos.h:59:
base::MemoryPressureListener::MemoryPressureLevel GetCurrentPressureLevel() {
And this should be virtual?

https://codereview.chromium.org/1045433002/diff/210001/content/browser/browse...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/1045433002/diff/210001/content/browser/browse...
content/browser/browser_main_loop.cc:592: if
(chromeos::switches::MemoryPressureHandlingEnabled()) {
Hmm... might need to promote this to a pan-OS switch. Another CL, I suppose...

https://codereview.chromium.org/1045433002/diff/210001/content/browser/browse...
File content/browser/browser_main_loop.h (right):

https://codereview.chromium.org/1045433002/diff/210001/content/browser/browse...
content/browser/browser_main_loop.h:182:
scoped_ptr<base::MemoryPressureMonitorChromeOS> memory_pressure_observer_;
Could get rid of the OS_CHROMEOS ifdefs and make this a
scoped_ptr<base::MemoryPressureMonitor> instance? (This could be done in a later
CL, but will be needed when we start using this on other platforms.)

Should also change the name to reflect _monitor_ and not _observer_.

[dmichael (off chromium), 2015-04-08 22:09:02]

dmichael@chromium.org changed reviewers:
+ oshima@chromium.org

[dmichael (off chromium), 2015-04-08 22:09:04]

Thanks for looking! +oshima since Mr4D is OOO.

https://codereview.chromium.org/1045433002/diff/210001/base/chromeos/memory_p...
File base/chromeos/memory_pressure_monitor_chromeos.h (right):

https://codereview.chromium.org/1045433002/diff/210001/base/chromeos/memory_p...
base/chromeos/memory_pressure_monitor_chromeos.h:26: class BASE_EXPORT
MemoryPressureMonitorChromeOS {
On 2015/04/08 21:38:16, chrisha wrote:
> Shouldn't this inherit from base::MemoryPressure?
Ugh, I had this right in an earlier patch but messed up when reshuffling to
base. Thanks.

https://codereview.chromium.org/1045433002/diff/210001/base/chromeos/memory_p...
base/chromeos/memory_pressure_monitor_chromeos.h:59:
base::MemoryPressureListener::MemoryPressureLevel GetCurrentPressureLevel() {
On 2015/04/08 21:38:16, chrisha wrote:
> And this should be virtual?
Ditto, done :)

https://codereview.chromium.org/1045433002/diff/210001/content/browser/browse...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/1045433002/diff/210001/content/browser/browse...
content/browser/browser_main_loop.cc:592: if
(chromeos::switches::MemoryPressureHandlingEnabled()) {
On 2015/04/08 21:38:16, chrisha wrote:
> Hmm... might need to promote this to a pan-OS switch. Another CL, I suppose...
Perhaps. It's just "always on" for Android I think, and of course there is no
pressure handling on Mac/Win. Stefan should know about the history of this flag
to inform whether it needs to work for other platforms.

https://codereview.chromium.org/1045433002/diff/210001/content/browser/browse...
File content/browser/browser_main_loop.h (right):

https://codereview.chromium.org/1045433002/diff/210001/content/browser/browse...
content/browser/browser_main_loop.h:182:
scoped_ptr<base::MemoryPressureMonitorChromeOS> memory_pressure_observer_;
On 2015/04/08 21:38:16, chrisha wrote:
> Could get rid of the OS_CHROMEOS ifdefs and make this a
> scoped_ptr<base::MemoryPressureMonitor> instance? (This could be done in a
later
> CL, but will be needed when we start using this on other platforms.)
Something somewhere will have to be able to choose the right concrete
MemoryPressureObserver implementation for the given process+platform (or we use
a factory like you had proposed). In any case, I think it makes sense to keep
this CL simple and ChromeOS-only.

> 
> Should also change the name to reflect _monitor_ and not _observer_.
Done, thanks.

[oshima, 2015-04-09 00:46:48]

bots are having compilation errors. Could you please look into it?

[oshima, 2015-04-09 00:46:48]

bots are having compilation errors. Could you please look into it?

[dmichael (off chromium), 2015-04-10 16:15:28]

Patchset #12 (id:250001) has been deleted

[dmichael (off chromium), 2015-04-10 16:15:37]

Patchset #12 (id:270001) has been deleted

[dmichael (off chromium), 2015-04-10 16:16:15]

Sorry about that, should be good now. Or you can punt to skuhne, who should be
back Monday.

[oshima, 2015-04-10 19:59:29]

Yeah, I'll leave this to skuhne@, just in case he has some opinion about naming,
etc.

[Mr4D (OOO till 08-26), 2015-04-13 15:10:52]

Great!

Only a few nits!

https://codereview.chromium.org/1045433002/diff/290001/chrome/browser/chromeo...
File chrome/browser/chromeos/memory/oom_priority_manager.cc (right):

https://codereview.chromium.org/1045433002/diff/290001/chrome/browser/chromeo...
chrome/browser/chromeos/memory/oom_priority_manager.cc:108:
base::MemoryPressureMonitorChromeOS* GetMemoryPressureMonitor() {
Why not using MemoryPressureMonitor as a return value here - and / or even
better eliminate this altogether and use the base::MemoryPressureMonitor::Get()
instead where this is called?

https://codereview.chromium.org/1045433002/diff/290001/content/browser/render...
File content/browser/renderer_host/renderer_frame_manager.cc (right):

https://codereview.chromium.org/1045433002/diff/290001/content/browser/render...
content/browser/renderer_host/renderer_frame_manager.cc:18: #include
"content/browser/browser_main_loop.h"
This can go. (with the OS_CHROMEOS) part.

https://codereview.chromium.org/1045433002/diff/290001/content/browser/render...
content/browser/renderer_host/renderer_frame_manager.cc:78: #if
defined(OS_CHROMEOS)
I think that you do not want here #if defined(OS_CHROMEOS) anymore since this
optimization is good for all OS'es now.

https://codereview.chromium.org/1045433002/diff/290001/content/browser/render...
content/browser/renderer_host/renderer_frame_manager.cc:80:
content::BrowserMainLoop::GetInstance();
You do not need the browser_main_loop anymore.

[dmichael (off chromium), 2015-04-13 20:20:22]

dmichael@chromium.org changed reviewers:
- oshima@chromium.org

[dmichael (off chromium), 2015-04-13 21:02:16]

Comments addressed, thanks!

https://codereview.chromium.org/1045433002/diff/290001/chrome/browser/chromeo...
File chrome/browser/chromeos/memory/oom_priority_manager.cc (right):

https://codereview.chromium.org/1045433002/diff/290001/chrome/browser/chromeo...
chrome/browser/chromeos/memory/oom_priority_manager.cc:108:
base::MemoryPressureMonitorChromeOS* GetMemoryPressureMonitor() {
On 2015/04/13 15:10:51, Mr4D wrote:
> Why not using MemoryPressureMonitor as a return value here - and / or even
> better eliminate this altogether and use the
base::MemoryPressureMonitor::Get()
> instead where this is called?

Done.

https://codereview.chromium.org/1045433002/diff/290001/content/browser/render...
File content/browser/renderer_host/renderer_frame_manager.cc (right):

https://codereview.chromium.org/1045433002/diff/290001/content/browser/render...
content/browser/renderer_host/renderer_frame_manager.cc:18: #include
"content/browser/browser_main_loop.h"
On 2015/04/13 15:10:52, Mr4D wrote:
> This can go. (with the OS_CHROMEOS) part.

Done.

https://codereview.chromium.org/1045433002/diff/290001/content/browser/render...
content/browser/renderer_host/renderer_frame_manager.cc:78: #if
defined(OS_CHROMEOS)
On 2015/04/13 15:10:52, Mr4D wrote:
> I think that you do not want here #if defined(OS_CHROMEOS) anymore since this
> optimization is good for all OS'es now.
I was aiming for minimizing the change here so we can turn on the optimization
carefully in a later CL, but I guess that will happen when a platform gets their
MemoryPressureMonitor implementation. Done.

https://codereview.chromium.org/1045433002/diff/290001/content/browser/render...
content/browser/renderer_host/renderer_frame_manager.cc:80:
content::BrowserMainLoop::GetInstance();
On 2015/04/13 15:10:52, Mr4D wrote:
> You do not need the browser_main_loop anymore.
Ah, thanks. Done.

[Mr4D (OOO till 08-26), 2015-04-13 23:54:18]

Thanks for doing this!

lgtm

[dmichael (off chromium), 2015-04-14 00:17:32]

The CQ bit was checked by dmichael@chromium.org

[commit-bot: I haz the power, 2015-04-14 00:19:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1045433002/310001

[commit-bot: I haz the power, 2015-04-14 00:51:41]

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

[commit-bot: I haz the power, 2015-04-14 00:51:42]

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

[dmichael (off chromium), 2015-04-14 15:21:12]

dmichael@chromium.org changed reviewers:
+ avi@chromium.org, thestig@chromium.org

[dmichael (off chromium), 2015-04-14 15:21:13]

+thestig for base/ OWNERS, avi for content/ OWNERS

[Avi (use Gerrit), 2015-04-14 16:18:38]

LGTM

[Lei Zhang, 2015-04-14 19:10:51]

lgtm

[dmichael (off chromium), 2015-04-14 19:36:55]

dmichael@chromium.org changed reviewers:
+ jamescook@chromium.org

[dmichael (off chromium), 2015-04-14 19:36:56]

+jamescook for chrome/browser/chromeos/memory

[James Cook, 2015-04-15 01:54:46]

c/b/chromeos/memory LGTM

[dmichael (off chromium), 2015-04-15 14:58:49]

The CQ bit was checked by dmichael@chromium.org

[commit-bot: I haz the power, 2015-04-15 14:59:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1045433002/310001

[commit-bot: I haz the power, 2015-04-15 16:07:39]

Committed patchset #13 (id:310001)

[commit-bot: I haz the power, 2015-04-15 16:08:24]

Patchset 13 (id:??) landed as
https://crrev.com/6d5c49f04dc84c1a8472fe3e2357dda3d80ab6f9
Cr-Commit-Position: refs/heads/master@{#325253}