Closed
Bug 666317
Opened 11 years ago
Closed 11 years ago
Discard decoded images on a memory-pressure notification
Categories
(Core :: ImageLib, defect)
Tracking
()
RESOLVED
FIXED
mozilla7
People
(Reporter: mfinkle, Assigned: justin.lebar+bug)
References
(Depends on 1 open bug)
Details
(Whiteboard: [MemShrink:P1][inbound])
Attachments
(1 file)
12.39 KB,
patch
|
joe
:
review+
|
Details | Diff | Splinter Review |
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•11 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.
Updated•11 years ago
|
OS: Linux → All
Hardware: x86 → All
Version: unspecified → Trunk
Assignee | ||
Updated•11 years ago
|
OS: All → Linux
Hardware: All → x86
Whiteboard: [MemShrink:P1]
Version: Trunk → unspecified
Updated•11 years ago
|
OS: Linux → All
Hardware: x86 → All
Assignee | ||
Comment 3•11 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
Comment 4•11 years ago
|
||
Yes, assume this should work. I have not actually checked.
Assignee | ||
Comment 5•11 years ago
|
||
Huh, it doesn't seem to work. It gets called, but doesn't discard images in a background tab.
Updated•11 years ago
|
Summary: Discard decoded images on a memory-pressure → Discarding decoded images on a memory-pressure notification is broken
Comment 6•11 years ago
|
||
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
Updated•11 years ago
|
Summary: Discarding decoded images on a memory-pressure notification → Discard decoded images on a memory-pressure notification
Assignee | ||
Comment 7•11 years ago
|
||
Attachment #543238 -
Flags: review?(joe)
Comment 8•11 years ago
|
||
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•11 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?
Comment 10•11 years ago
|
||
(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 | ||
Comment 11•11 years ago
|
||
Filed bug 668634.
Assignee | ||
Comment 12•11 years ago
|
||
inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/aa26a78af695
Whiteboard: [MemShrink:P1] → [MemShrink:P1][inbound]
Comment 13•11 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/aa26a78af695
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
You need to log in
before you can comment on or make changes to this bug.
Description
•