Inline a 1 bit payload in RefCounted
This changes RefCounted to increment and decrement the refcount by steps
of 2, so that a 1 bit payload can be inlined in the refcount. This
avoids using a whole boolean + padding (4 or 8 bytes in general) to know
if objects are errors.
Fixes a longstanding optimization TODO.
BUG=dawn:105
Change-Id: I5e8f66465d23772fb6082ea3e0d5d4aba2132648
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/13680
Commit-Queue: Corentin Wallez <cwallez@chromium.org>
Reviewed-by: Kai Ninomiya <kainino@chromium.org>
diff --git a/src/dawn_native/ObjectBase.cpp b/src/dawn_native/ObjectBase.cpp
index 3e40af4..0f8c1a3 100644
--- a/src/dawn_native/ObjectBase.cpp
+++ b/src/dawn_native/ObjectBase.cpp
@@ -16,10 +16,14 @@
namespace dawn_native {
- ObjectBase::ObjectBase(DeviceBase* device) : mDevice(device), mIsError(false) {
+ static constexpr uint64_t kErrorPayload = 0;
+ static constexpr uint64_t kNotErrorPayload = 1;
+
+ ObjectBase::ObjectBase(DeviceBase* device) : RefCounted(kNotErrorPayload), mDevice(device) {
}
- ObjectBase::ObjectBase(DeviceBase* device, ErrorTag) : mDevice(device), mIsError(true) {
+ ObjectBase::ObjectBase(DeviceBase* device, ErrorTag)
+ : RefCounted(kErrorPayload), mDevice(device) {
}
ObjectBase::~ObjectBase() {
@@ -30,7 +34,7 @@
}
bool ObjectBase::IsError() const {
- return mIsError;
+ return GetRefCountPayload() == kErrorPayload;
}
} // namespace dawn_native
diff --git a/src/dawn_native/ObjectBase.h b/src/dawn_native/ObjectBase.h
index 02dd7ec..2a1f86a 100644
--- a/src/dawn_native/ObjectBase.h
+++ b/src/dawn_native/ObjectBase.h
@@ -35,10 +35,6 @@
private:
DeviceBase* mDevice;
- // TODO(cwallez@chromium.org): This most likely adds 4 bytes to most Dawn objects, see if
- // that bit can be hidden in the refcount once it is a single 64bit refcount.
- // See https://bugs.chromium.org/p/dawn/issues/detail?id=105
- bool mIsError;
};
} // namespace dawn_native
diff --git a/src/dawn_native/RefCounted.cpp b/src/dawn_native/RefCounted.cpp
index 5ec8050..6782c14 100644
--- a/src/dawn_native/RefCounted.cpp
+++ b/src/dawn_native/RefCounted.cpp
@@ -18,26 +18,39 @@
namespace dawn_native {
- RefCounted::RefCounted() {
+ static constexpr size_t kPayloadBits = 1;
+ static constexpr uint64_t kPayloadMask = (uint64_t(1) << kPayloadBits) - 1;
+ static constexpr uint64_t kRefCountIncrement = (uint64_t(1) << kPayloadBits);
+
+ RefCounted::RefCounted(uint64_t payload) : mRefCount(kRefCountIncrement + payload) {
+ ASSERT((payload & kPayloadMask) == payload);
}
RefCounted::~RefCounted() {
}
- uint64_t RefCounted::GetRefCount() const {
- return mRefCount;
+ uint64_t RefCounted::GetRefCountForTesting() const {
+ return mRefCount >> kPayloadBits;
+ }
+
+ uint64_t RefCounted::GetRefCountPayload() const {
+ // We only care about the payload bits of the refcount. These never change after
+ // initialization so we can use the relaxed memory order. The order doesn't guarantee
+ // anything except the atomicity of the load, which is enough since any past values of the
+ // atomic will have the correct payload bits.
+ return kPayloadMask & mRefCount.load(std::memory_order_relaxed);
}
void RefCounted::Reference() {
- ASSERT(mRefCount != 0);
- mRefCount++;
+ ASSERT((mRefCount & ~kPayloadMask) != 0);
+ mRefCount += kRefCountIncrement;
}
void RefCounted::Release() {
- ASSERT(mRefCount != 0);
+ ASSERT((mRefCount & ~kPayloadMask) != 0);
- mRefCount--;
- if (mRefCount == 0) {
+ mRefCount -= kRefCountIncrement;
+ if (mRefCount < kRefCountIncrement) {
delete this;
}
}
diff --git a/src/dawn_native/RefCounted.h b/src/dawn_native/RefCounted.h
index 89b0666..7b94d2e 100644
--- a/src/dawn_native/RefCounted.h
+++ b/src/dawn_native/RefCounted.h
@@ -22,17 +22,18 @@
class RefCounted {
public:
- RefCounted();
+ RefCounted(uint64_t payload = 0);
virtual ~RefCounted();
- uint64_t GetRefCount() const;
+ uint64_t GetRefCountForTesting() const;
+ uint64_t GetRefCountPayload() const;
// Dawn API
void Reference();
void Release();
protected:
- std::atomic_uint64_t mRefCount = {1};
+ std::atomic_uint64_t mRefCount;
};
template <typename T>
diff --git a/src/tests/unittests/RefCountedTests.cpp b/src/tests/unittests/RefCountedTests.cpp
index e8a7f6f..03b2dff 100644
--- a/src/tests/unittests/RefCountedTests.cpp
+++ b/src/tests/unittests/RefCountedTests.cpp
@@ -20,8 +20,7 @@
using namespace dawn_native;
struct RCTest : public RefCounted {
- RCTest() {
- }
+ using RefCounted::RefCounted;
RCTest(bool* deleted): deleted(deleted) {
}
@@ -76,7 +75,7 @@
t1.join();
t2.join();
- ASSERT_EQ(test->GetRefCount(), 200001u);
+ ASSERT_EQ(test->GetRefCountForTesting(), 200001u);
auto releaseManyTimes = [test]() {
for (uint32_t i = 0; i < 100000; ++i) {
@@ -88,7 +87,7 @@
std::thread t4(releaseManyTimes);
t3.join();
t4.join();
- ASSERT_EQ(test->GetRefCount(), 1u);
+ ASSERT_EQ(test->GetRefCountForTesting(), 1u);
}
// Test Ref remove reference when going out of scope
@@ -207,3 +206,31 @@
destination = nullptr;
ASSERT_TRUE(deleted);
}
+
+// Test the payload initial value is set correctly
+TEST(Ref, InitialPayloadValue) {
+ RCTest* testDefaultConstructor = new RCTest();
+ ASSERT_EQ(testDefaultConstructor->GetRefCountPayload(), 0u);
+ testDefaultConstructor->Release();
+
+ RCTest* testZero = new RCTest(0);
+ ASSERT_EQ(testZero->GetRefCountPayload(), 0u);
+ testZero->Release();
+
+ RCTest* testOne = new RCTest(1);
+ ASSERT_EQ(testOne->GetRefCountPayload(), 1u);
+ testOne->Release();
+}
+
+// Test that the payload survives ref and release operations
+TEST(Ref, PayloadUnchangedByRefCounting) {
+ RCTest* test = new RCTest(1);
+ ASSERT_EQ(test->GetRefCountPayload(), 1u);
+
+ test->Reference();
+ ASSERT_EQ(test->GetRefCountPayload(), 1u);
+ test->Release();
+ ASSERT_EQ(test->GetRefCountPayload(), 1u);
+
+ test->Release();
+}
