Closed
Bug 792139
Opened 12 years ago
Closed 10 years ago
Discard decoded images as soon as possible on low-memory devices
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 32
People
(Reporter: kats, Assigned: mfinkle)
References
Details
Attachments
(2 files)
1.15 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
2.37 KB,
patch
|
Details | Diff | Splinter Review |
I believe this came up at the last memshrink meeting, and we might already be doing this, but we should verify that we discard decoded image data as soon as possible, as they take up a lot of space in memory.
Comment 1•12 years ago
|
||
I think this is simply a matter of setting image.mem.max_decoded_image_kb to 0, or otherwise a small value. That pref is the max that we'll willingly keep around, but we will keep around more decoded image memory if we must (e.g. we're showing a large image). I don't know if 0 will work well, but I've been browsing with it set to 1024 for a while now with no problems.
Reporter | ||
Comment 2•12 years ago
|
||
I played with setting it to 0 and it seems to be fine. Images take a little longer to re-paint but it does reduce memory usage according to about:memory. However I want to only drop the pref to zero when we know the device is a low-memory device, so making this bug block on bug 801818.
Depends on: 801818
Summary: [ARMv6] Discard decoded images as soon as possible → Discard decoded images as soon as possible on low-memory devices
Reporter | ||
Comment 3•11 years ago
|
||
tn, do you know if setting the image.mem.max_decoded_image_kb pref to 0 will have any effect now that bug 862602 has landed? Should we just close this bug?
Flags: needinfo?(tnikkel)
Comment 4•11 years ago
|
||
It will still have an effect. When our memory use of decoded image data goes over the value in image.mem.max_decoded_image_kb we will try to discard unlocked images until we go under that value. Bug 862602 means that we decode fewer images and we lock fewer images on the active tab. So they are related, but different things. So this bug could still have value.
Flags: needinfo?(tnikkel)
Assignee | ||
Comment 5•10 years ago
|
||
I have been running with "image.mem.max_decoded_image_kb=0" for a few hours and didn't really notice any image display issues. I see that b2g uses "image.mem.max_decoded_image_kb=30000" where Fennec defaults to 51200.
Reporter | ||
Comment 6•10 years ago
|
||
Ship it! (On nightly)
Assignee | ||
Comment 7•10 years ago
|
||
Kats - What do you think of this? If we like this approach, we could add more prefs: "browser.cache.memory.enable = false" "browser.cache.memory.capacity" "browser.cache.memory_limit" "image.cache.size" "javascript.options.mem.high_water_mark" "layers.max-active" "browser.sessionhistory.max_entries"
Assignee: nobody → mark.finkle
Attachment #8413520 -
Flags: review?(bugmail.mozilla)
Comment 8•10 years ago
|
||
Here's a version that does two extra things: * Doesn't wait for a low-memory event -- disables this functionality on Gecko startup if this is a low-memory device. * Also reduces the cache size.
Reporter | ||
Comment 9•10 years ago
|
||
Comment on attachment 8413520 [details] [diff] [review] image-memory-zero v0.1 Review of attachment 8413520 [details] [diff] [review]: ----------------------------------------------------------------- Cool, I didn't know you could do that. This looks fine to me! ::: mobile/android/chrome/content/MemoryObserver.js @@ +29,5 @@ > } > } > Telemetry.addData("FENNEC_LOWMEM_TAB_COUNT", tabs.length); > + > + // Chnage some preferences temporarily for only this session typo
Attachment #8413520 -
Flags: review?(bugmail.mozilla) → review+
Reporter | ||
Comment 10•10 years ago
|
||
Comment on attachment 8413528 [details] [diff] [review] Also pre-emptively set pref on low-memory devices. v1 Review of attachment 8413528 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/chrome/content/browser.js @@ +447,5 @@ > + // On low-memory platforms, never hold on to images. > + // This is also triggered on low memory events. > + if (BrowserApp.isOnLowMemoryPlatform) { > + let defaults = Services.prefs.getDefaultBranch(null); > + defaults.setIntPref("image.mem.max_decoded_image_kb", 0); I would rather this logic lived in MemoryObserver.js so that you don't have hunt in multiple files. Seems like a good idea to do it by default on low-memory platforms.
Comment 11•10 years ago
|
||
Comment on attachment 8413528 [details] [diff] [review] Also pre-emptively set pref on low-memory devices. v1 >+ defaults.setIntPref("image.cache.size", 524288); // Half. image.cache.size doesn't look to be a live pref, ie it's initialized once, so this probably won't have any effect.
Assignee | ||
Comment 12•10 years ago
|
||
remote: https://hg.mozilla.org/integration/fx-team/rev/4660db7b1ece Richard - Let's move your idea to a new bug. I think we'll need to add a MemoryObserver.init() method and remove MemoryObserver from the lazily loaded scripts.
Comment 13•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4660db7b1ece
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
Updated•10 years ago
|
Whiteboard: [MemShrink]
Reporter | ||
Comment 14•10 years ago
|
||
No need to add MemShrink tags on resolved bugs.
Whiteboard: [MemShrink]
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•