RESOLVED FIXED 103480
[chromium] Implement cache eviction logic in ImageDecodingStore
https://bugs.webkit.org/show_bug.cgi?id=103480
Summary [chromium] Implement cache eviction logic in ImageDecodingStore
Hin-Chung Lam
Reported 2012-11-27 21:56:16 PST
Implement the following items: * A simple LRU eviction logic * Setting cache limit * Remove all associated cache entries when cache key is gone (i.e. ImageFrameGenerator)
Attachments
Patch (26.20 KB, patch)
2012-11-27 22:09 PST, Hin-Chung Lam
no flags
Patch (26.28 KB, patch)
2012-11-29 19:37 PST, Hin-Chung Lam
no flags
Patch (26.35 KB, patch)
2012-12-03 21:01 PST, Hin-Chung Lam
no flags
Patch (26.38 KB, patch)
2012-12-03 21:03 PST, Hin-Chung Lam
no flags
Patch for landing (26.34 KB, patch)
2012-12-04 13:55 PST, Hin-Chung Lam
no flags
Hin-Chung Lam
Comment 1 2012-11-27 22:09:38 PST
James Robinson
Comment 2 2012-11-29 18:16:45 PST
Comment on attachment 176397 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176397&action=review > Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:37 > +static const unsigned long long cDefaultCacheLimit = 8192 * 1024; size_t, if it's a number of bytes? is it a number of bytes? can you put the unit in the type name? WebKit style is to omit the 'c' prefix and just name this like a normal variable > Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:58 > + MutexLocker lock(m_mutex); a lock inside an #ifndef NDEBUG is suspicious (as is taking a lock in a destructor). the lifetime of this object is over as soon as the destructor starts running it, so if there are other threads possibly using this class during destruction we're in trouble > Source/WebCore/platform/graphics/chromium/ImageDecodingStore.h:72 > + friend class WTF::DoublyLinkedListNode<CacheEntry>; DoublyLinkedList.h line 191: using WTF::DoublyLinkedListNode; so no WTF:: here > Source/WebCore/platform/graphics/chromium/ImageDecodingStore.h:100 > + // NOTE: Returns size trunscated to a 32-bits integer. > + unsigned memoryUsage() const { return cachedImage()->bitmap().getSafeSize(); } memory use as a 32-bit integer doesn't really make sense if we can use more than 2g of memory (which would be reasonable, at least on my box with 64g of ram). think gigapxl or something like that. can you change this API? what's the unit here? > Source/WebCore/platform/graphics/chromium/ImageDecodingStore.h:137 > + unsigned long long m_cacheLimit; > + unsigned long long m_memoryUsage; what unit are these? use size_t for counting number of bytes
Hin-Chung Lam
Comment 3 2012-11-29 19:37:06 PST
Hin-Chung Lam
Comment 4 2012-11-29 19:39:33 PST
Comment on attachment 176397 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176397&action=review >> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:37 >> +static const unsigned long long cDefaultCacheLimit = 8192 * 1024; > > size_t, if it's a number of bytes? > > is it a number of bytes? can you put the unit in the type name? > > WebKit style is to omit the 'c' prefix and just name this like a normal variable Both done. It's all size_t and no c. >> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:58 >> + MutexLocker lock(m_mutex); > > a lock inside an #ifndef NDEBUG is suspicious (as is taking a lock in a destructor). the lifetime of this object is over as soon as the destructor starts running it, so if there are other threads possibly using this class during destruction we're in trouble I agree. The lock is not necessary. I'll remove it. >> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.h:72 >> + friend class WTF::DoublyLinkedListNode<CacheEntry>; > > DoublyLinkedList.h line 191: > using WTF::DoublyLinkedListNode; > > so no WTF:: here okay. >> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.h:100 >> + unsigned memoryUsage() const { return cachedImage()->bitmap().getSafeSize(); } > > memory use as a 32-bit integer doesn't really make sense if we can use more than 2g of memory (which would be reasonable, at least on my box with 64g of ram). think gigapxl or something like that. can you change this API? > > what's the unit here? Done. However getSafeSize() still returns only 32-bits. It needs a build flag change in chromium to return long long. Would like to do it separately. >> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.h:137 >> + unsigned long long m_memoryUsage; > > what unit are these? > > use size_t for counting number of bytes Done.
Hin-Chung Lam
Comment 5 2012-12-03 18:06:22 PST
Any comments on this change? I have another one lining up already. :)
Nat Duca
Comment 6 2012-12-03 20:19:48 PST
Comment on attachment 176890 [details] Patch Lets size this to whatever we're using now (32?) Looks cool otherwise.
Hin-Chung Lam
Comment 7 2012-12-03 20:46:25 PST
This patch does size each SkBitmap to 32bits, it's the total that is 64bits.
Nat Duca
Comment 8 2012-12-03 20:57:49 PST
i meant max cache size... 32meg?
Hin-Chung Lam
Comment 9 2012-12-03 20:58:30 PST
Yeah okay.
Hin-Chung Lam
Comment 10 2012-12-03 21:01:40 PST
Hin-Chung Lam
Comment 11 2012-12-03 21:03:42 PST
Hin-Chung Lam
Comment 12 2012-12-03 21:04:10 PST
Updated to 32MB.
James Robinson
Comment 13 2012-12-03 22:40:08 PST
Comment on attachment 177408 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177408&action=review > Source/WebCore/ChangeLog:10 > + - Setting cache limit (default is 8MB) this needs an update > Source/WebCore/platform/graphics/chromium/ImageDecodingStore.h:99 > + // FIXME: getSafeSize() returns size in bytes trunscated to a 32-bits integer. typo trunscated -> truncated
Hin-Chung Lam
Comment 14 2012-12-04 13:55:14 PST
Created attachment 177544 [details] Patch for landing
WebKit Review Bot
Comment 15 2012-12-04 20:16:25 PST
Comment on attachment 177544 [details] Patch for landing Clearing flags on attachment: 177544 Committed r136616: <http://trac.webkit.org/changeset/136616>
WebKit Review Bot
Comment 16 2012-12-04 20:16:29 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.