Closed
Bug 732820
Opened 12 years ago
Closed 12 years ago
Cap to the amount of memory used by decoded images
Categories
(Core :: Graphics: ImageLib, defect)
Core
Graphics: ImageLib
Tracking
()
RESOLVED
FIXED
mozilla14
Tracking | Status | |
---|---|---|
firefox14 | --- | fixed |
People
(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)
References
(Blocks 1 open bug)
Details
(Whiteboard: [MemShrink:P2])
Attachments
(3 files, 4 obsolete files)
1003 bytes,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
31.45 KB,
patch
|
joe
:
review+
|
Details | Diff | Splinter Review |
843 bytes,
patch
|
joe
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
Assignee: nobody → justin.lebar+bug
Assignee | ||
Updated•12 years ago
|
Blocks: image-suck
Assignee | ||
Comment 1•12 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•12 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•12 years ago
|
||
Attachment #603128 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 3•12 years ago
|
||
Attachment #603129 -
Flags: review?(joe)
Updated•12 years ago
|
Whiteboard: [MemShrink] → [MemShrink:P2]
Assignee | ||
Comment 4•12 years ago
|
||
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•12 years ago
|
||
Attachment #603550 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 6•12 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•12 years ago
|
Attachment #603550 -
Attachment is obsolete: true
Attachment #603550 -
Flags: review?(bzbarsky)
Comment 7•12 years ago
|
||
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•12 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?
Comment 9•12 years ago
|
||
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•12 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•12 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•12 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•12 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•12 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 | ||
Comment 15•12 years ago
|
||
Attachment #604076 -
Flags: review?(joe)
Assignee | ||
Comment 16•12 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•12 years ago
|
Attachment #603129 -
Attachment is obsolete: true
Comment 17•12 years ago
|
||
(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•12 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•12 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 20•12 years ago
|
||
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•12 years ago
|
||
> Does b2g use the android mobile.js?
I don't think so. I think it uses only its b2g.js.
Comment 22•12 years ago
|
||
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•12 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 | ||
Comment 24•12 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•12 years ago
|
||
https://hg.mozilla.org/try/rev/b1e638301ebf
Assignee | ||
Comment 26•12 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•12 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•12 years ago
|
||
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)
Comment 29•12 years ago
|
||
Sure, go for it -- could be a useful helper in other code.
Comment 30•12 years ago
|
||
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•12 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•12 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•12 years ago
|
||
Attachment #605996 -
Attachment is obsolete: true
Attachment #606338 -
Flags: review?(joe)
Comment 34•12 years ago
|
||
(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.
Updated•12 years ago
|
Attachment #606338 -
Flags: review?(joe) → review+
Assignee | ||
Comment 35•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/7734e28e98f2 https://hg.mozilla.org/integration/mozilla-inbound/rev/e7d417980de6
status-firefox14:
--- → fixed
Comment 36•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7734e28e98f2 https://hg.mozilla.org/mozilla-central/rev/e7d417980de6
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
You need to log in
before you can comment on or make changes to this bug.
Description
•