Cap to the amount of memory used by decoded images

RESOLVED FIXED in Firefox 14

Status

()

Core
ImageLib
RESOLVED FIXED
6 years ago
5 years ago

People

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

Tracking

(Blocks: 1 bug)

unspecified
mozilla14
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox14 fixed)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(3 attachments, 4 obsolete attachments)

(Assignee)

Description

6 years ago
Right now, there's no limit on the amount of memory used by decoded, unlocked images.  (These are images which appear only in background tabs.)

We'll expire the decoded images after 20-40s, but it would be helpful to place a hard limit on how much memory we'll use for these unlocked images, so we don't have to worry about pathological cases where we keep tons of extra images around for no reason.
(Assignee)

Updated

6 years ago
Assignee: nobody → justin.lebar+bug
(Assignee)

Updated

6 years ago
Blocks: 683284
(Assignee)

Comment 1

6 years ago
Actually, I think it would be good to limit the amount of memory used by decoded images, in total.  We of course can't make this a hard limit atm -- if one tab wants to keep open a bazillion images, it can -- but we can impose this limit and make it a hard limit for background tabs.
(Assignee)

Updated

6 years ago
Summary: Cap to the amount of memory used by decoded, unlocked images → Cap to the amount of memory used by decoded images
(Assignee)

Comment 2

6 years ago
Created attachment 603128 [details] [diff] [review]
Part 1: Add clear() to LinkedList.
Attachment #603128 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 3

6 years ago
Created attachment 603129 [details] [diff] [review]
Part 2: Cap the amount of discardable image data we'll willingly keep around.
Attachment #603129 - Flags: review?(joe)
Whiteboard: [MemShrink] → [MemShrink:P2]
(Assignee)

Comment 4

6 years ago
Created attachment 603549 [details] [diff] [review]
Part 1: Discard image data immediately on tab close, imagelib changes.

The change in RasterImage::Discard to always remove the image from the discard tracker (on both normal and force discards) is safe with my rewrite of discarding in bug 732820.  Not sure if it is otherwise.
Attachment #603549 - Flags: review?(joe)
(Assignee)

Comment 5

6 years ago
Created attachment 603550 [details] [diff] [review]
Part 2: Discard image data immediately on tab close (DOM changes).
Attachment #603550 - Flags: review?(bzbarsky)
(Assignee)

Updated

6 years ago
Blocks: 731419
(Assignee)

Comment 6

6 years ago
Comment on attachment 603549 [details] [diff] [review]
Part 1: Discard image data immediately on tab close, imagelib changes.

Oops, these went into the wrong bug.  They should be bug 731419.
Attachment #603549 - Attachment is obsolete: true
Attachment #603549 - Flags: review?(joe)
(Assignee)

Updated

6 years ago
Attachment #603550 - Attachment is obsolete: true
Attachment #603550 - Flags: review?(bzbarsky)
Comment on attachment 603129 [details] [diff] [review]
Part 2: Cap the amount of discardable image data we'll willingly keep around.

Review of attachment 603129 [details] [diff] [review]:
-----------------------------------------------------------------

r- both for specific reasons and general reasons.

General reasons:

Trying to enforce a global size of decoded images won't work in the case of "lots of large images displayed on-screen." In fact, I think it will result in us endlessly churning the event queue until we have discarded every single unused image. This needs to be reworked to only apply to decoded size of discardable, unlocked images.

::: image/src/DiscardTracker.cpp
@@ +1,4 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */

\o/

::: image/src/imgFrame.cpp
@@ +235,5 @@
>  #endif
>    }
>  
> +  DiscardTracker::InformAllocation(4 * mSize.width * mSize.height);
> +  mInformedDiscardTracker = true;

I'm not really in favour of adding yet another piece of state. Can't we just use mImageSurface != nsnull as a proxy for this?

(Note that this is wrong in the case of a paletted image frame, too. Doesn't really matter, since we only use paletted frames for animated images, which we don't discard, but it should be written down.)

