Apply artificial memory pressure when backgrounded.

Apply artificial memory pressure when backgrounded.

In https://codereview.chromium.org/17106004/, I am adding discardable memory
emulation for platforms that do not support it natively. The emulation works by
listening for memory pressure notifications and purging memory in response.
But we don't yet provide memory pressure notifications on any platform other
than android.

In future, we'll periodically poll system memory usage, and send memory pressure
notifications as appropriate, but as a first step, we will artificially apply
memory pressure when tabs are backgrounded.

BUG=237681
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=207947

[Ian Vollick, 2013-06-20 14:38:38]

This is the non-base bit. PTAL.

[nduca, 2013-06-20 19:56:45]

https://codereview.chromium.org/17355004/diff/1/content/renderer/render_threa...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/17355004/diff/1/content/renderer/render_threa...
content/renderer/render_thread_impl.cc:602: #if
!defined(DISCARDABLE_MEMORY_SUPPORTED_NATIVELY)
i think we should always call the manager. there may be other systems that the
manager kicks.

[nduca, 2013-06-20 19:56:50]

lgtm otherwise

[Ian Vollick, 2013-06-20 20:18:38]

+avi for OWNERS
+shatch for FYI

On 2013/06/20 19:56:45, nduca wrote:
>
https://codereview.chromium.org/17355004/diff/1/content/renderer/render_threa...
> File content/renderer/render_thread_impl.cc (right):
> 
>
https://codereview.chromium.org/17355004/diff/1/content/renderer/render_threa...
> content/renderer/render_thread_impl.cc:602: #if
> !defined(DISCARDABLE_MEMORY_SUPPORTED_NATIVELY)
> i think we should always call the manager. there may be other systems that the
> manager kicks.

SG, done.

[Avi (use Gerrit), 2013-06-20 20:34:27]

https://codereview.chromium.org/17355004/diff/6001/content/renderer/render_pr...
File content/renderer/render_process_visibility_manager.cc (right):

https://codereview.chromium.org/17355004/diff/6001/content/renderer/render_pr...
content/renderer/render_process_visibility_manager.cc:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
New files don't get "(c)", use current year.

https://codereview.chromium.org/17355004/diff/6001/content/renderer/render_pr...
content/renderer/render_process_visibility_manager.cc:30:
base::MemoryPressureListener::MEMORY_PRESSURE_CRITICAL);
Wow, that's heavy-handed.

https://codereview.chromium.org/17355004/diff/6001/content/renderer/render_pr...
File content/renderer/render_process_visibility_manager.h (right):

https://codereview.chromium.org/17355004/diff/6001/content/renderer/render_pr...
content/renderer/render_process_visibility_manager.h:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
New files don't get "(c)", use current year.

