[Android WebView] OnMemoryPressure to drop tile memory

[Android WebView] OnMemoryPressure to drop tile memory

In OnMemoryPressure, drop tiles on invisible browser views.

Not reusing base::MemoryPressureListener because Android
WebView requires onTrimMemory to be called synchronously.

BUG=
NOTRY=true

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

[joth, 2013-10-01 11:10:46]

https://codereview.chromium.org/25082006/diff/1/android_webview/browser/gl_vi...
File android_webview/browser/gl_view_renderer_manager.cc (right):

https://codereview.chromium.org/25082006/diff/1/android_webview/browser/gl_vi...
android_webview/browser/gl_view_renderer_manager.cc:14:
memory_pressure_listener_.reset(new base::MemoryPressureListener(base::Bind(
the thread you construct this on is the thread you'll get callbacks on. Which I
think you need to be the UI thread.

is it guaranteed where  GLViewRendererManager is constructed? DCHECK? (see also
my comment about doing this direct in the view renderer though)

https://codereview.chromium.org/25082006/diff/1/android_webview/browser/gl_vi...
android_webview/browser/gl_view_renderer_manager.cc:15:
&GLViewRendererManager::OnMemoryPressure, base::Unretained(this))));
generally folks break before the bind() call --

   bind(foo,
        bar)

https://codereview.chromium.org/25082006/diff/1/android_webview/browser/gl_vi...
android_webview/browser/gl_view_renderer_manager.cc:55:
(*itr)->OnMemoryPressure(memory_pressure_level);
plumbing this via GLViewRendererManager doesn't seem to add anything. To you
plan to add resource arbitration policy here in future? (comment?) else we may
as well have each view renderer register direct with the MemoryPressureListener

https://codereview.chromium.org/25082006/diff/1/android_webview/browser/gl_vi...
File android_webview/browser/gl_view_renderer_manager.h (right):

https://codereview.chromium.org/25082006/diff/1/android_webview/browser/gl_vi...
android_webview/browser/gl_view_renderer_manager.h:11: #include
"base/memory/memory_pressure_listener.h"
you probably could fwd declare MemoryPressureListener but this is ok too

https://codereview.chromium.org/25082006/diff/1/android_webview/browser/in_pr...
File android_webview/browser/in_process_view_renderer.cc (right):

https://codereview.chromium.org/25082006/diff/1/android_webview/browser/in_pr...
android_webview/browser/in_process_view_renderer.cc:333: ScopedAllowGL allow_gl;
GL not valid if this is a 'fake' memory pressure?

Should we use the new "process with GL valid" Runnable thing ?

https://codereview.chromium.org/25082006/diff/1/android_webview/browser/in_pr...
android_webview/browser/in_process_view_renderer.cc:338:
compositor_->SetMemoryPolicy(policy);
comment this will get set back upward on the next draw?

[boliu, 2013-10-01 19:21:56]

Changed to one listener per ViewRenderer. The patch started out as some "only
drop tiles on views not recently drawn", but didn't end up that way.

I think I addressed all the comments, but still pending testing to see if this
actuall does anything.

https://codereview.chromium.org/25082006/diff/1/android_webview/browser/gl_vi...
File android_webview/browser/gl_view_renderer_manager.h (right):

https://codereview.chromium.org/25082006/diff/1/android_webview/browser/gl_vi...
android_webview/browser/gl_view_renderer_manager.h:11: #include
"base/memory/memory_pressure_listener.h"
On 2013/10/01 11:10:46, joth wrote:
> you probably could fwd declare MemoryPressureListener but this is ok too

Can't forward declare base::MemoryPressureListener::MemoryPressureLevel, it's an
inner enum.

[boliu, 2013-10-01 20:10:48]

https://codereview.chromium.org/25082006/diff/8001/android_webview/browser/in...
File android_webview/browser/in_process_view_renderer.cc (right):

https://codereview.chromium.org/25082006/diff/8001/android_webview/browser/in...
android_webview/browser/in_process_view_renderer.cc:335: if
(eglGetCurrentContext() != last_egl_context_)
This check is failing, so as is, this patch does nothing. Looking at why.

[boliu, 2013-10-01 20:42:00]

On 2013/10/01 20:10:48, boliu wrote:
>
https://codereview.chromium.org/25082006/diff/8001/android_webview/browser/in...
> File android_webview/browser/in_process_view_renderer.cc (right):
> 
>
https://codereview.chromium.org/25082006/diff/8001/android_webview/browser/in...
> android_webview/browser/in_process_view_renderer.cc:335: if
> (eglGetCurrentContext() != last_egl_context_)
> This check is failing, so as is, this patch does nothing. Looking at why.

Hmm, can it be? Base documentation is wrong. These two comments directly
conflicts with each other

https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/memory...
https://code.google.com/p/chromium/codesearch#chromium/src/base/observer_list...

I guess for this patch, we'll need to call invoke mode.

