From 4d91a1b3c318a28050d45895d722ce5f34e3d0b9 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Sat, 17 Aug 2019 12:59:37 -0400 Subject: [PATCH 1/3] BooObject: Make atomic ordering constraints less strict Increasing a reference count is able to always be relaxed. New references to an object can only be formed from an existing reference. The passing of an existing reference from one thread to another will already necessitate the use of synchronization primitives, so this is a safe change to make. Regardless, nothing other than the object itself directly relies on the reference count, so this will always be a suitably atomic operation, even in the face of no synchronization primitives. In the case of decrementing the reference count, it's sufficient to treat it with release semantics and follow it up with an atomic thread fence. This ensures that all accesses to the object in one thread will occur before the delete occurs in another thread (if the situation ever occurs). This should make for a slightly more efficient increment and decrement. --- include/boo/BooObject.hpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/include/boo/BooObject.hpp b/include/boo/BooObject.hpp index c1134e4..52d942e 100644 --- a/include/boo/BooObject.hpp +++ b/include/boo/BooObject.hpp @@ -14,9 +14,10 @@ protected: public: virtual std::unique_lock destructorLock() = 0; - void increment() { m_refCount++; } + void increment() { m_refCount.fetch_add(1, std::memory_order_relaxed); } void decrement() { - if (m_refCount.fetch_sub(1) == 1) { + if (m_refCount.fetch_sub(1, std::memory_order_acq_rel) == 1) { + std::atomic_thread_fence(std::memory_order_acquire); auto lk = destructorLock(); delete this; } From 84f62a0f2ca58a0f2dd8939ea311b9ec78763b40 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Sat, 17 Aug 2019 13:39:39 -0400 Subject: [PATCH 2/3] BooObject: Remove destructorLock() Now that we have the fencing and atomic operations in place to ensure access to data on other threads will always occur before the use of delete, we can remove the destructor lock. This will be useful for making ObjToken's move assignment operator noexcept. --- include/boo/BooObject.hpp | 6 +----- lib/audiodev/AQS.cpp | 4 +--- lib/audiodev/AudioSubmix.cpp | 3 --- lib/audiodev/AudioSubmix.hpp | 6 +++--- lib/audiodev/AudioVoice.cpp | 3 --- lib/audiodev/AudioVoice.hpp | 5 ++--- lib/graphicsdev/Common.hpp | 19 +++++-------------- lib/graphicsdev/D3D11.cpp | 18 ++++++++++-------- lib/graphicsdev/GL.cpp | 20 ++++++++++---------- lib/graphicsdev/Vulkan.cpp | 3 --- lib/inputdev/HIDDeviceWinUSB.cpp | 11 ++++++----- lib/mac/WindowCocoa.mm | 17 ++++++++++++----- lib/nx/ApplicationNX.cpp | 10 +++++++--- lib/win/ApplicationWin32.cpp | 3 +++ lib/win/Win32Common.hpp | 3 +++ lib/x11/ApplicationXlib.hpp | 8 ++++---- lib/x11/WindowXlib.cpp | 16 ++++++++-------- 17 files changed, 75 insertions(+), 80 deletions(-) diff --git a/include/boo/BooObject.hpp b/include/boo/BooObject.hpp index 52d942e..d82e864 100644 --- a/include/boo/BooObject.hpp +++ b/include/boo/BooObject.hpp @@ -1,8 +1,6 @@ #pragma once #include -#include -#include "nxstl/mutex" namespace boo { @@ -13,12 +11,10 @@ protected: virtual ~IObj() = default; public: - virtual std::unique_lock destructorLock() = 0; void increment() { m_refCount.fetch_add(1, std::memory_order_relaxed); } void decrement() { - if (m_refCount.fetch_sub(1, std::memory_order_acq_rel) == 1) { + if (m_refCount.fetch_sub(1, std::memory_order_release) == 1) { std::atomic_thread_fence(std::memory_order_acquire); - auto lk = destructorLock(); delete this; } } diff --git a/lib/audiodev/AQS.cpp b/lib/audiodev/AQS.cpp index 521eb48..412ba45 100644 --- a/lib/audiodev/AQS.cpp +++ b/lib/audiodev/AQS.cpp @@ -1,5 +1,4 @@ #include "AudioVoiceEngine.hpp" -#include "logvisor/logvisor.hpp" #include "boo/IApplication.hpp" #include "../CFPointer.hpp" @@ -7,8 +6,7 @@ #include #include -#include -#include +#include namespace boo { static logvisor::Module Log("boo::AQS"); diff --git a/lib/audiodev/AudioSubmix.cpp b/lib/audiodev/AudioSubmix.cpp index 9e7b2ad..66b7f66 100644 --- a/lib/audiodev/AudioSubmix.cpp +++ b/lib/audiodev/AudioSubmix.cpp @@ -21,9 +21,6 @@ AudioSubmix*& AudioSubmix::_getHeadPtr(BaseAudioVoiceEngine* head) { return head std::unique_lock AudioSubmix::_getHeadLock(BaseAudioVoiceEngine* head) { return std::unique_lock{head->m_dataMutex}; } -std::unique_lock AudioSubmix::destructorLock() { - return std::unique_lock{m_head->m_dataMutex}; -} bool AudioSubmix::_isDirectDependencyOf(AudioSubmix* send) { return m_sendGains.find(send) != m_sendGains.cend(); } diff --git a/lib/audiodev/AudioSubmix.hpp b/lib/audiodev/AudioSubmix.hpp index d78f98c..5b5b87b 100644 --- a/lib/audiodev/AudioSubmix.hpp +++ b/lib/audiodev/AudioSubmix.hpp @@ -1,10 +1,11 @@ #pragma once #include "boo/audiodev/IAudioSubmix.hpp" -#include -#include #include +#include +#include #include +#include #include "Common.hpp" #if __SSE__ @@ -80,7 +81,6 @@ class AudioSubmix : public ListNode _getHeadLock(BaseAudioVoiceEngine* head); - std::unique_lock destructorLock() override; AudioSubmix(BaseAudioVoiceEngine& root, IAudioSubmixCallback* cb, int busId, bool mainOut); ~AudioSubmix() override; diff --git a/lib/audiodev/AudioVoice.cpp b/lib/audiodev/AudioVoice.cpp index 3592ec1..d9c6eef 100644 --- a/lib/audiodev/AudioVoice.cpp +++ b/lib/audiodev/AudioVoice.cpp @@ -18,9 +18,6 @@ AudioVoice*& AudioVoice::_getHeadPtr(BaseAudioVoiceEngine* head) { return head-> std::unique_lock AudioVoice::_getHeadLock(BaseAudioVoiceEngine* head) { return std::unique_lock{head->m_dataMutex}; } -std::unique_lock AudioVoice::destructorLock() { - return std::unique_lock{m_head->m_dataMutex}; -} void AudioVoice::_setPitchRatio(double ratio, bool slew) { if (m_dynamicRate) { diff --git a/lib/audiodev/AudioVoice.hpp b/lib/audiodev/AudioVoice.hpp index d0e13bc..5ff996f 100644 --- a/lib/audiodev/AudioVoice.hpp +++ b/lib/audiodev/AudioVoice.hpp @@ -1,12 +1,12 @@ #pragma once #include -#include +#include #include #include "boo/audiodev/IAudioVoice.hpp" #include "AudioMatrix.hpp" -#include "Common.hpp" #include "AudioVoiceEngine.hpp" +#include "Common.hpp" struct AudioUnitVoiceEngine; struct VSTVoiceEngine; @@ -64,7 +64,6 @@ protected: public: static AudioVoice*& _getHeadPtr(BaseAudioVoiceEngine* head); static std::unique_lock _getHeadLock(BaseAudioVoiceEngine* head); - std::unique_lock destructorLock() override; ~AudioVoice() override; void resetSampleRate(double sampleRate) override; diff --git a/lib/graphicsdev/Common.hpp b/lib/graphicsdev/Common.hpp index 2fe8c93..5362ff0 100644 --- a/lib/graphicsdev/Common.hpp +++ b/lib/graphicsdev/Common.hpp @@ -3,14 +3,15 @@ /* Private header for managing shader data * binding lifetimes through rendering cycle */ +#include #include -#include -#include #include -#include -#include #include +#include #include +#include +#include + #include "boo/graphicsdev/IGraphicsDataFactory.hpp" #include "boo/graphicsdev/IGraphicsCommandQueue.hpp" #include "../Common.hpp" @@ -63,9 +64,6 @@ struct BaseGraphicsData : ListNode { auto* head = getHead(); return head ? head->countForward() : 0; } - std::unique_lock destructorLock() override { - return std::unique_lock{m_head->m_dataMutex}; - } explicit BaseGraphicsData(GraphicsDataFactoryHead& head __BooTraceArgs) : ListNode(&head) __BooTraceInitializer {} @@ -131,9 +129,6 @@ struct BaseGraphicsPool : ListNode { auto* head = getHead(); return head ? head->countForward() : 0; } - std::unique_lock destructorLock() override { - return std::unique_lock{m_head->m_dataMutex}; - } explicit BaseGraphicsPool(GraphicsDataFactoryHead& head __BooTraceArgs) : ListNode(&head) __BooTraceInitializer {} @@ -158,10 +153,6 @@ struct GraphicsDataNode : ListNode, ObjToken< return std::unique_lock{head->m_head->m_dataMutex}; } - std::unique_lock destructorLock() override { - return std::unique_lock{base::m_head->m_head->m_dataMutex}; - } - explicit GraphicsDataNode(const ObjToken& data) : ListNode, ObjToken, NodeCls>(data) {} diff --git a/lib/graphicsdev/D3D11.cpp b/lib/graphicsdev/D3D11.cpp index 82d602c..c8bced6 100644 --- a/lib/graphicsdev/D3D11.cpp +++ b/lib/graphicsdev/D3D11.cpp @@ -1,18 +1,20 @@ #include "../win/Win32Common.hpp" -#include "logvisor/logvisor.hpp" #include "boo/graphicsdev/D3D.hpp" #include "boo/IGraphicsContext.hpp" #include "Common.hpp" -#include -#include -#include -#include -#include -#include + #include #include +#include #include -#include "xxhash/xxhash.h" +#include +#include +#include + +#include +#include + +#include #undef min #undef max diff --git a/lib/graphicsdev/GL.cpp b/lib/graphicsdev/GL.cpp index bf663ac..23c465a 100644 --- a/lib/graphicsdev/GL.cpp +++ b/lib/graphicsdev/GL.cpp @@ -2,22 +2,22 @@ #include "boo/graphicsdev/glew.h" #include "boo/IApplication.hpp" #include "Common.hpp" -#include -#include + #include -#include -#include -#include "xxhash/xxhash.h" -#include "glslang/Public/ShaderLang.h" -#include "glslang/Include/Types.h" -#include "StandAlone/ResourceLimits.h" +#include +#include +#include + +#include +#include +#include + +#include #if _WIN32 #include "../win/WinCommon.hpp" #endif -#include "logvisor/logvisor.hpp" - #undef min #undef max diff --git a/lib/graphicsdev/Vulkan.cpp b/lib/graphicsdev/Vulkan.cpp index f39344f..ed0aa05 100644 --- a/lib/graphicsdev/Vulkan.cpp +++ b/lib/graphicsdev/Vulkan.cpp @@ -1060,9 +1060,6 @@ struct VulkanDescriptorPool : ListNodem_ctx->m_dev, m_descPool, nullptr); } - std::unique_lock destructorLock() override { - return std::unique_lock{m_head->m_dataMutex}; - } static std::unique_lock _getHeadLock(VulkanDataFactoryImpl* factory) { return std::unique_lock{factory->m_dataMutex}; } diff --git a/lib/inputdev/HIDDeviceWinUSB.cpp b/lib/inputdev/HIDDeviceWinUSB.cpp index 25e66f4..d63b586 100644 --- a/lib/inputdev/HIDDeviceWinUSB.cpp +++ b/lib/inputdev/HIDDeviceWinUSB.cpp @@ -2,12 +2,13 @@ #include "IHIDDevice.hpp" #include "boo/inputdev/DeviceToken.hpp" #include "boo/inputdev/DeviceBase.hpp" -#include -#include -#include -#include -#include + #include +#include +#include +#include +#include +#include #ifndef WIN32_LEAN_AND_MEAN #define WIN32_LEAN_AND_MEAN 1 diff --git a/lib/mac/WindowCocoa.mm b/lib/mac/WindowCocoa.mm index ff85daf..3fb4e23 100644 --- a/lib/mac/WindowCocoa.mm +++ b/lib/mac/WindowCocoa.mm @@ -1,12 +1,19 @@ #include "boo/graphicsdev/Metal.hpp" #include "CocoaCommon.hpp" + +#include "boo/IApplication.hpp" +#include "boo/IGraphicsContext.hpp" +#include "boo/IWindow.hpp" +#include "boo/audiodev/IAudioVoiceEngine.hpp" + +#include +#include +#include + #import #import -#include "boo/IApplication.hpp" -#include "boo/IWindow.hpp" -#include "boo/IGraphicsContext.hpp" -#include "boo/audiodev/IAudioVoiceEngine.hpp" -#include "logvisor/logvisor.hpp" + +#include #if !__has_feature(objc_arc) #error ARC Required diff --git a/lib/nx/ApplicationNX.cpp b/lib/nx/ApplicationNX.cpp index 9de71bb..f569c76 100644 --- a/lib/nx/ApplicationNX.cpp +++ b/lib/nx/ApplicationNX.cpp @@ -1,9 +1,13 @@ -#include "boo/IApplication.hpp" -#include "logvisor/logvisor.hpp" #include "nxstl/thread" #include "nxstl/condition_variable" +#include "nxstl/mutex" + +#include "boo/IApplication.hpp" #include "boo/graphicsdev/NX.hpp" -#include + +#include + +#include #include diff --git a/lib/win/ApplicationWin32.cpp b/lib/win/ApplicationWin32.cpp index b628285..27624b7 100644 --- a/lib/win/ApplicationWin32.cpp +++ b/lib/win/ApplicationWin32.cpp @@ -25,6 +25,9 @@ PFN_GetScaleFactorForMonitor MyGetScaleFactorForMonitor = nullptr; #include "boo/graphicsdev/Vulkan.hpp" #endif +#include +#include + DWORD g_mainThreadId = 0; std::mutex g_nwmt; std::condition_variable g_nwcv; diff --git a/lib/win/Win32Common.hpp b/lib/win/Win32Common.hpp index f861c4a..3ef3381 100644 --- a/lib/win/Win32Common.hpp +++ b/lib/win/Win32Common.hpp @@ -14,6 +14,9 @@ #endif #include "boo/graphicsdev/GL.hpp" +#include +#include + extern DWORD g_mainThreadId; extern std::mutex g_nwmt; extern std::condition_variable g_nwcv; diff --git a/lib/x11/ApplicationXlib.hpp b/lib/x11/ApplicationXlib.hpp index b35f7c8..f4ff30a 100644 --- a/lib/x11/ApplicationXlib.hpp +++ b/lib/x11/ApplicationXlib.hpp @@ -12,16 +12,16 @@ #include #include #include -#include #include DBusConnection* RegisterDBus(const char* appName, bool& isFirst); -#include +#include +#include +#include +#include #include #include -#include -#include #include "XlibCommon.hpp" #include diff --git a/lib/x11/WindowXlib.cpp b/lib/x11/WindowXlib.cpp index b229bd7..fa47584 100644 --- a/lib/x11/WindowXlib.cpp +++ b/lib/x11/WindowXlib.cpp @@ -11,16 +11,16 @@ #include #endif -#include -#include -#include -#include +#include #include +#include +#include #include +#include +#include +#include -#include -#include -#include +#include #include @@ -32,7 +32,7 @@ #include #include #include -#include "logvisor/logvisor.hpp" +#include #include "XlibCommon.hpp" From 37670dca2c46212c7d7be96d6312cc1449833ab9 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Sat, 17 Aug 2019 13:44:46 -0400 Subject: [PATCH 3/3] BooObject: Make SObjToken interface noexcept where applicable Now that decrement() doesn't lock a mutex every time its executed, we can mark it as noexcept. This allows us to make most of the interface noexcept. In particular, the move constructor and move assignment operator can now be noexcept. This allows all Boo objects to play nicely with interfaces that may make use of std::move_if_noexcept, like some of the standard library facilities, without always taking the copy constructor/copy assignment. --- include/boo/BooObject.hpp | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/include/boo/BooObject.hpp b/include/boo/BooObject.hpp index d82e864..a819836 100644 --- a/include/boo/BooObject.hpp +++ b/include/boo/BooObject.hpp @@ -11,8 +11,8 @@ protected: virtual ~IObj() = default; public: - void increment() { m_refCount.fetch_add(1, std::memory_order_relaxed); } - void decrement() { + void increment() noexcept { m_refCount.fetch_add(1, std::memory_order_relaxed); } + void decrement() noexcept { if (m_refCount.fetch_sub(1, std::memory_order_release) == 1) { std::atomic_thread_fence(std::memory_order_acquire); delete this; @@ -25,17 +25,17 @@ class ObjToken { SubCls* m_obj = nullptr; public: - ObjToken() = default; - ObjToken(SubCls* obj) : m_obj(obj) { + ObjToken() noexcept = default; + ObjToken(SubCls* obj) noexcept : m_obj(obj) { if (m_obj) m_obj->increment(); } - ObjToken(const ObjToken& other) : m_obj(other.m_obj) { + ObjToken(const ObjToken& other) noexcept : m_obj(other.m_obj) { if (m_obj) m_obj->increment(); } - ObjToken(ObjToken&& other) : m_obj(other.m_obj) { other.m_obj = nullptr; } - ObjToken& operator=(SubCls* obj) { + ObjToken(ObjToken&& other) noexcept : m_obj(other.m_obj) { other.m_obj = nullptr; } + ObjToken& operator=(SubCls* obj) noexcept { if (m_obj) m_obj->decrement(); m_obj = obj; @@ -43,7 +43,7 @@ public: m_obj->increment(); return *this; } - ObjToken& operator=(const ObjToken& other) { + ObjToken& operator=(const ObjToken& other) noexcept { if (m_obj) m_obj->decrement(); m_obj = other.m_obj; @@ -51,26 +51,26 @@ public: m_obj->increment(); return *this; } - ObjToken& operator=(ObjToken&& other) { + ObjToken& operator=(ObjToken&& other) noexcept { if (m_obj) m_obj->decrement(); m_obj = other.m_obj; other.m_obj = nullptr; return *this; } - ~ObjToken() { + ~ObjToken() noexcept { if (m_obj) m_obj->decrement(); } - SubCls* get() const { return m_obj; } - SubCls* operator->() const { return m_obj; } - SubCls& operator*() const { return *m_obj; } + SubCls* get() const noexcept { return m_obj; } + SubCls* operator->() const noexcept { return m_obj; } + SubCls& operator*() const noexcept { return *m_obj; } template - T* cast() const { + T* cast() const noexcept { return static_cast(m_obj); } - operator bool() const { return m_obj != nullptr; } - void reset() { + operator bool() const noexcept { return m_obj != nullptr; } + void reset() noexcept { if (m_obj) m_obj->decrement(); m_obj = nullptr;