From 317b9125b60e686eb7e25171902eacab88b59426 Mon Sep 17 00:00:00 2001 From: Andrew Belt Date: Tue, 12 Jan 2021 11:12:51 -0500 Subject: [PATCH] Make ReadWriteLock non-recursive and add *_NoLock() methods to Engine as needed. Split Module::bypass() into isBypass/setBypass(). Add more documentation to Engine methods. --- include/engine/Engine.hpp | 95 +++++++++++++++++++++++++++++++++++---- include/engine/Module.hpp | 3 +- include/rack.hpp | 2 +- src/app/ModuleWidget.cpp | 8 ++-- src/engine/Engine.cpp | 81 +++++++++++++++++++-------------- src/engine/Module.cpp | 7 ++- 6 files changed, 147 insertions(+), 49 deletions(-) diff --git a/include/engine/Engine.hpp b/include/engine/Engine.hpp index 9d5c2135..63697bc3 100644 --- a/include/engine/Engine.hpp +++ b/include/engine/Engine.hpp @@ -13,9 +13,9 @@ namespace engine { /** Manages Modules and Cables and steps them in time. -All methods are thread-safe and can safely be called from anywhere. -However, the following methods obtain a write-lock, so they cannot be called with the engine thread (e.g. inside Module::process()): -clear, stepBlock, setPrimaryModule, setSampleRate, addModule, removeModule, resetModule, randomizeModule, bypassModule, moduleFromJson, addCable, removeCable, addParamHandle, removeParamHandle, and fromJson +Engine contains a read/write mutex that locks when the Engine state is being read or written (manipulated). +Methods that read-lock (stated in their documentation) can be called simultaneously with other read-locking methods. +Methods that write-lock cannot be called simultaneously or recursively with another read-locking or write-locking method. */ struct Engine { struct Internal; @@ -24,15 +24,20 @@ struct Engine { Engine(); ~Engine(); - /** Removes all modules and cables. */ + /** Removes all modules and cables. + Write-locks. + */ void clear(); + PRIVATE void clear_NoLock(); /** Advances the engine by `frames` frames. Only call this method from the primary module. + Read-locks. Also locks so only one stepBlock() can be called simultaneously or recursively. */ void stepBlock(int frames); /** Module does not need to belong to the Engine. However, Engine will unset the primary module when it is removed from the Engine. NULL will unset the primary module. + Write-locks. */ void setPrimaryModule(Module* module); Module* getPrimaryModule(); @@ -40,7 +45,13 @@ struct Engine { /** Returns the sample rate used by the engine for stepping each module. */ float getSampleRate(); + /** Sets the sample rate to step the modules. + Write-locks. + */ PRIVATE void setSampleRate(float sampleRate); + /** Sets the sample rate if the sample rate in the settings is "Auto". + Write-locks. + */ void setSuggestedSampleRate(float suggestedSampleRate); /** Returns the inverse of the current sample rate. */ @@ -78,37 +89,85 @@ struct Engine { /** Fills `moduleIds` with up to `len` module IDs in the rack. Returns the number of IDs written. This C-like method does no allocations. The vector C++ version below does. + Read-locks. */ size_t getModuleIds(int64_t* moduleIds, size_t len); + /** Returns a vector of module IDs in the rack. + Read-locks. + */ std::vector getModuleIds(); - /** Adds a module to the rack engine. - The module ID must not be taken by another module. + /** Adds a Module to the rack. + The module ID must not be taken by another Module. If the module ID is -1, an ID is automatically assigned. Does not transfer pointer ownership. + Write-locks. */ void addModule(Module* module); + /** Removes a Module from the rack. + Write-locks. + */ void removeModule(Module* module); + PRIVATE void removeModule_NoLock(Module* module); + /** Checks whether a Module is in the rack. + Read-locks. + */ bool hasModule(Module* module); + /** Returns the Module with the given ID in the rack. + Read-locks. + */ Module* getModule(int64_t moduleId); + /** Triggers a ResetEvent for the given Module. + Write-locks. + */ void resetModule(Module* module); + /** Triggers a RandomizeEvent for the given Module. + Write-locks. + */ void randomizeModule(Module* module); + /** Sets the bypass state and triggers a BypassEvent or UnBypassEvent of the given Module. + Write-locks. + */ void bypassModule(Module* module, bool bypass); - /** Serializes/deserializes with locking, ensuring that Module::process() is not called during toJson()/fromJson(). + /** Serializes the given Module with locking, ensuring that Module::process() is not called simultaneously. + Read-locks. */ json_t* moduleToJson(Module* module); + /** Serializes the given Module with locking, ensuring that Module::process() is not called simultaneously. + Write-locks. + */ void moduleFromJson(Module* module, json_t* rootJ); // Cables size_t getNumCables(); + /** Fills `cableIds` with up to `len` cable IDs in the rack. + Returns the number of IDs written. + This C-like method does no allocations. The vector C++ version below does. + Read-locks. + */ size_t getCableIds(int64_t* cableIds, size_t len); + /** Returns a vector of cable IDs in the rack. + Read-locks. + */ std::vector getCableIds(); - /** Adds a cable to the rack engine. + /** Adds a Cable to the rack. The cable ID must not be taken by another cable. If the cable ID is -1, an ID is automatically assigned. Does not transfer pointer ownership. + Write-locks. */ void addCable(Cable* cable); + /** Removes a Cable from the rack. + Write-locks. + */ void removeCable(Cable* cable); + PRIVATE void removeCable_NoLock(Cable* cable); + /** Checks whether a Cable is in the rack. + Read-locks. + */ + bool hasCable(Cable* cable); + /** Returns the Cable with the given ID in the rack. + Read-locks. + */ Cable* getCable(int64_t cableId); // Params @@ -122,19 +181,37 @@ struct Engine { float getSmoothParam(Module* module, int paramId); // ParamHandles + /** Adds a ParamHandle to the rack. + Does not automatically update the ParamHandle. + Write-locks. + */ void addParamHandle(ParamHandle* paramHandle); + /** + Write-locks. + */ void removeParamHandle(ParamHandle* paramHandle); + PRIVATE void removeParamHandle_NoLock(ParamHandle* paramHandle); /** Returns the unique ParamHandle for the given paramId + Read-locks. */ ParamHandle* getParamHandle(int64_t moduleId, int paramId); - /** Use getParamHandle(moduleId, paramId) instead. */ + /** Use getParamHandle(moduleId, paramId) instead. + Read-locks. + */ DEPRECATED ParamHandle* getParamHandle(Module* module, int paramId); /** Sets the ParamHandle IDs and module pointer. If `overwrite` is true and another ParamHandle points to the same param, unsets that one and replaces it with the given handle. + Read-locks. */ void updateParamHandle(ParamHandle* paramHandle, int64_t moduleId, int paramId, bool overwrite = true); + /** Serializes the rack. + Read-locks. + */ json_t* toJson(); + /** Deserializes the rack. + Write-locks. + */ void fromJson(json_t* rootJ); }; diff --git a/include/engine/Module.hpp b/include/engine/Module.hpp index ff4d1592..6d4d617e 100644 --- a/include/engine/Module.hpp +++ b/include/engine/Module.hpp @@ -344,7 +344,8 @@ struct Module { /** DEPRECATED. Override `onSampleRateChange(e)` instead. */ virtual void onSampleRateChange() {} - PRIVATE bool& bypass(); + bool isBypass(); + PRIVATE void setBypass(bool bypass); PRIVATE const float* meterBuffer(); PRIVATE int meterLength(); PRIVATE int meterIndex(); diff --git a/include/rack.hpp b/include/rack.hpp index 5ab34ccc..5c4d3215 100644 --- a/include/rack.hpp +++ b/include/rack.hpp @@ -100,7 +100,7 @@ #undef PRIVATE -#define PRIVATE __attribute__((warning ("Using private function or symbol"))) +#define PRIVATE __attribute__((error ("Using private function or symbol"))) namespace rack { diff --git a/src/app/ModuleWidget.cpp b/src/app/ModuleWidget.cpp index c15d7a78..a0c5c246 100644 --- a/src/app/ModuleWidget.cpp +++ b/src/app/ModuleWidget.cpp @@ -394,7 +394,7 @@ ModuleWidget::~ModuleWidget() { void ModuleWidget::draw(const DrawArgs& args) { nvgScissor(args.vg, RECT_ARGS(args.clipBox)); - if (module && module->bypass()) { + if (module && module->isBypass()) { nvgGlobalAlpha(args.vg, 0.33); } @@ -1002,12 +1002,12 @@ void ModuleWidget::cloneAction() { void ModuleWidget::bypassAction() { assert(module); - APP->engine->bypassModule(module, !module->bypass()); + APP->engine->bypassModule(module, !module->isBypass()); // history::ModuleBypass history::ModuleBypass* h = new history::ModuleBypass; h->moduleId = module->id; - h->bypass = module->bypass(); + h->bypass = module->isBypass(); APP->history->push(h); } @@ -1075,7 +1075,7 @@ void ModuleWidget::createContextMenu() { ModuleBypassItem* bypassItem = new ModuleBypassItem; bypassItem->text = "Bypass"; bypassItem->rightText = RACK_MOD_CTRL_NAME "+E"; - if (module && module->bypass()) + if (module && module->isBypass()) bypassItem->rightText = CHECKMARK_STRING " " + bypassItem->rightText; bypassItem->moduleWidget = this; menu->addChild(bypassItem); diff --git a/src/engine/Engine.cpp b/src/engine/Engine.cpp index d90d0787..d73800d6 100644 --- a/src/engine/Engine.cpp +++ b/src/engine/Engine.cpp @@ -32,15 +32,11 @@ static void initMXCSR() { /** Allows multiple "reader" threads to obtain a lock simultaneously, but only one "writer" thread. -WriteLock can be used recursively. -Uses reader priority, which implies that ReadLock can also be used recursively. -This implementation is just a wrapper for pthreads. +This implementation is currently just a wrapper for pthreads, which works on Linux/Mac/. This is available in C++17 as std::shared_mutex, but unfortunately we're using C++11. */ struct ReadWriteMutex { pthread_rwlock_t rwlock; - std::atomic writerThread{0}; - int recursion = 0; ReadWriteMutex() { if (pthread_rwlock_init(&rwlock, NULL)) @@ -58,21 +54,12 @@ struct ReadWriteMutex { throw Exception("pthread_rwlock_unlock failed"); } void lockWriter() { - pthread_t self = pthread_self(); - if (writerThread != self) { - if (pthread_rwlock_wrlock(&rwlock)) - throw Exception("pthread_rwlock_wrlock failed"); - writerThread = self; - } - // We're safe behind a lock so we can increment the recursion count. - recursion++; + if (pthread_rwlock_wrlock(&rwlock)) + throw Exception("pthread_rwlock_wrlock failed"); } void unlockWriter() { - if (--recursion == 0) { - writerThread = 0; - if (pthread_rwlock_unlock(&rwlock)) - throw Exception("pthread_rwlock_unlock failed"); - } + if (pthread_rwlock_unlock(&rwlock)) + throw Exception("pthread_rwlock_unlock failed"); } }; @@ -210,9 +197,6 @@ struct EngineWorker { }; -static const float FALLBACK_SAMPLE_RATE = 44100; - - struct Engine::Internal { std::vector modules; std::vector cables; @@ -240,7 +224,7 @@ struct Engine::Internal { float smoothValue = 0.f; /** Mutex that guards the Engine state, such as settings, Modules, and Cables. - Writers lock when mutating the engine's state. + Writers lock when mutating the engine's state or stepping the block. Readers lock when using the engine's state. */ ReadWriteMutex mutex; @@ -483,7 +467,7 @@ Engine::Engine() { internal = new Internal; internal->context = contextGet(); - setSampleRate(FALLBACK_SAMPLE_RATE); + setSuggestedSampleRate(0.f); } @@ -507,21 +491,25 @@ Engine::~Engine() { void Engine::clear() { WriteLock lock(internal->mutex); + clear_NoLock(); +} + +void Engine::clear_NoLock() { // Copy lists because we'll be removing while iterating std::set paramHandles = internal->paramHandles; for (ParamHandle* paramHandle : paramHandles) { - removeParamHandle(paramHandle); - // Don't delete paramHandle because they're owned by other things (e.g. Modules) + removeParamHandle_NoLock(paramHandle); + // Don't delete paramHandle because they're normally owned by Module subclasses } std::vector cables = internal->cables; for (Cable* cable : cables) { - removeCable(cable); + removeCable_NoLock(cable); delete cable; } std::vector modules = internal->modules; for (Module* module : modules) { - removeModule(module); + removeModule_NoLock(module); delete module; } } @@ -604,7 +592,8 @@ void Engine::setSuggestedSampleRate(float suggestedSampleRate) { setSampleRate(suggestedSampleRate); } else { - setSampleRate(FALLBACK_SAMPLE_RATE); + // Fallback sample rate + setSampleRate(44100.f); } } @@ -711,6 +700,11 @@ void Engine::addModule(Module* module) { void Engine::removeModule(Module* module) { WriteLock lock(internal->mutex); + removeModule_NoLock(module); +} + + +void Engine::removeModule_NoLock(Module* module) { assert(module); // Check that the module actually exists auto it = std::find(internal->modules.begin(), internal->modules.end(), module); @@ -792,7 +786,7 @@ void Engine::bypassModule(Module* module, bool bypass) { WriteLock lock(internal->mutex); assert(module); - if (module->bypass() == bypass) + if (module->isBypass() == bypass) return; // Clear outputs and set to 1 channel for (Output& output : module->outputs) { @@ -800,7 +794,7 @@ void Engine::bypassModule(Module* module, bool bypass) { output.setChannels(0); } // Set bypass state - module->bypass() = bypass; + module->setBypass(bypass); // Trigger event if (bypass) { Module::BypassEvent eBypass; @@ -901,6 +895,11 @@ void Engine::addCable(Cable* cable) { void Engine::removeCable(Cable* cable) { WriteLock lock(internal->mutex); + removeCable_NoLock(cable); +} + + +void Engine::removeCable_NoLock(Cable* cable) { assert(cable); // Check that the cable is already added auto it = std::find(internal->cables.begin(), internal->cables.end(), cable); @@ -935,6 +934,14 @@ void Engine::removeCable(Cable* cable) { } +bool Engine::hasCable(Cable* cable) { + ReadLock lock(internal->mutex); + // TODO Performance could be improved by searching cablesCache, but more testing would be needed to make sure it's always valid. + auto it = std::find(internal->cables.begin(), internal->cables.end(), cable); + return it != internal->cables.end(); +} + + Cable* Engine::getCable(int64_t cableId) { ReadLock lock(internal->mutex); auto it = internal->cablesCache.find(cableId); @@ -996,6 +1003,11 @@ void Engine::addParamHandle(ParamHandle* paramHandle) { void Engine::removeParamHandle(ParamHandle* paramHandle) { WriteLock lock(internal->mutex); + removeParamHandle_NoLock(paramHandle); +} + + +void Engine::removeParamHandle_NoLock(ParamHandle* paramHandle) { // Check that the ParamHandle is already added auto it = internal->paramHandles.find(paramHandle); assert(it != internal->paramHandles.end()); @@ -1035,6 +1047,7 @@ void Engine::updateParamHandle(ParamHandle* paramHandle, int64_t moduleId, int p if (paramHandle->moduleId >= 0) { // Replace old ParamHandle, or reset the current ParamHandle + // TODO Maybe call getParamHandle_NoLock()? ParamHandle* oldParamHandle = getParamHandle(moduleId, paramId); if (oldParamHandle) { if (overwrite) { @@ -1052,6 +1065,7 @@ void Engine::updateParamHandle(ParamHandle* paramHandle, int64_t moduleId, int p // Set module pointer if the above block didn't reset it if (paramHandle->moduleId >= 0) { + // TODO Maybe call getModule_NoLock()? paramHandle->module = getModule(paramHandle->moduleId); } @@ -1086,8 +1100,9 @@ json_t* Engine::toJson() { void Engine::fromJson(json_t* rootJ) { - // Don't write-lock here because most of this function doesn't need it. + // Don't write-lock the entire method because most of it doesn't need it. + // Write-locks clear(); // modules json_t* modulesJ = json_object_get(rootJ, "modules"); @@ -1108,7 +1123,7 @@ void Engine::fromJson(json_t* rootJ) { module->id = moduleIndex; } - // This write-locks + // Write-locks addModule(module); } catch (Exception& e) { @@ -1131,7 +1146,7 @@ void Engine::fromJson(json_t* rootJ) { Cable* cable = new Cable; try { cable->fromJson(cableJ); - // This write-locks + // Write-locks addCable(cable); } catch (Exception& e) { diff --git a/src/engine/Module.cpp b/src/engine/Module.cpp index bd977b5c..f505744c 100644 --- a/src/engine/Module.cpp +++ b/src/engine/Module.cpp @@ -282,11 +282,16 @@ void Module::onRandomize(const RandomizeEvent& e) { } -bool& Module::bypass() { +bool Module::isBypass() { return internal->bypass; } +void Module::setBypass(bool bypass) { + internal->bypass = bypass; +} + + const float* Module::meterBuffer() { return internal->meterBuffer; }