732820 - Cap to the amount of memory used by decoded images

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Justin Lebar (not reading bugmail)]

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.

Comment 1

[Justin Lebar (not reading bugmail)]

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.

Comment 2

[Justin Lebar (not reading bugmail)]

Attachment: Part 1: Add clear() to LinkedList.

Comment 3

[Justin Lebar (not reading bugmail)]

Attachment: Part 2: Cap the amount of discardable image data we'll willingly keep around.

Comment 4

[Justin Lebar (not reading bugmail)]

Attachment: 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.

Comment 5

[Justin Lebar (not reading bugmail)]

Attachment: Part 2: Discard image data immediately on tab close (DOM changes).

Comment 6

[Justin Lebar (not reading bugmail)]

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.

Comment 7

[Joe Drew (not getting mail)]

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.

Comment 8

[Justin Lebar (not reading bugmail)]

> 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?

Comment 9

[Joe Drew (not getting mail)]

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.

Comment 10

[Justin Lebar (not reading bugmail)]

(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?

Comment 11

[Justin Lebar (not reading bugmail)]

+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...

Comment 12

[Justin Lebar (not reading bugmail)]

> 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.

Comment 13

[Justin Lebar (not reading bugmail)]

> 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.

Comment 14

[Justin Lebar (not reading bugmail)]

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.

Comment 15

[Justin Lebar (not reading bugmail)]

Attachment: Part 2, v2: imagelib changes

Comment 16

[Justin Lebar (not reading bugmail)]

> 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-.  :)

Comment 17

[Joe Drew (not getting mail)]

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

Comment 18

[Justin Lebar (not reading bugmail)]

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;
    }
  }

Comment 19

[Justin Lebar (not reading bugmail)]

> > 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 20

[Joe Drew (not getting mail)]

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?

Comment 21

[Justin Lebar (not reading bugmail)]

> Does b2g use the android mobile.js?

I don't think so.  I think it uses only its b2g.js.

Comment 22

[Jeff Walden [:Waldo]]

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 {}.

Comment 23

[Justin Lebar (not reading bugmail)]

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

Comment 24

[Justin Lebar (not reading bugmail)]

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.

Comment 25

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/try/rev/b1e638301ebf

Comment 26

[Justin Lebar (not reading bugmail)]

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

Comment 27

[Justin Lebar (not reading bugmail)]

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!

Comment 28

[Justin Lebar (not reading bugmail)]

Attachment: 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.

Comment 29

[Jeff Walden [:Waldo]]

Sure, go for it -- could be a useful helper in other code.

Comment 30

[Joe Drew (not getting mail)]

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.

Comment 31

[Justin Lebar (not reading bugmail)]

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

Comment 32

[Justin Lebar (not reading bugmail)]

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

Comment 33

[Justin Lebar (not reading bugmail)]

Attachment: Part 2a, v2: Actually discard images in DiscardAll

Comment 34

[Joe Drew (not getting mail)]

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

Comment 35

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7734e28e98f2
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7d417980de6

Comment 36

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/7734e28e98f2
https://hg.mozilla.org/mozilla-central/rev/e7d417980de6