Closed Bug 713032 Opened 8 years ago Closed 7 years ago

Implement ICS Activity.onTrimMemory

Categories

(Core :: Widget: Android, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: BenWa, Assigned: kats)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

We should implement onTrimMemory to dispatch proper memory pressure events.
Attached patch MemoryMonitor (obsolete) — Splinter Review
Add a MemoryMonitor class that's gonna be the brains of the operation.
Attachment #662596 - Flags: review?(snorp)
Assignee: nobody → bugmail.mozilla
Blocks: 789923, 792155
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();
}
Attachment #662596 - Flags: review?(snorp) → review+
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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.
(In reply to Kartikaya Gupta (:kats) from comment #3)
> (In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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.
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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.
I just changed the semantics of start() to cancel the old one if there was one going, and verified I didn't break anything.
Attachment #662650 - Flags: review?(snorp)
Comment on attachment 662650 [details] [diff] [review]
MemoryMonitor (v2)

Great
Attachment #662650 - Flags: review?(snorp) → review+
https://hg.mozilla.org/mozilla-central/rev/fdc9029194ed
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.