Replace GrTHash with SkTDynamicHash

Replace GrTHash with SkTDynamicHash

Mike: SkTDynamicHash changes
Brian: Ganesh changes

This removes three instances of GrTHash leaving the ones in GrTextStrike.h

Committed: https://skia.googlesource.com/skia/+/3d533ac917eaadf2fb3561f57d7266d8c0e665fd

[mtklein, 2014-07-17 19:36:21]

https://codereview.chromium.org/402693003/diff/60001/src/core/SkTDynamicHash.h
File src/core/SkTDynamicHash.h (right):

https://codereview.chromium.org/402693003/diff/60001/src/core/SkTDynamicHash....
src/core/SkTDynamicHash.h:139: template<class Functor>
I'd add a space between template and <

https://codereview.chromium.org/402693003/diff/60001/src/gpu/GrLayerCache.cpp
File src/gpu/GrLayerCache.cpp (right):

https://codereview.chromium.org/402693003/diff/60001/src/gpu/GrLayerCache.cpp...
src/gpu/GrLayerCache.cpp:83: fLayerHash.mutateAll(UnlockAndDeleteFunctor(this));
Now that I'm remembering, maybe adding mutateAll makes things overcomplicated.

for (SkDynamicHash<...>::Iter it(&fLayerHash); !it.done(); ++it) {
  GrCachedLayer* layer = &(*it);
  fCache->unlock(layer);
  SkDELETE(layer);
}

https://codereview.chromium.org/402693003/diff/60001/src/gpu/GrLayerCache.h
File src/gpu/GrLayerCache.h (right):

https://codereview.chromium.org/402693003/diff/60001/src/gpu/GrLayerCache.h#n...
src/gpu/GrLayerCache.h:27: static uint32_t Hash(const uint32_t& key) { return
key; }
Might consider trying SkChecksum::Mix(key) in the future.

