clean up some odd SkRefCnt features

SkRefCntBase is an implementation detail of SkRefCnt, so let's not
mention it in SkContext_Compute.h.  While I'm here, I notice SkNVRefCnt
would work fine too.

No one else calls internal_dispose_restore_refcnt_to_1(), so we can
inline it.  While here, I notice it's resetting the ref count to 1 even
in release builds.  I'm not sure if that is/can be optimized away, but
in any case I think we can wrap with #ifdef SK_DEBUG, if only to make it
clear that it's only there to support the assert in the destructor.

I've removed validate().  Most of the places it's called read pretty
weird, and I think suggest some sort of class-specific validation than
just checking that we're holding a ref.  SkWeakRefCnt::validate() isn't
called anywhere.

There were few users of getRefCnt() outside SkRefCnt itself, so I
removed the rest and made it private.

I've added a few this-> to self calls while at it.

Change-Id: I98be06677a6e8b8e66f44cbb17d14e38b0f39d38
Reviewed-on: https://skia-review.googlesource.com/c/167160
Auto-Submit: Mike Klein <mtklein@google.com>
Commit-Queue: Ben Wagner <bungeman@google.com>
Reviewed-by: Ben Wagner <bungeman@google.com>

include/core/SkRefCnt.h

@@ -37,24 +37,13 @@ public:
     /** Destruct, asserting that the reference count is 1.
     */
     virtual ~SkRefCntBase() {
-#ifdef SK_DEBUG
-        SkASSERTF(getRefCnt() == 1, "fRefCnt was %d", getRefCnt());
+    #ifdef SK_DEBUG
+        SkASSERTF(this->getRefCnt() == 1, "fRefCnt was %d", this->getRefCnt());
         // illegal value, to catch us if we reuse after delete
         fRefCnt.store(0, std::memory_order_relaxed);
-#endif
+    #endif
     }
 
-#ifdef SK_DEBUG
-    /** Return the reference count. Use only for debugging. */
-    int32_t getRefCnt() const {
-        return fRefCnt.load(std::memory_order_relaxed);
-    }
-
-    void validate() const {
-        SkASSERT(getRefCnt() > 0);
-    }
-#endif
-
     /** May return true if the caller is the only owner.
      *  Ensures that all previous owner's actions are complete.
      */
@@ -71,7 +60,7 @@ public:
     /** Increment the reference count. Must be balanced by a call to unref().
     */
     void ref() const {
-        SkASSERT(getRefCnt() > 0);
+        SkASSERT(this->getRefCnt() > 0);
         // No barrier required.
         (void)fRefCnt.fetch_add(+1, std::memory_order_relaxed);
     }
@@ -81,7 +70,7 @@ public:
         the object needs to have been allocated via new, and not on the stack.
     */
     void unref() const {
-        SkASSERT(getRefCnt() > 0);
+        SkASSERT(this->getRefCnt() > 0);
         // A release here acts in place of all releases we "should" have been doing in ref().
         if (1 == fRefCnt.fetch_add(-1, std::memory_order_acq_rel)) {
             // Like unique(), the acquire is only needed on success, to make sure
@@ -90,23 +79,23 @@ public:
         }
     }
 
-protected:
-    /**
-     *  Allow subclasses to call this if they've overridden internal_dispose
-     *  so they can reset fRefCnt before the destructor is called or if they
-     *  choose not to call the destructor (e.g. using a free list).
-     */
-    void internal_dispose_restore_refcnt_to_1() const {
-        SkASSERT(0 == getRefCnt());
-        fRefCnt.store(1, std::memory_order_relaxed);
-    }
-
 private:
+
+#ifdef SK_DEBUG
+    /** Return the reference count. Use only for debugging. */
+    int32_t getRefCnt() const {
+        return fRefCnt.load(std::memory_order_relaxed);
+    }
+#endif
+
     /**
      *  Called when the ref count goes to 0.
      */
     virtual void internal_dispose() const {
-        this->internal_dispose_restore_refcnt_to_1();
+    #ifdef SK_DEBUG
+        SkASSERT(0 == this->getRefCnt());
+        fRefCnt.store(1, std::memory_order_relaxed);
+    #endif
         delete this;
     }
 
