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>

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 @@ namespace dawn_native {
     }
 
     bool ObjectBase::IsError() const {
-        return mIsError;
+        return GetRefCountPayload() == kErrorPayload;
     }
 
 }  // namespace dawn_native

src/dawn_native/ObjectBase.h

@@ -35,10 +35,6 @@ namespace dawn_native {
 
       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

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 {
+    uint64_t RefCounted::GetRefCountForTesting() const {
-        return mRefCount;
+        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);
+        ASSERT((mRefCount & ~kPayloadMask) != 0);
-        mRefCount++;
+        mRefCount += kRefCountIncrement;
     }
 
     void RefCounted::Release() {
-        ASSERT(mRefCount != 0);
+        ASSERT((mRefCount & ~kPayloadMask) != 0);
 
-        mRefCount--;
+        mRefCount -= kRefCountIncrement;
-        if (mRefCount == 0) {
+        if (mRefCount < kRefCountIncrement) {
             delete this;
         }
     }

src/dawn_native/RefCounted.h

@@ -22,17 +22,18 @@ namespace dawn_native {
 
     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>

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 @@ TEST(RefCounted, RaceOnReferenceRelease) {
 
     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 @@ TEST(RefCounted, RaceOnReferenceRelease) {
     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 @@ TEST(Ref, MoveAssignment) {
     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();
+}