[joth, 2013-10-02 00:19:22]

On 1 October 2013 21:42, <boliu@chromium.org> wrote:

> On 2013/10/01 20:10:48, boliu wrote:
>
> https://codereview.chromium.**org/25082006/diff/8001/**
>
android_webview/browser/in_**process_view_renderer.cc<https://codereview.chromium.org/25082006/diff/8001/android_webview/browser/in_process_view_renderer.cc>
>
>> File android_webview/browser/in_**process_view_renderer.cc (right):
>>
>
>
> https://codereview.chromium.**org/25082006/diff/8001/**
>
android_webview/browser/in_**process_view_renderer.cc#**newcode335<https://codereview.chromium.org/25082006/diff/8001/android_webview/browser/in_process_view_renderer.cc#newcode335>
>
>> android_webview/browser/in_**process_view_renderer.cc:335: if
>> (eglGetCurrentContext() != last_egl_context_)
>> This check is failing, so as is, this patch does nothing. Looking at why.
>>
>
> Hmm, can it be? Base documentation is wrong. These two comments directly
> conflicts with each other
>
> https://code.google.com/p/**chromium/codesearch#chromium/**
>
src/base/memory/memory_**pressure_listener.h&l=27<https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/memory_pressure_listener.h&l=27>
> https://code.google.com/p/**chromium/codesearch#chromium/**
>
src/base/observer_list_**threadsafe.h&l=41<https://code.google.com/p/chromium/codesearch#chromium/src/base/observer_list_threadsafe.h&l=41>


They don't necessarily conflict: the latter is ambiguous

Notifications will always be done via PostTask() to another thread,

--> doesn't say what it will be done via to "this" thread (c.f. another
(any other?) thread)



>
>
> I guess for this patch, we'll need to call invoke mode.
>
> OK I missed the reasons.. - executeHardwareAction() can't be used for this?



>
https://codereview.chromium.**org/25082006/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[boliu, 2013-10-02 02:14:04]

So PS3 "works" as in I verified it actually does something.

About observer_list_threadsafe.h, if you look at the implementation, it calls
PostTask regardless of thread:
https://code.google.com/p/chromium/codesearch#chromium/src/base/observer_list...

Can't use gl process mode either since once the app is in the background,
requesting process mode are ignored as well. So instead I hooked up our own
callback.

The level is ignored for now, maybe it warrants some tweaking.

https://codereview.chromium.org/25082006/diff/16001/android_webview/browser/i...
File android_webview/browser/in_process_view_renderer.cc (right):

https://codereview.chromium.org/25082006/diff/16001/android_webview/browser/i...
android_webview/browser/in_process_view_renderer.cc:347: ForceCompositeSW();
Mad hacks, but don't really have a better idea for now.

We can add checks that this isn't repeated for no reason.

[joth, 2013-10-02 10:47:01]

https://codereview.chromium.org/25082006/diff/16001/android_webview/browser/i...
File android_webview/browser/in_process_view_renderer.cc (right):

https://codereview.chromium.org/25082006/diff/16001/android_webview/browser/i...
android_webview/browser/in_process_view_renderer.cc:408:
TRACE_EVENT0("android_webview", "EarlyOut_ModeProcess");
intended edit?

https://codereview.chromium.org/25082006/diff/16001/android_webview/browser/i...
android_webview/browser/in_process_view_renderer.cc:907: void
InProcessViewRenderer::ForceCompositeSW() {
maybe ForceFakeCompostieSW() will make it clear (at callsite) that this isn't
doing anything useful

https://codereview.chromium.org/25082006/diff/16001/android_webview/browser/s...
File android_webview/browser/scoped_app_gl_state_restore.h (right):

https://codereview.chromium.org/25082006/diff/16001/android_webview/browser/s...
android_webview/browser/scoped_app_gl_state_restore.h:22: MODE_ON_TRIM
nit: include the trailing comma on last item

as detach and trim are the same, should they be combined into a single "resource
management" enum? (I think it maybe better to have the enums reflect classes of
use rather than the exact function names it is used from)

