From 80cf6930cc65fd391881754d3dd9e39d69d0978a Mon Sep 17 00:00:00 2001 From: Nav Date: Fri, 2 Jun 2023 00:16:58 +0100 Subject: [PATCH] Refactored template class for synchronised resources --- src/EventManager/EventListener.cpp | 28 +++---- src/EventManager/EventListener.hpp | 58 ++++++-------- src/Helpers/SyncSafe.hpp | 42 ---------- src/Helpers/Synchronised.hpp | 78 +++++++++++++++++++ src/Insight/InsightWorker/InsightWorker.cpp | 26 +++---- src/Insight/InsightWorker/InsightWorker.hpp | 6 +- src/SignalHandler/SignalHandler.hpp | 2 +- .../TargetControllerComponent.cpp | 33 ++++---- .../TargetControllerComponent.hpp | 8 +- 9 files changed, 146 insertions(+), 135 deletions(-) delete mode 100644 src/Helpers/SyncSafe.hpp create mode 100644 src/Helpers/Synchronised.hpp diff --git a/src/EventManager/EventListener.cpp b/src/EventManager/EventListener.cpp index 7abd971d..06a83791 100644 --- a/src/EventManager/EventListener.cpp +++ b/src/EventManager/EventListener.cpp @@ -7,8 +7,7 @@ namespace Bloom using namespace Bloom::Events; std::set EventListener::getRegisteredEventTypes() { - const auto lock = this->registeredEventTypes.acquireLock(); - return this->registeredEventTypes.getValue(); + return *(this->registeredEventTypes.accessor()); } void EventListener::registerEvent(SharedGenericEventPointer event) { @@ -17,8 +16,8 @@ namespace Bloom + this->name ); - const auto queueLock = this->eventQueueByEventType.acquireLock(); - auto& eventQueueByType = this->eventQueueByEventType.getValue(); + auto eventQueueByTypeAccessor = this->eventQueueByEventType.accessor(); + auto& eventQueueByType = *(eventQueueByTypeAccessor); eventQueueByType[event->getType()].push(std::move(event)); this->eventQueueByEventTypeCV.notify_all(); @@ -30,8 +29,9 @@ namespace Bloom void EventListener::waitAndDispatch(int msTimeout) { { - auto queueLock = this->eventQueueByEventType.acquireLock(); - const auto& eventQueueByType = this->eventQueueByEventType.getValue(); + auto queueLock = this->eventQueueByEventType.lock(); + const auto& eventQueueByType = this->eventQueueByEventType.unsafeReference(); + const auto registeredEventTypes = this->getRegisteredEventTypes(); std::optional event; @@ -62,8 +62,12 @@ namespace Bloom auto callbacks = std::vector>(); { - const auto mappingLock = this->eventTypeToCallbacksMapping.acquireLock(); - callbacks = this->eventTypeToCallbacksMapping.getValue().find(event->getType())->second; + const auto callbackMappingAccessor = this->eventTypeToCallbacksMapping.accessor(); + + const auto callbacksIt = callbackMappingAccessor->find(event->getType()); + if (callbacksIt != callbackMappingAccessor->end()) { + callbacks = callbacksIt->second; + } } for (auto& callback : callbacks) { @@ -80,11 +84,10 @@ namespace Bloom } std::vector EventListener::getEvents() { - const auto queueLock = this->eventQueueByEventType.acquireLock(); - auto& eventQueueByType = this->eventQueueByEventType.getValue(); + auto eventQueueByType = this->eventQueueByEventType.accessor(); std::vector output; - for (auto& eventQueue: eventQueueByType) { + for (auto& eventQueue: *eventQueueByType) { while (!eventQueue.second.empty()) { output.push_back(std::move(eventQueue.second.front())); eventQueue.second.pop(); @@ -103,7 +106,6 @@ namespace Bloom } void EventListener::clearAllCallbacks() { - const auto lock = this->eventTypeToCallbacksMapping.acquireLock(); - this->eventTypeToCallbacksMapping.getValue().clear(); + this->eventTypeToCallbacksMapping.accessor()->clear(); } } diff --git a/src/EventManager/EventListener.hpp b/src/EventManager/EventListener.hpp index 505118d9..2c74b261 100644 --- a/src/EventManager/EventListener.hpp +++ b/src/EventManager/EventListener.hpp @@ -14,7 +14,7 @@ #include #include "src/EventManager/Events/Events.hpp" -#include "src/Helpers/SyncSafe.hpp" +#include "src/Helpers/Synchronised.hpp" #include "src/Helpers/NotifierInterface.hpp" namespace Bloom @@ -55,13 +55,8 @@ namespace Bloom */ std::set getRegisteredEventTypes(); - template - bool isEventTypeRegistered() { - return this->registeredEventTypes.getValue().contains(EventType::type); - } - bool isEventTypeRegistered(Events::EventType eventType) { - return this->registeredEventTypes.getValue().contains(eventType); + return this->registeredEventTypes.accessor()->contains(eventType); }; /** @@ -75,14 +70,12 @@ namespace Bloom */ template void registerEventType() { - const auto registeredEventTypesLock = this->registeredEventTypes.acquireLock(); - this->registeredEventTypes.getValue().insert(EventType::type); + this->registeredEventTypes.accessor()->insert(EventType::type); } template void deRegisterEventType() { - const auto registeredEventTypesLock = this->registeredEventTypes.acquireLock(); - this->registeredEventTypes.getValue().erase(EventType::type); + this->registeredEventTypes.accessor()->erase(EventType::type); } /** @@ -117,8 +110,8 @@ namespace Bloom } ; - const auto mappingLock = this->eventTypeToCallbacksMapping.acquireLock(); - auto& mapping = this->eventTypeToCallbacksMapping.getValue(); + auto mappingAccessor = this->eventTypeToCallbacksMapping.accessor(); + auto& mapping = *(mappingAccessor); mapping[EventType::type].push_back(parentCallback); this->template registerEventType(); @@ -137,24 +130,19 @@ namespace Bloom ); { - const auto mappingLock = this->eventTypeToCallbacksMapping.acquireLock(); - auto& mapping = this->eventTypeToCallbacksMapping.getValue(); + auto mappingAccessor = this->eventTypeToCallbacksMapping.accessor(); + auto& mapping = *(mappingAccessor); if (mapping.contains(EventType::type)) { mapping.at(EventType::type).clear(); } } - { - auto registeredEventTypesLock = this->registeredEventTypes.acquireLock(); - this->registeredEventTypes.getValue().erase(EventType::type); - } + this->registeredEventTypes.accessor()->erase(EventType::type); - const auto queueLock = this->eventQueueByEventType.acquireLock(); - auto& eventQueueByType = this->eventQueueByEventType.getValue(); - - if (eventQueueByType.contains(EventType::type)) { - eventQueueByType.erase(EventType::type); + auto eventQueueByType = this->eventQueueByEventType.accessor(); + if (eventQueueByType->contains(EventType::type)) { + eventQueueByType->erase(EventType::type); } } @@ -210,8 +198,8 @@ namespace Bloom ReturnType output = std::nullopt; - auto queueLock = this->eventQueueByEventType.acquireLock(); - auto& eventQueueByType = this->eventQueueByEventType.getValue(); + auto queueLock = this->eventQueueByEventType.lock(); + auto& eventQueueByType = this->eventQueueByEventType.unsafeReference(); auto eventTypes = std::set({EventTypeA::type}); auto eventTypesToDeRegister = std::set(); @@ -233,12 +221,11 @@ namespace Bloom } { - auto registeredEventTypesLock = this->registeredEventTypes.acquireLock(); - auto& registeredEventTypes = this->registeredEventTypes.getValue(); + auto registeredEventTypes = this->registeredEventTypes.accessor(); for (const auto& eventType : eventTypes) { - if (!registeredEventTypes.contains(eventType)) { - registeredEventTypes.insert(eventType); + if (!registeredEventTypes->contains(eventType)) { + registeredEventTypes->insert(eventType); eventTypesToDeRegister.insert(eventType); } } @@ -270,11 +257,10 @@ namespace Bloom } if (!eventTypesToDeRegister.empty()) { - auto registeredEventTypesLock = this->registeredEventTypes.acquireLock(); - auto& registeredEventTypes = this->registeredEventTypes.getValue(); + auto registeredEventTypes = this->registeredEventTypes.accessor(); for (const auto& eventType : eventTypesToDeRegister) { - registeredEventTypes.erase(eventType); + registeredEventTypes->erase(eventType); } } @@ -346,7 +332,7 @@ namespace Bloom * Events are grouped by event type, and removed from their queue just *before* the dispatching to * registered handlers begins. */ - SyncSafe>> eventQueueByEventType; + Synchronised>> eventQueueByEventType; std::condition_variable eventQueueByEventTypeCV; /** @@ -357,8 +343,8 @@ namespace Bloom * we perform a downcast before invoking the callback. See EventListener::registerCallbackForEventType() * for more) */ - SyncSafe>>> eventTypeToCallbacksMapping; - SyncSafe> registeredEventTypes; + Synchronised>>> eventTypeToCallbacksMapping; + Synchronised> registeredEventTypes; NotifierInterface* interruptEventNotifier = nullptr; diff --git a/src/Helpers/SyncSafe.hpp b/src/Helpers/SyncSafe.hpp deleted file mode 100644 index dcf82419..00000000 --- a/src/Helpers/SyncSafe.hpp +++ /dev/null @@ -1,42 +0,0 @@ -#pragma once - -#include - -namespace Bloom -{ - /** - * Template for synchronization safe types. - * - * Just a convenient template that allows us to create thread safe types without having to write - * the bloat of mutexes, unique_locks, etc etc. - * - * @tparam Type - */ - template - class SyncSafe - { - public: - SyncSafe() = default; - - explicit SyncSafe(Type value) - : value(value) - {} - - void setValue(const Type& value) { - auto lock = std::unique_lock(this->mutex); - this->value = value; - } - - Type& getValue() { - return this->value; - } - - std::unique_lock acquireLock() { - return std::unique_lock(this->mutex); - } - - private: - Type value; - std::mutex mutex; - }; -} diff --git a/src/Helpers/Synchronised.hpp b/src/Helpers/Synchronised.hpp new file mode 100644 index 00000000..45131f1a --- /dev/null +++ b/src/Helpers/Synchronised.hpp @@ -0,0 +1,78 @@ +#pragma once + +#include +#include + +namespace Bloom +{ + /** + * Wrapper for synchronised access to a resource. + * + * @tparam Type + */ + template + class Synchronised + { + public: + class Accessor + { + public: + constexpr Accessor(std::mutex& mutex, Type& value) + : lock(std::unique_lock(mutex)) + , value(value) + {} + + constexpr Type* operator -> () noexcept { + return &(this->value); + } + + constexpr const Type* operator -> () const noexcept { + return &(this->value); + } + + constexpr Type& operator * () noexcept { + return this->value; + } + + constexpr const Type& operator * () const noexcept { + return this->value; + } + + private: + std::unique_lock lock; + Type& value; + }; + + Synchronised() = default; + + explicit Synchronised(Type value) + : value(std::move(value)) + {} + + Accessor accessor() { + return Accessor(this->mutex, this->value); + } + + /** + * Don't use this unless you already hold a raw (not managed by an Accessor) lock to the contained value. + * + * This should only be used in instances where you need to hold a raw lock, like in the `stop_waiting` + * predicate function for a call to std::condition_variable::wait(). + * + * In all other instances, you should use Synchronised::accessor(). + * + * @return + */ + Type& unsafeReference() { + return this->value; + } + + std::unique_lock lock() { + return std::unique_lock(this->mutex); + } + + private: + Type value; + std::mutex mutex; + }; +} diff --git a/src/Insight/InsightWorker/InsightWorker.cpp b/src/Insight/InsightWorker/InsightWorker.cpp index 55b4593e..596e32d3 100644 --- a/src/Insight/InsightWorker/InsightWorker.cpp +++ b/src/Insight/InsightWorker/InsightWorker.cpp @@ -36,26 +36,21 @@ namespace Bloom void InsightWorker::queueTask(const QSharedPointer& task) { task->moveToThread(nullptr); - { - const auto taskQueueLock = InsightWorker::queuedTasksById.acquireLock(); - InsightWorker::queuedTasksById.getValue().emplace(task->id, task); - } + InsightWorker::queuedTasksById.accessor()->emplace(task->id, task); emit InsightSignals::instance()->taskQueued(task); } void InsightWorker::executeTasks() { static const auto getQueuedTask = [] () -> std::optional> { - const auto taskQueueLock = InsightWorker::queuedTasksById.acquireLock(); - auto& queuedTasks = InsightWorker::queuedTasksById.getValue(); + auto queuedTasks = InsightWorker::queuedTasksById.accessor(); - if (!queuedTasks.empty()) { - const auto taskGroupsLock = InsightWorker::taskGroupsInExecution.acquireLock(); - auto& taskGroupsInExecution = InsightWorker::taskGroupsInExecution.getValue(); + if (!queuedTasks->empty()) { + auto taskGroupsInExecution = InsightWorker::taskGroupsInExecution.accessor(); const auto canExecuteTask = [&taskGroupsInExecution] (const QSharedPointer& task) { for (const auto taskGroup : task->taskGroups()) { - if (taskGroupsInExecution.contains(taskGroup)) { + if (taskGroupsInExecution->contains(taskGroup)) { return false; } } @@ -63,11 +58,11 @@ namespace Bloom return true; }; - for (auto [queuedTaskId, task] : queuedTasks) { + for (auto [queuedTaskId, task] : *queuedTasks) { if (canExecuteTask(task)) { const auto taskGroups = task->taskGroups(); - taskGroupsInExecution.insert(taskGroups.begin(), taskGroups.end()); - queuedTasks.erase(queuedTaskId); + taskGroupsInExecution->insert(taskGroups.begin(), taskGroups.end()); + queuedTasks->erase(queuedTaskId); return task; } } @@ -84,11 +79,10 @@ namespace Bloom task->execute(this->targetControllerService); { - const auto taskGroupsLock = InsightWorker::taskGroupsInExecution.acquireLock(); - auto& taskGroupsInExecution = InsightWorker::taskGroupsInExecution.getValue(); + auto taskGroupsInExecution = InsightWorker::taskGroupsInExecution.accessor(); for (const auto& taskGroup : task->taskGroups()) { - taskGroupsInExecution.erase(taskGroup); + taskGroupsInExecution->erase(taskGroup); } } diff --git a/src/Insight/InsightWorker/InsightWorker.hpp b/src/Insight/InsightWorker/InsightWorker.hpp index 0b796122..43e2df7b 100644 --- a/src/Insight/InsightWorker/InsightWorker.hpp +++ b/src/Insight/InsightWorker/InsightWorker.hpp @@ -9,7 +9,7 @@ #include "Tasks/InsightWorkerTask.hpp" -#include "src/Helpers/SyncSafe.hpp" +#include "src/Helpers/Synchronised.hpp" #include "src/Services/TargetControllerService.hpp" namespace Bloom @@ -36,8 +36,8 @@ namespace Bloom private: static inline std::atomic lastWorkerId = 0; - static inline SyncSafe>> queuedTasksById = {}; - static inline SyncSafe taskGroupsInExecution = {}; + static inline Synchronised>> queuedTasksById = {}; + static inline Synchronised taskGroupsInExecution = {}; Services::TargetControllerService targetControllerService = Services::TargetControllerService(); diff --git a/src/SignalHandler/SignalHandler.hpp b/src/SignalHandler/SignalHandler.hpp index 79fbfbb4..5986048a 100644 --- a/src/SignalHandler/SignalHandler.hpp +++ b/src/SignalHandler/SignalHandler.hpp @@ -4,7 +4,7 @@ #include "src/Helpers/Thread.hpp" #include "src/EventManager/EventManager.hpp" -#include "src/Helpers/SyncSafe.hpp" +#include "src/Helpers/Synchronised.hpp" namespace Bloom { diff --git a/src/TargetController/TargetControllerComponent.cpp b/src/TargetController/TargetControllerComponent.cpp index ee0333c5..95c67ff8 100644 --- a/src/TargetController/TargetControllerComponent.cpp +++ b/src/TargetController/TargetControllerComponent.cpp @@ -94,14 +94,12 @@ namespace Bloom::TargetController if (atomicSessionId.has_value()) { // This command is part of an atomic session - put it in the dedicated queue - const auto commandQueueLock = TargetControllerComponent::atomicSessionCommandQueue.acquireLock(); - TargetControllerComponent::atomicSessionCommandQueue.getValue().push(std::move(command)); + TargetControllerComponent::atomicSessionCommandQueue.accessor()->push(std::move(command)); TargetControllerComponent::notifier.notify(); return; } - const auto commandQueueLock = TargetControllerComponent::commandQueue.acquireLock(); - TargetControllerComponent::commandQueue.getValue().push(std::move(command)); + TargetControllerComponent::commandQueue.accessor()->push(std::move(command)); TargetControllerComponent::notifier.notify(); } @@ -112,7 +110,8 @@ namespace Bloom::TargetController auto response = std::unique_ptr(nullptr); const auto predicate = [commandId, &response] { - auto& responsesByCommandId = TargetControllerComponent::responsesByCommandId.getValue(); + // We will already hold the lock here, so we can use Synchronised::unsafeReference() here. + auto& responsesByCommandId = TargetControllerComponent::responsesByCommandId.unsafeReference(); auto responseIt = responsesByCommandId.find(commandId); if (responseIt != responsesByCommandId.end()) { @@ -125,7 +124,7 @@ namespace Bloom::TargetController return false; }; - auto responsesByCommandIdLock = TargetControllerComponent::responsesByCommandId.acquireLock(); + auto responsesByCommandIdLock = TargetControllerComponent::responsesByCommandId.lock(); if (timeout.has_value()) { TargetControllerComponent::responsesByCommandIdCv.wait_for( @@ -395,14 +394,11 @@ namespace Bloom::TargetController void TargetControllerComponent::processQueuedCommands() { auto commands = std::queue>(); - if (this->activeAtomicSession.has_value()) { - const auto queueLock = TargetControllerComponent::atomicSessionCommandQueue.acquireLock(); - commands.swap(TargetControllerComponent::atomicSessionCommandQueue.getValue()); - - } else { - const auto queueLock = TargetControllerComponent::commandQueue.acquireLock(); - commands.swap(TargetControllerComponent::commandQueue.getValue()); - } + commands.swap( + this->activeAtomicSession.has_value() + ? *(TargetControllerComponent::atomicSessionCommandQueue.accessor()) + : *(TargetControllerComponent::commandQueue.accessor()) + ); while (!commands.empty()) { const auto command = std::move(commands.front()); @@ -455,10 +451,7 @@ namespace Bloom::TargetController CommandIdType commandId, std::unique_ptr response ) { - const auto responseMappingLock = TargetControllerComponent::responsesByCommandId.acquireLock(); - TargetControllerComponent::responsesByCommandId.getValue().insert( - std::pair(commandId, std::move(response)) - ); + TargetControllerComponent::responsesByCommandId.accessor()->emplace(commandId, std::move(response)); TargetControllerComponent::responsesByCommandIdCv.notify_all(); } @@ -550,9 +543,9 @@ namespace Bloom::TargetController } { - const auto commandQueueLock = TargetControllerComponent::atomicSessionCommandQueue.acquireLock(); + auto commandQueue = TargetControllerComponent::atomicSessionCommandQueue.accessor(); auto empty = std::queue>(); - TargetControllerComponent::atomicSessionCommandQueue.getValue().swap(empty); + commandQueue->swap(empty); } this->activeAtomicSession.reset(); diff --git a/src/TargetController/TargetControllerComponent.hpp b/src/TargetController/TargetControllerComponent.hpp index a62a6e8c..648d77fa 100644 --- a/src/TargetController/TargetControllerComponent.hpp +++ b/src/TargetController/TargetControllerComponent.hpp @@ -13,7 +13,7 @@ #include #include "src/Helpers/Thread.hpp" -#include "src/Helpers/SyncSafe.hpp" +#include "src/Helpers/Synchronised.hpp" #include "src/Helpers/ConditionVariableNotifier.hpp" #include "TargetControllerState.hpp" @@ -103,7 +103,7 @@ namespace Bloom::TargetController ); private: - static inline SyncSafe>> commandQueue; + static inline Synchronised>> commandQueue; /** * We have a dedicated queue for atomic sessions. @@ -111,9 +111,9 @@ namespace Bloom::TargetController * During an atomic session, all commands for the session are placed into this dedicated queue. * The TargetController will only serve commands from this dedicated queue, until the atomic session ends. */ - static inline SyncSafe>> atomicSessionCommandQueue; + static inline Synchronised>> atomicSessionCommandQueue; - static inline SyncSafe< + static inline Synchronised< std::map> > responsesByCommandId;