From 6c8d242b14355bf66c9137e9e4d6c7861d22168f Mon Sep 17 00:00:00 2001
From: Herb Derby <herb@google.com>
Date: Mon, 17 Sep 2018 12:07:02 -0400
Subject: [PATCH] Make atomic lists list for bitmaps and paths

Make the listner list for bitmas and paths use a simple atomic
stack. This stack implementation does not have the ABA problem
because you can only pop the entire stack at once
with exchange(nullptr).

BUG=skia:8324

Change-Id: I7b0438a42c473b36cd4b0cbf236bf1692c5afab3
Reviewed-on: https://skia-review.googlesource.com/154861
Reviewed-by: Robert Phillips <robertphillips@google.com>
Reviewed-by: Herb Derby <herb@google.com>
Commit-Queue: Herb Derby <herb@google.com>
---
 include/core/SkPixelRef.h   |  3 ++-
 include/private/SkPathRef.h |  5 ++++-
 src/core/SkPathRef.cpp      | 20 ++++++++++++++------
 src/core/SkPixelRef.cpp     | 16 ++++++++++++----
 4 files changed, 32 insertions(+), 12 deletions(-)

diff --git a/include/core/SkPixelRef.h b/include/core/SkPixelRef.h
index 14a23f1acd..12f78ad2a7 100644
--- a/include/core/SkPixelRef.h
+++ b/include/core/SkPixelRef.h
@@ -86,6 +86,7 @@ public:
     struct GenIDChangeListener {
         virtual ~GenIDChangeListener() {}
         virtual void onChange() = 0;
+        GenIDChangeListener* next;
     };
 
     // Takes ownership of listener.
@@ -116,7 +117,7 @@ private:
     const uint32_t fStableID;
 #endif
 
-    SkTDArray<GenIDChangeListener*> fGenIDChangeListeners;  // pointers are owned
+    std::atomic<GenIDChangeListener*> fGenIDChangeListeners{nullptr};  // pointers are owned
 
     // Set true by caches when they cache content that's derived from the current pixels.
     std::atomic<bool> fAddedToCache;
diff --git a/include/private/SkPathRef.h b/include/private/SkPathRef.h
index a4e59babba..e997e56ade 100644
--- a/include/private/SkPathRef.h
+++ b/include/private/SkPathRef.h
@@ -8,6 +8,8 @@
 #ifndef SkPathRef_DEFINED
 #define SkPathRef_DEFINED
 
+#include <atomic>
+
 #include "SkAtomics.h"
 #include "SkMatrix.h"
 #include "SkPoint.h"
@@ -311,6 +313,7 @@ public:
     struct GenIDChangeListener : SkRefCnt {
         virtual ~GenIDChangeListener() {}
         virtual void onChange() = 0;
+        GenIDChangeListener* next;
     };
 
     void addGenIDChangeListener(sk_sp<GenIDChangeListener>);
@@ -544,7 +547,7 @@ private:
     mutable uint32_t    fGenerationID;
     SkDEBUGCODE(int32_t fEditorsAttached;) // assert that only one editor in use at any time.
 
-    SkTDArray<GenIDChangeListener*> fGenIDChangeListeners;  // pointers are reffed
+    std::atomic<GenIDChangeListener*> fGenIDChangeListeners{nullptr};  // pointers are reffed
 
     mutable uint8_t  fBoundsIsDirty;
     mutable bool     fIsFinite;    // only meaningful if bounds are valid
diff --git a/src/core/SkPathRef.cpp b/src/core/SkPathRef.cpp
index 9c1ebe4e1e..fa0ddbaedd 100644
--- a/src/core/SkPathRef.cpp
+++ b/src/core/SkPathRef.cpp
@@ -700,20 +700,28 @@ uint32_t SkPathRef::genID() const {
 }
 
 void SkPathRef::addGenIDChangeListener(sk_sp<GenIDChangeListener> listener) {
+
     if (nullptr == listener || this == gEmpty) {
         return;
     }
-    *fGenIDChangeListeners.append() = listener.release();
+
+    SkPathRef::GenIDChangeListener* ptr = listener.release();
+    ptr->next = fGenIDChangeListeners;
+    while (!fGenIDChangeListeners.compare_exchange_weak(ptr->next, ptr)) {}
 }
 
 // we need to be called *before* the genID gets changed or zerod
 void SkPathRef::callGenIDChangeListeners() {
-    for (int i = 0; i < fGenIDChangeListeners.count(); i++) {
-        fGenIDChangeListeners[i]->onChange();
-    }
 
-    // Listeners get at most one shot, so whether these triggered or not, blow them away.
-    fGenIDChangeListeners.unrefAll();
+    SkPathRef::GenIDChangeListener* ptr = fGenIDChangeListeners.exchange(nullptr);
+
+    while (ptr != nullptr) {
+        ptr->onChange();
+        auto next = ptr->next;
+        // Listeners get at most one shot, so whether these triggered or not, blow them away.
+        ptr->unref();
+        ptr = next;
+    }
 }
 
 SkRRect SkPathRef::getRRect() const {
diff --git a/src/core/SkPixelRef.cpp b/src/core/SkPixelRef.cpp
index 6e1aaf2a83..c0cc42b2a1 100644
--- a/src/core/SkPixelRef.cpp
+++ b/src/core/SkPixelRef.cpp
@@ -92,15 +92,19 @@ void SkPixelRef::addGenIDChangeListener(GenIDChangeListener* listener) {
         delete listener;
         return;
     }
-    *fGenIDChangeListeners.append() = listener;
+
+    listener->next = fGenIDChangeListeners;
+    while (!fGenIDChangeListeners.compare_exchange_weak(listener->next, listener)) {}
 }
 
 // we need to be called *before* the genID gets changed or zerod
 void SkPixelRef::callGenIDChangeListeners() {
     // We don't invalidate ourselves if we think another SkPixelRef is sharing our genID.
+    GenIDChangeListener* head = fGenIDChangeListeners.exchange(nullptr);
+
     if (this->genIDIsUnique()) {
-        for (int i = 0; i < fGenIDChangeListeners.count(); i++) {
-            fGenIDChangeListeners[i]->onChange();
+        for (auto cursor = head; cursor != nullptr; cursor = cursor->next) {
+            cursor->onChange();
         }
 
         // TODO: SkAtomic could add "old_value = atomic.xchg(new_value)" to make this clearer.
@@ -110,7 +114,11 @@ void SkPixelRef::callGenIDChangeListeners() {
         }
     }
     // Listeners get at most one shot, so whether these triggered or not, blow them away.
-    fGenIDChangeListeners.deleteAll();
+    for (auto cursor = head; cursor != nullptr;) {
+        auto next = cursor->next;
+        delete cursor;
+        cursor = next;
+    }
 }
 
 void SkPixelRef::notifyPixelsChanged() {