Flush Picasso LRU cache on memory pressure events

RESOLVED FIXED in Firefox 42

Status

()

Firefox for Android
General
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: rnewman, Assigned: Mailkov, Mentored)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 42
All
Android
Points:
---

Firefox Tracking Flags

(firefox42 fixed)

Details

(Whiteboard: [good next bug][lang=java])

Attachments

(1 attachment, 1 obsolete attachment)

Comment hidden (empty)
(Reporter)

Comment 1

3 years ago
Google should be sufficient to figure out how to do this.

The right place to add the code is probably MemoryMonitor.java.
Mentor: rnewman@mozilla.com, michael.l.comella@gmail.com
Whiteboard: [good second bug][lang=java]
https://mail.mozilla.org/pipermail/mobile-firefox-dev/2015-February/001090.html
Whiteboard: [good second bug][lang=java] → [good next bug][lang=java]
(Assignee)

Comment 3

2 years ago
I'd like to work on this ... can you assign to me ?
Assignee: nobody → miticomilko
(Assignee)

Comment 4

2 years ago
Created attachment 8634747 [details] [diff] [review]
bug1116349.patch
Attachment #8634747 - Flags: review?(michael.l.comella)
(Assignee)

Comment 5

2 years ago
I create a patch ... I hope it is ok ... :)
Flags: needinfo?(michael.l.comella)
Comment on attachment 8634747 [details] [diff] [review]
bug1116349.patch

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

Eviction code seems correct, but I'm not sure under which memory pressure level we should be flushing Picasso - deferring to Richard.
Attachment #8634747 - Flags: review?(rnewman)
Attachment #8634747 - Flags: review?(michael.l.comella)
Attachment #8634747 - Flags: review+
(Reporter)

Comment 7

2 years ago
Comment on attachment 8634747 [details] [diff] [review]
bug1116349.patch

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

This fires at MEDIUM and above. MEDIUM corresponds to:


    /**
     * Level for {@link #onTrimMemory(int)}: the process is not an expendable
     * background process, but the device is running moderately low on memory.
     * Your running process may want to release some unneeded resources for
     * use elsewhere.
     */
    static final int TRIM_MEMORY_RUNNING_MODERATE = 5;


That seems right to me. Worst-case, Picasso can go back to disk.

::: mobile/android/base/home/ImageLoader.java
@@ +47,5 @@
>          }
>      }
>  
>      private static Picasso instance;
> +    private static LruCache lrucache;

Add a comment that these are protected by synchronization on the class.

@@ +62,5 @@
>  
>          return instance;
>      }
>  
> +    public static void clearLruCache() {

This method should be synchronized.
Attachment #8634747 - Flags: review?(rnewman) → review+
(Assignee)

Comment 8

2 years ago
Created attachment 8637069 [details] [diff] [review]
Bug 1116349 - Flush Picasso LRU cache on memory pressure events

I synchronized the function clearLruCache() and add comment as you suggested.

I think now the patch is ok ... If it is, please can you push my patch to Try server? Because I have not yet access. 
Thanks.
Attachment #8634747 - Attachment is obsolete: true
Flags: needinfo?(rnewman)
Attachment #8637069 - Flags: review?(rnewman)
(Reporter)

Comment 9

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b2507071cc8
(Reporter)

Updated

2 years ago
Attachment #8637069 - Flags: review?(rnewman) → review+
(Reporter)

Comment 10

2 years ago
Leaving ni for me to check the try run and land.
Status: NEW → ASSIGNED
Flags: needinfo?(michael.l.comella)
(Reporter)

Comment 11

2 years ago
url:        https://hg.mozilla.org/integration/fx-team/rev/9b6528285beac04e2a29cc6ad6220067f6930604
changeset:  9b6528285beac04e2a29cc6ad6220067f6930604
user:       Melchiorre Alastra <miticomilko@hotmail.com>
date:       Wed Jul 22 18:00:07 2015 -0700
description:
Bug 1116349 - Flush Picasso LRU cache on memory pressure events
(Reporter)

Updated

2 years ago
Flags: needinfo?(rnewman)
https://hg.mozilla.org/mozilla-central/rev/9b6528285bea
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
You need to log in before you can comment on or make changes to this bug.