@@ -171,7 +160,12 @@ template <typename Derived>
 class SkNVRefCnt {
 public:
     SkNVRefCnt() : fRefCnt(1) {}
-    ~SkNVRefCnt() { SkASSERTF(1 == getRefCnt(), "NVRefCnt was %d", getRefCnt()); }
+    ~SkNVRefCnt() {
+    #ifdef SK_DEBUG
+        int rc = fRefCnt.load(std::memory_order_relaxed);
+        SkASSERTF(rc == 1, "NVRefCnt was %d", rc);
+    #endif
+    }
 
     // Implementation is pretty much the same as SkRefCntBase. All required barriers are the same:
     //   - unique() needs acquire when it returns true, and no barrier if it returns false;
@@ -191,9 +185,6 @@ public:
 
 private:
     mutable std::atomic<int32_t> fRefCnt;
-    int32_t getRefCnt() const {
-        return fRefCnt.load(std::memory_order_relaxed);
-    }
 
     SkNVRefCnt(SkNVRefCnt&&) = delete;
     SkNVRefCnt(const SkNVRefCnt&) = delete;

include/private/SkWeakRefCnt.h

@@ -72,11 +72,6 @@ public:
     int32_t getWeakCnt() const {
         return fWeakCnt.load(std::memory_order_relaxed);
     }
-
-    void validate() const {
-        this->INHERITED::validate();
-        SkASSERT(getWeakCnt() > 0);
-    }
 #endif
 
 private:

src/compute/sk/SkContext_Compute.h

@@ -33,7 +33,7 @@ extern "C" {
 //
 //
 
-class SkContext_Compute : public SkRefCntBase
+class SkContext_Compute : public SkNVRefCnt<SkContext_Compute>
 {
  public:
 

src/core/SkTypefaceCache.cpp

@@ -89,8 +89,8 @@ static bool DumpProc(SkTypeface* face, void* ctx) {
     face->getFamilyName(&n);
     SkFontStyle s = face->fontStyle();
     SkFontID id = face->uniqueID();
-    SkDebugf("SkTypefaceCache: face %p fontID %d weight %d width %d style %d refcnt %d name %s\n",
-             face, id, s.weight(), s.width(), s.slant(), face->getRefCnt(), n.c_str());
+    SkDebugf("SkTypefaceCache: face %p fontID %d weight %d width %d style %d name %s\n",
+             face, id, s.weight(), s.width(), s.slant(), n.c_str());
     return false;
 }
 #endif

src/gpu/SkGpuDevice.cpp

@@ -1668,7 +1668,6 @@ void SkGpuDevice::drawAtlas(const SkImage* atlas, const SkRSXform xform[],
         }
     }
 
-    SkDEBUGCODE(this->validate();)
     fRenderTargetContext->drawAtlas(
             this->clip(), std::move(grPaint), this->ctm(), count, xform, texRect, colors);
 }
@@ -1678,7 +1677,6 @@ void SkGpuDevice::drawAtlas(const SkImage* atlas, const SkRSXform xform[],
 void SkGpuDevice::drawGlyphRunList(const SkGlyphRunList& glyphRunList) {
     ASSERT_SINGLE_OWNER
     GR_CREATE_TRACE_MARKER_CONTEXT("SkGpuDevice", "drawGlyphRunList", fContext.get());
-    SkDEBUGCODE(this->validate();)
 
     // Check for valid input
     const SkMatrix& ctm = this->ctm();

tests/SerialProcsTest.cpp

@@ -196,16 +196,7 @@ DEF_TEST(serial_typeface, reporter) {
         return; // need two different typefaces for this test to make sense.
     }
 
-#ifdef SK_DEBUG
-    REPORTER_ASSERT(reporter, tf0->getRefCnt() == 1);
-    REPORTER_ASSERT(reporter, tf1->getRefCnt() == 1);
-#endif
     auto pic = make_picture(tf0, tf1);
-#ifdef SK_DEBUG
-    // picture should add 2 more references to each typeface
-    REPORTER_ASSERT(reporter, tf0->getRefCnt() == 3);
-    REPORTER_ASSERT(reporter, tf1->getRefCnt() == 3);
-#endif
 
     int counter = 0;
     SkSerialProcs procs;

tests/SurfaceTest.cpp

@@ -584,7 +584,6 @@ static void test_no_canvas1(skiatest::Reporter* reporter,
                             SkSurface::ContentChangeMode mode) {
     // Test passes by not asserting
     surface->notifyContentWillChange(mode);
-    SkDEBUGCODE(surface->validate();)
 }
 static void test_no_canvas2(skiatest::Reporter* reporter,
                             SkSurface* surface,
@@ -593,15 +592,9 @@ static void test_no_canvas2(skiatest::Reporter* reporter,
     // are made before a canvas is created.
     sk_sp<SkImage> image1 = surface->makeImageSnapshot();
     sk_sp<SkImage> aur_image1(image1);
-    SkDEBUGCODE(image1->validate();)
-    SkDEBUGCODE(surface->validate();)
     surface->notifyContentWillChange(mode);
-    SkDEBUGCODE(image1->validate();)
-    SkDEBUGCODE(surface->validate();)
     sk_sp<SkImage> image2 = surface->makeImageSnapshot();
     sk_sp<SkImage> aur_image2(image2);
-    SkDEBUGCODE(image2->validate();)
-    SkDEBUGCODE(surface->validate();)
     REPORTER_ASSERT(reporter, image1 != image2);
 }
 DEF_TEST(SurfaceNoCanvas, reporter) {