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>
diff --git a/src/dawn_native/RefCounted.cpp b/src/dawn_native/RefCounted.cpp
index 64a4817..5ec8050 100644
--- a/src/dawn_native/RefCounted.cpp
+++ b/src/dawn_native/RefCounted.cpp
@@ -24,53 +24,21 @@
RefCounted::~RefCounted() {
}
- void RefCounted::ReferenceInternal() {
- ASSERT(mInternalRefs != 0);
-
- // TODO(cwallez@chromium.org): what to do on overflow?
- mInternalRefs++;
- }
-
- void RefCounted::ReleaseInternal() {
- ASSERT(mInternalRefs != 0);
-
- mInternalRefs--;
-
- if (mInternalRefs == 0) {
- ASSERT(mExternalRefs == 0);
- // TODO(cwallez@chromium.org): would this work with custom allocators?
- delete this;
- }
- }
-
- uint32_t RefCounted::GetExternalRefs() const {
- return mExternalRefs;
- }
-
- uint32_t RefCounted::GetInternalRefs() const {
- return mInternalRefs;
+ uint64_t RefCounted::GetRefCount() const {
+ return mRefCount;
}
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++;
+ ASSERT(mRefCount != 0);
+ mRefCount++;
}
void RefCounted::Release() {
- ASSERT(mInternalRefs != 0);
- ASSERT(mExternalRefs != 0);
+ ASSERT(mRefCount != 0);
- mExternalRefs--;
- // mExternalRefs != 0 counts as one internal ref.
- if (mExternalRefs == 0) {
- ReleaseInternal();
+ mRefCount--;
+ if (mRefCount == 0) {
+ delete this;
}
}
diff --git a/src/dawn_native/RefCounted.h b/src/dawn_native/RefCounted.h
index ccc63f9..6eb4ab0 100644
--- a/src/dawn_native/RefCounted.h
+++ b/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 @@
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 @@
private:
void Reference() const {
if (mPointee != nullptr) {
- mPointee->ReferenceInternal();
+ mPointee->Reference();
}
}
void Release() const {
if (mPointee != nullptr) {
- mPointee->ReleaseInternal();
+ mPointee->Release();
}
}
diff --git a/src/tests/unittests/RefCountedTests.cpp b/src/tests/unittests/RefCountedTests.cpp
index 38dfef9..e8a7f6f 100644
--- a/src/tests/unittests/RefCountedTests.cpp
+++ b/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 @@
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 @@
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));
