From 2857207cae8ccd8677ef3586add44102790dea92 Mon Sep 17 00:00:00 2001 From: binary1248 Date: Sun, 27 Nov 2016 18:31:21 +0100 Subject: [PATCH] Replaced TransientContextLock implementation with a more elaborate one which relies on locking a single mutex and thus avoids lock order inversion. Fixes #1165. --- src/SFML/Window/GlContext.cpp | 206 +++++++++++++++++++++++++++++------------ src/SFML/Window/GlContext.hpp | 25 +++-- src/SFML/Window/GlResource.cpp | 48 +--------- 3 files changed, 161 insertions(+), 118 deletions(-) diff --git a/src/SFML/Window/GlContext.cpp b/src/SFML/Window/GlContext.cpp index 8ae4b3ab..d773ed00 100644 --- a/src/SFML/Window/GlContext.cpp +++ b/src/SFML/Window/GlContext.cpp @@ -26,6 +26,7 @@ // Headers //////////////////////////////////////////////////////////// #include +#include #include #include #include @@ -131,18 +132,70 @@ namespace // We need to make sure that no operating system context // or pixel format operations are performed simultaneously // This mutex is also used to protect the shared context - // from being locked on multiple threads + // from being locked on multiple threads and for managing + // the resource count sf::Mutex mutex; + // OpenGL resources counter + unsigned int resourceCount = 0; + // This per-thread variable holds the current context for each thread sf::ThreadLocalPtr currentContext(NULL); // The hidden, inactive context that will be shared with all other contexts ContextType* sharedContext = NULL; - // This per-thread variable is set to point to the shared context - // if we had to acquire it when a TransientContextLock was required - sf::ThreadLocalPtr currentSharedContext(NULL); + // This structure contains all the state necessary to + // track TransientContext usage + struct TransientContext : private sf::NonCopyable + { + //////////////////////////////////////////////////////////// + /// \brief Constructor + /// + //////////////////////////////////////////////////////////// + TransientContext() : + referenceCount (0), + context (0), + sharedContextLock(0), + useSharedContext (false) + { + if (resourceCount == 0) + { + context = new sf::Context; + } + else if (!currentContext) + { + sharedContextLock = new sf::Lock(mutex); + useSharedContext = true; + sharedContext->setActive(true); + } + } + + //////////////////////////////////////////////////////////// + /// \brief Destructor + /// + //////////////////////////////////////////////////////////// + ~TransientContext() + { + if (useSharedContext) + sharedContext->setActive(false); + + delete sharedContextLock; + delete context; + } + + /////////////////////////////////////////////////////////// + // Member data + //////////////////////////////////////////////////////////// + unsigned int referenceCount; + sf::Context* context; + sf::Lock* sharedContextLock; + bool useSharedContext; + }; + + // This per-thread variable tracks if and how a transient + // context is currently being used on the current thread + sf::ThreadLocalPtr transientContext(NULL); // Supported OpenGL extensions std::vector extensions; @@ -154,107 +207,138 @@ namespace sf namespace priv { //////////////////////////////////////////////////////////// -void GlContext::globalInit() +void GlContext::initResource() { + // Protect from concurrent access Lock lock(mutex); - if (sharedContext) - return; + // If this is the very first resource, trigger the global context initialization + if (resourceCount == 0) + { + if (sharedContext) + { + // Increment the resources counter + resourceCount++; - // Create the shared context - sharedContext = new ContextType(NULL); - sharedContext->initialize(ContextSettings()); + return; + } - // Load our extensions vector - extensions.clear(); + // Create the shared context + sharedContext = new ContextType(NULL); + sharedContext->initialize(ContextSettings()); - // Check whether a >= 3.0 context is available - int majorVersion = 0; - glGetIntegerv(GL_MAJOR_VERSION, &majorVersion); + // Load our extensions vector + extensions.clear(); - if (glGetError() == GL_INVALID_ENUM) - { - // Try to load the < 3.0 way - const char* extensionString = reinterpret_cast(glGetString(GL_EXTENSIONS)); + // Check whether a >= 3.0 context is available + int majorVersion = 0; + glGetIntegerv(GL_MAJOR_VERSION, &majorVersion); - do + if (glGetError() == GL_INVALID_ENUM) { - const char* extension = extensionString; + // Try to load the < 3.0 way + const char* extensionString = reinterpret_cast(glGetString(GL_EXTENSIONS)); - while(*extensionString && (*extensionString != ' ')) - extensionString++; + do + { + const char* extension = extensionString; - extensions.push_back(std::string(extension, extensionString)); - } - while (*extensionString++); - } - else - { - // Try to load the >= 3.0 way - glGetStringiFuncType glGetStringiFunc = NULL; - glGetStringiFunc = reinterpret_cast(getFunction("glGetStringi")); + while(*extensionString && (*extensionString != ' ')) + extensionString++; - if (glGetStringiFunc) + extensions.push_back(std::string(extension, extensionString)); + } + while (*extensionString++); + } + else { - int numExtensions = 0; - glGetIntegerv(GL_NUM_EXTENSIONS, &numExtensions); + // Try to load the >= 3.0 way + glGetStringiFuncType glGetStringiFunc = NULL; + glGetStringiFunc = reinterpret_cast(getFunction("glGetStringi")); - if (numExtensions) + if (glGetStringiFunc) { - for (unsigned int i = 0; i < static_cast(numExtensions); ++i) + int numExtensions = 0; + glGetIntegerv(GL_NUM_EXTENSIONS, &numExtensions); + + if (numExtensions) { - const char* extensionString = reinterpret_cast(glGetStringiFunc(GL_EXTENSIONS, i)); + for (unsigned int i = 0; i < static_cast(numExtensions); ++i) + { + const char* extensionString = reinterpret_cast(glGetStringiFunc(GL_EXTENSIONS, i)); - extensions.push_back(extensionString); + extensions.push_back(extensionString); + } } } } + + // Deactivate the shared context so that others can activate it when necessary + sharedContext->setActive(false); } - // Deactivate the shared context so that others can activate it when necessary - sharedContext->setActive(false); + // Increment the resources counter + resourceCount++; } //////////////////////////////////////////////////////////// -void GlContext::globalCleanup() +void GlContext::cleanupResource() { + // Protect from concurrent access Lock lock(mutex); - if (!sharedContext) - return; + // Decrement the resources counter + resourceCount--; - // Destroy the shared context - delete sharedContext; - sharedContext = NULL; + // If there's no more resource alive, we can trigger the global context cleanup + if (resourceCount == 0) + { + if (!sharedContext) + return; + + // Destroy the shared context + delete sharedContext; + sharedContext = NULL; + } } //////////////////////////////////////////////////////////// void GlContext::acquireTransientContext() { - // If a capable context is already active on this thread - // there is no need to use the shared context for the operation - if (currentContext) - { - currentSharedContext = NULL; - return; - } + // Protect from concurrent access + Lock lock(mutex); - mutex.lock(); - currentSharedContext = sharedContext; - sharedContext->setActive(true); + // If this is the first TransientContextLock on this thread + // construct the state object + if (!transientContext) + transientContext = new TransientContext; + + // Increase the reference count + transientContext->referenceCount++; } //////////////////////////////////////////////////////////// void GlContext::releaseTransientContext() { - if (!currentSharedContext) - return; + // Protect from concurrent access + Lock lock(mutex); + + // Make sure a matching acquireTransientContext() was called + assert(transientContext); - sharedContext->setActive(false); - mutex.unlock(); + // Decrease the reference count + transientContext->referenceCount--; + + // If this is the last TransientContextLock that is released + // destroy the state object + if (transientContext->referenceCount == 0) + { + delete transientContext; + transientContext = NULL; + } } diff --git a/src/SFML/Window/GlContext.hpp b/src/SFML/Window/GlContext.hpp index 55b6c1f1..757c2dc1 100644 --- a/src/SFML/Window/GlContext.hpp +++ b/src/SFML/Window/GlContext.hpp @@ -49,28 +49,25 @@ class GlContext : NonCopyable public: //////////////////////////////////////////////////////////// - /// \brief Perform the global initialization + /// \brief Perform resource initialization /// - /// This function is called once, before the very first OpenGL - /// resource is created. It makes sure that everything is ready - /// for contexts to work properly. - /// Note: this function doesn't need to be thread-safe, as it - /// can be called only once. + /// This function is called every time an OpenGL resource is + /// created. When the first resource is initialized, it makes + /// sure that everything is ready for contexts to work properly. /// //////////////////////////////////////////////////////////// - static void globalInit(); + static void initResource(); //////////////////////////////////////////////////////////// - /// \brief Perform the global cleanup + /// \brief Perform resource cleanup /// - /// This function is called after the very last OpenGL resource - /// is destroyed. It makes sure that everything that was - /// created by initialize() is properly released. - /// Note: this function doesn't need to be thread-safe, as it - /// can be called only once. + /// This function is called every time an OpenGL resource is + /// destroyed. When the last resource is destroyed, it makes + /// sure that everything that was created by initResource() + /// is properly released. /// //////////////////////////////////////////////////////////// - static void globalCleanup(); + static void cleanupResource(); //////////////////////////////////////////////////////////// /// \brief Acquires a context for short-term use on the current thread diff --git a/src/SFML/Window/GlResource.cpp b/src/SFML/Window/GlResource.cpp index e9a9ecc9..e8169954 100644 --- a/src/SFML/Window/GlResource.cpp +++ b/src/SFML/Window/GlResource.cpp @@ -27,17 +27,6 @@ //////////////////////////////////////////////////////////// #include #include -#include -#include -#include - - -namespace -{ - // OpenGL resources counter and its mutex - unsigned int count = 0; - sf::Mutex mutex; -} namespace sf @@ -45,37 +34,21 @@ namespace sf //////////////////////////////////////////////////////////// GlResource::GlResource() { - // Protect from concurrent access - Lock lock(mutex); - - // If this is the very first resource, trigger the global context initialization - if (count == 0) - priv::GlContext::globalInit(); - - // Increment the resources counter - count++; + priv::GlContext::initResource(); } //////////////////////////////////////////////////////////// GlResource::~GlResource() { - // Protect from concurrent access - Lock lock(mutex); - - // Decrement the resources counter - count--; - - // If there's no more resource alive, we can trigger the global context cleanup - if (count == 0) - priv::GlContext::globalCleanup(); + priv::GlContext::cleanupResource(); } //////////////////////////////////////////////////////////// void GlResource::ensureGlContext() { - // Empty function for ABI compatibility, use acquireTransientContext instead + // Empty function for ABI compatibility, use TransientContextLock instead } @@ -83,13 +56,8 @@ void GlResource::ensureGlContext() GlResource::TransientContextLock::TransientContextLock() : m_context(0) { - Lock lock(mutex); - - if (count == 0) - { - m_context = new Context; - return; - } + // m_context is no longer used + // Remove it when ABI can be broken priv::GlContext::acquireTransientContext(); } @@ -98,12 +66,6 @@ m_context(0) //////////////////////////////////////////////////////////// GlResource::TransientContextLock::~TransientContextLock() { - if (m_context) - { - delete m_context; - return; - } - priv::GlContext::releaseTransientContext(); } -- 2.11.0