Revert of Switch uses of SkChecksum::Compute to Murmur3. (patchset #2 id:20001 of...
authormtklein <mtklein@google.com>
Fri, 13 Nov 2015 15:07:47 +0000 (07:07 -0800)
committerCommit bot <commit-bot@chromium.org>
Fri, 13 Nov 2015 15:07:47 +0000 (07:07 -0800)
Reason for revert:
I would not have expected this to change any images.

Looks like it's changed both MSAA and non-MSAA GMs:
https://gold.skia.org/search2?blame=1d024a3c909ae5cefa5e8b339e2b52dc73ee85ac&unt=true&head=true&query=source_type%3Dgm

Original issue's description:
> Switch uses of SkChecksum::Compute to Murmur3.
>
> SkChecksum::Compute is a very, very poorly distributed hash function.
> This replaces all remaining uses with Murmur3.
>
> The only interesting stuff is in src/gpu.
>
> BUG=skia:
>
> Committed: https://skia.googlesource.com/skia/+/1d024a3c909ae5cefa5e8b339e2b52dc73ee85ac

TBR=bsalomon@google.com,mtklein@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=skia:

Review URL: https://codereview.chromium.org/1445523003

bench/ChecksumBench.cpp
include/private/SkChecksum.h
src/core/SkPictureFlat.h
src/gpu/GrProgramDesc.h
src/gpu/GrResourceCache.cpp
tests/ChecksumTest.cpp

index 92e7da662f39e5db1eb1c2d678666d9b1d8670b5..a2c282435e65eaf28120284d8d86a162fa5c1006 100644 (file)
@@ -13,6 +13,7 @@
 #include "SkTemplates.h"
 
 enum ChecksumType {
+    kChecksum_ChecksumType,
     kMD5_ChecksumType,
     kSHA1_ChecksumType,
     kMurmur3_ChecksumType,
@@ -41,6 +42,7 @@ public:
 protected:
     const char* onGetName() override {
         switch (fType) {
+            case kChecksum_ChecksumType: return "compute_checksum";
             case kMD5_ChecksumType: return "compute_md5";
             case kSHA1_ChecksumType: return "compute_sha1";
             case kMurmur3_ChecksumType: return "compute_murmur3";
@@ -51,6 +53,12 @@ protected:
 
     void onDraw(int loops, SkCanvas*) override {
         switch (fType) {
+            case kChecksum_ChecksumType: {
+                for (int i = 0; i < loops; i++) {
+                    volatile uint32_t result = SkChecksum::Compute(fData, sizeof(fData));
+                    sk_ignore_unused_variable(result);
+                }
+            } break;
             case kMD5_ChecksumType: {
                 for (int i = 0; i < loops; i++) {
                     SkMD5 md5;
@@ -83,6 +91,7 @@ private:
 
 ///////////////////////////////////////////////////////////////////////////////
 
+DEF_BENCH( return new ComputeChecksumBench(kChecksum_ChecksumType); )
 DEF_BENCH( return new ComputeChecksumBench(kMD5_ChecksumType); )
 DEF_BENCH( return new ComputeChecksumBench(kSHA1_ChecksumType); )
 DEF_BENCH( return new ComputeChecksumBench(kMurmur3_ChecksumType); )
index 6289a444ae78c3993f89a50e23cb14ec3f3d7b85..4526416fc1535433939304a3c952475e74526b3e 100644 (file)
 #include "SkTLogic.h"
 #include "SkTypes.h"
 
+/**
+ *  Computes a 32bit checksum from a blob of 32bit aligned data. This is meant
+ *  to be very very fast, as it is used internally by the font cache, in
+ *  conjuction with the entire raw key. This algorithm does not generate
+ *  unique values as well as others (e.g. MD5) but it performs much faster.
+ *  Skia's use cases can survive non-unique values (since the entire key is
+ *  always available). Clients should only be used in circumstances where speed
+ *  over uniqueness is at a premium.
+ */
 class SkChecksum : SkNoncopyable {
+private:
+    /*
+     *  Our Rotate and Mash helpers are meant to automatically do the right
+     *  thing depending if sizeof(uintptr_t) is 4 or 8.
+     */
+    enum {
+        ROTR = 17,
+        ROTL = sizeof(uintptr_t) * 8 - ROTR,
+        HALFBITS = sizeof(uintptr_t) * 4
+    };
+
+    static inline uintptr_t Mash(uintptr_t total, uintptr_t value) {
+        return ((total >> ROTR) | (total << ROTL)) ^ value;
+    }
+
 public:
     /**
      * uint32_t -> uint32_t hash, useful for when you're about to trucate this hash but you
@@ -44,6 +68,7 @@ public:
 
     /**
      * Calculate 32-bit Murmur hash (murmur3).
+     * This should take 2-3x longer than SkChecksum::Compute, but is a considerably better hash.
      * See en.wikipedia.org/wiki/MurmurHash.
      *
      *  @param data Memory address of the data block to be processed.
@@ -52,6 +77,70 @@ public:
      *  @return hash result
      */
     static uint32_t Murmur3(const void* data, size_t bytes, uint32_t seed=0);
+
+    /**
+     *  Compute a 32-bit checksum for a given data block
+     *
+     *  WARNING: this algorithm is tuned for efficiency, not backward/forward
+     *  compatibility.  It may change at any time, so a checksum generated with
+     *  one version of the Skia code may not match a checksum generated with
+     *  a different version of the Skia code.
+     *
+     *  @param data Memory address of the data block to be processed. Must be
+     *              32-bit aligned.
+     *  @param size Size of the data block in bytes. Must be a multiple of 4.
+     *  @return checksum result
+     */
+    static uint32_t Compute(const uint32_t* data, size_t size) {
+        // Use may_alias to remind the compiler we're intentionally violating strict aliasing,
+        // and so not to apply strict-aliasing-based optimizations.
+        typedef uint32_t SK_ATTRIBUTE(may_alias) aliased_uint32_t;
+        const aliased_uint32_t* safe_data = (const aliased_uint32_t*)data;
+
+        SkASSERT(SkIsAlign4(size));
+
+        /*
+         *  We want to let the compiler use 32bit or 64bit addressing and math
+         *  so we use uintptr_t as our magic type. This makes the code a little
+         *  more obscure (we can't hard-code 32 or 64 anywhere, but have to use
+         *  sizeof()).
+         */
+        uintptr_t result = 0;
+        const uintptr_t* ptr = reinterpret_cast<const uintptr_t*>(safe_data);
+
+        /*
+         *  count the number of quad element chunks. This takes into account
+         *  if we're on a 32bit or 64bit arch, since we use sizeof(uintptr_t)
+         *  to compute how much to shift-down the size.
+         */
+        size_t n4 = size / (sizeof(uintptr_t) << 2);
+        for (size_t i = 0; i < n4; ++i) {
+            result = Mash(result, *ptr++);
+            result = Mash(result, *ptr++);
+            result = Mash(result, *ptr++);
+            result = Mash(result, *ptr++);
+        }
+        size &= ((sizeof(uintptr_t) << 2) - 1);
+
+        safe_data = reinterpret_cast<const aliased_uint32_t*>(ptr);
+        const aliased_uint32_t* stop = safe_data + (size >> 2);
+        while (safe_data < stop) {
+            result = Mash(result, *safe_data++);
+        }
+
+        /*
+         *  smash us down to 32bits if we were 64. Note that when uintptr_t is
+         *  32bits, this code-path should go away, but I still got a warning
+         *  when I wrote
+         *      result ^= result >> 32;
+         *  since >>32 is undefined for 32bit ints, hence the wacky HALFBITS
+         *  define.
+         */
+        if (8 == sizeof(result)) {
+            result ^= result >> HALFBITS;
+        }
+        return static_cast<uint32_t>(result);
+    }
 };
 
 // SkGoodHash should usually be your first choice in hashing data.
index 8befca0538daf1f0f6f236cdab13cdcadf6b6a80..98a43ecb579454d22dc8042d97c0771a6c85509c 100644 (file)
@@ -179,7 +179,7 @@ class SkFlatData;
 
 class SkFlatController : public SkRefCnt {
 public:
-
+    
 
     SkFlatController(uint32_t writeBufferFlags = 0);
     virtual ~SkFlatController();
@@ -357,7 +357,7 @@ private:
         fIndex     = index;
         fFlatSize  = size;
         fTopBot[0] = SK_ScalarNaN;  // Mark as unwritten.
-        fChecksum  = SkChecksum::Murmur3(this->data(), size);
+        fChecksum  = SkChecksum::Compute((uint32_t*)this->data(), size);
     }
 
     int fIndex;
index 96411953540df82a8f8d009cd495fa7c987eb1f5..05b52cc7cd7dfaa948854487e90101ea382fd58d 100644 (file)
@@ -107,7 +107,8 @@ protected:
         *(this->atOffset<uint32_t, GrProgramDesc::kLengthOffset>()) = SkToU32(keyLength);
 
         uint32_t* checksum = this->atOffset<uint32_t, GrProgramDesc::kChecksumOffset>();
-        *checksum = SkChecksum::Murmur3(fKey.begin(), keyLength);
+        *checksum = 0;
+        *checksum = SkChecksum::Compute(reinterpret_cast<uint32_t*>(fKey.begin()), keyLength);
     }
 
     // The key, stored in fKey, is composed of four parts:
index 4a0c9b2b369bbc12cb4254dedeaae16b929454d6..cb6bcf75fd7fa284276828679b24dc2df888e1c5 100644 (file)
@@ -42,7 +42,7 @@ GrUniqueKey::Domain GrUniqueKey::GenerateDomain() {
 }
 
 uint32_t GrResourceKeyHash(const uint32_t* data, size_t size) {
-    return SkChecksum::Murmur3(data, size);
+    return SkChecksum::Compute(data, size);
 }
 
 //////////////////////////////////////////////////////////////////////////////
@@ -564,7 +564,7 @@ uint32_t GrResourceCache::getNextTimestamp() {
             int currP = 0;
             int currNP = 0;
             while (currP < sortedPurgeableResources.count() &&
-                   currNP < fNonpurgeableResources.count()) {
+                   currNP < fNonpurgeableResources.count()) {                
                 uint32_t tsP = sortedPurgeableResources[currP]->cacheAccess().timestamp();
                 uint32_t tsNP = fNonpurgeableResources[currNP]->cacheAccess().timestamp();
                 SkASSERT(tsP != tsNP);
@@ -596,10 +596,10 @@ uint32_t GrResourceCache::getNextTimestamp() {
 
             // count should be the next timestamp we return.
             SkASSERT(fTimestamp == SkToU32(count));
-
+            
             // The historical timestamps of flushes are now invalid.
             this->resetFlushTimestamps();
-        }
+        }        
     }
     return fTimestamp++;
 }
index cf9d65c59edbb6f344c0f7b945443d7b21fbdf3c..c095a5ac11d356b4d0c7f4be0c9dccf8259e3f6a 100644 (file)
@@ -18,7 +18,7 @@ static uint32_t murmur_noseed(const uint32_t* d, size_t l) { return SkChecksum::
 DEF_TEST(Checksum, r) {
     // Algorithms to test.  They're currently all uint32_t(const uint32_t*, size_t).
     typedef uint32_t(*algorithmProc)(const uint32_t*, size_t);
-    const algorithmProc kAlgorithms[] = { &murmur_noseed };
+    const algorithmProc kAlgorithms[] = { &SkChecksum::Compute, &murmur_noseed };
 
     // Put 128 random bytes into two identical buffers.  Any multiple of 4 will do.
     const size_t kBytes = SkAlign4(128);