From 52c4aef72125ed834c444e3a1868aab827621bdb Mon Sep 17 00:00:00 2001 From: SamGondelman Date: Fri, 15 Dec 2017 16:23:53 -0800 Subject: [PATCH 1/5] keep _cacheFile alive --- libraries/gpu/src/gpu/Texture.h | 5 ++--- libraries/gpu/src/gpu/Texture_ktx.cpp | 29 +++++---------------------- 2 files changed, 7 insertions(+), 27 deletions(-) diff --git a/libraries/gpu/src/gpu/Texture.h b/libraries/gpu/src/gpu/Texture.h index 1877b494cf..26de5a0804 100755 --- a/libraries/gpu/src/gpu/Texture.h +++ b/libraries/gpu/src/gpu/Texture.h @@ -324,9 +324,8 @@ public: protected: std::shared_ptr maybeOpenFile() const; - mutable std::mutex _cacheFileCreateMutex; - mutable std::mutex _cacheFileWriteMutex; - mutable std::weak_ptr _cacheFile; + mutable std::mutex _cacheFileMutex; + mutable std::shared_ptr _cacheFile; std::string _filename; cache::FilePointer _cacheEntry; diff --git a/libraries/gpu/src/gpu/Texture_ktx.cpp b/libraries/gpu/src/gpu/Texture_ktx.cpp index 08fc4ec101..49e18aaf78 100644 --- a/libraries/gpu/src/gpu/Texture_ktx.cpp +++ b/libraries/gpu/src/gpu/Texture_ktx.cpp @@ -188,35 +188,17 @@ KtxStorage::KtxStorage(const std::string& filename) : _filename(filename) { } std::shared_ptr KtxStorage::maybeOpenFile() const { - // 1. Try to get the shared ptr - // 2. If it doesn't exist, grab the mutex around its creation - // 3. If it was created before we got the mutex, return it - // 4. Otherwise, create it - - std::shared_ptr file = _cacheFile.lock(); - if (file) { - return file; + if (!_cacheFile) { + _cacheFile = std::make_shared(_filename.c_str()); } - - { - std::lock_guard lock{ _cacheFileCreateMutex }; - - file = _cacheFile.lock(); - if (file) { - return file; - } - - file = std::make_shared(_filename.c_str()); - _cacheFile = file; - } - - return file; + return _cacheFile; } PixelsPointer KtxStorage::getMipFace(uint16 level, uint8 face) const { auto faceOffset = _ktxDescriptor->getMipFaceTexelsOffset(level, face); auto faceSize = _ktxDescriptor->getMipFaceTexelsSize(level, face); if (faceSize != 0 && faceOffset != 0) { + std::lock_guard lock(_cacheFileMutex); auto file = maybeOpenFile(); if (file) { auto storageView = file->createView(faceSize, faceOffset); @@ -262,6 +244,7 @@ void KtxStorage::assignMipData(uint16 level, const storage::StoragePointer& stor return; } + std::lock_guard lock(_cacheFileMutex); auto file = maybeOpenFile(); if (!file) { qWarning() << "Failed to open file to assign mip data " << QString::fromStdString(_filename); @@ -279,8 +262,6 @@ void KtxStorage::assignMipData(uint16 level, const storage::StoragePointer& stor imageData += ktx::IMAGE_SIZE_WIDTH; { - std::lock_guard lock { _cacheFileWriteMutex }; - if (level != _minMipLevelAvailable - 1) { qWarning() << "Invalid level to be stored"; return; From 3477aa57a8980413a66d7cd2ed3c55f03eeadd9a Mon Sep 17 00:00:00 2001 From: SamGondelman Date: Fri, 15 Dec 2017 16:36:07 -0800 Subject: [PATCH 2/5] remove extra recycle --- .../src/display-plugins/OpenGLDisplayPlugin.cpp | 5 ----- 1 file changed, 5 deletions(-) diff --git a/libraries/display-plugins/src/display-plugins/OpenGLDisplayPlugin.cpp b/libraries/display-plugins/src/display-plugins/OpenGLDisplayPlugin.cpp index 0993daaa8b..cb9d06dce1 100644 --- a/libraries/display-plugins/src/display-plugins/OpenGLDisplayPlugin.cpp +++ b/libraries/display-plugins/src/display-plugins/OpenGLDisplayPlugin.cpp @@ -685,11 +685,6 @@ void OpenGLDisplayPlugin::present() { } incrementPresentCount(); - { - PROFILE_RANGE_EX(render, "recycle", 0xff00ff00, frameId) - _gpuContext->recycle(); - } - if (_currentFrame) { { withPresentThreadLock([&] { From 3ae52c0e0ef2cc54b2c31c6e02ddb71e7a13b6c8 Mon Sep 17 00:00:00 2001 From: SamGondelman Date: Fri, 15 Dec 2017 17:52:48 -0800 Subject: [PATCH 3/5] clear all _cacheFiles on recycle --- libraries/gpu-gl/src/gpu/gl/GLBackend.cpp | 2 +- libraries/gpu/src/gpu/Texture.h | 7 ++++++- libraries/gpu/src/gpu/Texture_ktx.cpp | 18 ++++++++++++++++-- 3 files changed, 23 insertions(+), 4 deletions(-) diff --git a/libraries/gpu-gl/src/gpu/gl/GLBackend.cpp b/libraries/gpu-gl/src/gpu/gl/GLBackend.cpp index 6fb0d7b152..fa336665a5 100644 --- a/libraries/gpu-gl/src/gpu/gl/GLBackend.cpp +++ b/libraries/gpu-gl/src/gpu/gl/GLBackend.cpp @@ -772,7 +772,7 @@ void GLBackend::recycle() const { GLVariableAllocationSupport::manageMemory(); GLVariableAllocationSupport::_frameTexturesCreated = 0; - + Texture::KtxStorage::clearKtxFiles(); } void GLBackend::setCameraCorrection(const Mat4& correction) { diff --git a/libraries/gpu/src/gpu/Texture.h b/libraries/gpu/src/gpu/Texture.h index 26de5a0804..10edd895c0 100755 --- a/libraries/gpu/src/gpu/Texture.h +++ b/libraries/gpu/src/gpu/Texture.h @@ -321,10 +321,15 @@ public: void reset() override { } + // Don't keep files open forever. We close them at the beginning of each frame (GLBackend::recycle) + static std::vector, std::shared_ptr>> _cachedKtxFiles; + static std::mutex _cachedKtxFilesMutex; + static void clearKtxFiles(); + protected: std::shared_ptr maybeOpenFile() const; - mutable std::mutex _cacheFileMutex; + mutable std::shared_ptr _cacheFileMutex { std::make_shared() }; mutable std::shared_ptr _cacheFile; std::string _filename; diff --git a/libraries/gpu/src/gpu/Texture_ktx.cpp b/libraries/gpu/src/gpu/Texture_ktx.cpp index 49e18aaf78..46804b4b44 100644 --- a/libraries/gpu/src/gpu/Texture_ktx.cpp +++ b/libraries/gpu/src/gpu/Texture_ktx.cpp @@ -23,6 +23,9 @@ using namespace gpu; using PixelsPointer = Texture::PixelsPointer; using KtxStorage = Texture::KtxStorage; +std::vector, std::shared_ptr>> KtxStorage::_cachedKtxFiles; +std::mutex KtxStorage::_cachedKtxFilesMutex; + struct GPUKTXPayload { using Version = uint8; @@ -190,15 +193,26 @@ KtxStorage::KtxStorage(const std::string& filename) : _filename(filename) { std::shared_ptr KtxStorage::maybeOpenFile() const { if (!_cacheFile) { _cacheFile = std::make_shared(_filename.c_str()); + std::lock_guard lock(KtxStorage::_cachedKtxFilesMutex); + _cachedKtxFiles.emplace_back(_cacheFile, _cacheFileMutex); } return _cacheFile; } +void KtxStorage::clearKtxFiles() { + std::lock_guard lock(KtxStorage::_cachedKtxFilesMutex); + for (auto& cacheFileAndMutex : KtxStorage::_cachedKtxFiles) { + std::lock_guard lock(*(cacheFileAndMutex.second)); + cacheFileAndMutex.first.reset(); + } + _cachedKtxFiles.clear(); +} + PixelsPointer KtxStorage::getMipFace(uint16 level, uint8 face) const { auto faceOffset = _ktxDescriptor->getMipFaceTexelsOffset(level, face); auto faceSize = _ktxDescriptor->getMipFaceTexelsSize(level, face); if (faceSize != 0 && faceOffset != 0) { - std::lock_guard lock(_cacheFileMutex); + std::lock_guard lock(*_cacheFileMutex); auto file = maybeOpenFile(); if (file) { auto storageView = file->createView(faceSize, faceOffset); @@ -244,7 +258,7 @@ void KtxStorage::assignMipData(uint16 level, const storage::StoragePointer& stor return; } - std::lock_guard lock(_cacheFileMutex); + std::lock_guard lock(*_cacheFileMutex); auto file = maybeOpenFile(); if (!file) { qWarning() << "Failed to open file to assign mip data " << QString::fromStdString(_filename); From 6990dda48d9d4504f732871968a44da3ff9cd5df Mon Sep 17 00:00:00 2001 From: Sam Gondelman Date: Sun, 17 Dec 2017 15:56:34 -0800 Subject: [PATCH 4/5] cr and cleanup --- libraries/gpu-gl/src/gpu/gl/GLBackend.cpp | 2 +- libraries/gpu/src/gpu/Texture.h | 7 ++++--- libraries/gpu/src/gpu/Texture_ktx.cpp | 13 ++++++++----- 3 files changed, 13 insertions(+), 9 deletions(-) diff --git a/libraries/gpu-gl/src/gpu/gl/GLBackend.cpp b/libraries/gpu-gl/src/gpu/gl/GLBackend.cpp index fa336665a5..f93d430152 100644 --- a/libraries/gpu-gl/src/gpu/gl/GLBackend.cpp +++ b/libraries/gpu-gl/src/gpu/gl/GLBackend.cpp @@ -772,7 +772,7 @@ void GLBackend::recycle() const { GLVariableAllocationSupport::manageMemory(); GLVariableAllocationSupport::_frameTexturesCreated = 0; - Texture::KtxStorage::clearKtxFiles(); + Texture::KtxStorage::releaseOpenKtxFiles(); } void GLBackend::setCameraCorrection(const Mat4& correction) { diff --git a/libraries/gpu/src/gpu/Texture.h b/libraries/gpu/src/gpu/Texture.h index 10edd895c0..7c087fec2b 100755 --- a/libraries/gpu/src/gpu/Texture.h +++ b/libraries/gpu/src/gpu/Texture.h @@ -322,9 +322,7 @@ public: void reset() override { } // Don't keep files open forever. We close them at the beginning of each frame (GLBackend::recycle) - static std::vector, std::shared_ptr>> _cachedKtxFiles; - static std::mutex _cachedKtxFilesMutex; - static void clearKtxFiles(); + static void releaseOpenKtxFiles(); protected: std::shared_ptr maybeOpenFile() const; @@ -332,6 +330,9 @@ public: mutable std::shared_ptr _cacheFileMutex { std::make_shared() }; mutable std::shared_ptr _cacheFile; + static std::vector, std::shared_ptr>> _cachedKtxFiles; + static std::mutex _cachedKtxFilesMutex; + std::string _filename; cache::FilePointer _cacheEntry; std::atomic _minMipLevelAvailable; diff --git a/libraries/gpu/src/gpu/Texture_ktx.cpp b/libraries/gpu/src/gpu/Texture_ktx.cpp index 46804b4b44..8b054816bb 100644 --- a/libraries/gpu/src/gpu/Texture_ktx.cpp +++ b/libraries/gpu/src/gpu/Texture_ktx.cpp @@ -193,19 +193,22 @@ KtxStorage::KtxStorage(const std::string& filename) : _filename(filename) { std::shared_ptr KtxStorage::maybeOpenFile() const { if (!_cacheFile) { _cacheFile = std::make_shared(_filename.c_str()); - std::lock_guard lock(KtxStorage::_cachedKtxFilesMutex); + std::lock_guard lock(_cachedKtxFilesMutex); _cachedKtxFiles.emplace_back(_cacheFile, _cacheFileMutex); } return _cacheFile; } -void KtxStorage::clearKtxFiles() { - std::lock_guard lock(KtxStorage::_cachedKtxFilesMutex); - for (auto& cacheFileAndMutex : KtxStorage::_cachedKtxFiles) { +void KtxStorage::releaseOpenKtxFiles() { + std::vector, std::shared_ptr>> localKtxFiles; + { + std::lock_guard lock(_cachedKtxFilesMutex); + localKtxFiles.swap(_cachedKtxFiles); + } + for (auto& cacheFileAndMutex : localKtxFiles) { std::lock_guard lock(*(cacheFileAndMutex.second)); cacheFileAndMutex.first.reset(); } - _cachedKtxFiles.clear(); } PixelsPointer KtxStorage::getMipFace(uint16 level, uint8 face) const { From 6814dfcbfc5eee145e6201ebb86de729842952bf Mon Sep 17 00:00:00 2001 From: Sam Gondelman Date: Sun, 17 Dec 2017 22:09:20 -0800 Subject: [PATCH 5/5] weak_ptr _cacheFile --- libraries/gpu/src/gpu/Texture.h | 2 +- libraries/gpu/src/gpu/Texture_ktx.cpp | 22 +++++++++++++++++----- 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/libraries/gpu/src/gpu/Texture.h b/libraries/gpu/src/gpu/Texture.h index 7c087fec2b..06208179e0 100755 --- a/libraries/gpu/src/gpu/Texture.h +++ b/libraries/gpu/src/gpu/Texture.h @@ -328,7 +328,7 @@ public: std::shared_ptr maybeOpenFile() const; mutable std::shared_ptr _cacheFileMutex { std::make_shared() }; - mutable std::shared_ptr _cacheFile; + mutable std::weak_ptr _cacheFile; static std::vector, std::shared_ptr>> _cachedKtxFiles; static std::mutex _cachedKtxFilesMutex; diff --git a/libraries/gpu/src/gpu/Texture_ktx.cpp b/libraries/gpu/src/gpu/Texture_ktx.cpp index 8b054816bb..883d9abf15 100644 --- a/libraries/gpu/src/gpu/Texture_ktx.cpp +++ b/libraries/gpu/src/gpu/Texture_ktx.cpp @@ -190,13 +190,25 @@ KtxStorage::KtxStorage(const std::string& filename) : _filename(filename) { } } +// maybeOpenFile should be called with _cacheFileMutex already held to avoid modifying the file from multiple threads std::shared_ptr KtxStorage::maybeOpenFile() const { - if (!_cacheFile) { - _cacheFile = std::make_shared(_filename.c_str()); - std::lock_guard lock(_cachedKtxFilesMutex); - _cachedKtxFiles.emplace_back(_cacheFile, _cacheFileMutex); + // Try to get the shared_ptr + std::shared_ptr file = _cacheFile.lock(); + if (file) { + return file; } - return _cacheFile; + + // If the file isn't open, create it and save a weak_ptr to it + file = std::make_shared(_filename.c_str()); + _cacheFile = file; + + { + // Add the shared_ptr to the global list of open KTX files, to be released at the beginning of the next present thread frame + std::lock_guard lock(_cachedKtxFilesMutex); + _cachedKtxFiles.emplace_back(file, _cacheFileMutex); + } + + return file; } void KtxStorage::releaseOpenKtxFiles() {