https://codereview.chromium.org/25082006/diff/16001/android_webview/java/src/...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/25082006/diff/16001/android_webview/java/src/...
android_webview/java/src/org/chromium/android_webview/AwContents.java:529:
containerView.getContext().registerComponentCallbacks(
could you move this up to line 520? onVisibilityChanged() onwards is potentially
moving to a new method.

declaring the innerclass out of line would keep this c'tor a bit more readable.
(see IoThreadClientImpl etc. maybe group with this on those lines at top of
method)

https://codereview.chromium.org/25082006/diff/21001/android_webview/browser/i...
File android_webview/browser/in_process_view_renderer.cc (right):

https://codereview.chromium.org/25082006/diff/21001/android_webview/browser/i...
android_webview/browser/in_process_view_renderer.cc:319: void
InProcessViewRenderer::TrimMemory(int level) {
not using level param? 

ideas:

if level < 10 return

if level > 40 skip over the visibility tests too (line 326)

https://codereview.chromium.org/25082006/diff/21001/android_webview/browser/i...
android_webview/browser/in_process_view_renderer.cc:378: }
intended change?

[boliu, 2013-10-02 14:09:54]

Still haven't made the level change since that needs testing.

https://codereview.chromium.org/25082006/diff/16001/android_webview/browser/i...
File android_webview/browser/in_process_view_renderer.cc (right):

https://codereview.chromium.org/25082006/diff/16001/android_webview/browser/i...
android_webview/browser/in_process_view_renderer.cc:408:
TRACE_EVENT0("android_webview", "EarlyOut_ModeProcess");
On 2013/10/02 10:47:01, joth wrote:
> intended edit?

reverted

https://codereview.chromium.org/25082006/diff/16001/android_webview/browser/i...
android_webview/browser/in_process_view_renderer.cc:907: void
InProcessViewRenderer::ForceCompositeSW() {
On 2013/10/02 10:47:01, joth wrote:
> maybe ForceFakeCompostieSW() will make it clear (at callsite) that this isn't
> doing anything useful

Done.

https://codereview.chromium.org/25082006/diff/16001/android_webview/browser/s...
File android_webview/browser/scoped_app_gl_state_restore.h (right):

https://codereview.chromium.org/25082006/diff/16001/android_webview/browser/s...
android_webview/browser/scoped_app_gl_state_restore.h:22: MODE_ON_TRIM
On 2013/10/02 10:47:01, joth wrote:
> nit: include the trailing comma on last item
> 
> as detach and trim are the same, should they be combined into a single
"resource
> management" enum? (I think it maybe better to have the enums reflect classes
of
> use rather than the exact function names it is used from)

both done

https://codereview.chromium.org/25082006/diff/16001/android_webview/java/src/...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/25082006/diff/16001/android_webview/java/src/...
android_webview/java/src/org/chromium/android_webview/AwContents.java:529:
containerView.getContext().registerComponentCallbacks(
On 2013/10/02 10:47:01, joth wrote:
> could you move this up to line 520? onVisibilityChanged() onwards is
potentially
> moving to a new method.
> 

This moved to attach/detach since we need to explicitly call unregister as well.

> declaring the innerclass out of line would keep this c'tor a bit more
readable.
> (see IoThreadClientImpl etc. maybe group with this on those lines at top of
> method)

Done

https://codereview.chromium.org/25082006/diff/21001/android_webview/browser/i...
File android_webview/browser/in_process_view_renderer.cc (right):

https://codereview.chromium.org/25082006/diff/21001/android_webview/browser/i...
android_webview/browser/in_process_view_renderer.cc:319: void
InProcessViewRenderer::TrimMemory(int level) {
On 2013/10/02 10:47:01, joth wrote:
> not using level param? 
> 
> ideas:
> 
> if level < 10 return
> 
> if level > 40 skip over the visibility tests too (line 326) 

Not done yet. I'll have to play with this when I get back to my desk.

https://codereview.chromium.org/25082006/diff/21001/android_webview/browser/i...
android_webview/browser/in_process_view_renderer.cc:378: }
On 2013/10/02 10:47:01, joth wrote:
> intended change?

Reverted.

[boliu, 2013-10-02 17:53:51]

Ready for full review.

Took all your suggestions about level plus one more: ignore
TRIM_MEMORY_UI_HIDDEN, since it does not indicate memory pressure, just merely
the app has been put into the background, which is way too aggressive since
dropping all tiles will slow down bringing the app back up again.

[joth, 2013-10-02 18:13:56]

lgtm

[boliu, 2013-10-02 18:24:32]

+ted for content/public/browser/android

[Ted C, 2013-10-02 22:54:39]

lgtm - content/public/browser/android

https://codereview.chromium.org/25082006/diff/32001/content/public/browser/an...
File content/public/browser/android/synchronous_compositor.cc (right):

https://codereview.chromium.org/25082006/diff/32001/content/public/browser/an...
content/public/browser/android/synchronous_compositor.cc:15: num_resources_limit
== other.num_resources_limit;
should only be indented 4

[boliu, 2013-10-02 23:04:42]

https://codereview.chromium.org/25082006/diff/32001/content/public/browser/an...
File content/public/browser/android/synchronous_compositor.cc (right):

https://codereview.chromium.org/25082006/diff/32001/content/public/browser/an...
content/public/browser/android/synchronous_compositor.cc:15: num_resources_limit
== other.num_resources_limit;
On 2013/10/02 22:54:39, Ted C wrote:
> should only be indented 4

But..but..clang-format did this...ok done.

:)

[commit-bot: I haz the power, 2013-10-03 00:19:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/boliu@chromium.org/25082006/42001

[commit-bot: I haz the power, 2013-10-03 01:08:17]

Change committed as 226643