Last Comment Bug 666317 - Discard decoded images on a memory-pressure notification
: Discard decoded images on a memory-pressure notification
Status: RESOLVED FIXED
[MemShrink:P1][inbound]
:
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: unspecified
: x86 Linux
: -- normal with 1 vote (vote)
: mozilla7
Assigned To: Justin Lebar (not reading bugmail)
:
:
Mentors:
Depends on: 1207012
Blocks: 668634
  Show dependency treegraph
 
Reported: 2011-06-22 10:51 PDT by Mark Finkle (:mfinkle) (use needinfo?)
Modified: 2015-09-21 20:23 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (12.39 KB, patch)
2011-06-30 13:48 PDT, Justin Lebar (not reading bugmail)
joe: review+
Details | Diff | Splinter Review

Description Mark Finkle (:mfinkle) (use needinfo?) 2011-06-22 10:51:06 PDT
On Android, when the app goes into the background it will be killed by the OS if memory becomes scare. We should discard decoded images in repsonse to "memory-pressure" notifications. Mobile will fire that notification when going inot the background, and other times memory is needed.
Comment 1 Justin Lebar (not reading bugmail) 2011-06-22 11:21:58 PDT
On desktop, bug 664291 detects memory pressure on Linux when we swap pages in.  (AIUI, Android doesn't have any swap?)  It would also be good to dump decoded images in this case.
Comment 2 Johnny Stenback (:jst, jst@mozilla.com) 2011-06-28 13:32:11 PDT
Justin says he'll have a look.
Comment 3 Justin Lebar (not reading bugmail) 2011-06-30 07:51:29 PDT
It looks like we already do this on memory-pressure?  In imgLoader.cpp:

imgCacheObserver::Observe(nsISupports* aSubject, const char* aTopic, const PRUnichar* aSomeData)
{
  if (strcmp(aTopic, "memory-pressure") == 0) {
    mLoader.MinimizeCaches();

Jeff, can you confirm this is worksforme?
Comment 4 Jeff Muizelaar [:jrmuizel] 2011-06-30 08:11:16 PDT
Yes, assume this should work. I have not actually checked.
Comment 5 Justin Lebar (not reading bugmail) 2011-06-30 09:19:33 PDT
Huh, it doesn't seem to work.  It gets called, but doesn't discard images in a background tab.
Comment 6 Joe Drew (not getting mail) 2011-06-30 09:42:01 PDT
We never discarded images on memory-pressure. We remove things from the cache (which can actually be counterproductive, because if we remove things from the cache that are *currently in use*, the next time we go to load that image, we'll duplicate it in memory.

However, the reason I got Mark to file this bug is because we don't discard images on memory pressure, so let's keep it focused.
Comment 7 Justin Lebar (not reading bugmail) 2011-06-30 13:48:23 PDT
Created attachment 543238 [details] [diff] [review]
Patch v1
Comment 8 Joe Drew (not getting mail) 2011-06-30 13:59:31 PDT
Comment on attachment 543238 [details] [diff] [review]
Patch v1

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

This won't have the full effect of discarding all decoded data, but I'm OK with that being a follow-up (possibly one that we WONTFIX).

::: modules/libpr0n/src/imgLoader.h
@@ +250,5 @@
>    static void MinimizeCaches();
>  
> +  // Discard all decoded images, even those on a focused tab.
> +  static void DiscardDecodedImages();
> +

You didn't end up doing this, I guess?
Comment 9 Justin Lebar (not reading bugmail) 2011-06-30 14:01:55 PDT
(midaired with joe's previous comment.)

Joe and I discussed in IRC discarding *all* decoded images, not just those in background tabs, as this patch does.

Discarding in the foreground tab is a trickier problem.  On desktop, if memory is low, we will fire low-memory notifications once a second.  Surely we don't want to drop all images and re-decode those in the viewport once a second, especially if we're paging and already running slowly.

Optimally, we'd discard all images except those in the viewport, but my understanding is that this is currently hard to do.

We could do something like drop foreground images on the first memory-pressure call within 60s, but then only drop background images on other memory-pressure notifications.

Anyway, I think dropping background images is a good start.  Since foreground images will be trickier, maybe we can handle it in a separate bug?
Comment 10 Joe Drew (not getting mail) 2011-06-30 14:04:36 PDT
(In reply to comment #9)
> Anyway, I think dropping background images is a good start.  Since
> foreground images will be trickier, maybe we can handle it in a separate bug?

Sounds good!
Comment 11 Justin Lebar (not reading bugmail) 2011-06-30 14:12:19 PDT
Filed bug 668634.
Comment 12 Justin Lebar (not reading bugmail) 2011-06-30 16:56:09 PDT
inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/aa26a78af695
Comment 13 Marco Bonardo [::mak] 2011-07-02 01:54:20 PDT
http://hg.mozilla.org/mozilla-central/rev/aa26a78af695

Note You need to log in before you can comment on or make changes to this bug.