|
|
DescriptionDon't cache GIF frames if it would be too large.
The GIF decoder now respects m_maxDecodedBytes and checks for overflow.
If the GIF would go over the memory limit then cached frames provide no
benefit. By the time the animation loops we would have been forced to
purge those frames. So caching any extra (not currently needed) frames
would be a waste. In that case, run lean and purge aggressively.
R=scroggo@chromium.org
BUG=619173
Committed: https://crrev.com/9899eade00c556b8086b74867f02edf8c98cb4b9
Cr-Commit-Position: refs/heads/master@{#402536}
Patch Set 1 #
Total comments: 16
Patch Set 2 : Changes to overflow detection and comments #
Total comments: 18
Patch Set 3 : Fixing build errors. #Patch Set 4 : Handling cases overflow-infinity. Clarifying. #
Total comments: 8
Patch Set 5 : Moving code to private member function. #
Messages
Total messages: 49 (16 generated)
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2075093002/1
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
> The GIF decoder now respects the memory limit. > Which limit? It looks like it respects m_maxDecodedBytes and some other amount. > If the GIF would go over the memory limit than cached frames provide no nit: then* > benefit. By the time the animation loops we would have been forced to > purge those frames. So caching any extra (not currently needed) frames > would be a waste. In that case, run lean and purge aggressively. https://codereview.chromium.org/2075093002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp (right): https://codereview.chromium.org/2075093002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:312: // image memory usage. It might be interesting to note that the "MemoryUsage" you count is in units of pixels (rather than bytes, which might be expected). https://codereview.chromium.org/2075093002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:314: unsigned frameMemoryUsage = decodedImageSize.width() * decodedImageSize.height(); Why is this unsigned? Should we call decodedImageSize.area()? (That method converts up to uin64_t to protect against overflow.) https://codereview.chromium.org/2075093002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:315: unsigned long long imageMemoryUsage = static_cast<unsigned long long>(frameMemoryUsage) The meaning of the word "image" here is not entirely obvious to me. Maybe call this totalMemoryUsage? On another note, I think this is really the worst case scenario - where we actually have to store all frames from 0 - index. If we wanted to be more conservative, we could only purge after we've stored enough frames to reach the limit. (That would require us to do the check on each call to decode, which could have an effect on performance. Although as I mention elsewhere, we're currently always doing this check.) https://codereview.chromium.org/2075093002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:318: bool overflowed = imageMemoryUsage > ((1 << 29) - 1); How'd you pick this number? https://codereview.chromium.org/2075093002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:319: if (overflowed) { It looks like this should be if (overflowed || imageMemoryUsage > m_maxDecodedBytes) m_purgeAggressively = true; Maybe it'd be even simpler to say if (imageMemoryUsage > min((1 << 29) - 1), m_maxDecodedBytes)) m_purgeAggressively = true; https://codereview.chromium.org/2075093002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:322: if (imageMemoryUsage > m_maxDecodedBytes) As mentioned above, imageMemoryUsage is in units of pixels. m_maxDecodedBytes is in bytes, so this seems like an inappropriate comparison. https://codereview.chromium.org/2075093002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:339: if (m_purgeAggressively) As written, m_purgeAggressively could be a local variable. The only effect of it being a member variable is that once it's set to true, it remains true on future iterations. But you still check the size on future calls in order to decide whether to set it to true or do nothing. - If we should continue purging aggressively even if we go back to the first frame and we are well under the limit, should we skip the calculations to determine whether to purge if we are already purging? (Or maybe I'm optimizing prematurely?) - If we only want to purge aggressively when we're over the limit, we could make it a local variable.
https://codereview.chromium.org/2075093002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp (right): https://codereview.chromium.org/2075093002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:312: // image memory usage. On 2016/06/20 17:59:40, scroggo_chromium wrote: > It might be interesting to note that the "MemoryUsage" you count is in units of > pixels (rather than bytes, which might be expected). Oh, oops. I originally wrote this with a * 4 since it becomes 32bpp. I really want to compare against memory size so I'll make sure to check the size of the frame. https://codereview.chromium.org/2075093002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:314: unsigned frameMemoryUsage = decodedImageSize.width() * decodedImageSize.height(); On 2016/06/20 17:59:40, scroggo_chromium wrote: > Why is this unsigned? Should we call decodedImageSize.area()? (That method > converts up to uin64_t to protect against overflow.) Hrmmm, the unsigned was a mistake. the .width() and .height() return int. The unsigned comes from sizeCalculationMayOverflow https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/image... which is called from ImageDecoder::setSize(). This is also where the (1 << 29) - 1 comes from, which was mentioned in another comment. The 29 is specifically because there is sometimes width * height * 4. 32-bit * 32bit might consume 64-bit. Multiply that by 4 and you need more bits. Although, we then (currently) multiply that by the index. I'll clean this up. https://codereview.chromium.org/2075093002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:315: unsigned long long imageMemoryUsage = static_cast<unsigned long long>(frameMemoryUsage) On 2016/06/20 17:59:40, scroggo_chromium wrote: > The meaning of the word "image" here is not entirely obvious to me. Maybe call > this totalMemoryUsage? > > On another note, I think this is really the worst case scenario - where we > actually have to store all frames from 0 - index. > > If we wanted to be more conservative, we could only purge after we've stored > enough frames to reach the limit. (That would require us to do the check on each > call to decode, which could have an effect on performance. Although as I mention > elsewhere, we're currently always doing this check.) I did not parse what you said very well. If I understand correctly, are you suggesting that we keep caching frames until we learn that we have crossed the limit, and then we purge? That is what this code does. It attempts to keep the benefit of caching until it learns the the cost of caching would blow our memory budget. It then switches into purge mode. Because gif doesn't let you know in advance how many frames there will be while downloading, we have to do it as we go. I suppose once we have made a full loop we can not check since we then know it won't need to purge aggressively. https://codereview.chromium.org/2075093002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:339: if (m_purgeAggressively) On 2016/06/20 17:59:40, scroggo_chromium wrote: > As written, m_purgeAggressively could be a local variable. The only effect of it > being a member variable is that once it's set to true, it remains true on future > iterations. But you still check the size on future calls in order to decide > whether to set it to true or do nothing. > > - If we should continue purging aggressively even if we go back to the first > frame and we are well under the limit, should we skip the calculations to > determine whether to purge if we are already purging? (Or maybe I'm optimizing > prematurely?) > - If we only want to purge aggressively when we're over the limit, we could make > it a local variable. I do not want it to be a local variable. If it was local we might not purge until we hit frame 20 (for example), but then we would purge all the unneeded frames and have wasted the memory those frames consumed. Keeping the aggressive purge after we loop means we won't have memory spikes for the first part of the animation. I'll modify it to skip the calculation if we have already determined we should purge aggressively.
>> The GIF decoder now respects the memory limit. >> > >Which limit? It looks like it respects m_maxDecodedBytes and >some other amount. I don't see that. When I do a code search I only see m_maxDecodedBytes being used as a check inside the JPEGImageDecoder for determining that it should scale. But either way, it definitely wasn't respecting this limit when attempting to catch up. It would add every frame it decoded to the cache while it caught up. It went WAY over this limit. (See bug) That said, I actually don't think it was respecting it outside of this, either. I believe CC was telling the decoder to purge so once it finished catching up it would not use as much memory. But that isn't because the decoder saw it was over the limit.
On 2016/06/21 01:49:11, cblume wrote: > >> The GIF decoder now respects the memory limit. > >> > > > >Which limit? It looks like it respects m_maxDecodedBytes and >some other > amount. > > I don't see that. When I do a code search I only see m_maxDecodedBytes being > used as a check inside the JPEGImageDecoder for determining that it should > scale. Agreed. I was speaking about your CL (which does respect m_maxDecodedBytes, plus the overflow limit), rather than the code base without your CL. I find the term "memory limit" generic. I think the description should say something like: The GIF decoder now respects m_maxDecodedBytes and checks for overflow. That said, the comparison for overflow is somewhat arbitrary, right? Maybe it's the right value, but we could still potentially have two separate images at the maximum size (i.e. just under overflow), which would not trigger any purges (or maybe it would if they're in discardable memory?), but with your CL two frames in a single GIF at the maximum size would trigger a purge. > > But either way, it definitely wasn't respecting this limit when attempting to > catch up. It would add every frame it decoded to the cache while it caught up. > It went WAY over this limit. (See bug) > > That said, I actually don't think it was respecting it outside of this, either. > I believe CC was telling the decoder to purge so once it finished catching up it > would not use as much memory. But that isn't because the decoder saw it was over > the limit. Agreed on both counts. https://codereview.chromium.org/2075093002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp (right): https://codereview.chromium.org/2075093002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:315: unsigned long long imageMemoryUsage = static_cast<unsigned long long>(frameMemoryUsage) On 2016/06/21 01:00:52, cblume wrote: > On 2016/06/20 17:59:40, scroggo_chromium wrote: > > The meaning of the word "image" here is not entirely obvious to me. Maybe call > > this totalMemoryUsage? > > > > On another note, I think this is really the worst case scenario - where we > > actually have to store all frames from 0 - index. > > > > If we wanted to be more conservative, we could only purge after we've stored > > enough frames to reach the limit. (That would require us to do the check on > each > > call to decode, which could have an effect on performance. Although as I > mention > > elsewhere, we're currently always doing this check.) > > I did not parse what you said very well. Sorry, I'll try to explain it better: You compute how much memory is cached in the GIFImageDecoder to be |index| * |frameMemoryUsage|. But as I understand it, it is possible that GIFImageDecoder has already been told to purge, in which case the actual amount cached is less than |index| * |frameMemoryUsage|. If |N| is the number of frames (< |index|) that have already been purged (by the client calling clearCacheExceptFrame before decoding frame |index|), the memory cached in GIFImageDecoder is actually (|index| - |N|) * |frameMemoryUsage|. It sounds like it is possible that GIFImageDecoder will have to decode frames 0 - |N|, which is the worst case scenario, but we don't know that yet. We'll find out the actual number down below, at the end of the do while loop. > If I understand correctly, are you suggesting that we keep caching frames until > we learn that we have crossed the limit, and then we purge? > That is what this code does. Almost. It counts frames that we might not actually have to decode. > It attempts to keep the benefit of caching until it > learns the the cost of caching would blow our memory budget. It then switches > into purge mode. > Because gif doesn't let you know in advance how many frames there will be while > downloading, we have to do it as we go. > > I suppose once we have made a full loop we can not check since we then know it > won't need to purge aggressively. I agree - if we make a full loop and never set m_purgeAggressively to true (based on the current computation), the second loop does not need to check. But once we've set m_purgeAggressively to true, we can also skip the check for the remainder of the first loop (and for future loops, because again, we currently never set m_purgeAggressively back to false). https://codereview.chromium.org/2075093002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:333: for (auto i = framesToDecode.rbegin(); i != framesToDecode.rend(); ++i) { At this point, we know the actual amount of data we'll be required to cache - it's framesToDecode.size() * frameMemoryUsage. https://codereview.chromium.org/2075093002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:339: if (m_purgeAggressively) On 2016/06/21 01:00:52, cblume wrote: > On 2016/06/20 17:59:40, scroggo_chromium wrote: > > As written, m_purgeAggressively could be a local variable. The only effect of > it > > being a member variable is that once it's set to true, it remains true on > future > > iterations. But you still check the size on future calls in order to decide > > whether to set it to true or do nothing. > > > > - If we should continue purging aggressively even if we go back to the first > > frame and we are well under the limit, should we skip the calculations to > > determine whether to purge if we are already purging? (Or maybe I'm optimizing > > prematurely?) > > - If we only want to purge aggressively when we're over the limit, we could > make > > it a local variable. > > I do not want it to be a local variable. > > If it was local we might not purge until we hit frame 20 (for example), but then > we would purge all the unneeded frames and have wasted the memory those frames > consumed. But isn't that already true in this patch set, at least on the first iteration through the loop? I guess your next sentence tells me you're really concerned about the second iteration. That seems fine to me, but I do think it's a tradeoff between using memory and having the frames you need. i.e. on the second iteration, we'll be purging aggressively before frame 20, but isn't it possible we'll purge frames that we'll need later? > > Keeping the aggressive purge after we loop means we won't have memory spikes for > the first part of the animation. > > I'll modify it to skip the calculation if we have already determined we should > purge aggressively.
Description was changed from ========== Don't cache GIF frames if it would be too large. The GIF decoder now respects the memory limit. If the GIF would go over the memory limit than cached frames provide no benefit. By the time the animation loops we would have been forced to purge those frames. So caching any extra (not currently needed) frames would be a waste. In that case, run lean and purge aggressively. R=scroggo@chromium.org BUG=619173 ========== to ========== Don't cache GIF frames if it would be too large. The GIF decoder now respects m_maxDecodedBytes and checks for overflow. If the GIF would go over the memory limit than cached frames provide no benefit. By the time the animation loops we would have been forced to purge those frames. So caching any extra (not currently needed) frames would be a waste. In that case, run lean and purge aggressively. R=scroggo@chromium.org BUG=619173 ==========
On 2016/06/21 15:46:08, scroggo_chromium wrote: > That said, the comparison for overflow is somewhat arbitrary, right? Maybe it's > the right value, but we could still potentially have two separate images at the > maximum size (i.e. just under overflow), which would not trigger any purges (or > maybe it would if they're in discardable memory?), but with your CL two frames > in a single GIF at the maximum size would trigger a purge. I completely agree. I could have 1,000 small images that are all under their own limit but collectively impose a lot of memory pressure. I think the *real* correct fix for this would be for CC to control the cache. We ran into this same issue with CC/Skia as well. The decoder itself is not aware of its usage and so its caching cannot be that smart. It relies on CC to be smart for it. But that really is just complicating things -- CC can exersise *some* control over the cache by calling clearCacheExceptFrame() but it cannot read from the cache or do fine-grained cache manipulation. The better solution would be for CC to have a cache and to give the cached frames to the decoder when the decoder needs them. I could see 2 functions for 1.) decoding frame #N, and 2.) asking what frames are needed to decode frame #N. Something like that. However, that solution would be a significant change and we can have a significant win with a smaller, easier change. https://codereview.chromium.org/2075093002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp (right): https://codereview.chromium.org/2075093002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:315: unsigned long long imageMemoryUsage = static_cast<unsigned long long>(frameMemoryUsage) On 2016/06/21 15:46:08, scroggo_chromium wrote: > it is possible that GIFImageDecoder has already been told to purge, in which case the actual amount cached is less than |index| * |frameMemoryUsage|. Ah, okay. I think I understand better now. You are correct that we might not have to decode 0-N, or that the current memory usage is all-the-previous-frames. I think that might be what you meant about the worst-case scenario. However, I'm not trying to find the amount of frames we need to decode or the current amount of memory used. I'm trying to find the amount of memory we could potentially use if every frame were cached. So I'm not bothered by whether previous frames were purged or not. My reasoning is... - If we had unlimited memory we would cache all frames. - If we were tightly memory confined we would only cache what is required to decode the next frame (so we aren't wasting work). - We could stay somewhere in the middle and maintain specially-selected frames. This would require some smarter awareness from the decoder. If we wanted to do that it should be a separate CL. And it would probably be in the purge function, which we would need to call anyway. So this portion of the smarts would still be correct. So that said, given our current constraint of each image having a separate budget, ... - Either we can fit the entire image in cache, or - We cannot, and we should be purging (the purge can become smarter in the future) So that is exactly what my code looks for. Will the whole image fit or not? Should we purge at all or not? Although, maybe we should *always* call purge and the purge itself can be smart about managing the cache size. Maybe this code belongs in the purge function.
On 2016/06/22 01:23:06, cblume wrote: > On 2016/06/21 15:46:08, scroggo_chromium wrote: > > That said, the comparison for overflow is somewhat arbitrary, right? Maybe > it's > > the right value, but we could still potentially have two separate images at > the > > maximum size (i.e. just under overflow), which would not trigger any purges > (or > > maybe it would if they're in discardable memory?), but with your CL two frames > > in a single GIF at the maximum size would trigger a purge. > > I completely agree. > I could have 1,000 small images that are all under their own limit but > collectively impose a lot of memory pressure. > > I think the *real* correct fix for this would be for CC to control the cache. +1. Another option: it's been suggested that we use discardable memory for the frames, although I don't think there's a bug for that. We would keep the memory locked for the frames that we currently decide not to throw away since we think they'll be needed for a future frame. > We ran into this same issue with CC/Skia as well. > The decoder itself is not aware of its usage and so its caching cannot be that > smart. It relies on CC to be smart for it. But that really is just complicating > things -- CC can exersise *some* control over the cache by calling > clearCacheExceptFrame() but it cannot read from the cache or do fine-grained > cache manipulation. > The better solution would be for CC to have a cache and to give the cached > frames to the decoder when the decoder needs them. I could see 2 functions for > 1.) decoding frame #N, and 2.) asking what frames are needed to decode frame #N. > Something like that. Makes sense to me. > > However, that solution would be a significant change and we can have a > significant win with a smaller, easier change. +1. I think this change is the right direction, but I'd like to see a comment/code that explains how we came up with the limit. Right now it's described as overflow, but that's not really what it's checking. It comes from a place where we would multiply two numbers together and we want to make sure that number doesn't overflow. But in this case, we'll have added the numbers together, but that sum is only used to compare against immediately. I could easily seeing us deciding the number should be lower later, and it would be misleading to think that it's this number because we were worried about overflow. Maybe m_maxDecodedBytes should be set to whatever limit we decide, and then you only need to compare to it. https://codereview.chromium.org/2075093002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp (right): https://codereview.chromium.org/2075093002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:315: unsigned long long imageMemoryUsage = static_cast<unsigned long long>(frameMemoryUsage) On 2016/06/22 01:23:06, cblume wrote: > On 2016/06/21 15:46:08, scroggo_chromium wrote: > > it is possible that GIFImageDecoder has already been told to purge, in which > case the actual amount cached is less than |index| * |frameMemoryUsage|. > > Ah, okay. I think I understand better now. > > You are correct that we might not have to decode 0-N, or that the current memory > usage is all-the-previous-frames. I think that might be what you meant about the > worst-case scenario. Yes, that is what I mean. > > However, I'm not trying to find the amount of frames we need to decode or the > current amount of memory used. > I'm trying to find the amount of memory we could potentially use if every frame > were cached. > > So I'm not bothered by whether previous frames were purged or not. That sounds fine to me. Will you please update the comments to reflect that? Something like (slightly modifying your comments above): // So instead, simply purge unused frames if caching all the frames of the image would // use too much memory. // As we decode we will learn the total number of frames, and thus total // possible image memory usage. > > > > My reasoning is... > > - If we had unlimited memory we would cache all frames. > > - If we were tightly memory confined we would only cache what is required to > decode the next frame (so we aren't wasting work). > > - We could stay somewhere in the middle and maintain specially-selected frames. > This would require some smarter awareness from the decoder. If we wanted to do > that it should be a separate CL. And it would probably be in the purge function, > which we would need to call anyway. So this portion of the smarts would still be > correct. +1 > > So that said, given our current constraint of each image having a separate > budget, ... > - Either we can fit the entire image in cache, or > - We cannot, and we should be purging (the purge can become smarter in the > future) > > So that is exactly what my code looks for. Will the whole image fit or not? > Should we purge at all or not? > > > > Although, maybe we should *always* call purge and the purge itself can be smart > about managing the cache size. Maybe this code belongs in the purge function. When you say "the purge function", do you mean clearCacheExceptFrame? I think it makes more sense here, since clearCache means it will clear it, but this code decides whether to clear it. (Although come to think of it, that method might decide to keep another frame anyway...)
On 2016/06/22 15:10:04, scroggo_chromium wrote: > Maybe m_maxDecodedBytes should be set to whatever limit we decide, and then you > only need to compare to it. Yes, I agree. Sorry I haven't uploaded the changes yet. I'm currently sheriff so I'm getting to this when I can. The code itself is behind our conversation. I will try to catch it up. > That sounds fine to me. Will you please update the comments to reflect that? > Something like (slightly modifying your comments above): > > // So instead, simply purge unused frames if caching all the frames of the image > would > // use too much memory. > // As we decode we will learn the total number of frames, and thus total > // possible image memory usage. Will do. > When you say "the purge function", do you mean clearCacheExceptFrame? I think it > makes more sense here, since clearCache means it will clear it, but this code > decides whether to clear it. (Although come to think of it, that method might > decide to keep another frame anyway...) Yes, I meant clearCacheExceptFrame. And you are right, that actually might keep 2 frames. So right now clearCacheExceptFrame has *some* smarts about which frames to keep. I feel like that would be the place to handle a wiser "which frames should be kept?" Also, I 100% agree with your subtle design hint with one area deciding and another area acting.
Description was changed from ========== Don't cache GIF frames if it would be too large. The GIF decoder now respects m_maxDecodedBytes and checks for overflow. If the GIF would go over the memory limit than cached frames provide no benefit. By the time the animation loops we would have been forced to purge those frames. So caching any extra (not currently needed) frames would be a waste. In that case, run lean and purge aggressively. R=scroggo@chromium.org BUG=619173 ========== to ========== Don't cache GIF frames if it would be too large. The GIF decoder now respects m_maxDecodedBytes and checks for overflow. If the GIF would go over the memory limit then cached frames provide no benefit. By the time the animation loops we would have been forced to purge those frames. So caching any extra (not currently needed) frames would be a waste. In that case, run lean and purge aggressively. R=scroggo@chromium.org BUG=619173 ==========
Thank you for waiting on this while I took care of other fires. I think I have updated everything we had talked about. Please let me know if I missed something or if you would like more changes. Thanks!
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2075093002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp (right): https://codereview.chromium.org/2075093002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:311: // the image would use too much memory. Maybe specify what you mean by "too much memory"? https://codereview.chromium.org/2075093002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:313: // possible image memoroy used. memory* https://codereview.chromium.org/2075093002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:315: uint64_t frameArea = this->decodedSize().area(); nit: I don't think Blink uses this-> to call class methods. nit: could be const. https://codereview.chromium.org/2075093002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:317: bool wouldOverflow = frameMemoryUsage > (1 << 62); It looks like "frameMemoryUsage" has not yet been declared? Did you mean frameArea? https://codereview.chromium.org/2075093002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:318: if (!wouldOverflow) { Just to make sure I understand - it looks like if we would have overflowed, we do *not* purge aggressively (here and below)? I know I said something like "overflow is probably not the right limit", but it looks like we end up with something like: [0, LIMIT]: OK [LIMIT, overflow): Too much memory [overflow, infinity): OK I was thinking more like one of the following: A) LIMIT < overflow: [0, LIMIT]: OK [LIMIT, overflow]: Too much memory [overflow, infinity]: Too much memory, but we have to calculate differently B) LIMIT > overflow: [0, overflow): OK [overflow, LIMIT]: OK, but we have to calculate differently (LIMIT, infinity): Too much memory https://codereview.chromium.org/2075093002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:326: wouldOverflow |= index > (1 << 31); Why not combine these into one statement? wouldOverflow = (frameMemoryUsage > (1 << 32)) || (index > (1 << 31)); (Arguably, you could drop the variable wouldOverflow, which is typically used in a conditional and then modified later before it's used again. The variable name "wouldOverflow" might make it a little clearer what you're doing, though.) https://codereview.chromium.org/2075093002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:328: uint64_t totalMemoryUsage = static_cast<uint32_t>(frameMemoryUsage) Why do you need to cast this? https://codereview.chromium.org/2075093002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:330: if (wouldOverflow || totalMemoryUsage < m_maxDecodedBytes) { wouldOverflow can never be true here, right? Also, do you really want totalMemoryUsage > m_maxDecodedBytes ? https://codereview.chromium.org/2075093002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:351: this->clearCacheExceptFrame(*i); nit: I think "this->" is unnecessary here?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
https://codereview.chromium.org/2075093002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp (right): https://codereview.chromium.org/2075093002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:311: // the image would use too much memory. On 2016/06/27 19:04:32, scroggo_chromium wrote: > Maybe specify what you mean by "too much memory"? Done. https://codereview.chromium.org/2075093002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:313: // possible image memoroy used. On 2016/06/27 19:04:32, scroggo_chromium wrote: > memory* Done. https://codereview.chromium.org/2075093002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:315: uint64_t frameArea = this->decodedSize().area(); On 2016/06/27 19:04:32, scroggo_chromium wrote: > nit: I don't think Blink uses this-> to call class methods. > > nit: could be const. Done. https://codereview.chromium.org/2075093002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:317: bool wouldOverflow = frameMemoryUsage > (1 << 62); On 2016/06/27 19:04:32, scroggo_chromium wrote: > It looks like "frameMemoryUsage" has not yet been declared? Did you mean > frameArea? Yeah, this was a mistake which I fixed in Patch Set 3. https://codereview.chromium.org/2075093002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:318: if (!wouldOverflow) { On 2016/06/27 19:04:32, scroggo_chromium wrote: > Just to make sure I understand - it looks like if we would have overflowed, we > do *not* purge aggressively (here and below)? I know I said something like > "overflow is probably not the right limit", but it looks like we end up with > something like: > > [0, LIMIT]: OK > [LIMIT, overflow): Too much memory > [overflow, infinity): OK > > I was thinking more like one of the following: > > A) LIMIT < overflow: > [0, LIMIT]: OK > [LIMIT, overflow]: Too much memory > [overflow, infinity]: Too much memory, but we have to calculate differently > > B) LIMIT > overflow: > [0, overflow): OK > [overflow, LIMIT]: OK, but we have to calculate differently > (LIMIT, infinity): Too much memory Oh whoops. Yeah, I definitely only wanted to allow within-memory. https://codereview.chromium.org/2075093002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:326: wouldOverflow |= index > (1 << 31); On 2016/06/27 19:04:32, scroggo_chromium wrote: > Why not combine these into one statement? > > wouldOverflow = (frameMemoryUsage > (1 << 32)) || (index > (1 << 31)); > > (Arguably, you could drop the variable wouldOverflow, which is typically used in > a conditional and then modified later before it's used again. The variable name > "wouldOverflow" might make it a little clearer what you're doing, though.) I like combining. I think I prefer keeping the variable name. I think it does make the code more clear. https://codereview.chromium.org/2075093002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:328: uint64_t totalMemoryUsage = static_cast<uint32_t>(frameMemoryUsage) On 2016/06/27 19:04:32, scroggo_chromium wrote: > Why do you need to cast this? Well, we could actually do uint128_t = uint64_t * uint64_t. This would be more simple and would allow for crazy huge memory usage which might be possible. But there is no uint128_t. Given that, since we just confirmed above that these do not have values over 32-bits, we could just multiply them (uint64_t = uint64_t * uint64_t). The casts aren't necessary. I did this to try to make it more clear that I'm enforcing overflow protection. But maybe I'm too verbose. :) It is already pretty clear. I'll take them out. https://codereview.chromium.org/2075093002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:330: if (wouldOverflow || totalMemoryUsage < m_maxDecodedBytes) { On 2016/06/27 19:04:32, scroggo_chromium wrote: > wouldOverflow can never be true here, right? > > Also, do you really want > > totalMemoryUsage > m_maxDecodedBytes > > ? Oops, I definitely meant > (which means this should be enter-able. https://codereview.chromium.org/2075093002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:351: this->clearCacheExceptFrame(*i); On 2016/06/27 19:04:32, scroggo_chromium wrote: > nit: I think "this->" is unnecessary here? Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2075093002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp (right): https://codereview.chromium.org/2075093002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:305: if (!m_purgeAggressively) { How do you feel about making this a private method? e.g. void determineWhetherToPurgeAggressively() { if (m_purgeAggressively) return; const uint64_t frameArea = ... bool wouldOverflow = ... if (wouldOverflow) { m_purgeAggressively = true; return; } const uint64_t frameMemoryUsage = ... wouldOverflow = ... if (wouldOverflow) { m_purgeAggressively = true; return; } const uint64_t totalMemoryUsage = ... if (totalMemoryUsage > m_maxDecodedBytes) m_purgeAggressively = true; } That isolates the code around deciding whether to purge aggressively. It also reduces your nested if/elses, and I think it makes the code a little easier to follow. (And maybe there's a shorter name that is clear about what it does...) https://codereview.chromium.org/2075093002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:313: // (m_maxDecodedBytes). or overflow. https://codereview.chromium.org/2075093002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:321: if (!wouldOverflow) { I think you want if (wouldOverflow) ? (No "!") https://codereview.chromium.org/2075093002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:336: if (wouldOverflow || totalMemoryUsage > m_maxDecodedBytes) { No need to check wouldOverflow here. It is guaranteed to be false, since this is the else statement to if (wouldOverflow)
https://codereview.chromium.org/2075093002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp (right): https://codereview.chromium.org/2075093002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:305: if (!m_purgeAggressively) { On 2016/06/28 15:55:28, scroggo_chromium wrote: > How do you feel about making this a private method? e.g. > > void determineWhetherToPurgeAggressively() { > if (m_purgeAggressively) > return; > > const uint64_t frameArea = ... > bool wouldOverflow = ... > if (wouldOverflow) { > m_purgeAggressively = true; > return; > } > > const uint64_t frameMemoryUsage = ... > wouldOverflow = ... > if (wouldOverflow) { > m_purgeAggressively = true; > return; > } > > const uint64_t totalMemoryUsage = ... > if (totalMemoryUsage > m_maxDecodedBytes) > m_purgeAggressively = true; > } > > That isolates the code around deciding whether to purge aggressively. It also > reduces your nested if/elses, and I think it makes the code a little easier to > follow. > > (And maybe there's a shorter name that is clear about what it does...) I like it. Done. https://codereview.chromium.org/2075093002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:313: // (m_maxDecodedBytes). On 2016/06/28 15:55:28, scroggo_chromium wrote: > or overflow. Done. https://codereview.chromium.org/2075093002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:321: if (!wouldOverflow) { On 2016/06/28 15:55:28, scroggo_chromium wrote: > I think you want > > if (wouldOverflow) > > ? (No "!") Done. https://codereview.chromium.org/2075093002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:336: if (wouldOverflow || totalMemoryUsage > m_maxDecodedBytes) { On 2016/06/28 15:55:28, scroggo_chromium wrote: > No need to check wouldOverflow here. It is guaranteed to be false, since this is > the else statement to if (wouldOverflow) Done.
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
On 2016/06/28 16:56:16, scroggo_chromium wrote: > lgtm Thanks for reviewing so diligently. I'm red in the face from some of my mistakes here. :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by cblume@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
boliu@chromium.org changed reviewers: + boliu@chromium.org
committer rubberstamp lgtm
The CQ bit was checked by cblume@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Don't cache GIF frames if it would be too large. The GIF decoder now respects m_maxDecodedBytes and checks for overflow. If the GIF would go over the memory limit then cached frames provide no benefit. By the time the animation loops we would have been forced to purge those frames. So caching any extra (not currently needed) frames would be a waste. In that case, run lean and purge aggressively. R=scroggo@chromium.org BUG=619173 ========== to ========== Don't cache GIF frames if it would be too large. The GIF decoder now respects m_maxDecodedBytes and checks for overflow. If the GIF would go over the memory limit then cached frames provide no benefit. By the time the animation loops we would have been forced to purge those frames. So caching any extra (not currently needed) frames would be a waste. In that case, run lean and purge aggressively. R=scroggo@chromium.org BUG=619173 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Don't cache GIF frames if it would be too large. The GIF decoder now respects m_maxDecodedBytes and checks for overflow. If the GIF would go over the memory limit then cached frames provide no benefit. By the time the animation loops we would have been forced to purge those frames. So caching any extra (not currently needed) frames would be a waste. In that case, run lean and purge aggressively. R=scroggo@chromium.org BUG=619173 ========== to ========== Don't cache GIF frames if it would be too large. The GIF decoder now respects m_maxDecodedBytes and checks for overflow. If the GIF would go over the memory limit then cached frames provide no benefit. By the time the animation loops we would have been forced to purge those frames. So caching any extra (not currently needed) frames would be a waste. In that case, run lean and purge aggressively. R=scroggo@chromium.org BUG=619173 Committed: https://crrev.com/9899eade00c556b8086b74867f02edf8c98cb4b9 Cr-Commit-Position: refs/heads/master@{#402536} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/9899eade00c556b8086b74867f02edf8c98cb4b9 Cr-Commit-Position: refs/heads/master@{#402536}
Message was sent while issue was closed.
On 2016/06/28 20:39:29, commit-bot: I haz the power wrote: > Patchset 5 (id:??) landed as > https://crrev.com/9899eade00c556b8086b74867f02edf8c98cb4b9 > Cr-Commit-Position: refs/heads/master@{#402536} I don’t know if it OK to post this to already closed review. Nice find and a good patch. Some input maybe for follow up patch… Discussion about linking this cache to cc cache makes a lot of sense since some of the frames in later loops could be already cached from the previous animation loops and in addition that would also remove memcpy (bitmap.copyPixelsTo) from ImageFrameDecoder::decodeAndScale [1] About purgeAggressively – we could try to do this for every animation and remove the calculation? GIFImageDecoder::clearCacheExceptFrame(size_t clearExceptFrame) keeps all the frames required for animation frames with higher index. Some discussion here with pkasting@ about it: https://codereview.chromium.org/1574283002 [1] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph...
Message was sent while issue was closed.
On 2016/06/28 21:22:31, aleksandar.stojiljkovic wrote: > On 2016/06/28 20:39:29, commit-bot: I haz the power wrote: > > Patchset 5 (id:??) landed as > > https://crrev.com/9899eade00c556b8086b74867f02edf8c98cb4b9 > > Cr-Commit-Position: refs/heads/master@{#402536} > > I don’t know if it OK to post this to already closed review. > > Nice find and a good patch. Some input maybe for follow up patch… > > Discussion about linking this cache to cc cache makes a lot of sense since some > of the frames in later loops could be already cached from the previous animation > loops and in addition that would also remove memcpy (bitmap.copyPixelsTo) from > ImageFrameDecoder::decodeAndScale [1] > > About purgeAggressively – we could try to do this for every animation and remove > the calculation? GIFImageDecoder::clearCacheExceptFrame(size_t clearExceptFrame) > keeps all the frames required for animation frames with higher index. Some > discussion here with pkasting@ about it: > https://codereview.chromium.org/1574283002 > > [1] > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph... It is always okay to discuss. :) We can also discuss over on the bug: https://bugs.chromium.org/p/chromium/issues/detail?id=619173 I agree 100% about linking the decoder's cache to CC's cache. In fact, we discussed that above in comment #11 https://codereview.chromium.org/2075093002/#msg11 That is a more major change, which will take more work. This is more of a for-now solution. When aggressively purging, it will eventually call clearCacheExceptFrame(). So we should still keep the frames that are needed. And it does this per-frame. So next frame, it calls clearCacheExceptFrame() again to allow clearCacheExceptFrame() to continue to hold onto the frames it thinks would be useful. This takes advantage of the wisdom inside clearCacheExceptFrame(). We actually discussed that as well in comment #13 https://codereview.chromium.org/2075093002/#msg13 Is that what you meant? I may not have understood well.
Message was sent while issue was closed.
On 2016/06/28 21:42:02, cblume wrote: > On 2016/06/28 21:22:31, aleksandar.stojiljkovic wrote: > > On 2016/06/28 20:39:29, commit-bot: I haz the power wrote: > > > Patchset 5 (id:??) landed as > > > https://crrev.com/9899eade00c556b8086b74867f02edf8c98cb4b9 > > > Cr-Commit-Position: refs/heads/master@{#402536} > > > > I don’t know if it OK to post this to already closed review. > > > > Nice find and a good patch. Some input maybe for follow up patch… > > > > Discussion about linking this cache to cc cache makes a lot of sense since > some > > of the frames in later loops could be already cached from the previous > animation > > loops and in addition that would also remove memcpy (bitmap.copyPixelsTo) from > > ImageFrameDecoder::decodeAndScale [1] > > > > About purgeAggressively – we could try to do this for every animation and > remove > > the calculation? GIFImageDecoder::clearCacheExceptFrame(size_t > clearExceptFrame) > > keeps all the frames required for animation frames with higher index. Some > > discussion here with pkasting@ about it: > > https://codereview.chromium.org/1574283002 > > > > [1] > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph... > > It is always okay to discuss. :) > We can also discuss over on the bug: > https://bugs.chromium.org/p/chromium/issues/detail?id=619173 > > I agree 100% about linking the decoder's cache to CC's cache. In fact, we > discussed that above in comment #11 > https://codereview.chromium.org/2075093002/#msg11 > That is a more major change, which will take more work. This is more of a > for-now solution. > > When aggressively purging, it will eventually call clearCacheExceptFrame(). So > we should still keep the frames that are needed. And it does this per-frame. So > next frame, it calls clearCacheExceptFrame() again to allow > clearCacheExceptFrame() to continue to hold onto the frames it thinks would be > useful. This takes advantage of the wisdom inside clearCacheExceptFrame(). We > actually discussed that as well in comment #13 > https://codereview.chromium.org/2075093002/#msg13 > > Is that what you meant? I may not have understood well. Yes, correct like you said in comment #13. After clearCacheExceptFrame() we still keep the frames (it keeps one or two depending on dispose method) that are needed - so it should be fine to call clearCacheExceptFrame() always after decoding a frame. I meant to replace code: if (m_purgeAggressively) clearCacheExceptFrame(*i); with just: clearCacheExceptFrame(*i);
Message was sent while issue was closed.
On 2016/06/28 22:00:30, aleksandar.stojiljkovic wrote: > On 2016/06/28 21:42:02, cblume wrote: > > On 2016/06/28 21:22:31, aleksandar.stojiljkovic wrote: > > > On 2016/06/28 20:39:29, commit-bot: I haz the power wrote: > > > > Patchset 5 (id:??) landed as > > > > https://crrev.com/9899eade00c556b8086b74867f02edf8c98cb4b9 > > > > Cr-Commit-Position: refs/heads/master@{#402536} > > > > > > I don’t know if it OK to post this to already closed review. > > > > > > Nice find and a good patch. Some input maybe for follow up patch… > > > > > > Discussion about linking this cache to cc cache makes a lot of sense since > > some > > > of the frames in later loops could be already cached from the previous > > animation > > > loops and in addition that would also remove memcpy (bitmap.copyPixelsTo) > from > > > ImageFrameDecoder::decodeAndScale [1] > > > > > > About purgeAggressively – we could try to do this for every animation and > > remove > > > the calculation? GIFImageDecoder::clearCacheExceptFrame(size_t > > clearExceptFrame) > > > keeps all the frames required for animation frames with higher index. Some > > > discussion here with pkasting@ about it: > > > https://codereview.chromium.org/1574283002 > > > > > > [1] > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph... > > > > It is always okay to discuss. :) > > We can also discuss over on the bug: > > https://bugs.chromium.org/p/chromium/issues/detail?id=619173 > > > > I agree 100% about linking the decoder's cache to CC's cache. In fact, we > > discussed that above in comment #11 > > https://codereview.chromium.org/2075093002/#msg11 > > That is a more major change, which will take more work. This is more of a > > for-now solution. > > > > When aggressively purging, it will eventually call clearCacheExceptFrame(). So > > we should still keep the frames that are needed. And it does this per-frame. > So > > next frame, it calls clearCacheExceptFrame() again to allow > > clearCacheExceptFrame() to continue to hold onto the frames it thinks would be > > useful. This takes advantage of the wisdom inside clearCacheExceptFrame(). We > > actually discussed that as well in comment #13 > > https://codereview.chromium.org/2075093002/#msg13 > > > > Is that what you meant? I may not have understood well. > > Yes, correct like you said in comment #13. After clearCacheExceptFrame() we > still keep the frames (it keeps one or two depending on dispose method) that are > needed - so it should be fine to call clearCacheExceptFrame() always after > decoding a frame. > > I meant to replace code: > if (m_purgeAggressively) > clearCacheExceptFrame(*i); > > with just: > clearCacheExceptFrame(*i); Ah. If the image is small, we can keep it in cache without taking much of a memory hit. I only really want to ignore the cache when we couldn't cache the whole image anyway. Right now, each image has its own cache limit so this goes back to the CC cache topic. It would be better if CC maintained a total cache limit for images, rather than a per-image cache limit. 1000 small images could actually use a lot of memory via their caches. But we'll address this when we consolidate caches. |