Update SubAllocatedDescriptorSet.h

Some suggestions to optimize and make stuff correct

Author: devshgraphicsprogramming

include/nbl/video/alloc/SubAllocatedDescriptorSet.h

@@ -34,24 +34,27 @@ class SubAllocatedDescriptorSet : public core::IReferenceCounted
 			memcpy(m_addresses->data(), addresses, count * sizeof(value_type));
 		}
 
-		// Just does the de-allocation
-		inline void operator()()
+		//
+		inline auto getWorstCaseCount() const {return m_addresses->size();}
+
+		// Just does the de-allocation, note that the parameter is a reference
+		inline void operator()(IGPUDescriptorSet::SDropDescriptorSet* &outNullify)
 		{
-			// isn't assert already debug-only?
 			#ifdef _NBL_DEBUG
 			assert(m_composed);
 			#endif // _NBL_DEBUG
-			m_composed->multi_deallocate(m_binding, m_addresses->size(), m_addresses->data());
+			outNullify = m_composed->multi_deallocate(outNullify, m_binding, m_addresses->size(), m_addresses->data());
+			m_composed->m_totalDeferredFrees -= getWorstCaseCount();
 		}
 
 		// Takes count of allocations we want to free up as reference, true is returned if
 		// the amount of allocations freed was >= allocationsToFreeUp
 		// False is returned if there are more allocations to free up
-		inline bool operator()(size_type& allocationsToFreeUp)
+		inline bool operator()(size_type& allocationsToFreeUp, IGPUDescriptorSet::SDropDescriptorSet* &outNullify)
 		{
-			auto prevCount = m_addresses->size();
-			operator()();
-			auto totalFreed = m_addresses->size() - prevCount;
+			auto prevNullify = outNullify;
+			operator()(outNullify);
+			auto totalFreed = outNullify-prevNullify;
 
 			// This does the same logic as bool operator()(size_type&) on 
 			// CAsyncSingleBufferSubAllocator
@@ -63,7 +66,7 @@ class SubAllocatedDescriptorSet : public core::IReferenceCounted
 		}
 	protected:
 		core::smart_refctd_dynamic_array<value_type> m_addresses;
-		SubAllocatedDescriptorSet* m_composed;
+		SubAllocatedDescriptorSet* m_composed; // TODO: shouldn't be called `composed`, maybe `parent` or something
 		uint32_t m_binding;
 	};
 	using EventHandler = MultiTimelineEventHandlerST<DeferredFreeFunctor>;
@@ -93,6 +96,7 @@ class SubAllocatedDescriptorSet : public core::IReferenceCounted
 	std::map<uint32_t, SubAllocDescriptorSetRange> m_allocatableRanges = {};
 	core::smart_refctd_ptr<video::IGPUDescriptorSet> m_descriptorSet;
 	core::smart_refctd_ptr<video::ILogicalDevice> m_logicalDevice;
+	value_type m_totalDeferredFrees = 0;
 
 	#ifdef _NBL_DEBUG
 	std::recursive_mutex stAccessVerfier;
@@ -141,7 +145,7 @@ class SubAllocatedDescriptorSet : public core::IReferenceCounted
 		m_descriptorSet = std::move(descriptorSet);
 	}
 
-	~SubAllocatedDescriptorSet()
+	inline ~SubAllocatedDescriptorSet()
 	{
 		for (uint32_t i = 0; i < m_allocatableRanges.size(); i++)
 		{
@@ -155,9 +159,9 @@ class SubAllocatedDescriptorSet : public core::IReferenceCounted
 	}
 
 	// whether that binding index can be sub-allocated
-	bool isBindingAllocatable(uint32_t binding) { return m_allocatableRanges.find(binding) != m_allocatableRanges.end(); }
+	inline bool isBindingAllocatable(uint32_t binding) { return m_allocatableRanges.find(binding) != m_allocatableRanges.end(); }
 
-	AddressAllocator* getBindingAllocator(uint32_t binding) 
+	inline AddressAllocator* getBindingAllocator(uint32_t binding) 
 	{ 
 		auto range = m_allocatableRanges.find(binding);
 		// Check if this binding has an allocator
@@ -169,36 +173,26 @@ class SubAllocatedDescriptorSet : public core::IReferenceCounted
 	// main methods
 
 #ifdef _NBL_DEBUG
-	std::unique_lock<std::recursive_mutex> stAccessVerifyDebugGuard()
+	inline std::unique_lock<std::recursive_mutex> stAccessVerifyDebugGuard()
 	{
 		std::unique_lock<std::recursive_mutex> tLock(stAccessVerfier,std::try_to_lock_t());
 		assert(tLock.owns_lock());
 		return tLock;
 	}
 #else
-	bool stAccessVerifyDebugGuard() { return false; }
+	inline bool stAccessVerifyDebugGuard() { return false; }
 #endif
 
-	video::IGPUDescriptorSet* getDescriptorSet() { return m_descriptorSet.get(); }
+	inline video::IGPUDescriptorSet* getDescriptorSet() { return m_descriptorSet.get(); }
 
 	//! Warning `outAddresses` needs to be primed with `invalid_value` values, otherwise no allocation happens for elements not equal to `invalid_value`
-	inline size_type try_multi_allocate(
-		uint32_t binding, 
-		size_type count, 
-		video::IGPUDescriptorSet::SDescriptorInfo* descriptors, 
-		video::IGPUDescriptorSet::SWriteDescriptorSet* outDescriptorWrites,
-		value_type* outAddresses
-	)
+	inline size_type try_multi_allocate(const uint32_t binding, const size_type count, value_type* outAddresses) noexcept
 	{
 		auto debugGuard = stAccessVerifyDebugGuard();
 
+		// we assume you've validated that the binding is allocatable before trying this
 		auto allocator = getBindingAllocator(binding);
 
-		std::vector<video::IGPUDescriptorSet::SWriteDescriptorSet> writes;
-		std::vector<video::IGPUDescriptorSet::SDescriptorInfo> infos;
-		writes.reserve(count);
-		infos.reserve(count);
-
 		size_type unallocatedSize = 0u;
 		for (size_type i=0; i<count; i++)
 		{
@@ -211,35 +205,13 @@ class SubAllocatedDescriptorSet : public core::IReferenceCounted
 				unallocatedSize = count - i;
 				break;
 			}
-
-			auto& descriptor = descriptors[i];
-			
-			video::IGPUDescriptorSet::SWriteDescriptorSet write;
-			{
-				write.dstSet = m_descriptorSet.get();
-				write.binding = binding;
-				write.arrayElement = outAddresses[i];
-				write.count = 1u;
-				// descriptors could be a const pointer, but the problem is that this pointer in
-				// SWriteDescriptorSet.info isn't const
-				// can we change it?
-				write.info = &descriptor;
-			}
-			outDescriptorWrites[i] = write;
 		}
 
 		return unallocatedSize;
 	}
 
 	template<class Clock=typename std::chrono::steady_clock>
-	inline size_type multi_allocate(
-		const std::chrono::time_point<Clock>& maxWaitPoint, 
-		uint32_t binding, 
-		size_type count, 
-		video::IGPUDescriptorSet::SDescriptorInfo* descriptors, 
-		video::IGPUDescriptorSet::SWriteDescriptorSet* outDescriptorWrites,
-		value_type* outAddresses
-	) noexcept
+	inline size_type multi_allocate(const std::chrono::time_point<Clock>& maxWaitPoint, const uint32_t binding, const size_type count, value_type* outAddresses) noexcept
 	{
 		auto debugGuard = stAccessVerifyDebugGuard();
 
@@ -248,76 +220,60 @@ class SubAllocatedDescriptorSet : public core::IReferenceCounted
 		if (range == m_allocatableRanges.end())
 			return count;
 
-		auto& eventHandler = range->second.eventHandler;
-
 		// try allocate once
-		size_type unallocatedSize = try_multi_allocate(binding, count, descriptors, outDescriptorWrites, outAddresses);
+		size_type unallocatedSize = try_multi_allocate(binding,count,outAddresses);
 		if (!unallocatedSize)
 			return 0u;
 
 		// then try to wait at least once and allocate
+		auto& eventHandler = range->second.eventHandler;
+		core::vector<IGPUDescriptorSet::SDropDescriptorSet> nulls(m_totalDeferredFrees);
+		auto outNulls = nulls.data();
 		do
 		{
-			eventHandler.wait(maxWaitPoint, unallocatedSize);
-
-			unallocatedSize = try_multi_allocate(
-				binding, 
-				unallocatedSize, 
-				&descriptors[count - unallocatedSize], 
-				&outDescriptorWrites[count - unallocatedSize], 
-				&outAddresses[count - unallocatedSize]
-			);
+			eventHandler.wait(maxWaitPoint, unallocatedSize, outNulls);
+
+			// always call with the same parameters, otherwise this turns into a mess with the non invalid_address gaps
+			unallocatedSize = try_multi_allocate(binding,count,outAddresses);
 			if (!unallocatedSize)
-				return 0u;
+				break;
 		} while(Clock::now()<maxWaitPoint);
+		m_logicalDevice->nullifyDescriptors({nulls.data(),outNulls});
 
 		return unallocatedSize;
 	}
 
-	inline size_type multi_allocate(
-		uint32_t binding, 
-		size_type count, 
-		video::IGPUDescriptorSet::SDescriptorInfo* descriptors, 
-		video::IGPUDescriptorSet::SWriteDescriptorSet* outDescriptorWrites, 
-		value_type* outAddresses
-	) noexcept
+	// default timeout overload
+	inline size_type multi_allocate(const uint32_t binding, const size_type count, value_type* outAddresses) noexcept
 	{
-		auto range = m_allocatableRanges.find(binding);
-		// Check if this binding has an allocator
-		if (range == m_allocatableRanges.end())
-			return count;
-
-		return multi_allocate(TimelineEventHandlerBase::default_wait(), binding, count, descriptors, outDescriptorWrites, outAddresses);
+		// check that the binding is allocatable is done inside anyway
+		return multi_allocate(TimelineEventHandlerBase::default_wait(), binding, count, outAddresses);
 	}
 
-	inline void multi_deallocate(uint32_t binding, size_type count, const size_type* addr)
+	// Very explicit low level call you'd need to sync and drop descriptors by yourself
+	// Returns: the one-past the last `outNullify` write pointer, this allows you to work out how many descriptors were freed
+	inline void multi_deallocate(IGPUDescriptorSet::SDropDescriptorSet* outNullify, uint32_t binding, size_type count, const size_type* addr)
 	{
 		auto debugGuard = stAccessVerifyDebugGuard();
 
 		auto allocator = getBindingAllocator(binding);
-		if (!allocator)
-			return;
+		if (allocator)
 		for (size_type i=0; i<count; i++)
 		{
 			if (addr[i]==AddressAllocator::invalid_address)
 				continue;
 
 			allocator->free_addr(addr[i],1);
-			// TODO: should also write something to the descriptor sets
-			// basically if nullDescriptor device feature is enabled, you would 
-			// indeed write to the DS, else you'd just drop the refcounted references
-			//
-			// this needs to be done as a IGPUDescriptorSet::nullify(const uint32_t binding, 
-			// std::span<uint32_t> indices) function + a virtual nullify_impl
-			video::IGPUDescriptorSet::SDropDescriptorSet dropDescriptorSet;
-			dropDescriptorSet.dstSet = m_descriptorSet.get();
-			dropDescriptorSet.binding = binding;
-			dropDescriptorSet.arrayElement = i;
-			dropDescriptorSet.count = 1;
-			m_logicalDevice->nullifyDescriptors({ &dropDescriptorSet, 1 });
+			outNullify->dstSet = m_descriptorSet.get();
+			outNullify->binding = binding;
+			outNullify->arrayElement = i;
+			outNullify->count = 1;
+			outNullify++;
 		}
+		return outNullify;
 	}
 
+	// 100% will defer
 	inline void multi_deallocate(uint32_t binding, const ISemaphore::SWaitInfo& futureWait, DeferredFreeFunctor&& functor) noexcept
 	{
 		auto range = m_allocatableRanges.find(binding);
@@ -327,26 +283,36 @@ class SubAllocatedDescriptorSet : public core::IReferenceCounted
 
 		auto& eventHandler = range->second.eventHandler;
 		auto debugGuard = stAccessVerifyDebugGuard();
+		m_totalDeferredFrees += functor.getWorstCaseCount();
 		eventHandler.latch(futureWait,std::move(functor));
 	}
 
+	// defers based on the conservative estimation if `futureWait` needs to be waited on, if doesn't will call nullify descriiptors internally immediately
 	inline void multi_deallocate(uint32_t binding, size_type count, const value_type* addr, const ISemaphore::SWaitInfo& futureWait) noexcept
 	{
 		if (futureWait.semaphore)
 			multi_deallocate(binding, futureWait, DeferredFreeFunctor(this, binding, count, addr));
 		else
-			multi_deallocate(binding, count, addr);
+		{
+			core::vector<IGPUDescriptorSet::SDropDescriptorSet> nulls(count);
+			auto actualEnd = multi_deallocate(nulls.data(), binding, count, addr);
+			m_logicalDevice->nullifyDescriptors({nulls.data(),actualEnd});
+		}
 	}
+
 	//! Returns free events still outstanding
 	inline uint32_t cull_frees() noexcept
 	{
 		auto debugGuard = stAccessVerifyDebugGuard();
 		uint32_t frees = 0;
+		core::vector<IGPUDescriptorSet::SDropDescriptorSet> nulls(m_totalDeferredFrees);
+		auto outNulls = nulls.data();
 		for (uint32_t i = 0; i < m_allocatableRanges.size(); i++)
 		{
 			auto& it = m_allocatableRanges[i];
-			frees += it.eventHandler.poll().eventsLeft;
+			frees += it.eventHandler.poll(outNulls).eventsLeft;
 		}
+		m_logicalDevice->nullifyDescriptors({nulls.data(),outNulls});
 		return frees;
 	}
 };