713032 - Implement ICS Activity.onTrimMemory

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Android, defect)

Description

[Benoit Girard (:BenWa)]

We should implement onTrimMemory to dispatch proper memory pressure events.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Attachment: MemoryMonitor

Add a MemoryMonitor class that's gonna be the brains of the operation.

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 662596 [details] [diff] [review]
MemoryMonitor

Review of attachment 662596 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good overall

::: mobile/android/base/MemoryMonitor.java
@@ +97,5 @@
> +        }
> +    }
> +
> +    @Override
> +    public void onReceive(Context context, Intent intent) {

Does this actually get used? Would be better to receive the intent in GeckoApp or whatever anyway, no? Also it seems out of scope for a class named MemoryMonitor.

@@ +169,5 @@
> +            GeckoAppShell.getHandler().postDelayed(this, DECREMENT_DELAY);
> +            mPosted = true;
> +        }
> +
> +        synchronized void skipNextDecrement() {

Probably better to just change this to restart() or something, which would be something like:

cancel() {
    GeckoAppShell.getHandler().removeCallbacks(this);
    mPosted = false;
}

restart() {
    cancel();
    start();
}

Comment 3

[Kartikaya Gupta (email:[e-mail])]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #2)
> > +    @Override
> > +    public void onReceive(Context context, Intent intent) {
> 
> Does this actually get used? Would be better to receive the intent in
> GeckoApp or whatever anyway, no? Also it seems out of scope for a class
> named MemoryMonitor.
> 

It does get triggered, if that's what you're asking. I spent a while dd'ing from /dev/zero to fill up my /data partition to verify that the code was getting hit (it got run when the free space was ~200 MB free, on a disk that df claims is ~13GB). I don't think it's a good idea to receive the intent in GeckoApp - GeckoApp can go away while the process is still running and so we should be listening for it on an application context. See bug 761706 for more details. I agree that perhaps MemoryMonitor would be better named MemoryAndDiskMonitor or something but I figured MemoryMonitor was more concise and accurate enough. :) I can change it to ResourceMonitor though if you prefer.

> @@ +169,5 @@
> > +        synchronized void skipNextDecrement() {
> 
> Probably better to just change this to restart() or something, which would
> be something like:
> 
> cancel() {
>     GeckoAppShell.getHandler().removeCallbacks(this);
>     mPosted = false;
> }
> 
> restart() {
>     cancel();
>     start();
> }

Ok, will do.

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Kartikaya Gupta (:kats) from comment #3)
> (In reply to James Willcox (:snorp) ([e-mail]) from comment #2)
> > > +    @Override
> > > +    public void onReceive(Context context, Intent intent) {
> > 
> > Does this actually get used? Would be better to receive the intent in
> > GeckoApp or whatever anyway, no? Also it seems out of scope for a class
> > named MemoryMonitor.
> > 
> 
> It does get triggered, if that's what you're asking. I spent a while dd'ing
> from /dev/zero to fill up my /data partition to verify that the code was
> getting hit (it got run when the free space was ~200 MB free, on a disk that
> df claims is ~13GB). I don't think it's a good idea to receive the intent in
> GeckoApp - GeckoApp can go away while the process is still running and so we
> should be listening for it on an application context. See bug 761706 for
> more details. I agree that perhaps MemoryMonitor would be better named
> MemoryAndDiskMonitor or something but I figured MemoryMonitor was more
> concise and accurate enough. :) I can change it to ResourceMonitor though if
> you prefer.

Ah, I missed the registerReceiver() call. Don't we also need to unregister it at some point? Anyway, just nitpicking about the class name; MemoryMonitor seems fine.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #4)
> Don't we also need to unregister
> it at some point? 

Ideally on app shutdown, but Android will clean it up for us anyway; GeckoBatteryManager does the same thing. It's only the activity-level unregistered receivers that it complains about.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Attachment: MemoryMonitor (v2)

I just changed the semantics of start() to cancel the old one if there was one going, and verified I didn't break anything.

Comment 7

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 662650 [details] [diff] [review]
MemoryMonitor (v2)

Great

Comment 8

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fdc9029194ed

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/fdc9029194ed