@@ +285,5 @@
> +
> +        // We just dumped all our allocated memory, so let the discard tracker
> +        // know.
> +        if (mInformedDiscardTracker) {
> +          DiscardTracker::InformAllocation(-4 * mSize.width * mSize.height);

This isn't strictly true, since the optimized surface takes up space. But it's truthy, at least.

::: mobile/xul/app/mobile.js
@@ +612,5 @@
>  // optimize images memory usage
>  pref("image.mem.decodeondraw", true);
>  pref("content.image.allow_locking", false);
>  pref("image.mem.min_discard_timeout_ms", 10000);
> +pref("image.mem.max_decoded_image_kb", 10240);

It'd be nice to keep these values expressed the same way in mobile.js and all.js.
Attachment #603129 - Flags: review?(joe) → review-
(Assignee)

Comment 8

6 years ago
> Trying to enforce a global size of decoded images won't work in the case of "lots of large images 
> displayed on-screen." In fact, I think it will result in us endlessly churning the event queue until 
> we have discarded every single unused image. This needs to be reworked to only apply to decoded size 
> of discardable, unlocked images.

This is a point of confusion not helped by the misleading summary I put in this bug (njn had the same concern).  Either that, or I misunderstand your point.

The cap is only binding for unlocked images.  That is, every decoded image counts towards the cap.  But if we exceed the cap, we'll discard only unlocked images.  So if lots of images are displayed on-screen, we'll discard all unused images -- that is, all images in background tabs.  But we will not discard any of the images on the foreground tab.

Isn't that what we want?
Right. My concern is that, since we evaluate the condition every time, we can end up in a situation where we're over the cap but can't do anything about it.
(Assignee)

Comment 10

6 years ago
(In reply to Joe Drew (:JOEDREW!) from comment #9)
> Right. My concern is that, since we evaluate the condition every time, we
> can end up in a situation where we're over the cap but can't do anything
> about it.

Sure, and in that case, we'll discard all unlocked images.  How does this create a problem?
(Assignee)

Comment 11

6 years ago
+void
+DiscardTracker::MaybeDiscardSoon()
+{
+  // Are we carrying around too much decoded image data?  If so, enqueue an
+  // event to try to get us down under our limit.
+  if (sCurrentDecodedImageBytes > sMaxDecodedImageKB * 1024 &&
+      !sDiscardableImages.isEmpty() && !sDiscardRunnablePending) {
+    sDiscardRunnablePending = true;
+    nsRefPtr<DiscardRunnable> runnable = new DiscardRunnable();
+    NS_DispatchToCurrentThread(runnable);
+  }
 }

Notice that we won't spin the event loop if there's nothing to discard.  So it'll just take one trip through the event loop to clear everything out.  No endless spinning, at least not here...
(Assignee)

Comment 12

6 years ago
> I'm not really in favour of adding yet another piece of state. Can't we just use mImageSurface != 
> nsnull as a proxy for this?

mImageSurface goes to null at the end of Optimize(), but we don't decrease the counter there.

(It's really important that the counter be 100% correct, because just a few errors will screw us up permanently, so I wanted to implement the counter in a way which was obviously correct, rather than relying on other state which, even if it's currently correct, might not always be correct.)


> (Note that this is wrong in the case of a paletted image frame, too. Doesn't really matter, since we 
> only use paletted frames for animated images, which we don't discard, but it should be written down.)

I'll fix this, if only so we get the right accounting.
(Assignee)

Comment 13

6 years ago
> It'd be nice to keep these values expressed the same way in mobile.js and all.js.

You mean with the same set of comments and in the same order?  I can do that.
(Assignee)

Comment 14

6 years ago
I noticed that mobile has content.image.allow_locking set to false.  This means that the document will never lock images.

In this case, I can see the 50mb cap being pretty bad, if we try to display more than 50mb of images at a time.  (Although it's just as bad regardless of whether we count locked and unlocked images or only unlocked images, in this case, since all images are unlocked.)

Does allow_locking = false even do what we want on mobile?  Does it end up discarding images which haven't been drawn for a while?  I'm suspicious, to say the least, that it works as intended.
(Assignee)

Updated

6 years ago
Depends on: 734089
(Assignee)

Comment 15

6 years ago
Created attachment 604076 [details] [diff] [review]
Part 2, v2: imagelib changes
Attachment #604076 - Flags: review?(joe)
(Assignee)

Comment 16

6 years ago
> I noticed that mobile has content.image.allow_locking set to false.  This means that the document 
> will never lock images.

This is now bug 734089.

Part 2, v2 still counts locked and unlocked images towards the cap, because I don't yet understand the concern with that.  But I will not be hurt by another r-.  :)
(Assignee)

Updated

6 years ago
Attachment #603129 - Attachment is obsolete: true
(In reply to Justin Lebar [:jlebar] from comment #10)
> (In reply to Joe Drew (:JOEDREW!) from comment #9)
> > Right. My concern is that, since we evaluate the condition every time, we
> > can end up in a situation where we're over the cap but can't do anything
> > about it.
> 
> Sure, and in that case, we'll discard all unlocked images.  How does this
> create a problem?

We won't discard unlocked images that are too young, unless my reading is incorrect.
(Assignee)

Comment 18

6 years ago
My intent is to discard an image if it's too old or if it's the oldest image in the list and we're over the limit.  I think the if statement below does this.

void DiscardTracker::DiscardNow()
{
  TimeStamp now = TimeStamp::Now();
  Node* node;
  while ((node = sDiscardableImages.getLast())) {
    if ((now - node->timestamp).ToMilliseconds() > sMinDiscardTimeoutMs ||
        sCurrentDecodedImageBytes > sMaxDecodedImageKB * 1024) {

      node->img->Discard();
      Remove(node);
    }
    else {
      break;
    }
  }
(Assignee)

Comment 19

6 years ago
> > I noticed that mobile has content.image.allow_locking set to false.  This means that the document 
> > will never lock images.
>
> This is now bug 734089.

...which has been duped and basically wontfix'ed, it seems.

If we don't turn on locking on mobile, I think we'll have to set a high cap on mobile, since otherwise we'll discard even more aggressively than we do now, which will create even more fail.  Maybe 100mb would be enough.
Comment on attachment 604076 [details] [diff] [review]
Part 2, v2: imagelib changes

Review of attachment 604076 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not enthusiastic about non-discardable images affecting the discarding of other images, but I'm willing to give it a try now that I've been convinced it won't lead to obviously bad behaviour. (I misread the condition in DiscardNow() as an &&, not an ||.)

I suspect I will need to review this patch again, but you can have an r+.

::: image/src/DiscardTracker.h
@@ +74,5 @@
> +     * Inform the discard tracker that we've allocated or deallocated some
> +     * memory for a decoded image.  We use this to determine when we've
> +     * allocated too much memory and should discard some images.
> +     */
> +    static void InformAllocation(size_t bytes);

This needs to be ssize_t or ptrdiff_t.

::: image/src/imgFrame.cpp
@@ +289,5 @@
>  #endif
> +
> +        // We just dumped most of our allocated memory, so tell the discard
> +        // tracker that we're not using any at all.
> +        if (mInformedDiscardTracker) {

You said

> mImageSurface goes to null at the end of Optimize(), but we don't decrease the counter there.

But isn't this in imgFrame::Optimize()?

::: mobile/android/app/mobile.js
@@ +624,5 @@
>  pref("image.mem.min_discard_timeout_ms", 10000);
>  
> +// The maximum amount of decoded image data we'll willingly keep around (we
> +// might keep around more than this, but we'll try to get down to this value).
> +pref("image.mem.max_decoded_image_kb", 10240);

Does b2g use the android mobile.js?
Attachment #604076 - Flags: review?(joe) → review+
(Assignee)

Comment 21

6 years ago
> Does b2g use the android mobile.js?

I don't think so.  I think it uses only its b2g.js.
Comment on attachment 603128 [details] [diff] [review]
Part 1: Add clear() to LinkedList.

Review of attachment 603128 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/LinkedList.h
@@ +363,5 @@
> +     */
> +    void clear()
> +    {
> +        while (popFirst())
> +        { }

while (popFirst())
    continue;

is slightly more explicit, I think.  JS has taken to this style recently for empty-body loops, and it's nicely readable -- better than a ; or {}.
Attachment #603128 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 23

6 years ago
> You said
>> mImageSurface goes to null at the end of Optimize(), but we don't decrease the counter there.
> But isn't this in imgFrame::Optimize()?

To be clear, there are two ways that mImageSurface might become null in imgFrame::Optimize:

 1) All pixels are the same, or
 2) mOptSurface is set.

AIUI we only want to do DiscardTracker::InformAllocation(-4 * width * height) in case (1) and not case (2), because after optimizing, the image still lives and takes up memory somewhere.

We might be able to do something in the destructor like

  if (mImageSurface || mOptSurface || mWinSurface || mQuartzSurface)
    DiscardTracker::InformAllocation(-4 * width * height)

but I'm just worried about getting this wrong, or the code becoming wrong in the future.  With the explicit bool, there's no question that we'll always subtract exactly as much as we added (unless the size changes, I guess).
(Assignee)

Updated

6 years ago
No longer blocks: 731419
(Assignee)

Comment 24

6 years ago
PSA: You cannot use computed values in our pref code.  That is,

  pref("image.mem.max_decoded_image_kb", 50 * 1024);

must be

  pref("image.mem.max_decoded_image_kb", 51200);

otherwise tests will bizarrely fail.
(Assignee)

Comment 25

6 years ago
https://hg.mozilla.org/try/rev/b1e638301ebf
(Assignee)

Comment 26

6 years ago
(In reply to Justin Lebar [:jlebar] from comment #25)
> https://hg.mozilla.org/try/rev/b1e638301ebf

This looks good, save for 

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/image/test/browser/browser_bug666317.js | Image should be discarded. 

which I bet is caused by the test assuming the image should be discarded synchronously.
(Assignee)

Comment 27

6 years ago
Oh, no.  It's caused by this completely brain-dead code I wrote:

> void
> DiscardTracker::DiscardAll()
> {
>   if (!sInitialized)
>     return;
> 
>   sDiscardableImages.clear();
> 
>   // The list is empty, so there's no need to leave the timer on.
>   DisableTimer();
> }

Nowhere does this actually discard anything!
(Assignee)

Comment 28

6 years ago
Created attachment 605996 [details] [diff] [review]
Part 2a, v1: Actually discard images in DiscardAll

This obviates the immediate need for LinkedList::clear, but if you're OK with it, Waldo, I'll still push LinkedList::clear.
Attachment #605996 - Flags: review?(joe)
(Assignee)

Updated

6 years ago
Blocks: 735894
Sure, go for it -- could be a useful helper in other code.
Comment on attachment 605996 [details] [diff] [review]
Part 2a, v1: Actually discard images in DiscardAll

Review of attachment 605996 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/src/DiscardTracker.cpp
@@ +116,5 @@
> +  // from the list!
> +  Node *n;
> +  while ((n = sDiscardableImages.popFirst())) {
> +    n->img->Discard();
> +    Remove(n);

Doesn't popFirst() remove this element from the linked list?

Also, DiscardTracker::Remove() doesn't call remove() on the item, which seems wrong.
Attachment #605996 - Flags: review?(joe) → review-
(Assignee)

Comment 31

6 years ago
> Also, DiscardTracker::Remove() doesn't call remove() on the item, which seems wrong.

void
DiscardTracker::Remove(Node *node)
{
  if (node->isInList())
    node->remove();

  if (sDiscardableImages.isEmpty())
    DisableTimer();
}

?

> Doesn't popFirst() remove this element from the linked list?

Yes.  We could do getFirst() and call Remove()...
(Assignee)

Comment 32

6 years ago
> Yes.  We could do getFirst() and call Remove()...

I guess the Remove() call isn't necessary, since we're calling DisableTimer() at the end anyway.  This is better too because there's no fear that |img->Discard()| might free the image (and thus the node).
(Assignee)

Comment 33

6 years ago
Created attachment 606338 [details] [diff] [review]
Part 2a, v2: Actually discard images in DiscardAll
Attachment #605996 - Attachment is obsolete: true
Attachment #606338 - Flags: review?(joe)
(In reply to Justin Lebar [:jlebar] from comment #31)
> > Also, DiscardTracker::Remove() doesn't call remove() on the item, which seems wrong.
> 
> void
> DiscardTracker::Remove(Node *node)
> {
>   if (node->isInList())
>     node->remove();
> 
>   if (sDiscardableImages.isEmpty())
>     DisableTimer();
> }

I failed to take into account that these patches haven't landed. Sorry.
Attachment #606338 - Flags: review?(joe) → review+
(Assignee)

Comment 35

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/7734e28e98f2
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7d417980de6
status-firefox14: --- → fixed
https://hg.mozilla.org/mozilla-central/rev/7734e28e98f2
https://hg.mozilla.org/mozilla-central/rev/e7d417980de6
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14

Updated

6 years ago
Depends on: 736761

Updated

5 years ago
Depends on: 746055
You need to log in before you can comment on or make changes to this bug.