https://codereview.chromium.org/402693003/diff/60001/src/gpu/GrLayerCache.h#n...
src/gpu/GrLayerCache.h:49: bool operator<(const Key& other) const {
SkTDynamicHash won't need this.  Is this dead code?

https://codereview.chromium.org/402693003/diff/60001/src/gpu/GrLayerCache.h#n...
src/gpu/GrLayerCache.h:60: uint32_t getHash() const { return (fPictureID << 16)
| fLayerID; }
Again, SkChecksum::Mix() is a good idea.  Using this hash means the table will
ignore fPictureID until it gets up to 65K capacity.

Or even just do SkChecksum::{Compute,Murmur3}(reinterpret_cast<const
uint32_t*>(this), sizeof(*this)); ?

https://codereview.chromium.org/402693003/diff/60001/src/gpu/GrLayerCache.h#n...
src/gpu/GrLayerCache.h:73: static uint32_t Hash(const Key& key) { return
key.getHash(); }
Consider inlining Key::getHash into this?

https://codereview.chromium.org/402693003/diff/60001/src/gpu/GrLayerCache.h#n...
src/gpu/GrLayerCache.h:80: , fPlot(NULL) {
Stray tab?

https://codereview.chromium.org/402693003/diff/60001/tests/DynamicHashTest.cpp
File tests/DynamicHashTest.cpp (right):

https://codereview.chromium.org/402693003/diff/60001/tests/DynamicHashTest.cp...
tests/DynamicHashTest.cpp:139: template<typename T> static void
TestIter(skiatest::Reporter* reporter) {
+1.  I like this enforces the API stays the same.

[bsalomon, 2014-07-17 19:51:30]

I defer to Mike

[robertphillips, 2014-07-17 20:02:47]

https://codereview.chromium.org/402693003/diff/60001/src/core/SkTDynamicHash.h
File src/core/SkTDynamicHash.h (right):

https://codereview.chromium.org/402693003/diff/60001/src/core/SkTDynamicHash....
src/core/SkTDynamicHash.h:139: template<class Functor>
On 2014/07/17 19:36:20, mtklein wrote:
> I'd add a space between template and <

Done.

https://codereview.chromium.org/402693003/diff/60001/src/gpu/GrLayerCache.cpp
File src/gpu/GrLayerCache.cpp (right):

https://codereview.chromium.org/402693003/diff/60001/src/gpu/GrLayerCache.cpp...
src/gpu/GrLayerCache.cpp:83: fLayerHash.mutateAll(UnlockAndDeleteFunctor(this));
Yeah it's not a fantastic clarity win. I think it is okay in that case since the
functor is used twice (but a sub-routine would also serve).

https://codereview.chromium.org/402693003/diff/60001/src/gpu/GrLayerCache.h
File src/gpu/GrLayerCache.h (right):

https://codereview.chromium.org/402693003/diff/60001/src/gpu/GrLayerCache.h#n...
src/gpu/GrLayerCache.h:27: static uint32_t Hash(const uint32_t& key) { return
key; }
On 2014/07/17 19:36:20, mtklein wrote:
> Might consider trying SkChecksum::Mix(key) in the future.

Done.

https://codereview.chromium.org/402693003/diff/60001/src/gpu/GrLayerCache.h#n...
src/gpu/GrLayerCache.h:49: bool operator<(const Key& other) const {
On 2014/07/17 19:36:20, mtklein wrote:
> SkTDynamicHash won't need this.  Is this dead code?

Done.

https://codereview.chromium.org/402693003/diff/60001/src/gpu/GrLayerCache.h#n...
src/gpu/GrLayerCache.h:60: uint32_t getHash() const { return (fPictureID << 16)
| fLayerID; }
On 2014/07/17 19:36:20, mtklein wrote:
> Again, SkChecksum::Mix() is a good idea.  Using this hash means the table will
> ignore fPictureID until it gets up to 65K capacity.
> 
> Or even just do SkChecksum::{Compute,Murmur3}(reinterpret_cast<const
> uint32_t*>(this), sizeof(*this)); ?

Done.

https://codereview.chromium.org/402693003/diff/60001/src/gpu/GrLayerCache.h#n...
src/gpu/GrLayerCache.h:73: static uint32_t Hash(const Key& key) { return
key.getHash(); }
On 2014/07/17 19:36:20, mtklein wrote:
> Consider inlining Key::getHash into this?

Done.

https://codereview.chromium.org/402693003/diff/60001/src/gpu/GrLayerCache.h#n...
src/gpu/GrLayerCache.h:80: , fPlot(NULL) {
On 2014/07/17 19:36:20, mtklein wrote:
> Stray tab?

Done.

[robertphillips, 2014-07-17 20:16:27]

removed mutateAll entry point

[mtklein, 2014-07-17 20:23:30]

lgtm

https://codereview.chromium.org/402693003/diff/110001/src/gpu/GrLayerCache.cpp
File src/gpu/GrLayerCache.cpp (right):

https://codereview.chromium.org/402693003/diff/110001/src/gpu/GrLayerCache.cp...
src/gpu/GrLayerCache.cpp:70: UnlockAndDeleteFunctor(GrLayerCache* cache) :
fCache(cache) {}
Dead code now?

[robertphillips, 2014-07-18 15:00:36]

PTAL. I had to adjust the hash function to achieve performance parity in the
TextureStripAtlas.

https://codereview.chromium.org/402693003/diff/110001/src/gpu/GrLayerCache.cpp
File src/gpu/GrLayerCache.cpp (right):

https://codereview.chromium.org/402693003/diff/110001/src/gpu/GrLayerCache.cp...
src/gpu/GrLayerCache.cpp:70: UnlockAndDeleteFunctor(GrLayerCache* cache) :
fCache(cache) {}
On 2014/07/17 20:23:29, mtklein wrote:
> Dead code now?

Done.

[mtklein, 2014-07-19 14:38:57]

Still LGTM.

https://codereview.chromium.org/402693003/diff/170001/src/gpu/GrBinHashKey.h
File src/gpu/GrBinHashKey.h (right):

https://codereview.chromium.org/402693003/diff/170001/src/gpu/GrBinHashKey.h#...
src/gpu/GrBinHashKey.h:46: if (fHash != other.fHash) {
I've always had half a mind to cache the hash inside the table, so this happens
for every user without having to code up another class like this.  Any opinion? 
I always hold back for fear of imposing storage overhead on use cases that don't
need the caching, but maybe we should just try it out if all the big boys are
caching hashes.

[robertphillips, 2014-07-20 16:12:21]

The CQ bit was checked by robertphillips@google.com

[robertphillips, 2014-07-20 16:12:30]

https://codereview.chromium.org/402693003/diff/170001/src/gpu/GrBinHashKey.h
File src/gpu/GrBinHashKey.h (right):

https://codereview.chromium.org/402693003/diff/170001/src/gpu/GrBinHashKey.h#...
src/gpu/GrBinHashKey.h:46: if (fHash != other.fHash) {
I think it is worth a try.

[commit-bot: I haz the power, 2014-07-20 16:12:56]

CQ is trying da patch. Follow status at

https://skia-tree-status.appspot.com/cq/robertphillips@google.com/402693003/1...

[commit-bot: I haz the power, 2014-07-20 16:40:05]

Change committed as 3d533ac917eaadf2fb3561f57d7266d8c0e665fd