Use single 64bit atomic refcount for Dawn objects

Bug: dawn:105
Change-Id: I7741d5f01579f269db272200d39088c07e2acd92
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/7440
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Reviewed-by: Kai Ninomiya <kainino@chromium.org>
Commit-Queue: Austin Eng <enga@chromium.org>

src/dawn_native/RefCounted.cpp

@@ -24,54 +24,22 @@ namespace dawn_native {
     RefCounted::~RefCounted() {
     }
 
-    void RefCounted::ReferenceInternal() {
-        ASSERT(mInternalRefs != 0);
-
-        // TODO(cwallez@chromium.org): what to do on overflow?
-        mInternalRefs++;
+    uint64_t RefCounted::GetRefCount() const {
+        return mRefCount;
     }
 
-    void RefCounted::ReleaseInternal() {
-        ASSERT(mInternalRefs != 0);
+    void RefCounted::Reference() {
+        ASSERT(mRefCount != 0);
+        mRefCount++;
+    }
 
-        mInternalRefs--;
+    void RefCounted::Release() {
+        ASSERT(mRefCount != 0);
 
-        if (mInternalRefs == 0) {
-            ASSERT(mExternalRefs == 0);
-            // TODO(cwallez@chromium.org): would this work with custom allocators?
+        mRefCount--;
+        if (mRefCount == 0) {
             delete this;
         }
     }
 
-    uint32_t RefCounted::GetExternalRefs() const {
-        return mExternalRefs;
-    }
-
-    uint32_t RefCounted::GetInternalRefs() const {
-        return mInternalRefs;
-    }
-
-    void RefCounted::Reference() {
-        ASSERT(mInternalRefs != 0);
-
-        // mExternalRefs != 0 counts as one internal ref.
-        if (mExternalRefs == 0) {
-            ReferenceInternal();
-        }
-
-        // TODO(cwallez@chromium.org): what to do on overflow?
-        mExternalRefs++;
-    }
-
-    void RefCounted::Release() {
-        ASSERT(mInternalRefs != 0);
-        ASSERT(mExternalRefs != 0);
-
-        mExternalRefs--;
-        // mExternalRefs != 0 counts as one internal ref.
-        if (mExternalRefs == 0) {
-            ReleaseInternal();
-        }
-    }
-
 }  // namespace dawn_native

src/dawn_native/RefCounted.h

@@ -15,6 +15,7 @@
 #ifndef DAWNNATIVE_REFCOUNTED_H_
 #define DAWNNATIVE_REFCOUNTED_H_
 
+#include <atomic>
 #include <cstdint>
 
 namespace dawn_native {
@@ -24,19 +25,14 @@ namespace dawn_native {
         RefCounted();
         virtual ~RefCounted();
 
-        void ReferenceInternal();
-        void ReleaseInternal();
-
-        uint32_t GetExternalRefs() const;
-        uint32_t GetInternalRefs() const;
+        uint64_t GetRefCount() const;
 
         // Dawn API
         void Reference();
         void Release();
 
       protected:
-        uint32_t mExternalRefs = 1;
-        uint32_t mInternalRefs = 1;
+        std::atomic_uint64_t mRefCount = {1};
     };
 
     template <typename T>
@@ -111,12 +107,12 @@ namespace dawn_native {
       private:
         void Reference() const {
             if (mPointee != nullptr) {
-                mPointee->ReferenceInternal();
+                mPointee->Reference();
             }
         }
         void Release() const {
             if (mPointee != nullptr) {
-                mPointee->ReleaseInternal();
+                mPointee->Release();
             }
         }
 

src/tests/unittests/RefCountedTests.cpp

@@ -13,6 +13,7 @@
 // limitations under the License.
 
 #include <gtest/gtest.h>
+#include <thread>
 
 #include "dawn_native/RefCounted.h"
 
@@ -38,8 +39,8 @@ struct RCTest : public RefCounted {
     bool* deleted = nullptr;
 };
 
-// Test that RCs start with one external ref, and removing it destroys the object.
-TEST(RefCounted, StartsWithOneExternalRef) {
+// Test that RCs start with one ref, and removing it destroys the object.
+TEST(RefCounted, StartsWithOneRef) {
     bool deleted = false;
     auto test = new RCTest(&deleted);
 
@@ -47,39 +48,51 @@ TEST(RefCounted, StartsWithOneExternalRef) {
     ASSERT_TRUE(deleted);
 }
 
-// Test internal refs keep the RC alive.
-TEST(RefCounted, InternalRefKeepsAlive) {
+// Test adding refs keep the RC alive.
+TEST(RefCounted, AddingRefKeepsAlive) {
     bool deleted = false;
     auto test = new RCTest(&deleted);
 
-    test->ReferenceInternal();
-    test->Release();
-    ASSERT_FALSE(deleted);
-
-    test->ReleaseInternal();
-    ASSERT_TRUE(deleted);
-}
-
-// Test that when adding an external ref from 0, an internal ref is added
-TEST(RefCounted, AddExternalRefFromZero) {
-    bool deleted = false;
-    auto test = new RCTest(&deleted);
-
-    test->ReferenceInternal();
-    test->Release();
-    ASSERT_FALSE(deleted);
-
-    // Reference adds an internal ref and release removes one
     test->Reference();
     test->Release();
     ASSERT_FALSE(deleted);
 
-    test->ReleaseInternal();
+    test->Release();
     ASSERT_TRUE(deleted);
 }
 
-// Test Ref remove internal reference when going out of scope
-TEST(Ref, EndOfScopeRemovesInternalRef) {
+// Test that Reference and Release atomically change the refcount.
+TEST(RefCounted, RaceOnReferenceRelease) {
+    bool deleted = false;
+    auto* test = new RCTest(&deleted);
+
+    auto referenceManyTimes = [test]() {
+        for (uint32_t i = 0; i < 100000; ++i) {
+            test->Reference();
+        }
+    };
+    std::thread t1(referenceManyTimes);
+    std::thread t2(referenceManyTimes);
+
+    t1.join();
+    t2.join();
+    ASSERT_EQ(test->GetRefCount(), 200001u);
+
+    auto releaseManyTimes = [test]() {
+        for (uint32_t i = 0; i < 100000; ++i) {
+            test->Release();
+        }
+    };
+
+    std::thread t3(releaseManyTimes);
+    std::thread t4(releaseManyTimes);
+    t3.join();
+    t4.join();
+    ASSERT_EQ(test->GetRefCount(), 1u);
+}
+
+// Test Ref remove reference when going out of scope
+TEST(Ref, EndOfScopeRemovesRef) {
     bool deleted = false;
     {
         Ref<RCTest> test(new RCTest(&deleted));