WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(26.28 KB, patch)
2012-11-29 19:37 PST
,
Hin-Chung Lam
no flags
Details
Formatted Diff
Diff
Patch
(26.35 KB, patch)
2012-12-03 21:01 PST
,
Hin-Chung Lam
no flags
Details
Formatted Diff
Diff
Patch
(26.38 KB, patch)
2012-12-03 21:03 PST
,
Hin-Chung Lam
no flags
Details
Formatted Diff
Diff
Patch for landing
(26.34 KB, patch)
2012-12-04 13:55 PST
,
Hin-Chung Lam
no flags
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Hin-Chung Lam
Comment 1
2012-11-27 22:09:38 PST
Created
attachment 176397
[details]
Patch
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
Created
attachment 176890
[details]
Patch
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
Created
attachment 177406
[details]
Patch
Hin-Chung Lam
Comment 11
2012-12-03 21:03:42 PST
Created
attachment 177408
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug