[WIP] Fix data races

WickedEngine/wiArchive.h

@@ -4,6 +4,7 @@
 #include "wiVector.h"
 #include "wiColor.h"
 #include "wiGraphics.h"
+#include "wiAtomic.h"
 
 #include <string>
 
@@ -469,6 +470,22 @@ namespace wi
 			}
 			return *this;
 		}
+		template<typename T>
+		inline Archive& operator>>(std::atomic<T>& data)
+		{
+			T val;
+			(*this) >> val;
+			data = val;
+			return *this;
+		}
+		template<typename T>
+		inline Archive& operator>>(wi::relaxed_atomic<T>& data)
+		{
+			T val;
+			(*this) >> val;
+			data = val;
+			return *this;
+		}
 
 
 

WickedEngine/wiAtomic.h

@@ -0,0 +1,71 @@
+#pragma once
+
+#include <atomic>
+
+// MSVC does not have constexpr atomic load(), so for now
+// we just test for that, in C++20 we could also use
+// type traits to check
+#if defined(_MSC_VER) && !defined(__clang__)
+    #define WI_ATOMIC_CONSTEXPR inline
+#else
+    #define WI_ATOMIC_CONSTEXPR constexpr
+#endif
+
+namespace wi {
+
+
+    // subclass of std::atomic that allows copying
+    // note that copying *is not* atomic; this is a helper class that allows us
+    // to copy classes/structs that contain atomic members where we can accept
+    // that copying is not atomic
+	template<typename T>
+	struct copyable_atomic : public std::atomic<T> {
+		copyable_atomic() noexcept = default;
+		constexpr copyable_atomic(T v) noexcept : std::atomic<T>(v) {};
+		copyable_atomic(const copyable_atomic& other) {
+			std::atomic<T>::store(other.load());
+		}
+		copyable_atomic& operator=(const copyable_atomic& other) {
+			std::atomic<T>::store(other.load());
+			return *this;
+		}
+	};
+
+	// simple wrapper that always defaults to memory_order_relaxed
+	template<typename T>
+	struct relaxed_atomic {
+		std::atomic<T> val;
+
+		relaxed_atomic() noexcept = default;
+		constexpr relaxed_atomic(T v) noexcept : val(v) {};
+		relaxed_atomic(const relaxed_atomic& other) : val(other.load(std::memory_order_relaxed)) {};
+		relaxed_atomic& operator=(const relaxed_atomic& other) {
+			val.store(other.val.load(std::memory_order_relaxed), std::memory_order_relaxed);
+			return *this;
+		}
+		void store(T desired, std::memory_order order = std::memory_order_relaxed) {
+			return val.store(desired, order);
+		}
+		T load(std::memory_order order = std::memory_order_relaxed) const {
+			return val.load(order);
+		}
+
+		T fetch_xor(T arg, std::memory_order order = std::memory_order_relaxed) {
+			return val.fetch_xor(arg, order);
+		}
+		T fetch_and(T arg, std::memory_order order = std::memory_order_relaxed) {
+			return val.fetch_add(arg, order);
+		}
+		T fetch_or(T arg, std::memory_order order = std::memory_order_relaxed) {
+			return val.fetch_or(arg, order);
+		}
+
+		operator T() const noexcept { return load(); }
+		T operator=(T desired) noexcept { store(desired); return desired;}
+		T operator&=(T arg) { return fetch_and(arg); }
+		T operator|=(T arg) { return fetch_or(arg); }
+		T operator^=(T arg) { return fetch_xor(arg); }
+	};
+
+
+}

WickedEngine/wiBacklog.cpp

@@ -36,15 +36,15 @@ namespace wi::backlog
 	wi::font::Params font_params;
 	wi::Color backgroundColor = wi::Color(17, 30, 43, 255);
 	Texture backgroundTex;
-	bool refitscroll = false;
+	std::atomic<bool> refitscroll = false;
 	wi::gui::TextInputField inputField;
 	wi::gui::Button toggleButton;
 	wi::gui::GUI GUI;
 
 	bool locked = false;
 	bool blockLuaExec = false;
 	LogLevel logLevel = LogLevel::Default;
-	LogLevel unseen = LogLevel::None;
+	std::atomic<LogLevel> unseen = LogLevel::None;
 
 	std::deque<LogEntry> history;
 	std::mutex historyLock;
@@ -494,15 +494,14 @@ namespace wi::backlog
 		wi::font::SetCanvas(canvas); // always set here as it can be called from outside...
 		wi::font::Params params = font_params;
 		params.cursor = {};
-		if (refitscroll)
+		if (refitscroll.exchange(false, std::memory_order_relaxed))
 		{
 			const float textheight = wi::font::TextHeight(getText(), params);
 			const float limit = canvas.GetLogicalHeight() - 50;
 			if (scroll + textheight > limit)
 			{
 				scroll = limit - textheight;
 			}
-			refitscroll = false;
 		}
 		params.posX = 5;
 		params.posY = pos + scroll;
@@ -599,7 +598,7 @@ namespace wi::backlog
 		// Enqueue for async file writing
 		asyncWriter.Enqueue(str);
 
-		refitscroll = true;
+		refitscroll.store(true);
 
 		switch (level)
 		{
@@ -615,7 +614,11 @@ namespace wi::backlog
 			break;
 		}
 
-		unseen = std::max(unseen, level);
+		// atomic version of unseen = max(unseen, level)
+		LogLevel current_unseen = unseen.load(std::memory_order_relaxed);
+		while (current_unseen < level) {
+			unseen.compare_exchange_weak(current_unseen, level, std::memory_order_acq_rel, std::memory_order_relaxed);
+		}
 
 		// Force an immediate flush on errors to prevent potential data loss
 		//	in case the application is about to crash

WickedEngine/wiECS.h

@@ -12,6 +12,7 @@
 #include <cassert>
 #include <atomic>
 #include <memory>
+#include <shared_mutex>
 #include <string>
 
 // Entity-Component System
@@ -604,14 +605,17 @@ namespace wi::ecs
 				};
 				Item items[64];
 			};
+			mutable std::shared_mutex mutex;
 			wi::unordered_map<uint64_t, Block> table;
 
 			inline void clear()
 			{
+				std::unique_lock lock(mutex);
 				table.clear();
 			}
 			inline void erase(Entity entity)
 			{
+				std::unique_lock lock(mutex);
 				const uint64_t block_index = entity >> 6ull; // entity / 64
 				auto it = table.find(block_index);
 				if (it == table.end())
@@ -631,6 +635,7 @@ namespace wi::ecs
 			}
 			inline void insert(Entity entity, size_t index)
 			{
+				std::unique_lock lock(mutex);
 				const uint64_t block_index = entity >> 6ull; // entity / 64
 				const uint64_t item_index = entity & 63ull; // entity % 64
 				Block& block = table[block_index];
@@ -639,6 +644,7 @@ namespace wi::ecs
 			}
 			inline size_t get(Entity entity) const
 			{
+				std::shared_lock lock(mutex);
 				const uint64_t block_index = entity >> 6ull; // entity / 64
 				const auto it = table.find(block_index);
 				if (it == table.end())
@@ -653,21 +659,25 @@ namespace wi::ecs
 			// Implementation with hash table:
 			// The standard hashing method, performance depends on hashing, hash collisions
 			wi::unordered_map<Entity, size_t> table;
-
+			mutable std::shared_mutex mutex;
 			inline void clear()
 			{
+				std::unique_lock lock(mutex);
 				table.clear();
 			}
 			inline void erase(Entity entity)
 			{
+				std::unique_lock lock(mutex);
 				table.erase(entity);
 			}
 			inline void insert(Entity entity, size_t index)
 			{
+				std::unique_lock lock(mutex);
 				table[entity] = index;
 			}
 			inline size_t get(Entity entity) const
 			{
+				std::shared_lock lock(mutex);
 				if (table.empty())
 					return INVALID_INDEX;
 				auto it = table.find(entity);
@@ -694,13 +704,15 @@ namespace wi::ecs
 			uint64_t version = 0;
 		};
 		wi::unordered_map<std::string, LibraryEntry> entries;
+		mutable std::shared_mutex mutex;
 
 		// Create an instance of ComponentManager of a certain data type
 		//	The name must be unique, it will be used in serialization
 		//	version is optional, it will be propagated to ComponentManager::Serialize() inside the EntitySerializer parameter
 		template<typename T>
 		inline ComponentManager<T>& Register(const std::string& name, uint64_t version = 0)
 		{
+			std::unique_lock lock(mutex);
 			entries[name].component_manager = std::make_unique<ComponentManager<T>>();
 			entries[name].version = version;
 			return static_cast<ComponentManager<T>&>(*entries[name].component_manager);
@@ -709,6 +721,7 @@ namespace wi::ecs
 		template<typename T>
 		inline ComponentManager<T>* Get(const std::string& name)
 		{
+			std::shared_lock lock(mutex);
 			auto it = entries.find(name);
 			if (it == entries.end())
 				return nullptr;
@@ -718,6 +731,7 @@ namespace wi::ecs
 		template<typename T>
 		inline const ComponentManager<T>* Get(const std::string& name) const
 		{
+			std::shared_lock lock(mutex);
 			auto it = entries.find(name);
 			if (it == entries.end())
 				return nullptr;
@@ -726,6 +740,7 @@ namespace wi::ecs
 
 		inline uint64_t GetVersion(std::string name) const
 		{
+			std::shared_lock lock(mutex);
 			auto it = entries.find(name);
 			if (it == entries.end())
 				return 0;

WickedEngine/wiGraphicsDevice.h

@@ -3,6 +3,7 @@
 #include "wiGraphics.h"
 #include "wiPlatform.h"
 
+#include <atomic>
 #include <cassert>
 #include <cstring>
 #include <algorithm>
@@ -52,7 +53,7 @@ namespace wi::graphics
 	{
 	protected:
 		static constexpr uint32_t BUFFERCOUNT = 2;
-		uint64_t FRAMECOUNT = 0;
+		std::atomic<uint64_t> FRAMECOUNT = 0;
 		size_t SHADER_IDENTIFIER_SIZE = 0;
 		size_t TOPLEVEL_ACCELERATION_STRUCTURE_INSTANCE_SIZE = 0;
 		uint64_t TIMESTAMP_FREQUENCY = 0;
@@ -116,17 +117,23 @@ namespace wi::graphics
 		//	One PipelineState object can be compiled internally for multiple render target or depth-stencil formats, or sample counts
 		virtual size_t GetActivePipelineCount() const = 0;
 
+
+#if defined(_MSC_VER) && !defined(__clang__)
+    #define constexpr_no_msvc
+#else
+	#define constexpr_no_msvc constexpr
+#endif
 		// Returns the number of elapsed frames (submits)
 		//	It is incremented when calling SubmitCommandLists()
-		constexpr uint64_t GetFrameCount() const { return FRAMECOUNT; }
+		constexpr_no_msvc uint64_t GetFrameCount() const { return FRAMECOUNT; }
 
 		// Check whether the graphics device supports a feature or not
 		constexpr bool CheckCapability(GraphicsDeviceCapability capability) const { return has_flag(capabilities, capability); }
 
 		// Returns the buffer count, which is the array size of buffered resources used by both the CPU and GPU
 		static constexpr uint32_t GetBufferCount() { return BUFFERCOUNT; }
 		// Returns the current buffer index, which is in range [0, GetBufferCount() - 1]
-		constexpr uint32_t GetBufferIndex() const { return GetFrameCount() % GetBufferCount(); }
+		constexpr_no_msvc uint32_t GetBufferIndex() const { return GetFrameCount() % GetBufferCount(); }
 
 		// Returns whether the graphics debug layer is enabled. It can be enabled when creating the device.
 		constexpr bool IsDebugDevice() const { return validationMode != ValidationMode::Disabled; }
@@ -294,7 +301,7 @@ namespace wi::graphics
 			inline bool IsValid() const { return data != nullptr && buffer.IsValid(); }
 		};
 
-		// Allocates temporary memory that the CPU can write and GPU can read. 
+		// Allocates temporary memory that the CPU can write and GPU can read.
 		//	It is only alive for one frame and automatically invalidated after that.
 		GPUAllocation AllocateGPU(uint64_t dataSize, CommandList cmd)
 		{

WickedEngine/wiGraphicsDevice_DX12.cpp

@@ -5388,7 +5388,7 @@ std::mutex queue_locker;
 					else
 					{
 						allocationhandler->destroylocker.lock();
-						allocationhandler->destroyer_pipelines.push_back(std::make_pair(x.second, FRAMECOUNT));
+						allocationhandler->destroyer_pipelines.push_back(std::make_pair(x.second, GetFrameCount()));
 						allocationhandler->destroylocker.unlock();
 					}
 				}
@@ -5767,15 +5767,15 @@ std::mutex queue_locker;
 
 		for (auto& x : pipelines_global)
 		{
-			allocationhandler->destroyer_pipelines.push_back(std::make_pair(x.second, FRAMECOUNT));
+			allocationhandler->destroyer_pipelines.push_back(std::make_pair(x.second, GetFrameCount()));
 		}
 		pipelines_global.clear();
 
 		for (auto& x : commandlists)
 		{
 			for (auto& y : x->pipelines_worker)
 			{
-				allocationhandler->destroyer_pipelines.push_back(std::make_pair(y.second, FRAMECOUNT));
+				allocationhandler->destroyer_pipelines.push_back(std::make_pair(y.second, GetFrameCount()));
 			}
 			x->pipelines_worker.clear();
 		}

WickedEngine/wiGraphicsDevice_Vulkan.cpp

@@ -1635,7 +1635,7 @@ using namespace vulkan_internal;
 		if (descriptorPool != VK_NULL_HANDLE)
 		{
 			device->allocationhandler->destroylocker.lock();
-			device->allocationhandler->destroyer_descriptorPools.push_back(std::make_pair(descriptorPool, device->FRAMECOUNT));
+			device->allocationhandler->destroyer_descriptorPools.push_back(std::make_pair(descriptorPool, device->GetFrameCount()));
 			descriptorPool = VK_NULL_HANDLE;
 			device->allocationhandler->destroylocker.unlock();
 		}
@@ -7412,7 +7412,7 @@ using namespace vulkan_internal;
 					else
 					{
 						allocationhandler->destroylocker.lock();
-						allocationhandler->destroyer_pipelines.push_back(std::make_pair(x.second, FRAMECOUNT));
+						allocationhandler->destroyer_pipelines.push_back(std::make_pair(x.second, GetFrameCount()));
 						allocationhandler->destroylocker.unlock();
 					}
 				}
@@ -7510,15 +7510,15 @@ using namespace vulkan_internal;
 
 		for (auto& x : pipelines_global)
 		{
-			allocationhandler->destroyer_pipelines.push_back(std::make_pair(x.second, FRAMECOUNT));
+			allocationhandler->destroyer_pipelines.push_back(std::make_pair(x.second, GetFrameCount()));
 		}
 		pipelines_global.clear();
 
 		for (auto& x : commandlists)
 		{
 			for (auto& y : x->pipelines_worker)
 			{
-				allocationhandler->destroyer_pipelines.push_back(std::make_pair(y.second, FRAMECOUNT));
+				allocationhandler->destroyer_pipelines.push_back(std::make_pair(y.second, GetFrameCount()));
 			}
 			x->pipelines_worker.clear();
 		}