Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1514893003:
Create a hash table from id<-->key in SkImageFilter::CacheImpl (Closed)
DescriptionCreate a hash table from id<-->key in SkImageFilter::CacheImpl
There is memory leak in the SkImageFilter::Cache. There are two sources
of memory leak:
1. The cache filling up quickly.
2. A slow small leak that never stops.
This CL solves the first issue, which prevents the cache filling up quickly.
This CL creates a new hash table that index the
SkImageFilter::uniqueID to an array of keys, and with the existing
key<-->Value hash table, we can have SkImageFilters proactively
purge content derived cached content when destroyed.
BUG=489543
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1514893003
Committed: https://skia.googlesource.com/skia/+/f5d1f8dcc841516d7ea63c151b13059af40ca76d
Committed: https://skia.googlesource.com/skia/+/627769144d233b3abce5ee86cf315df61fa8dcd7
Patch Set 1 #Patch Set 2 : working code #
Total comments: 11
Patch Set 3 : switch to SkTHashMap #
Total comments: 12
Patch Set 4 : better var names #
Total comments: 8
Patch Set 5 : Remove KeyArray #
Total comments: 6
Patch Set 6 : fix minor issues #Patch Set 7 : remove const in purgeByImageFilterId argument list #
Total comments: 2
Patch Set 8 : clean up in ~CacheImpl #Messages
Total messages: 43 (14 generated)
Description was changed from ========== Create a hash table from id<-->key in SkImageFilter::CacheImpl There is memory leak somewhere in the SkImageFilter::Cache. This CL adds a hash table that index the SkImageFilter::uniqueID to key, and with the existing key<-->value hash table, we should be able to free the memory occupied by the value. BUG=489543 ========== to ========== Create a hash table from id<-->key in SkImageFilter::CacheImpl There is memory leak in the SkImageFilter::Cache. It appears that CacheImpl clears memory by the key<-->Value hash. The problem here is that the SkImageFilter has an uniqueID, and with the key<-->Value hash, it will only clean one Value object that is associated with that key using its uniqueID. This CL adds a hash table that index the SkImageFilter::uniqueID to an array of keys, and with the existing key<-->Value hash table, we should be able to free all the memories occupied by a set of Values that are associated with the set of keys. BUG=489543 ==========
xidachen@chromium.org changed reviewers: + junov@chromium.org, mtklein@google.com, reed@google.com
PTAL, needs some help please. https://codereview.chromium.org/1514893003/diff/20001/src/core/SkImageFilter.cpp File src/core/SkImageFilter.cpp (right): https://codereview.chromium.org/1514893003/diff/20001/src/core/SkImageFilter.... src/core/SkImageFilter.cpp:624: fIDLookup.add(arr); mtklein@: this patch effectively stops the memory leak. But it breaks chrome debug build. By looking at the stack trace, it appears that it crashes at this line which calls SkTDynamicHash::add(). It calls SkTDynamicHash::validate(), and at line 180 of SkTDynamicHash.h it crashes. I checked, line 180 is: SKTDYNAMICHASH_CHECK(this->find(GetKey(*fArray[i]))); I think it makes sure that this->find(GetKey(*fArray[i])) is not null. I don't know why my code gives null in here, could you give me some suggestions? Is it because the hash function of the structure "KeyArray" that I created is not correct?
https://codereview.chromium.org/1514893003/diff/20001/include/core/SkImageFil... File include/core/SkImageFilter.h (right): https://codereview.chromium.org/1514893003/diff/20001/include/core/SkImageFil... include/core/SkImageFilter.h:46: virtual void destroyKeysFromUniqueID(uint32_t) {} The important thing that this method destroys is not Keys, but rather cache entries. https://codereview.chromium.org/1514893003/diff/20001/src/core/SkImageFilter.cpp File src/core/SkImageFilter.cpp (right): https://codereview.chromium.org/1514893003/diff/20001/src/core/SkImageFilter.... src/core/SkImageFilter.cpp:618: if (KeyArray* kArr = fIDLookup.find(key.fUniqueID)) { 'k' prefix is for constants in skia, which is not the case here. https://codereview.chromium.org/1514893003/diff/20001/src/core/SkImageFilter.... src/core/SkImageFilter.cpp:622: KeyArray *arr = new KeyArray(key.fUniqueID); Style: the '*' goes before the space https://codereview.chromium.org/1514893003/diff/20001/src/core/SkImageFilter.... src/core/SkImageFilter.cpp:624: fIDLookup.add(arr); On 2015/12/11 12:04:22, xidachen wrote: > mtklein@: this patch effectively stops the memory leak. But it breaks chrome > debug build. By looking at the stack trace, it appears that it crashes at this > line which calls SkTDynamicHash::add(). It calls SkTDynamicHash::validate(), and > at line 180 of SkTDynamicHash.h it crashes. I checked, line 180 is: > SKTDYNAMICHASH_CHECK(this->find(GetKey(*fArray[i]))); > I think it makes sure that this->find(GetKey(*fArray[i])) is not null. I don't > know why my code gives null in here, could you give me some suggestions? Is it > because the hash function of the structure "KeyArray" that I created is not > correct? This is a check to make sure you are not adding a duplicate entry. I think the problem is that removeInternal does not remove entries from fIDLookup https://codereview.chromium.org/1514893003/diff/20001/src/core/SkImageFilter.... src/core/SkImageFilter.cpp:656: kArr->fKeyArray.reset(); this is unnecessary
I'm not quite sure I see yet how this could fix a memory leak. Isn't everything in the LRU? https://codereview.chromium.org/1514893003/diff/20001/src/core/SkImageFilter.cpp File src/core/SkImageFilter.cpp (right): https://codereview.chromium.org/1514893003/diff/20001/src/core/SkImageFilter.... src/core/SkImageFilter.cpp:624: fIDLookup.add(arr); On 2015/12/11 at 13:53:30, Justin Novosad wrote: > On 2015/12/11 12:04:22, xidachen wrote: > > mtklein@: this patch effectively stops the memory leak. But it breaks chrome > > debug build. By looking at the stack trace, it appears that it crashes at this > > line which calls SkTDynamicHash::add(). It calls SkTDynamicHash::validate(), and > > at line 180 of SkTDynamicHash.h it crashes. I checked, line 180 is: > > SKTDYNAMICHASH_CHECK(this->find(GetKey(*fArray[i]))); > > I think it makes sure that this->find(GetKey(*fArray[i])) is not null. I don't > > know why my code gives null in here, could you give me some suggestions? Is it > > because the hash function of the structure "KeyArray" that I created is not > > correct? > > This is a check to make sure you are not adding a duplicate entry. I think the problem is that removeInternal does not remove entries from fIDLookup I don't think it's the duplicate entry assert. If this assert fails, it means there is some data in the hash table that can no longer be looked up. Usually this means we've got a bad hash function or we mutated the Key while the object was in the table. The first thing I'd try is to switch your new hashtable to be a SkTHashMap from SkTHash.h. It's a bit less error-prone for this sort of thing because it stores its keys and values separately. It takes 3 template arguments, a Key type, a Value type, and a Hash functor type with an uint32_t operator()(const Key&).
On 2015/12/11 14:02:50, mtklein wrote: > I'm not quite sure I see yet how this could fix a memory leak. Isn't everything > in the LRU? > Hi Mike, My understanding of the leak is like this (could be wrong): the current hash table does key<-->value lookup. So when cleaning the cache, it has to find the exact key which requires more than just a uniqueID. I think what we want is that in the ~SkImageFilter(), we try to look for the set of keys (can be more than one) that have the same SkImageFilter::funiqueID, and find a set of Values that are associated with the set of keys, and remove those Values. I hope this makes sense.
On 2015/12/11 14:02:50, mtklein wrote: > I'm not quite sure I see yet how this could fix a memory leak. Isn't everything > in the LRU? The issue is not an actual memory leak, but it kind of behaves like one. What happens is that the cache fills up with stale data. So it is kind of looks like a memory leak that is capped by the cache capacity.
On 2015/12/11 at 14:18:26, junov wrote: > On 2015/12/11 14:02:50, mtklein wrote: > > I'm not quite sure I see yet how this could fix a memory leak. Isn't everything > > in the LRU? > > The issue is not an actual memory leak, but it kind of behaves like one. What happens is that the cache fills up with stale data. So it is kind of looks like a memory leak that is capped by the cache capacity. So, we three all have personally observed that there is a real leak, right? A slow one not capped by a budget? Given that we suspect this leak is related to this cache, and that this CL does not fix this leak, let's get very precise about what we're talking about here. Let me offer these terms: 1) memory leak 2) cache laziness Have I got it right that this CL makes the cache less lazy by purging some entries earlier than we do it today at head, but those entries have always been tracked properly and were always purgeable, so we're not affecting the leak at all?
Hi Mike, As you pointed out in the original bug: Back in May (#4-#12) we identified two apparent "leaks": - the cache filling up quickly - a real slow leak that never stops. This CL prevents the cache filling up quickly. For the slow leak that never stops, we have not yet identified the leak source.
On 2015/12/11 at 15:29:44, xidachen wrote: > Hi Mike, > > As you pointed out in the original bug: > > Back in May (#4-#12) we identified two apparent "leaks": > - the cache filling up quickly > - a real slow leak that never stops. > > This CL prevents the cache filling up quickly. For the slow leak that never stops, we have not yet identified the leak source. Right, so let's rewrite the description on the CL to reflect this.
Description was changed from ========== Create a hash table from id<-->key in SkImageFilter::CacheImpl There is memory leak in the SkImageFilter::Cache. It appears that CacheImpl clears memory by the key<-->Value hash. The problem here is that the SkImageFilter has an uniqueID, and with the key<-->Value hash, it will only clean one Value object that is associated with that key using its uniqueID. This CL adds a hash table that index the SkImageFilter::uniqueID to an array of keys, and with the existing key<-->Value hash table, we should be able to free all the memories occupied by a set of Values that are associated with the set of keys. BUG=489543 ========== to ========== Create a hash table from id<-->key in SkImageFilter::CacheImpl There is memory leak in the SkImageFilter::Cache. There are two sources of memory leak: 1. The cache filling up quickly. 2. A slow small leak that never stops. This CL solves the first issue, which prevents the cache filling up quickly. This CL creates a new hash table that index the SkImageFilter::uniqueID to an array of keys, and with the existing key<-->Value hash table, we should be able to free all the memories occupied by a set of Values that are associated with the set of keys. BUG=489543 ==========
Hi Mike, I have switched to SkTHashMap, and it works fine. I have tested the debug and it runs fine. Thank you for your suggestions. https://codereview.chromium.org/1514893003/diff/20001/src/core/SkImageFilter.cpp File src/core/SkImageFilter.cpp (right): https://codereview.chromium.org/1514893003/diff/20001/src/core/SkImageFilter.... src/core/SkImageFilter.cpp:618: if (KeyArray* kArr = fIDLookup.find(key.fUniqueID)) { On 2015/12/11 13:53:30, Justin Novosad wrote: > 'k' prefix is for constants in skia, which is not the case here. Done. https://codereview.chromium.org/1514893003/diff/20001/src/core/SkImageFilter.... src/core/SkImageFilter.cpp:622: KeyArray *arr = new KeyArray(key.fUniqueID); On 2015/12/11 13:53:30, Justin Novosad wrote: > Style: the '*' goes before the space Done. https://codereview.chromium.org/1514893003/diff/20001/src/core/SkImageFilter.... src/core/SkImageFilter.cpp:624: fIDLookup.add(arr); On 2015/12/11 14:02:50, mtklein wrote: > On 2015/12/11 at 13:53:30, Justin Novosad wrote: > > On 2015/12/11 12:04:22, xidachen wrote: > > > mtklein@: this patch effectively stops the memory leak. But it breaks chrome > > > debug build. By looking at the stack trace, it appears that it crashes at > this > > > line which calls SkTDynamicHash::add(). It calls SkTDynamicHash::validate(), > and > > > at line 180 of SkTDynamicHash.h it crashes. I checked, line 180 is: > > > SKTDYNAMICHASH_CHECK(this->find(GetKey(*fArray[i]))); > > > I think it makes sure that this->find(GetKey(*fArray[i])) is not null. I > don't > > > know why my code gives null in here, could you give me some suggestions? Is > it > > > because the hash function of the structure "KeyArray" that I created is not > > > correct? > > > > This is a check to make sure you are not adding a duplicate entry. I think > the problem is that removeInternal does not remove entries from fIDLookup > > I don't think it's the duplicate entry assert. If this assert fails, it means > there is some data in the hash table that can no longer be looked up. Usually > this means we've got a bad hash function or we mutated the Key while the object > was in the table. > > The first thing I'd try is to switch your new hashtable to be a SkTHashMap from > SkTHash.h. It's a bit less error-prone for this sort of thing because it stores > its keys and values separately. It takes 3 template arguments, a Key type, a > Value type, and a Hash functor type with an uint32_t operator()(const Key&). switching to SkTHashMap solves the problem, thank you so much Mike. https://codereview.chromium.org/1514893003/diff/20001/src/core/SkImageFilter.... src/core/SkImageFilter.cpp:656: kArr->fKeyArray.reset(); On 2015/12/11 13:53:30, Justin Novosad wrote: > this is unnecessary Done.
The hardest two things in computer programming: Cache invalidations, naming things, and off by one errors. I have a lot of nits about naming things here... https://codereview.chromium.org/1514893003/diff/40001/include/core/SkImageFil... File include/core/SkImageFilter.h (right): https://codereview.chromium.org/1514893003/diff/40001/include/core/SkImageFil... include/core/SkImageFilter.h:46: virtual void destroyKeysFromUniqueID(uint32_t) {} Find a better name for this function. Destroying keys is not what it is all about. suggestion: purgeByImageFilterId. https://codereview.chromium.org/1514893003/diff/40001/src/core/SkImageFilter.cpp File src/core/SkImageFilter.cpp (right): https://codereview.chromium.org/1514893003/diff/40001/src/core/SkImageFilter.... src/core/SkImageFilter.cpp:135: uint32_t fUniqueID; This name is a bit confusing. What exacly is uniquely identified? Suggestion: fImageFilterUniqueID https://codereview.chromium.org/1514893003/diff/40001/src/core/SkImageFilter.... src/core/SkImageFilter.cpp:136: SkTArray<Key> fKeyArray; This name is confising: Given there is a class named KeyArray, one might expect a member named fKeyArray to be of that class. Suggestion: fData (I know it's boring, welcome to find something more imaginative). https://codereview.chromium.org/1514893003/diff/40001/src/core/SkImageFilter.... src/core/SkImageFilter.cpp:613: if (KeyArray** arr = fIDLookup.find(key.fUniqueID)) { Abbreviated names are frowned upon. https://codereview.chromium.org/1514893003/diff/40001/src/core/SkImageFilter.... src/core/SkImageFilter.cpp:617: KeyArray* keyArr = new KeyArray(key.fUniqueID); abbreviated https://codereview.chromium.org/1514893003/diff/40001/src/core/SkImageFilter.... src/core/SkImageFilter.cpp:665: SkTHashMap<uint32_t, KeyArray*> fIDLookup; Suggestion: fIdToKeyMap
https://codereview.chromium.org/1514893003/diff/40001/include/core/SkImageFil... File include/core/SkImageFilter.h (right): https://codereview.chromium.org/1514893003/diff/40001/include/core/SkImageFil... include/core/SkImageFilter.h:46: virtual void destroyKeysFromUniqueID(uint32_t) {} On 2015/12/11 16:41:51, Justin Novosad wrote: > Find a better name for this function. Destroying keys is not what it is all > about. suggestion: purgeByImageFilterId. Done. https://codereview.chromium.org/1514893003/diff/40001/src/core/SkImageFilter.cpp File src/core/SkImageFilter.cpp (right): https://codereview.chromium.org/1514893003/diff/40001/src/core/SkImageFilter.... src/core/SkImageFilter.cpp:135: uint32_t fUniqueID; On 2015/12/11 16:41:51, Justin Novosad wrote: > This name is a bit confusing. What exacly is uniquely identified? Suggestion: > fImageFilterUniqueID Done. https://codereview.chromium.org/1514893003/diff/40001/src/core/SkImageFilter.... src/core/SkImageFilter.cpp:136: SkTArray<Key> fKeyArray; On 2015/12/11 16:41:51, Justin Novosad wrote: > This name is confising: Given there is a class named KeyArray, one might expect > a member named fKeyArray to be of that class. Suggestion: fData (I know it's > boring, welcome to find something more imaginative). Done. https://codereview.chromium.org/1514893003/diff/40001/src/core/SkImageFilter.... src/core/SkImageFilter.cpp:613: if (KeyArray** arr = fIDLookup.find(key.fUniqueID)) { On 2015/12/11 16:41:51, Justin Novosad wrote: > Abbreviated names are frowned upon. Done. https://codereview.chromium.org/1514893003/diff/40001/src/core/SkImageFilter.... src/core/SkImageFilter.cpp:617: KeyArray* keyArr = new KeyArray(key.fUniqueID); On 2015/12/11 16:41:51, Justin Novosad wrote: > abbreviated Done. https://codereview.chromium.org/1514893003/diff/40001/src/core/SkImageFilter.... src/core/SkImageFilter.cpp:665: SkTHashMap<uint32_t, KeyArray*> fIDLookup; On 2015/12/11 16:41:51, Justin Novosad wrote: > Suggestion: fIdToKeyMap Done.
mtklein@: gentle ping, could you take a look. Thank you.
Let's add a note something like this to the CL description: "Have SkImageFilters proactively purge content derived cached content when destroyed." ? https://codereview.chromium.org/1514893003/diff/60001/src/core/SkImageFilter.cpp File src/core/SkImageFilter.cpp (right): https://codereview.chromium.org/1514893003/diff/60001/src/core/SkImageFilter.... src/core/SkImageFilter.cpp:135: uint32_t fImageFilterUniqueID; It doesn't appear that fImageFilterUniqueID is used. I think that means KeyArray probably doesn't need to be a named concept. Why not do something like this? SkTHashMap<uint32_t, SkTArray<Key>> fIdToKeys; if that doesn't work, it should be fine to use SkTHashMap<uint32_t, SkTArray<Key>*> but I think having values as value-types should work fine with SkTHashMap. https://codereview.chromium.org/1514893003/diff/60001/src/core/SkImageFilter.... src/core/SkImageFilter.cpp:213: Cache::Get()->purgeByImageFilterId(fUniqueID); I think this means all image filters will try to remove themselves from the global (CPU-raster) image filter cache on destruction. That may be fine. Out of curiosity, is there a way to short-circuit this for image filters that never participated in that cache (e.g. GPU image filters)? It might be nice to avoid the mutex, lookup, etc, if we know a-priori we're not in there. https://codereview.chromium.org/1514893003/diff/60001/src/core/SkImageFilter.... src/core/SkImageFilter.cpp:646: for (int i = 0; i < (*array)->fData.count(); i++) { i think you can use for (auto&& key : (*array)->fData) here if you like. (Or auto&& key : *array if we can drop KeyArray.) https://codereview.chromium.org/1514893003/diff/60001/src/core/SkImageFilter.... src/core/SkImageFilter.cpp:648: removeInternal(v); we typically prefix method calls with this->
Description was changed from ========== Create a hash table from id<-->key in SkImageFilter::CacheImpl There is memory leak in the SkImageFilter::Cache. There are two sources of memory leak: 1. The cache filling up quickly. 2. A slow small leak that never stops. This CL solves the first issue, which prevents the cache filling up quickly. This CL creates a new hash table that index the SkImageFilter::uniqueID to an array of keys, and with the existing key<-->Value hash table, we should be able to free all the memories occupied by a set of Values that are associated with the set of keys. BUG=489543 ========== to ========== Create a hash table from id<-->key in SkImageFilter::CacheImpl There is memory leak in the SkImageFilter::Cache. There are two sources of memory leak: 1. The cache filling up quickly. 2. A slow small leak that never stops. This CL solves the first issue, which prevents the cache filling up quickly. This CL creates a new hash table that index the SkImageFilter::uniqueID to an array of keys, and with the existing key<-->Value hash table, we can have SkImageFilters proactively purge content derived cached content when destroyed. BUG=489543 ==========
Description was changed from ========== Create a hash table from id<-->key in SkImageFilter::CacheImpl There is memory leak in the SkImageFilter::Cache. There are two sources of memory leak: 1. The cache filling up quickly. 2. A slow small leak that never stops. This CL solves the first issue, which prevents the cache filling up quickly. This CL creates a new hash table that index the SkImageFilter::uniqueID to an array of keys, and with the existing key<-->Value hash table, we can have SkImageFilters proactively purge content derived cached content when destroyed. BUG=489543 ========== to ========== Create a hash table from id<-->key in SkImageFilter::CacheImpl There is memory leak in the SkImageFilter::Cache. There are two sources of memory leak: 1. The cache filling up quickly. 2. A slow small leak that never stops. This CL solves the first issue, which prevents the cache filling up quickly. This CL creates a new hash table that index the SkImageFilter::uniqueID to an array of keys, and with the existing key<-->Value hash table, we can have SkImageFilters proactively purge content derived cached content when destroyed. BUG=489543 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
https://codereview.chromium.org/1514893003/diff/60001/src/core/SkImageFilter.cpp File src/core/SkImageFilter.cpp (right): https://codereview.chromium.org/1514893003/diff/60001/src/core/SkImageFilter.... src/core/SkImageFilter.cpp:135: uint32_t fImageFilterUniqueID; On 2015/12/15 20:32:15, mtklein wrote: > It doesn't appear that fImageFilterUniqueID is used. I think that means > KeyArray probably doesn't need to be a named concept. > > Why not do something like this? > SkTHashMap<uint32_t, SkTArray<Key>> fIdToKeys; > > if that doesn't work, it should be fine to use > SkTHashMap<uint32_t, SkTArray<Key>*> > but I think having values as value-types should work fine with SkTHashMap. This makes perfect sense. This structure was here because previously I was using SkTDynamicHash which requires a structure to have hash function. Now that we use SkTHashMap, we can remove this. https://codereview.chromium.org/1514893003/diff/60001/src/core/SkImageFilter.... src/core/SkImageFilter.cpp:213: Cache::Get()->purgeByImageFilterId(fUniqueID); On 2015/12/15 20:32:15, mtklein wrote: > I think this means all image filters will try to remove themselves from the > global (CPU-raster) image filter cache on destruction. That may be fine. Out > of curiosity, is there a way to short-circuit this for image filters that never > participated in that cache (e.g. GPU image filters)? It might be nice to avoid > the mutex, lookup, etc, if we know a-priori we're not in there. Excellent suggestion. So if we check whether CacheImpl::fCurrentBytes is 0 or not, would that work? I mean, if that is 0, we ignore the call of this purge. https://codereview.chromium.org/1514893003/diff/60001/src/core/SkImageFilter.... src/core/SkImageFilter.cpp:646: for (int i = 0; i < (*array)->fData.count(); i++) { On 2015/12/15 20:32:15, mtklein wrote: > i think you can use for (auto&& key : (*array)->fData) here if you like. (Or > auto&& key : *array if we can drop KeyArray.) Done. https://codereview.chromium.org/1514893003/diff/60001/src/core/SkImageFilter.... src/core/SkImageFilter.cpp:648: removeInternal(v); On 2015/12/15 20:32:15, mtklein wrote: > we typically prefix method calls with this-> Done.
lgtm, with some small suggestions https://codereview.chromium.org/1514893003/diff/80001/src/core/SkImageFilter.cpp File src/core/SkImageFilter.cpp (right): https://codereview.chromium.org/1514893003/diff/80001/src/core/SkImageFilter.... src/core/SkImageFilter.cpp:205: Cache::Get()->purgeByImageFilterId(fUniqueID); I was thinking it'd be nice if we had a way for each image filter to know it had never participated in the global cache without having to check with the cache itself. I think what we've got here is fine. We can circle back to add a bit like that if we find it necessary for performance. https://codereview.chromium.org/1514893003/diff/80001/src/core/SkImageFilter.... src/core/SkImageFilter.cpp:607: } Usually we write this as '} else {' https://codereview.chromium.org/1514893003/diff/80001/src/core/SkImageFilter.... src/core/SkImageFilter.cpp:644: delete *array; Might be nice to add a reminder // This can be deleted outside the lock. in case we come back in here looking for performance knobs to tweak. https://codereview.chromium.org/1514893003/diff/80001/src/core/SkImageFilter.... src/core/SkImageFilter.cpp:656: SkTDynamicHash<Value, Key> fLookup; If you're feeling really tidy, we can line up all these field names at the 'f'.
The CQ bit was checked by xidachen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@google.com Link to the patchset: https://codereview.chromium.org/1514893003/#ps100001 (title: "fix minor issues")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1514893003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1514893003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: skia_presubmit-Trybot on client.skia.fyi (JOB_FAILED, http://build.chromium.org/p/client.skia.fyi/builders/skia_presubmit-Trybot/bu...)
reed1@: since this CL changes the SkImageFilter API, it needs your approval. PTAL. https://codereview.chromium.org/1514893003/diff/80001/src/core/SkImageFilter.cpp File src/core/SkImageFilter.cpp (right): https://codereview.chromium.org/1514893003/diff/80001/src/core/SkImageFilter.... src/core/SkImageFilter.cpp:607: } On 2015/12/16 13:54:32, mtklein wrote: > Usually we write this as '} else {' Done. https://codereview.chromium.org/1514893003/diff/80001/src/core/SkImageFilter.... src/core/SkImageFilter.cpp:644: delete *array; On 2015/12/16 13:54:32, mtklein wrote: > Might be nice to add a reminder > // This can be deleted outside the lock. > in case we come back in here looking for performance knobs to tweak. Done.
reed1@: gentle ping, could you take a look? This CL changes an API which requires approval from an API owner.
lgtm
The CQ bit was checked by xidachen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@google.com Link to the patchset: https://codereview.chromium.org/1514893003/#ps120001 (title: "remove const in purgeByImageFilterId argument list")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1514893003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1514893003/120001
Message was sent while issue was closed.
Description was changed from ========== Create a hash table from id<-->key in SkImageFilter::CacheImpl There is memory leak in the SkImageFilter::Cache. There are two sources of memory leak: 1. The cache filling up quickly. 2. A slow small leak that never stops. This CL solves the first issue, which prevents the cache filling up quickly. This CL creates a new hash table that index the SkImageFilter::uniqueID to an array of keys, and with the existing key<-->Value hash table, we can have SkImageFilters proactively purge content derived cached content when destroyed. BUG=489543 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Create a hash table from id<-->key in SkImageFilter::CacheImpl There is memory leak in the SkImageFilter::Cache. There are two sources of memory leak: 1. The cache filling up quickly. 2. A slow small leak that never stops. This CL solves the first issue, which prevents the cache filling up quickly. This CL creates a new hash table that index the SkImageFilter::uniqueID to an array of keys, and with the existing key<-->Value hash table, we can have SkImageFilters proactively purge content derived cached content when destroyed. BUG=489543 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/f5d1f8dcc841516d7ea63c151b13059af40ca76d ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://skia.googlesource.com/skia/+/f5d1f8dcc841516d7ea63c151b13059af40ca76d
Message was sent while issue was closed.
On 2015/12/15 20:32:16, mtklein wrote: > Let's add a note something like this to the CL description: > > "Have SkImageFilters proactively purge content derived cached content when > destroyed." > > ? > > https://codereview.chromium.org/1514893003/diff/60001/src/core/SkImageFilter.cpp > File src/core/SkImageFilter.cpp (right): > > https://codereview.chromium.org/1514893003/diff/60001/src/core/SkImageFilter.... > src/core/SkImageFilter.cpp:135: uint32_t fImageFilterUniqueID; > It doesn't appear that fImageFilterUniqueID is used. I think that means > KeyArray probably doesn't need to be a named concept. > > Why not do something like this? > SkTHashMap<uint32_t, SkTArray<Key>> fIdToKeys; > > if that doesn't work, it should be fine to use > SkTHashMap<uint32_t, SkTArray<Key>*> > but I think having values as value-types should work fine with SkTHashMap. > > https://codereview.chromium.org/1514893003/diff/60001/src/core/SkImageFilter.... > src/core/SkImageFilter.cpp:213: Cache::Get()->purgeByImageFilterId(fUniqueID); > I think this means all image filters will try to remove themselves from the > global (CPU-raster) image filter cache on destruction. That may be fine. Out > of curiosity, is there a way to short-circuit this for image filters that never > participated in that cache (e.g. GPU image filters)? It might be nice to avoid > the mutex, lookup, etc, if we know a-priori we're not in there. <driveby> Another way to fix this would be to have the SkImageFilter maintain a list of Keys (or Listeners, in an abstract sense), in the same way that SkPixelRef does with its GenIDChangeListeners, and notify them on destruction. Then you wouldn't need the hash table in Cache. That would probably require its own mutex for adding/removing entries from the list, but you could avoid locking it if the list is empty, so the GPU case would be cheaper. </driveby>
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.chromium.org/1537923002/ by reed@google.com. The reason for reverting is: speculative revert to try to unblock DEPS roll https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium....
Message was sent while issue was closed.
https://codereview.chromium.org/1514893003/diff/120001/src/core/SkImageFilter... File src/core/SkImageFilter.cpp (right): https://codereview.chromium.org/1514893003/diff/120001/src/core/SkImageFilter... src/core/SkImageFilter.cpp:570: } I think we missed cleanup, something like this: fIdToKeys.foreach([](uint32_t, SkTArray<Key>** array) { delete *array; });
Message was sent while issue was closed.
https://codereview.chromium.org/1514893003/diff/120001/src/core/SkImageFilter... File src/core/SkImageFilter.cpp (right): https://codereview.chromium.org/1514893003/diff/120001/src/core/SkImageFilter... src/core/SkImageFilter.cpp:570: } On 2015/12/18 14:43:22, mtklein wrote: > I think we missed cleanup, something like this: > fIdToKeys.foreach([](uint32_t, SkTArray<Key>** array) { delete *array; }); Done.
The CQ bit was checked by mtklein@google.com
lgtm
The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com Link to the patchset: https://codereview.chromium.org/1514893003/#ps140001 (title: "clean up in ~CacheImpl")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1514893003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1514893003/140001
Message was sent while issue was closed.
Description was changed from ========== Create a hash table from id<-->key in SkImageFilter::CacheImpl There is memory leak in the SkImageFilter::Cache. There are two sources of memory leak: 1. The cache filling up quickly. 2. A slow small leak that never stops. This CL solves the first issue, which prevents the cache filling up quickly. This CL creates a new hash table that index the SkImageFilter::uniqueID to an array of keys, and with the existing key<-->Value hash table, we can have SkImageFilters proactively purge content derived cached content when destroyed. BUG=489543 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/f5d1f8dcc841516d7ea63c151b13059af40ca76d ========== to ========== Create a hash table from id<-->key in SkImageFilter::CacheImpl There is memory leak in the SkImageFilter::Cache. There are two sources of memory leak: 1. The cache filling up quickly. 2. A slow small leak that never stops. This CL solves the first issue, which prevents the cache filling up quickly. This CL creates a new hash table that index the SkImageFilter::uniqueID to an array of keys, and with the existing key<-->Value hash table, we can have SkImageFilters proactively purge content derived cached content when destroyed. BUG=489543 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/f5d1f8dcc841516d7ea63c151b13059af40ca76d Committed: https://skia.googlesource.com/skia/+/627769144d233b3abce5ee86cf315df61fa8dcd7 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://skia.googlesource.com/skia/+/627769144d233b3abce5ee86cf315df61fa8dcd7 |
