From 011a9867efdf2b0e85d4389d8438efb827d85b9c Mon Sep 17 00:00:00 2001 From: Powei Feng Date: Tue, 29 Oct 2024 10:27:00 -0700 Subject: [PATCH] addressed more comments --- .../backend/src/vulkan/memory/Resource.cpp | 8 +- filament/backend/src/vulkan/memory/Resource.h | 96 +++++++++++-------- .../src/vulkan/memory/ResourceManager.h | 14 +-- .../src/vulkan/memory/ResourcePointer.h | 8 +- 4 files changed, 70 insertions(+), 56 deletions(-) diff --git a/filament/backend/src/vulkan/memory/Resource.cpp b/filament/backend/src/vulkan/memory/Resource.cpp index aec9897a747..9a464d1eae5 100644 --- 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::destroy(ResourceType type, HandleId id); -template void ResourceImpl::destroy(ResourceType type, HandleId id); +void Resource::destroy(ResourceType type, HandleId id) { + resManager->destructLaterWithType(type, id); -template -void ResourceImpl::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 index dd7ae7e9554..eddde6d577e 100644 --- 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 -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 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 + inline void init(HandleId id, ResourceManager* resManager) { + this->id = id; + this->resManager = resManager; + this->restype = getTypeEnum(); + } + + 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 + 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 lock(mMutex); + mCount++; + } + inline void dec() noexcept { - if UTILS_UNLIKELY (mDestroyed) { - return; + assert_invariant(!mDestroyed); + bool doDestroy = false; + { + std::unique_lock lock(mMutex); + doDestroy = --mCount == 0; } - if constexpr (THREAD_SAFE) { - bool shouldDestroy = false; - { - std::unique_lock 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,13 +135,13 @@ 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 mMutex; + utils::Mutex mMutex; friend class ResourceManager; @@ -130,9 +149,6 @@ struct ResourceImpl { friend struct resource_ptr; }; -using Resource = ResourceImpl; -using ThreadSafeResource = ResourceImpl; - } // 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 index 04ce32a584c..fa2b62da260 100644 --- a/filament/backend/src/vulkan/memory/ResourceManager.h +++ b/filament/backend/src/vulkan/memory/ResourceManager.h @@ -54,9 +54,12 @@ class ResourceManager { template inline D* construct(Handle const& handle, ARGS&&... args) noexcept { constexpr bool THREAD_SAFETY = requires_thread_safety::value; - using ResourceT = ResourceImpl; D* obj = mHandleAllocatorImpl.construct(handle, std::forward(args)...); - ((ResourceT*) obj)->template init(handle.getId(), this); + if constexpr (THREAD_SAFETY) { + ((ThreadSafeResource*) obj)->template init(handle.getId(), this); + } else { + ((Resource*) obj)->template init(handle.getId(), this); + } traceConstruction(getTypeEnum(), handle.getId()); return obj; } @@ -100,11 +103,8 @@ class ResourceManager { template friend struct resource_ptr; - static constexpr bool IS_THREAD_SAFE = true; - static constexpr bool IS_NOT_THREAD_SAFE = false; - - friend struct ResourceImpl; - friend struct ResourceImpl; + 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 index c0b1cc77ca5..6f34b9252ab 100644 --- 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& operator=(resource_ptr && 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(); }