From c391fb7c69fd50c55e566f9612d92b2f5d251524 Mon Sep 17 00:00:00 2001 From: Austin Eng Date: Thu, 23 May 2019 23:55:14 +0000 Subject: [PATCH] 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 Reviewed-by: Kai Ninomiya Commit-Queue: Austin Eng --- src/dawn_native/RefCounted.cpp | 52 ++++---------------- src/dawn_native/RefCounted.h | 14 ++---- src/tests/unittests/RefCountedTests.cpp | 63 +++++++++++++++---------- 3 files changed, 53 insertions(+), 76 deletions(-) diff --git a/src/dawn_native/RefCounted.cpp b/src/dawn_native/RefCounted.cpp index 64a4817b04..5ec8050de7 100644 --- a/src/dawn_native/RefCounted.cpp +++ b/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 diff --git a/src/dawn_native/RefCounted.h b/src/dawn_native/RefCounted.h index ccc63f9344..6eb4ab0b61 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 #include 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 @@ -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(); } } diff --git a/src/tests/unittests/RefCountedTests.cpp b/src/tests/unittests/RefCountedTests.cpp index 38dfef93ea..e8a7f6f25c 100644 --- a/src/tests/unittests/RefCountedTests.cpp +++ b/src/tests/unittests/RefCountedTests.cpp @@ -13,6 +13,7 @@ // limitations under the License. #include +#include #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 test(new RCTest(&deleted));