Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Discard decoded images on a memory-pressure notification

RESOLVED FIXED in mozilla7

Status

()

Core
ImageLib
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: mfinkle, Assigned: Justin Lebar (not reading bugmail))

Tracking

(Depends on: 1 bug)

unspecified
mozilla7
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P1][inbound])

Attachments

(1 attachment)

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.
(Assignee)

Comment 1

6 years ago
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.
OS: Linux → All
Hardware: x86 → All
Version: unspecified → Trunk
(Assignee)

Updated

6 years ago
OS: All → Linux
Hardware: All → x86
Whiteboard: [MemShrink:P1]
Version: Trunk → unspecified
Justin says he'll have a look.
Assignee: nobody → justin.lebar+bug

Updated

6 years ago
OS: Linux → All
Hardware: x86 → All
(Assignee)

Comment 3

6 years ago
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?
OS: All → Linux
Hardware: All → x86
Yes, assume this should work. I have not actually checked.
(Assignee)

Comment 5

6 years ago
Huh, it doesn't seem to work.  It gets called, but doesn't discard images in a background tab.
Summary: Discard decoded images on a memory-pressure → Discarding decoded images on a memory-pressure notification is broken
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.
Summary: Discarding decoded images on a memory-pressure notification is broken → Discarding decoded images on a memory-pressure notification
Summary: Discarding decoded images on a memory-pressure notification → Discard decoded images on a memory-pressure notification
(Assignee)

Comment 7

6 years ago
Created attachment 543238 [details] [diff] [review]
Patch v1
Attachment #543238 - Flags: review?(joe)
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?
Attachment #543238 - Flags: review?(joe) → review+
(Assignee)

Comment 9

6 years ago
(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?
(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!
(Assignee)

Updated

6 years ago
Blocks: 668634
(Assignee)

Comment 11

6 years ago
Filed bug 668634.
(Assignee)

Comment 12

6 years ago
inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/aa26a78af695
Whiteboard: [MemShrink:P1] → [MemShrink:P1][inbound]
http://hg.mozilla.org/mozilla-central/rev/aa26a78af695
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
Depends on: 1207012
You need to log in before you can comment on or make changes to this bug.