666317 - Discard decoded images on a memory-pressure notification

Status: RESOLVED FIXED

(Core :: ImageLib, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

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)]

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)]

Justin says he'll have a look.

Comment 3

[Justin Lebar (not reading bugmail)]

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] [Parental leave until Sept 21]]

Yes, assume this should work. I have not actually checked.

Comment 5

[Justin Lebar (not reading bugmail)]

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)]

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)]

Attachment: Patch v1

Comment 8

[Joe Drew (not getting mail)]

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)]

(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)]

(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)]

Filed bug 668634.

Comment 12

[Justin Lebar (not reading bugmail)]

inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/aa26a78af695

Comment 13

[Marco Bonardo [:mak] (OoO 09-23 Aug)]

http://hg.mozilla.org/mozilla-central/rev/aa26a78af695