From 217d484cbadd51006fa47048574b2c4edb74526a Mon Sep 17 00:00:00 2001 From: PixelyIon Date: Mon, 27 Jun 2022 21:17:49 +0530 Subject: [PATCH] Abstract `TextureView`/`BufferDelegate` locking into `LockableSharedPtr` An atomic transactional loop was performed on the backing `std::shared_ptr` inside `BufferView`/`TextureView`'s `lock`/`LockWithTag`/`try_lock` functions, these locks utilized `std::atomic_load` for atomically loading the value from the `shared_ptr` recursively till it was the same value pre/post-locking. This commit abstracts the locking functionality of `TextureView`/`BufferDelegate` into `LockableSharedPtr` to avoid code duplication and removes the usage of `std::atomic_load` in either case as it is not necessary due to the implicit memory barrier provided by locking a mutex. --- .../cpp/skyline/common/lockable_shared_ptr.h | 78 +++++++++++++++++++ app/src/main/cpp/skyline/gpu/buffer.cpp | 44 ++--------- app/src/main/cpp/skyline/gpu/buffer.h | 3 +- .../main/cpp/skyline/gpu/buffer_manager.cpp | 2 +- .../main/cpp/skyline/gpu/texture/texture.cpp | 44 ++--------- .../main/cpp/skyline/gpu/texture/texture.h | 3 +- 6 files changed, 97 insertions(+), 77 deletions(-) create mode 100644 app/src/main/cpp/skyline/common/lockable_shared_ptr.h diff --git a/app/src/main/cpp/skyline/common/lockable_shared_ptr.h b/app/src/main/cpp/skyline/common/lockable_shared_ptr.h new file mode 100644 index 00000000..a0ec9488 --- /dev/null +++ b/app/src/main/cpp/skyline/common/lockable_shared_ptr.h @@ -0,0 +1,78 @@ +// SPDX-License-Identifier: MPL-2.0 +// Copyright © 2022 Skyline Team and Contributors (https://github.com/skyline-emu/) + +#pragma once + +#include +#include + +namespace skyline { + /** + * @brief A wrapper around a shared_ptr which can be utilized to perform transactional atomic operations to lock the underlying resource and attain stability in the pointer value + * @note Any operations directly accessing the value are **NOT** atomic and should be done after a locking transaction + */ + template + class LockableSharedPtr : public std::shared_ptr { + public: + using std::shared_ptr::shared_ptr; + using std::shared_ptr::operator=; + + LockableSharedPtr(std::shared_ptr &&ptr) : std::shared_ptr{std::move(ptr)} {} + + private: + /** + * @brief A lock function for the underlying object that conforms to the BasicLockable named requirement + */ + static void DefaultLockFunction(Type *object) { + object->lock(); + } + + /** + * @brief An unlock function for the underlying object that conforms to the BasicLockable named requirement + */ + static void DefaultUnlockFunction(Type *object) { + object->unlock(); + } + + /** + * @brief A try_lock function for the underlying object that conforms to the Lockable named requirement + */ + static bool DefaultTryLockFunction(Type *object) { + return object->try_lock(); + } + + public: + /** + * @brief Locks the underlying object with the supplied lock/unlock functions + */ + template + void Lock(LockFunction lock = DefaultLockFunction, UnlockFunction unlock = DefaultUnlockFunction) const { + while (true) { + auto object{this->get()}; + lock(object); + + if (this->get() == object) + return; + + unlock(object); + } + } + + /** + * @brief Attempts to lock the underlying object with the supplied try_lock/unlock functions + */ + template + bool TryLock(TryLockFunction tryLock = DefaultTryLockFunction, UnlockFunction unlock = DefaultUnlockFunction) const { + while (true) { + auto object{this->get()}; + bool wasLocked{tryLock(object)}; + + if (this->get() == object) + return wasLocked; + + if (wasLocked) + unlock(object); + } + } + }; +} diff --git a/app/src/main/cpp/skyline/gpu/buffer.cpp b/app/src/main/cpp/skyline/gpu/buffer.cpp index b5678207..6fd293b0 100644 --- a/app/src/main/cpp/skyline/gpu/buffer.cpp +++ b/app/src/main/cpp/skyline/gpu/buffer.cpp @@ -275,32 +275,15 @@ namespace skyline::gpu { } void Buffer::BufferDelegate::lock() { - auto lBuffer{std::atomic_load(&buffer)}; - while (true) { - lBuffer->lock(); - - auto latestBacking{std::atomic_load(&buffer)}; - if (lBuffer == latestBacking) - return; - - lBuffer->unlock(); - lBuffer = latestBacking; - } + buffer.Lock(); } bool Buffer::BufferDelegate::LockWithTag(ContextTag pTag) { - auto lBuffer{std::atomic_load(&buffer)}; - while (true) { - bool didLock{lBuffer->LockWithTag(pTag)}; - - auto latestBacking{std::atomic_load(&buffer)}; - if (lBuffer == latestBacking) - return didLock; - - if (didLock) - lBuffer->unlock(); - lBuffer = latestBacking; - } + bool result{}; + buffer.Lock([pTag, &result](Buffer* pBuffer) { + result = pBuffer->LockWithTag(pTag); + }); + return result; } void Buffer::BufferDelegate::unlock() { @@ -308,20 +291,7 @@ namespace skyline::gpu { } bool Buffer::BufferDelegate::try_lock() { - auto lBuffer{std::atomic_load(&buffer)}; - while (true) { - bool success{lBuffer->try_lock()}; - - auto latestBuffer{std::atomic_load(&buffer)}; - if (lBuffer == latestBuffer) - // We want to ensure that the try_lock() was on the latest backing and not on an outdated one - return success; - - if (success) - // We only unlock() if the try_lock() was successful and we acquired the mutex - lBuffer->unlock(); - lBuffer = latestBuffer; - } + return buffer.TryLock(); } BufferView::BufferView(std::shared_ptr buffer, const Buffer::BufferViewStorage *view) : bufferDelegate(std::make_shared(std::move(buffer), view)) {} diff --git a/app/src/main/cpp/skyline/gpu/buffer.h b/app/src/main/cpp/skyline/gpu/buffer.h index 8b08d2bd..f62d1764 100644 --- a/app/src/main/cpp/skyline/gpu/buffer.h +++ b/app/src/main/cpp/skyline/gpu/buffer.h @@ -5,6 +5,7 @@ #include #include +#include #include #include #include "memory_manager.h" @@ -94,7 +95,7 @@ namespace skyline::gpu { * @note This class conforms to the Lockable and BasicLockable C++ named requirements */ struct BufferDelegate { - std::shared_ptr buffer; + LockableSharedPtr buffer; const Buffer::BufferViewStorage *view; std::function &)> usageCallback; std::list::iterator iterator; diff --git a/app/src/main/cpp/skyline/gpu/buffer_manager.cpp b/app/src/main/cpp/skyline/gpu/buffer_manager.cpp index 1111672e..652b36a2 100644 --- a/app/src/main/cpp/skyline/gpu/buffer_manager.cpp +++ b/app/src/main/cpp/skyline/gpu/buffer_manager.cpp @@ -86,7 +86,7 @@ namespace skyline::gpu { // Transfer all delegates references from the overlapping buffer to the new buffer for (auto &delegate : overlap->delegates) { - atomic_exchange(&delegate->buffer, newBuffer); + delegate->buffer = newBuffer; if (delegate->usageCallback) delegate->usageCallback(*delegate->view, newBuffer); } diff --git a/app/src/main/cpp/skyline/gpu/texture/texture.cpp b/app/src/main/cpp/skyline/gpu/texture/texture.cpp index a18d64e5..eae085e4 100644 --- a/app/src/main/cpp/skyline/gpu/texture/texture.cpp +++ b/app/src/main/cpp/skyline/gpu/texture/texture.cpp @@ -93,32 +93,15 @@ namespace skyline::gpu { } void TextureView::lock() { - auto backing{std::atomic_load(&texture)}; - while (true) { - backing->lock(); - - auto latestBacking{std::atomic_load(&texture)}; - if (backing == latestBacking) - return; - - backing->unlock(); - backing = latestBacking; - } + texture.Lock(); } bool TextureView::LockWithTag(ContextTag tag) { - auto backing{std::atomic_load(&texture)}; - while (true) { - bool didLock{backing->LockWithTag(tag)}; - - auto latestBacking{std::atomic_load(&texture)}; - if (backing == latestBacking) - return didLock; - - if (didLock) - backing->unlock(); - backing = latestBacking; - } + bool result{}; + texture.Lock([tag, &result](Texture* pTexture) { + result = pTexture->LockWithTag(tag); + }); + return result; } void TextureView::unlock() { @@ -126,20 +109,7 @@ namespace skyline::gpu { } bool TextureView::try_lock() { - auto backing{std::atomic_load(&texture)}; - while (true) { - bool success{backing->try_lock()}; - - auto latestBacking{std::atomic_load(&texture)}; - if (backing == latestBacking) - // We want to ensure that the try_lock() was on the latest backing and not on an outdated one - return success; - - if (success) - // We only unlock() if the try_lock() was successful and we acquired the mutex - backing->unlock(); - backing = latestBacking; - } + return texture.TryLock(); } void Texture::SetupGuestMappings() { diff --git a/app/src/main/cpp/skyline/gpu/texture/texture.h b/app/src/main/cpp/skyline/gpu/texture/texture.h index e1006afe..310579b1 100644 --- a/app/src/main/cpp/skyline/gpu/texture/texture.h +++ b/app/src/main/cpp/skyline/gpu/texture/texture.h @@ -3,6 +3,7 @@ #pragma once +#include #include #include #include @@ -298,7 +299,7 @@ namespace skyline::gpu { vk::ImageView vkView{}; public: - std::shared_ptr texture; + LockableSharedPtr texture; vk::ImageViewType type; texture::Format format; vk::ComponentMapping mapping;