Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
364193004:
Reopened: Caching the result of readPixelsSupported (Closed)
|
Created:
6 years, 5 months ago by Rémi Piotaix Modified:
6 years, 2 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Project:
skia Visibility:
Public. |
DescriptionCaching the result of readPixelsSupported
The call was calling GR_GL_GetIntegerv 2 times for each readPixels
and thus was causing a loss of performance
(resubmit of issue 344793008)
Benchmark url: http://packages.gkny.fr/tst/index.html
BUG=skia:2681
Committed: https://skia.googlesource.com/skia/+/753a2964afe5661ce9b2a8ca77ca9d0aabd3173c
Committed: https://skia.googlesource.com/skia/+/8339371f1ec3c57a0741932fd96bff32c53d4e54
Committed: https://skia.googlesource.com/skia/+/e4b231428e8c14cbc82d20cfb12eb08fc45f8df6
Patch Set 1 #
Total comments: 4
Patch Set 2 : Reimplemented caching system #Patch Set 3 : Fixed bug #
Total comments: 9
Patch Set 4 : Corrections #
Total comments: 25
Patch Set 5 : Correcting code as suggested in the comments #
Total comments: 2
Patch Set 6 : Using SkTDynamicHash::Iter #Patch Set 7 : Adding tests for SkTHashCache #Patch Set 8 : Error in the test... #Patch Set 9 : Rebase (merge) to master #
Total comments: 14
Patch Set 10 : Correcting code as suggested in comments #
Total comments: 8
Patch Set 11 : Again #Patch Set 12 : Typo in one filename #Patch Set 13 : Rebase master #Patch Set 14 : Rebase master #Patch Set 15 : Correcting build #Patch Set 16 : Rebase master #Patch Set 17 : Again #Patch Set 18 : Fix bug #
Messages
Total messages: 45 (3 generated)
PTAL (again)
On 2014/07/03 13:36:00, Rémi Piotaix wrote: > PTAL (again) lgtm
The CQ bit was checked by junov@chromium.org
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/piotaixr@chromium.org/364193004/1
Message was sent while issue was closed.
Change committed as 753a2964afe5661ce9b2a8ca77ca9d0aabd3173c
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/367323003/ by reed@chromium.org. The reason for reverting is: appears to crash GM on Ubuntu and Win8 http://108.170.220.120:10117/builders/Test-Ubuntu12-ShuttleA-GTX660-x86-Relea....
Message was sent while issue was closed.
https://codereview.chromium.org/364193004/diff/1/src/gpu/gl/GrGLCaps.cpp File src/gpu/gl/GrGLCaps.cpp (right): https://codereview.chromium.org/364193004/diff/1/src/gpu/gl/GrGLCaps.cpp#newc... src/gpu/gl/GrGLCaps.cpp:582: if (cachedValue == NULL) { Skia style: if (NULL == cachedValue) https://codereview.chromium.org/364193004/diff/1/src/gpu/gl/GrGLCaps.cpp#newc... src/gpu/gl/GrGLCaps.cpp:584: cachedValue = new ReadPixelsSupportedFormats(key, value); This is leaked.
Message was sent while issue was closed.
Still breaking tests ... https://codereview.chromium.org/364193004/diff/1/src/gpu/gl/GrGLCaps.cpp File src/gpu/gl/GrGLCaps.cpp (right): https://codereview.chromium.org/364193004/diff/1/src/gpu/gl/GrGLCaps.cpp#newc... src/gpu/gl/GrGLCaps.cpp:582: if (cachedValue == NULL) { On 2014/07/03 15:43:51, junov wrote: > Skia style: if (NULL == cachedValue) Done. https://codereview.chromium.org/364193004/diff/1/src/gpu/gl/GrGLCaps.cpp#newc... src/gpu/gl/GrGLCaps.cpp:584: cachedValue = new ReadPixelsSupportedFormats(key, value); On 2014/07/03 15:43:51, junov wrote: > This is leaked. Done.
Comparison from the first patchset: - The bug was in CrGpuGl:188 Instead of using the parameter "SurfaceConfig", i was getting the fbo format from fHWBoundRenderTarget which can be null. - There was a memory leak in the cache mechanism. To fix that, i created a new class 'DynamicHashCache' which holds the values in a SkTArray and uses the Dictionary for searches. Do you think it can be useful to tweak a little more the DynamicHashCache class to be able to use it in other parts of the code?
https://codereview.chromium.org/364193004/diff/40001/src/gpu/gl/GrGLCaps.cpp File src/gpu/gl/GrGLCaps.cpp (right): https://codereview.chromium.org/364193004/diff/40001/src/gpu/gl/GrGLCaps.cpp#... src/gpu/gl/GrGLCaps.cpp:586: ReadPixelsSupportedFormats newElement = ReadPixelsSupportedFormats(key, value); Should be: ReadPixelsSupportedFormats newElement(key, value); https://codereview.chromium.org/364193004/diff/40001/src/gpu/gl/GrGLCaps.h File src/gpu/gl/GrGLCaps.h (right): https://codereview.chromium.org/364193004/diff/40001/src/gpu/gl/GrGLCaps.h#ne... src/gpu/gl/GrGLCaps.h:409: DynamicHashCache(): fDict(), fValues() { } No need to explicitly call default constructors https://codereview.chromium.org/364193004/diff/40001/src/gpu/gl/GrGLCaps.h#ne... src/gpu/gl/GrGLCaps.h:437: while(fValues.count()) { Looping should not be necessary here. SkTDynamicHash and SkTArray can just be reset. https://codereview.chromium.org/364193004/diff/40001/src/gpu/gl/GrGLCaps.h#ne... src/gpu/gl/GrGLCaps.h:450: mutable DynamicHashCache<ReadPixelsSupportedFormats, ReadPixelsSupportedFormatsKey> fReadPixelsSupportedCache; Why doe this need to be mutable?
https://codereview.chromium.org/364193004/diff/40001/src/gpu/gl/GrGLCaps.cpp File src/gpu/gl/GrGLCaps.cpp (right): https://codereview.chromium.org/364193004/diff/40001/src/gpu/gl/GrGLCaps.cpp#... src/gpu/gl/GrGLCaps.cpp:586: ReadPixelsSupportedFormats newElement = ReadPixelsSupportedFormats(key, value); On 2014/07/03 18:57:44, junov wrote: > Should be: ReadPixelsSupportedFormats newElement(key, value); Done. https://codereview.chromium.org/364193004/diff/40001/src/gpu/gl/GrGLCaps.h File src/gpu/gl/GrGLCaps.h (right): https://codereview.chromium.org/364193004/diff/40001/src/gpu/gl/GrGLCaps.h#ne... src/gpu/gl/GrGLCaps.h:409: DynamicHashCache(): fDict(), fValues() { } On 2014/07/03 18:57:44, junov wrote: > No need to explicitly call default constructors Done. https://codereview.chromium.org/364193004/diff/40001/src/gpu/gl/GrGLCaps.h#ne... src/gpu/gl/GrGLCaps.h:437: while(fValues.count()) { On 2014/07/03 18:57:44, junov wrote: > Looping should not be necessary here. SkTDynamicHash and SkTArray can just be > reset. Done. https://codereview.chromium.org/364193004/diff/40001/src/gpu/gl/GrGLCaps.h#ne... src/gpu/gl/GrGLCaps.h:450: mutable DynamicHashCache<ReadPixelsSupportedFormats, ReadPixelsSupportedFormatsKey> fReadPixelsSupportedCache; On 2014/07/03 18:57:44, junov wrote: > Why doe this need to be mutable? Because readPixelsSupported() is const and DynamicHashCache::put() is not.
https://codereview.chromium.org/364193004/diff/40001/src/gpu/gl/GrGLCaps.h File src/gpu/gl/GrGLCaps.h (right): https://codereview.chromium.org/364193004/diff/40001/src/gpu/gl/GrGLCaps.h#ne... src/gpu/gl/GrGLCaps.h:450: mutable DynamicHashCache<ReadPixelsSupportedFormats, ReadPixelsSupportedFormatsKey> fReadPixelsSupportedCache; On 2014/07/03 20:38:27, Rémi Piotaix wrote: > On 2014/07/03 18:57:44, junov wrote: > > Why doe this need to be mutable? > > Because readPixelsSupported() is const and DynamicHashCache::put() is not. Ok. I guess cached results are usually an acceptable use for mutables. https://codereview.chromium.org/364193004/diff/60001/src/gpu/gl/GrGLCaps.h File src/gpu/gl/GrGLCaps.h (right): https://codereview.chromium.org/364193004/diff/60001/src/gpu/gl/GrGLCaps.h#ne... src/gpu/gl/GrGLCaps.h:407: class DynamicHashCache { This seems like a generally useful wrapper for SkTDynamicHash, not to mention that it is generic. I think it would make sense to move it to its own header file. Not sure where it should go though. Seems like a good fit for src/core, but nothing in core depends on it so it could reside in src/gpu. Mike?
https://codereview.chromium.org/364193004/diff/60001/src/gpu/gl/GrGLCaps.cpp File src/gpu/gl/GrGLCaps.cpp (right): https://codereview.chromium.org/364193004/diff/60001/src/gpu/gl/GrGLCaps.cpp#... src/gpu/gl/GrGLCaps.cpp:59: GrGLCaps& GrGLCaps::operator= (const GrGLCaps& caps) { Shouldn't we copy the cache here? It's not necessary but I don't see a good reason not to. https://codereview.chromium.org/364193004/diff/60001/src/gpu/gl/GrGLCaps.cpp#... src/gpu/gl/GrGLCaps.cpp:547: GrGLenum format, can you realign the params? https://codereview.chromium.org/364193004/diff/60001/src/gpu/gl/GrGLCaps.h File src/gpu/gl/GrGLCaps.h (right): https://codereview.chromium.org/364193004/diff/60001/src/gpu/gl/GrGLCaps.h#ne... src/gpu/gl/GrGLCaps.h:407: class DynamicHashCache { On 2014/07/04 14:42:27, junov wrote: > This seems like a generally useful wrapper for SkTDynamicHash, not to mention > that it is generic. I think it would make sense to move it to its own header > file. Not sure where it should go though. Seems like a good fit for src/core, > but nothing in core depends on it so it could reside in src/gpu. Mike? Adding the other Mike WRT this comment.
https://codereview.chromium.org/364193004/diff/60001/src/gpu/gl/GrGLCaps.h File src/gpu/gl/GrGLCaps.h (right): https://codereview.chromium.org/364193004/diff/60001/src/gpu/gl/GrGLCaps.h#ne... src/gpu/gl/GrGLCaps.h:407: class DynamicHashCache { On 2014/07/07 13:31:10, bsalomon wrote: > On 2014/07/04 14:42:27, junov wrote: > > This seems like a generally useful wrapper for SkTDynamicHash, not to mention > > that it is generic. I think it would make sense to move it to its own header > > file. Not sure where it should go though. Seems like a good fit for src/core, > > but nothing in core depends on it so it could reside in src/gpu. Mike? > > Adding the other Mike WRT this comment. Yup, does sound useful to me too. We seem to have managed to keep SkTDynamicHash itself in src/core so far, so maybe just put it right in SkTDynamicHash.h? https://codereview.chromium.org/364193004/diff/60001/src/gpu/gl/GrGLCaps.h#ne... src/gpu/gl/GrGLCaps.h:407: class DynamicHashCache { If possible, class DynamicHashCache : SkNoncopyable { ? https://codereview.chromium.org/364193004/diff/60001/src/gpu/gl/GrGLCaps.h#ne... src/gpu/gl/GrGLCaps.h:410: fDict = new SkTDynamicHash<T, Key, Traits, kGrowPercent>(); fDict = SkNEW(SkTDynamicHash<T, Key, Traits, kGrowPercent>); or, even better, use SkAutoTDelete<SkTDynamicHash<...> > and call fDict.reset(...) here? https://codereview.chromium.org/364193004/diff/60001/src/gpu/gl/GrGLCaps.h#ne... src/gpu/gl/GrGLCaps.h:413: virtual ~DynamicHashCache() { Don't make this virtual? Nothing else is virtual. https://codereview.chromium.org/364193004/diff/60001/src/gpu/gl/GrGLCaps.h#ne... src/gpu/gl/GrGLCaps.h:417: bool has(Key& key) const { Can any of these parameters be passed as const Key& / const T& ? https://codereview.chromium.org/364193004/diff/60001/src/gpu/gl/GrGLCaps.h#ne... src/gpu/gl/GrGLCaps.h:418: return NULL != find(key); return NULL != this->find(key); https://codereview.chromium.org/364193004/diff/60001/src/gpu/gl/GrGLCaps.h#ne... src/gpu/gl/GrGLCaps.h:428: if(has(key)) if (this->has(key)) { return *this->find(key); } https://codereview.chromium.org/364193004/diff/60001/src/gpu/gl/GrGLCaps.h#ne... src/gpu/gl/GrGLCaps.h:429: return *find(key); Does this all optimize away into one call to find()? I'd have a hard time seeing that. Maybe if (T* val = this->find(key)) { return *val; } ? https://codereview.chromium.org/364193004/diff/60001/src/gpu/gl/GrGLCaps.h#ne... src/gpu/gl/GrGLCaps.h:433: fDict->add(&e); Aren't these pointers invalidated when fValues resizes? https://codereview.chromium.org/364193004/diff/60001/src/gpu/gl/GrGLCaps.h#ne... src/gpu/gl/GrGLCaps.h:445: fDict = new SkTDynamicHash<T, Key, Traits, kGrowPercent>(); Seems like we could make fDict a non-pointer member if SkTDynamicHash gained a clear() method? I'm up for that.
https://codereview.chromium.org/364193004/diff/60001/src/gpu/gl/GrGLCaps.cpp File src/gpu/gl/GrGLCaps.cpp (right): https://codereview.chromium.org/364193004/diff/60001/src/gpu/gl/GrGLCaps.cpp#... src/gpu/gl/GrGLCaps.cpp:59: GrGLCaps& GrGLCaps::operator= (const GrGLCaps& caps) { On 2014/07/07 13:31:10, bsalomon wrote: > Shouldn't we copy the cache here? It's not necessary but I don't see a good > reason not to. Since we 'have to' use pointers in the array to store the values, i think it would be easier to make the Cache Noncopyable as mtklein suggests. https://codereview.chromium.org/364193004/diff/60001/src/gpu/gl/GrGLCaps.cpp#... src/gpu/gl/GrGLCaps.cpp:547: GrGLenum format, On 2014/07/07 13:31:10, bsalomon wrote: > can you realign the params? Done. https://codereview.chromium.org/364193004/diff/60001/src/gpu/gl/GrGLCaps.h File src/gpu/gl/GrGLCaps.h (right): https://codereview.chromium.org/364193004/diff/60001/src/gpu/gl/GrGLCaps.h#ne... src/gpu/gl/GrGLCaps.h:407: class DynamicHashCache { On 2014/07/07 13:44:30, mtklein wrote: > If possible, class DynamicHashCache : SkNoncopyable { ? Done. https://codereview.chromium.org/364193004/diff/60001/src/gpu/gl/GrGLCaps.h#ne... src/gpu/gl/GrGLCaps.h:407: class DynamicHashCache { On 2014/07/07 13:44:30, mtklein wrote: > On 2014/07/07 13:31:10, bsalomon wrote: > > On 2014/07/04 14:42:27, junov wrote: > > > This seems like a generally useful wrapper for SkTDynamicHash, not to > mention > > > that it is generic. I think it would make sense to move it to its own header > > > file. Not sure where it should go though. Seems like a good fit for > src/core, > > > but nothing in core depends on it so it could reside in src/gpu. Mike? > > > > Adding the other Mike WRT this comment. > > Yup, does sound useful to me too. We seem to have managed to keep > SkTDynamicHash itself in src/core so far, so maybe just put it right in > SkTDynamicHash.h? Done. https://codereview.chromium.org/364193004/diff/60001/src/gpu/gl/GrGLCaps.h#ne... src/gpu/gl/GrGLCaps.h:410: fDict = new SkTDynamicHash<T, Key, Traits, kGrowPercent>(); On 2014/07/07 13:44:30, mtklein wrote: > fDict = SkNEW(SkTDynamicHash<T, Key, Traits, kGrowPercent>); > > or, even better, use SkAutoTDelete<SkTDynamicHash<...> > and call > fDict.reset(...) here? Done. https://codereview.chromium.org/364193004/diff/60001/src/gpu/gl/GrGLCaps.h#ne... src/gpu/gl/GrGLCaps.h:413: virtual ~DynamicHashCache() { On 2014/07/07 13:44:30, mtklein wrote: > Don't make this virtual? Nothing else is virtual. Done. https://codereview.chromium.org/364193004/diff/60001/src/gpu/gl/GrGLCaps.h#ne... src/gpu/gl/GrGLCaps.h:417: bool has(Key& key) const { On 2014/07/07 13:44:30, mtklein wrote: > Can any of these parameters be passed as const Key& / const T& ? Done. https://codereview.chromium.org/364193004/diff/60001/src/gpu/gl/GrGLCaps.h#ne... src/gpu/gl/GrGLCaps.h:418: return NULL != find(key); On 2014/07/07 13:44:30, mtklein wrote: > return NULL != this->find(key); Done. https://codereview.chromium.org/364193004/diff/60001/src/gpu/gl/GrGLCaps.h#ne... src/gpu/gl/GrGLCaps.h:428: if(has(key)) On 2014/07/07 13:44:30, mtklein wrote: > if (this->has(key)) { > return *this->find(key); > } Done. https://codereview.chromium.org/364193004/diff/60001/src/gpu/gl/GrGLCaps.h#ne... src/gpu/gl/GrGLCaps.h:429: return *find(key); On 2014/07/07 13:44:30, mtklein wrote: > Does this all optimize away into one call to find()? I'd have a hard time > seeing that. > > Maybe > > if (T* val = this->find(key)) { > return *val; > } > > ? Done. https://codereview.chromium.org/364193004/diff/60001/src/gpu/gl/GrGLCaps.h#ne... src/gpu/gl/GrGLCaps.h:433: fDict->add(&e); On 2014/07/07 13:44:30, mtklein wrote: > Aren't these pointers invalidated when fValues resizes? Yes, you're right. I've corrected this in the new patchset. fValues now holds pointers so the objects addresses don't change when the array is resized. https://codereview.chromium.org/364193004/diff/80001/src/core/SkTHashCache.h File src/core/SkTHashCache.h (right): https://codereview.chromium.org/364193004/diff/80001/src/core/SkTHashCache.h#... src/core/SkTHashCache.h:71: SkTDArray<T*> fValues; Note that the use of an Array here is necessary because there is no way to iterate over the values in the DynamicHash (to be able to free all the pointers memory). Maybe add a way to iterate over the elements in addition to a 'clear' or 'reset' method?
https://codereview.chromium.org/364193004/diff/80001/src/core/SkTHashCache.h File src/core/SkTHashCache.h (right): https://codereview.chromium.org/364193004/diff/80001/src/core/SkTHashCache.h#... src/core/SkTHashCache.h:71: SkTDArray<T*> fValues; On 2014/07/07 18:10:17, Rémi Piotaix wrote: > Note that the use of an Array here is necessary because there is no way to > iterate over the values in the DynamicHash (to be able to free all the pointers > memory). > > Maybe add a way to iterate over the elements in addition to a 'clear' or 'reset' > method? Ah, interesting. Going a step further, is the only reason we iterate over the elements to delete them? I don't mind exposing an iterator from SkTDynamicHash (obviously as long as order doesn't matter), and I'd be even happier to restrict this as much as possible, to say, a visitAll() method that calls a functor on each element in the hash in unspecified order. If we add clear() and that visitAll(), no need for SkHashCache at all right?
On 2014/07/07 18:33:27, mtklein wrote: > https://codereview.chromium.org/364193004/diff/80001/src/core/SkTHashCache.h > File src/core/SkTHashCache.h (right): > > https://codereview.chromium.org/364193004/diff/80001/src/core/SkTHashCache.h#... > src/core/SkTHashCache.h:71: SkTDArray<T*> fValues; > On 2014/07/07 18:10:17, Rémi Piotaix wrote: > > Note that the use of an Array here is necessary because there is no way to > > iterate over the values in the DynamicHash (to be able to free all the > pointers > > memory). > > > > Maybe add a way to iterate over the elements in addition to a 'clear' or > 'reset' > > method? > > Ah, interesting. Going a step further, is the only reason we iterate over the > elements to delete them? I don't mind exposing an iterator from SkTDynamicHash > (obviously as long as order doesn't matter), and I'd be even happier to restrict > this as much as possible, to say, a visitAll() method that calls a functor on > each element in the hash in unspecified order. > > If we add clear() and that visitAll(), no need for SkHashCache at all right? Yes, I think so...
On 2014/07/07 18:36:48, Rémi Piotaix wrote: > On 2014/07/07 18:33:27, mtklein wrote: > > https://codereview.chromium.org/364193004/diff/80001/src/core/SkTHashCache.h > > File src/core/SkTHashCache.h (right): > > > > > https://codereview.chromium.org/364193004/diff/80001/src/core/SkTHashCache.h#... > > src/core/SkTHashCache.h:71: SkTDArray<T*> fValues; > > On 2014/07/07 18:10:17, Rémi Piotaix wrote: > > > Note that the use of an Array here is necessary because there is no way to > > > iterate over the values in the DynamicHash (to be able to free all the > > pointers > > > memory). > > > > > > Maybe add a way to iterate over the elements in addition to a 'clear' or > > 'reset' > > > method? > > > > Ah, interesting. Going a step further, is the only reason we iterate over the > > elements to delete them? I don't mind exposing an iterator from > SkTDynamicHash > > (obviously as long as order doesn't matter), and I'd be even happier to > restrict > > this as much as possible, to say, a visitAll() method that calls a functor on > > each element in the hash in unspecified order. > > > > If we add clear() and that visitAll(), no need for SkHashCache at all right? > > Yes, I think so... How about an unrefAll method?
> How about an unrefAll method? Yeah, users can make unrefAll(), deleteAll(), refAll(), whatever out of a generic visitAll(). It's all template code anyway, so we might as well have the thing take an arbitrary functor.
On 2014/07/07 18:44:11, mtklein wrote: > > How about an unrefAll method? > > Yeah, users can make unrefAll(), deleteAll(), refAll(), whatever out of a > generic visitAll(). It's all template code anyway, so we might as well have > the thing take an arbitrary functor. Aha... bad joke: I don't know why i didn't see it before: there IS a class (SkTDynamicHash::Iter) that allow us to iterate over the Dict. ( Im so dumb ). So, no need for visitAll or clear/reset.
On 2014/07/07 20:33:50, Rémi Piotaix wrote: > On 2014/07/07 18:44:11, mtklein wrote: > > > How about an unrefAll method? > > > > Yeah, users can make unrefAll(), deleteAll(), refAll(), whatever out of a > > generic visitAll(). It's all template code anyway, so we might as well have > > the thing take an arbitrary functor. > > Aha... bad joke: I don't know why i didn't see it before: there IS a class > (SkTDynamicHash::Iter) that allow us to iterate over the Dict. ( Im so dumb ). > > So, no need for visitAll or clear/reset. New patch, tests green :-)
https://codereview.chromium.org/364193004/diff/160001/src/core/SkTHashCache.h File src/core/SkTHashCache.h (right): https://codereview.chromium.org/364193004/diff/160001/src/core/SkTHashCache.h... src/core/SkTHashCache.h:22: SkTHashCache() { Consider this: SkTHashCache { this->reset(); } ~SkTHashCache { this->clear(); } void reset() { this->clear(); fDict.reset(SkNEW(DictType)); } private: void clear() { if (fDict.get()) { .... } } https://codereview.chromium.org/364193004/diff/160001/src/core/SkTHashCache.h... src/core/SkTHashCache.h:27: fValues.deleteAll(); Looks like we leak the last values? https://codereview.chromium.org/364193004/diff/160001/src/core/SkTHashCache.h... src/core/SkTHashCache.h:41: T& add(T& add) { Looks like argument to add can be const&? https://codereview.chromium.org/364193004/diff/160001/src/core/SkTHashCache.h... src/core/SkTHashCache.h:43: if (T * val = this->find(key)) { stray space before *? https://codereview.chromium.org/364193004/diff/160001/src/core/SkTHashCache.h... src/core/SkTHashCache.h:47: T* element = new T(add); SkNEW https://codereview.chromium.org/364193004/diff/160001/src/core/SkTHashCache.h... src/core/SkTHashCache.h:63: delete &(*it); SkDELETE https://codereview.chromium.org/364193004/diff/160001/src/gpu/gl/GrGLCaps.h File src/gpu/gl/GrGLCaps.h (right): https://codereview.chromium.org/364193004/diff/160001/src/gpu/gl/GrGLCaps.h#n... src/gpu/gl/GrGLCaps.h:15: #include "GrTHashTable.h" Do you still need this?
https://codereview.chromium.org/364193004/diff/160001/src/core/SkTHashCache.h File src/core/SkTHashCache.h (right): https://codereview.chromium.org/364193004/diff/160001/src/core/SkTHashCache.h... src/core/SkTHashCache.h:22: SkTHashCache() { On 2014/07/11 17:08:08, mtklein wrote: > Consider this: > > SkTHashCache { > this->reset(); > } > > ~SkTHashCache { > this->clear(); > } > > void reset() { > this->clear(); > fDict.reset(SkNEW(DictType)); > } > > private: > > void clear() { > if (fDict.get()) { .... } > } Done. https://codereview.chromium.org/364193004/diff/160001/src/core/SkTHashCache.h... src/core/SkTHashCache.h:27: fValues.deleteAll(); On 2014/07/11 17:08:08, mtklein wrote: > Looks like we leak the last values? I'm sorry, I don't understand your question. https://codereview.chromium.org/364193004/diff/160001/src/core/SkTHashCache.h... src/core/SkTHashCache.h:41: T& add(T& add) { On 2014/07/11 17:08:08, mtklein wrote: > Looks like argument to add can be const&? Done. https://codereview.chromium.org/364193004/diff/160001/src/core/SkTHashCache.h... src/core/SkTHashCache.h:43: if (T * val = this->find(key)) { On 2014/07/11 17:08:08, mtklein wrote: > stray space before *? Done. https://codereview.chromium.org/364193004/diff/160001/src/core/SkTHashCache.h... src/core/SkTHashCache.h:47: T* element = new T(add); On 2014/07/11 17:08:08, mtklein wrote: > SkNEW Done. https://codereview.chromium.org/364193004/diff/160001/src/core/SkTHashCache.h... src/core/SkTHashCache.h:63: delete &(*it); On 2014/07/11 17:08:08, mtklein wrote: > SkDELETE Done. https://codereview.chromium.org/364193004/diff/160001/src/gpu/gl/GrGLCaps.h File src/gpu/gl/GrGLCaps.h (right): https://codereview.chromium.org/364193004/diff/160001/src/gpu/gl/GrGLCaps.h#n... src/gpu/gl/GrGLCaps.h:15: #include "GrTHashTable.h" On 2014/07/11 17:08:08, mtklein wrote: > Do you still need this? This one is not needed: "GrTHashTable.h" Deleted.
https://codereview.chromium.org/364193004/diff/180001/src/core/SkTHashCache.h File src/core/SkTHashCache.h (right): https://codereview.chromium.org/364193004/diff/180001/src/core/SkTHashCache.h... src/core/SkTHashCache.h:30: bool has(const Key& key) const { This appears to be unused except in the test? Might remove it for now? https://codereview.chromium.org/364193004/diff/180001/src/core/SkTHashCache.h... src/core/SkTHashCache.h:79: SkTDArray<T*> fValues; So, there was a memory leak before, but you've fixed it by calling clear() in ~SkTHashCache() instead of fValues.deleteAll(), which you had there previously. Looks like fValues can just be deleted now? https://codereview.chromium.org/364193004/diff/180001/src/gpu/gl/GrGLCaps.h File src/gpu/gl/GrGLCaps.h (right): https://codereview.chromium.org/364193004/diff/180001/src/gpu/gl/GrGLCaps.h#n... src/gpu/gl/GrGLCaps.h:393: return hash; You may have better results if you mix these bits. As written, most of the time only your low bits (from fFboFormat) are going to matter to the hash table. return SkChecksum::Mix(hash); an alternative requiring even less thought is uint32_t getHash() const { return SkChecksum::Murmur3(reinterpret_cast<const uint32_t*>(this), sizeof(*this)); } https://codereview.chromium.org/364193004/diff/180001/src/gpu/gl/GrGLCaps.h#n... src/gpu/gl/GrGLCaps.h:397: ReadPixelsSupportedFormats(Key key, Strange formatting?
New patch :-) https://codereview.chromium.org/364193004/diff/180001/src/core/SkTHashCache.h File src/core/SkTHashCache.h (right): https://codereview.chromium.org/364193004/diff/180001/src/core/SkTHashCache.h... src/core/SkTHashCache.h:30: bool has(const Key& key) const { On 2014/07/11 18:11:38, mtklein wrote: > This appears to bè unused except in the test? Might remove it for now? Done. https://codereview.chromium.org/364193004/diff/180001/src/core/SkTHashCache.h... src/core/SkTHashCache.h:79: SkTDArray<T*> fValues; On 2014/07/11 18:11:38, mtklein wrote: > So, there was a memory leak before, but you've fixed it by calling clear() in > ~SkTHashCache() instead of fValues.deleteAll(), which you had there previously. > > Looks like fValues can just be deleted now? Done. https://codereview.chromium.org/364193004/diff/180001/src/gpu/gl/GrGLCaps.h File src/gpu/gl/GrGLCaps.h (right): https://codereview.chromium.org/364193004/diff/180001/src/gpu/gl/GrGLCaps.h#n... src/gpu/gl/GrGLCaps.h:393: return hash; On 2014/07/11 18:11:39, mtklein wrote: > You may have better results if you mix these bits. As written, most of the time > only your low bits (from fFboFormat) are going to matter to the hash table. > > return SkChecksum::Mix(hash); > > an alternative requiring even less thought is > > uint32_t getHash() const { > return SkChecksum::Murmur3(reinterpret_cast<const uint32_t*>(this), > sizeof(*this)); > } Done. https://codereview.chromium.org/364193004/diff/180001/src/gpu/gl/GrGLCaps.h#n... src/gpu/gl/GrGLCaps.h:397: ReadPixelsSupportedFormats(Key key, On 2014/07/11 18:11:39, mtklein wrote: > Strange formatting? Done.
lgtm
The CQ bit was checked by piotaixr@chromium.org
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/piotaixr@chromium.org/364193004/200001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: Build-Win7-VS2010-x86-Debug-Trybot on tryserver.skia (http://108.170.220.76:10117/builders/Build-Win7-VS2010-x86-Debug-Trybot/build...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build-Win7-VS2010-x86-Debug-Trybot on tryserver.skia (http://108.170.220.76:10117/builders/Build-Win7-VS2010-x86-Debug-Trybot/build...)
On 2014/07/14 21:24:16, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > Build-Win7-VS2010-x86-Debug-Trybot on tryserver.skia > (http://108.170.220.76:10117/builders/Build-Win7-VS2010-x86-Debug-Trybot/build...) The build failed because of a mistake in the name of a file in core.gypi (SkHashCache instead of SkTHashCache). Should I remove it (SKTDynamicHash.h is not in the gypi file) ou should I move (and rename) it to the line 202 (after '<(skia_src_path)/core/SkTextMapStateProc.h',) ?
On 2014/07/14 21:42:18, Rémi Piotaix wrote: > On 2014/07/14 21:24:16, I haz the power (commit-bot) wrote: > > Try jobs failed on following builders: > > Build-Win7-VS2010-x86-Debug-Trybot on tryserver.skia > > > (http://108.170.220.76:10117/builders/Build-Win7-VS2010-x86-Debug-Trybot/build...) > > The build failed because of a mistake in the name of a file in core.gypi > (SkHashCache instead of SkTHashCache). > Should I remove it (SKTDynamicHash.h is not in the gypi file) ou should I move > (and rename) it to the line 202 (after > '<(skia_src_path)/core/SkTextMapStateProc.h',) ? Either way is okay by me. Listing headers in the .gyp file doesn't do much beyond making sure they end up in the source lists of generated projects for XCode and Visual Studio.
The CQ bit was checked by piotaixr@chromium.org
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/piotaixr@chromium.org/364193004/210001
Message was sent while issue was closed.
Change committed as 8339371f1ec3c57a0741932fd96bff32c53d4e54
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/395203002/ by bungeman@google.com. The reason for reverting is: This appears to be causing failures on Android when running tests..
In patchset 16 (and probably in 17 too) the tests are failing on the android trybots. When executed locally on a nexus 10, all the tests are OK when executing : ./platform_tools/android/bin/android_run_skia dm --release --resourcePath /data/local/tmp --verbose --nocpu --match '~giantbitmap' Any idea on why this behavior?
The CQ bit was checked by piotaixr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/364193004/330001
The CQ bit was unchecked by piotaixr@chromium.org
On 2014/10/01 18:20:23, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/364193004/330001 lgtm
The CQ bit was checked by piotaixr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/364193004/330001
Message was sent while issue was closed.
Committed patchset #18 (id:330001) as e4b231428e8c14cbc82d20cfb12eb08fc45f8df6 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