https://codereview.chromium.org/17355004/diff/6001/content/renderer/render_th...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/17355004/diff/6001/content/renderer/render_th...
content/renderer/render_thread_impl.cc:620:
manager->WidgetVisibilityChanged(true);
We're doing this work even when we don't need to emulate? :(

https://codereview.chromium.org/17355004/diff/6001/content/renderer/render_th...
File content/renderer/render_thread_impl.h (right):

https://codereview.chromium.org/17355004/diff/6001/content/renderer/render_th...
content/renderer/render_thread_impl.h:87: class
RenderProcessDiscardableMemoryProvider;
Why?

[Ian Vollick, 2013-06-20 20:49:57]

On 2013/06/20 20:34:27, Avi wrote:
>
https://codereview.chromium.org/17355004/diff/6001/content/renderer/render_pr...
> File content/renderer/render_process_visibility_manager.cc (right):
> 
>
https://codereview.chromium.org/17355004/diff/6001/content/renderer/render_pr...
> content/renderer/render_process_visibility_manager.cc:1: // Copyright (c) 2012
> The Chromium Authors. All rights reserved.
> New files don't get "(c)", use current year.
Done. I'm sorry, you mentioned this on the other CL and I should have caught
this.
> 
>
https://codereview.chromium.org/17355004/diff/6001/content/renderer/render_pr...
> content/renderer/render_process_visibility_manager.cc:30:
> base::MemoryPressureListener::MEMORY_PRESSURE_CRITICAL);
> Wow, that's heavy-handed.
Yeah, it is. It's a little naive, too. Once we have the code in place to
periodically poll the real system memory pressure, this could be eliminated.
I've added a FIXME.
> 
>
https://codereview.chromium.org/17355004/diff/6001/content/renderer/render_pr...
> File content/renderer/render_process_visibility_manager.h (right):
> 
>
https://codereview.chromium.org/17355004/diff/6001/content/renderer/render_pr...
> content/renderer/render_process_visibility_manager.h:1: // Copyright (c) 2012
> The Chromium Authors. All rights reserved.
> New files don't get "(c)", use current year.
Done.
> 
>
https://codereview.chromium.org/17355004/diff/6001/content/renderer/render_th...
> File content/renderer/render_thread_impl.cc (right):
> 
>
https://codereview.chromium.org/17355004/diff/6001/content/renderer/render_th...
> content/renderer/render_thread_impl.cc:620:
> manager->WidgetVisibilityChanged(true);
> We're doing this work even when we don't need to emulate? :(
Yes, but it does nothing in this case.
> 
>
https://codereview.chromium.org/17355004/diff/6001/content/renderer/render_th...
> File content/renderer/render_thread_impl.h (right):
> 
>
https://codereview.chromium.org/17355004/diff/6001/content/renderer/render_th...
> content/renderer/render_thread_impl.h:87: class
> RenderProcessDiscardableMemoryProvider;
> Why?
Should be here. Removed.

[Ian Vollick, 2013-06-20 20:51:17]

> We're doing this work even when we don't need to emulate? :(

Also, as Nat was saying, in the future there may be other systems the manager
will kick.

[Avi (use Gerrit), 2013-06-20 20:54:59]

On 2013/06/20 20:51:17, vollick wrote:
> > We're doing this work even when we don't need to emulate? :(
> 
> Also, as Nat was saying, in the future there may be other systems the manager
> will kick.

Right. Just feeling weird that we're adding overhead that we're not using now,
rather than #ifdefing where the root of the useless work is.

[Ian Vollick, 2013-06-21 13:36:20]

On 2013/06/20 20:54:59, Avi wrote:
> On 2013/06/20 20:51:17, vollick wrote:
> > > We're doing this work even when we don't need to emulate? :(
> > 
> > Also, as Nat was saying, in the future there may be other systems the
manager
> > will kick.
> 
> Right. Just feeling weird that we're adding overhead that we're not using now,
> rather than #ifdefing where the root of the useless work is.

Put back the #ifdef's.

[Avi (use Gerrit), 2013-06-21 14:38:08]

In any case, you have an owners LG from me when you get your expert's OK.

[nduca, 2013-06-21 17:34:16]

lgtm

[nduca, 2013-06-21 17:35:17]

oh wait, no Avi you're being sillly. Make the call. This is not a critical path
function. Avoiding overhead is just pointless.

[nduca, 2013-06-21 17:37:28]

And in case you want some context: when a RendeerWidget is restored, the first
thing we do is repaint it entirely. Thats 10+ milliseconds worth of work that
dangles right off this one call. A single call to a singleton is hardly worth
it.

And imagine when we try to use the visibility manager for something else (say
reducing the process priority when backgrounded, needed for power conservation?)
Then we're going to find it suddenly doesn't work on some platforms.

Visibility manager is not a discardable system. Its just a visibility manager.
It should always be on. If the calls into discardable should be made optional
inside its implementation, that is up to you and Ian. Again, I think its not
worth it.

[Avi (use Gerrit), 2013-06-21 17:46:54]

On 2013/06/21 17:37:28, nduca wrote:
> And in case you want some context: when a RendeerWidget is restored, the first
> thing we do is repaint it entirely. Thats 10+ milliseconds worth of work that
> dangles right off this one call. A single call to a singleton is hardly worth
> it.
> 
> And imagine when we try to use the visibility manager for something else (say
> reducing the process priority when backgrounded, needed for power
conservation?)
> Then we're going to find it suddenly doesn't work on some platforms.
> 
> Visibility manager is not a discardable system. Its just a visibility manager.
> It should always be on. If the calls into discardable should be made optional
> inside its implementation, that is up to you and Ian. Again, I think its not
> worth it.

OK, I see your point. Never mind me.

And LGTM.

[commit-bot: I haz the power, 2013-06-21 17:53:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vollick@chromium.org/17355004/32001

[commit-bot: I haz the power, 2013-06-21 22:57:06]

Change committed as 207947