addressed more comments

google · Nov 1, 2024 · 011a986 · 011a986
1 parent 900e222
commit 011a986
Show file tree

Hide file tree

Showing 4 changed files with 70 additions and 56 deletions.
diff --git a/filament/backend/src/vulkan/memory/Resource.cpp b/filament/backend/src/vulkan/memory/Resource.cpp
@@ -118,11 +118,11 @@ std::string getTypeStr(ResourceType type) {
     }
 }
 
-template void ResourceImpl<true>::destroy(ResourceType type, HandleId id);
-template void ResourceImpl<false>::destroy(ResourceType type, HandleId id);
+void Resource::destroy(ResourceType type, HandleId id) {
+    resManager->destructLaterWithType(type, id);
 
-template <bool THREAD_SAFE>
-void ResourceImpl<THREAD_SAFE>::destroy(ResourceType type, HandleId id) {
+}
+void ThreadSafeResource::destroy(ResourceType type, HandleId id) {
     resManager->destructLaterWithType(type, id);
 }
 

diff --git a/filament/backend/src/vulkan/memory/Resource.h b/filament/backend/src/vulkan/memory/Resource.h
@@ -61,49 +61,68 @@ inline bool isThreadSafeType(ResourceType type) {
     return type == ResourceType::FENCE || type == ResourceType::TIMER_QUERY;
 }
 
-template<bool THREAD_SAFE = false>
-struct ResourceImpl {
-    ResourceImpl()
-        : id(HandleBase::nullid),
-          resManager(nullptr),
-          mDestroyed(false),
+struct Resource {
+    Resource()
+        : resManager(nullptr),
+          id(HandleBase::nullid),
           mCount(0) {}
 
 private:
     inline void inc() noexcept {
-        if UTILS_UNLIKELY (mDestroyed) {
-            return;
-        }
-        if constexpr (THREAD_SAFE) {
-            std::unique_lock<utils::Mutex> lock(mMutex);
-            if UTILS_LIKELY (!mDestroyed) {
-                mCount++;
-            }
-        } else {
-            mCount++;
+        mCount++;
+    }
+
+    inline void dec() noexcept {
+        assert_invariant(mCount > 0);
+        if (--mCount == 0) {
+            destroy(restype, id);
         }
     }
 
+    template <typename T>
+    inline void init(HandleId id, ResourceManager* resManager) {
+        this->id = id;
+        this->resManager = resManager;
+        this->restype = getTypeEnum<T>();
+    }
+
+    void destroy(ResourceType type, HandleId id);
+
+    ResourceManager* resManager; // 8
+    HandleId id;                 // 4
+    ResourceType restype : 6;
+    uint32_t mCount      : 24;   // restype + mDestroyed + mCount is 4 bytes.
+
+    friend class ResourceManager;
+
+    template <typename D>
+    friend struct resource_ptr;
+};
+
+struct ThreadSafeResource {
+    ThreadSafeResource()
+        : resManager(nullptr),
+          id(HandleBase::nullid),
+          mDestroyed(false),
+          mCount(0) {}
+
+private:
+    inline void inc() noexcept {
+        assert_invariant(!mDestroyed);
+        std::unique_lock<utils::Mutex> lock(mMutex);
+        mCount++;
+    }
+
     inline void dec() noexcept {
-        if UTILS_UNLIKELY (mDestroyed) {
-            return;
+        assert_invariant(!mDestroyed);
+        bool doDestroy = false;
+        {
+            std::unique_lock<utils::Mutex> lock(mMutex);
+            doDestroy = --mCount == 0;
         }
-        if constexpr (THREAD_SAFE) {
-            bool shouldDestroy = false;
-            {
-                std::unique_lock<utils::Mutex> lock(mMutex);
-                shouldDestroy = --mCount == 0;
-            }
-            if (shouldDestroy) {
-                destroy(restype, id);
-                mDestroyed = true;
-            }
-        } else {
-            assert_invariant(mCount > 0);
-            if (--mCount == 0) {
-                destroy(restype, id);
-                mDestroyed = true;
-            }
+        if (doDestroy) {
+            destroy(restype, id);
+            mDestroyed = true;
         }
     }
 
@@ -116,23 +135,20 @@ struct ResourceImpl {
 
     void destroy(ResourceType type, HandleId id);
 
-    HandleId id;                 // 4
     ResourceManager* resManager; // 8
-    ResourceType restype : 7;
+    HandleId id;                 // 4
+    ResourceType restype : 5;
     bool mDestroyed      : 1;    // only for thread-safe
     uint32_t mCount      : 24;   // restype + mDestroyed + mCount is 4 bytes.
 
-    typename std::conditional_t<THREAD_SAFE, utils::Mutex, std::monostate> mMutex;
+    utils::Mutex mMutex;
 
     friend class ResourceManager;
 
     template <typename D>
     friend struct resource_ptr;
 };
 
-using Resource = ResourceImpl<false>;
-using ThreadSafeResource = ResourceImpl<true>;
-
 } // namespace filament::backend::fvkmemory
 
 #endif // TNT_FILAMENT_BACKEND_VULKAN_MEMORY_RESOURCE_H
diff --git a/filament/backend/src/vulkan/memory/ResourceManager.h b/filament/backend/src/vulkan/memory/ResourceManager.h
@@ -54,9 +54,12 @@ class ResourceManager {
     template<typename D, typename B, typename... ARGS>
     inline D* construct(Handle<B> const& handle, ARGS&&... args) noexcept {
         constexpr bool THREAD_SAFETY = requires_thread_safety<D>::value;
-        using ResourceT = ResourceImpl<THREAD_SAFETY>;
         D* obj = mHandleAllocatorImpl.construct<D, B>(handle, std::forward<ARGS>(args)...);
-        ((ResourceT*) obj)->template init<D>(handle.getId(), this);
+        if constexpr (THREAD_SAFETY) {
+            ((ThreadSafeResource*) obj)->template init<D>(handle.getId(), this);
+        } else {
+            ((Resource*) obj)->template init<D>(handle.getId(), this);
+        }
         traceConstruction(getTypeEnum<D>(), handle.getId());
         return obj;
     }
@@ -100,11 +103,8 @@ class ResourceManager {
     template<typename D>
     friend struct resource_ptr;
 
-    static constexpr bool IS_THREAD_SAFE = true;
-    static constexpr bool IS_NOT_THREAD_SAFE = false;
-
-    friend struct ResourceImpl<IS_THREAD_SAFE>;
-    friend struct ResourceImpl<IS_NOT_THREAD_SAFE>;
+    friend struct Resource;
+    friend struct ThreadSafeResource;
 };
 
 } // namespace filament::backend::fvkmemory

diff --git a/filament/backend/src/vulkan/memory/ResourcePointer.h b/filament/backend/src/vulkan/memory/ResourcePointer.h
@@ -94,11 +94,7 @@ struct resource_ptr {
 
     // move operator
     inline resource_ptr<D>& operator=(resource_ptr<D> && rhs) {
-        if (mRef && mRef != rhs.mRef) {
-            mRef->dec();
-        }
-        mRef = rhs.mRef; // There should be no change in the reference count.
-        rhs.mRef = nullptr;
+        std::swap(mRef, rhs.mRef);
         return *this;
     }
 
@@ -159,10 +155,12 @@ struct resource_ptr {
     // inc() and dec() For tracking ref-count with respect to create/destroy backend APIs. They can
     // only be used from VulkanDriver.
     inline void dec() {
+        assert_invariant(mRef);
         mRef->dec();
     }
 
     inline void inc() {
+        assert_invariant(mRef);
         mRef->inc();
     }