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)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 32

People

(Reporter: kats, Assigned: mfinkle)

References

Details

Attachments

(2 files)

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.
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.
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
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)
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)
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.
Ship it! (On nightly)
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)
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.
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+
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 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.
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.
https://hg.mozilla.org/mozilla-central/rev/4660db7b1ece
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
Blocks: 1003596
Whiteboard: [MemShrink]
No need to add MemShrink tags on resolved bugs.
Whiteboard: [MemShrink]
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.