Bug 103480 – [chromium] Implement cache eviction logic in ImageDecodingStore

• Component: Images
• Status: RESOLVED FIXED

[Hin-Chung Lam]

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)

[Hin-Chung Lam]

Created attachment 176397 [details]
Patch

[James Robinson]

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]

Created attachment 176890 [details]
Patch

[Hin-Chung Lam]

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]

Any comments on this change? I have another one lining up already. :)

[Nat Duca]

Comment on attachment 176890 [details]
Patch

Lets size this to whatever we're using now (32?) Looks cool otherwise.

[Hin-Chung Lam]

This patch does size each SkBitmap to 32bits, it's the total that is 64bits.

[Nat Duca]

i meant max cache size... 32meg?

[Hin-Chung Lam]

Yeah okay.

[Hin-Chung Lam]

Created attachment 177406 [details]
Patch

[Hin-Chung Lam]

Created attachment 177408 [details]
Patch

[Hin-Chung Lam]

Updated to 32MB.

[James Robinson]

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]

Created attachment 177544 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 177544 [details]
Patch for landing

Clearing flags on attachment: 177544

Committed r136616: <http://trac.webkit.org/changeset/136616>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.