From: Clément DAVID Date: Mon, 23 Nov 2015 13:36:25 +0000 (+0100) Subject: Xcos MVC: use std::atomic to be thread safe X-Git-Tag: 6.0.0-beta-1~206 X-Git-Url: http://gitweb.scilab.org/?p=scilab.git;a=commitdiff_plain;h=375f8fe4ef97a8d5b8595f4f8983c46923a7cb2c Xcos MVC: use std::atomic to be thread safe Xcos use both the Scilab execution thread, the EDT and the Finalizer Thread to create / access / delete MVC objects. This commit manage this thread safety using spin-locks implemented as std::atomic_flag (fast on the more common threadsafe case). Change-Id: Ic6ea51276de2a38e1530de6f08d138d4a04047e0 --- diff --git a/scilab/modules/scicos/includes/Controller.hxx b/scilab/modules/scicos/includes/Controller.hxx index 53d35c6..583d5dd 100644 --- a/scilab/modules/scicos/includes/Controller.hxx +++ b/scilab/modules/scicos/includes/Controller.hxx @@ -13,6 +13,7 @@ #ifndef CONTROLLER_HXX_ #define CONTROLLER_HXX_ +#include #include #include #include @@ -20,6 +21,7 @@ #include "utilities.hxx" #include "Model.hxx" #include "View.hxx" +#include "model/BaseObject.hxx" #include "dynlib_scicos.h" @@ -66,17 +68,28 @@ public: template bool getObjectProperty(ScicosID uid, kind_t k, object_properties_t p, T& v) const { - return m_instance.model.getObjectProperty(uid, k, p, v); + while (m_instance.onModelStructuralModification.test_and_set(std::memory_order_acquire)) // acquire lock + ; // spin + bool ret = m_instance.model.getObjectProperty(uid, k, p, v); + m_instance.onModelStructuralModification.clear(std::memory_order_release); // unlock + return ret; }; template update_status_t setObjectProperty(const ScicosID& uid, kind_t k, object_properties_t p, T v) { + while (m_instance.onModelStructuralModification.test_and_set(std::memory_order_acquire)) // acquire lock + ; // spin update_status_t status = m_instance.model.setObjectProperty(uid, k, p, v); + m_instance.onModelStructuralModification.clear(std::memory_order_release); // unlock + + while (m_instance.onViewsStructuralModification.test_and_set(std::memory_order_acquire)) // acquire lock + ; // spin for (view_set_t::iterator iter = m_instance.allViews.begin(); iter != m_instance.allViews.end(); ++iter) { (*iter)->propertyUpdated(uid, k, p, status); } + m_instance.onViewsStructuralModification.clear(std::memory_order_release); // unlock return status; } @@ -90,7 +103,10 @@ private: */ struct SharedData { + std::atomic_flag onModelStructuralModification; Model model; + + std::atomic_flag onViewsStructuralModification; view_name_set_t allNamedViews; view_set_t allViews; @@ -101,7 +117,7 @@ private: /** * Shared instance of the data * - * This will be allocated on-demand be Controller::get_instance() + * This will be allocated on-demand by Controller::get_instance() */ static SharedData m_instance; @@ -110,24 +126,8 @@ private: */ ScicosID cloneObject(std::map& mapped, ScicosID uid, bool cloneChildren, bool clonePorts); - template - void cloneProperties(model::BaseObject* initial, ScicosID clone) - { - for (int i = 0; i < MAX_OBJECT_PROPERTIES; ++i) - { - enum object_properties_t p = static_cast(i); - - T value; - bool status = getObjectProperty(initial->id(), initial->kind(), p, value); - if (status) - { - setObjectProperty(clone, initial->kind(), p, value); - } - } - - }; - + void cloneProperties(model::BaseObject* initial, ScicosID clone); void deepClone(std::map& mapped, ScicosID uid, ScicosID clone, kind_t k, object_properties_t p, bool cloneIfNotFound); void deepCloneVector(std::map& mapped, ScicosID uid, ScicosID clone, kind_t k, object_properties_t p, bool cloneIfNotFound); void unlinkVector(ScicosID uid, kind_t k, object_properties_t uid_prop, object_properties_t ref_prop); diff --git a/scilab/modules/scicos/src/cpp/Controller.cpp b/scilab/modules/scicos/src/cpp/Controller.cpp index ad8f013..635c50c 100644 --- a/scilab/modules/scicos/src/cpp/Controller.cpp +++ b/scilab/modules/scicos/src/cpp/Controller.cpp @@ -10,6 +10,7 @@ * */ +#include #include #include #include @@ -47,39 +48,62 @@ #endif #include "Controller.hxx" +#include "model/BaseObject.hxx" #include "LoggerView.hxx" namespace org_scilab_modules_scicos { +static inline void lock(std::atomic_flag* m) +{ + while (m->test_and_set(std::memory_order_acquire)) // acquire lock + ; // spin +} + +static inline void unlock(std::atomic_flag* m) +{ + m->clear(std::memory_order_release); +} + /* * Implement SharedData methods */ Controller::SharedData::SharedData() : - model(), allNamedViews(), allViews() + onModelStructuralModification(), model(), + onViewsStructuralModification(), allNamedViews(), allViews() { + onModelStructuralModification.clear(); + onViewsStructuralModification.clear(); } Controller::SharedData::~SharedData() { - for (view_set_t::iterator iter = m_instance.allViews.begin(); iter != m_instance.allViews.end(); ++iter) + lock(&onViewsStructuralModification); + for (view_set_t::iterator iter = allViews.begin(); iter != allViews.end(); ++iter) { delete *iter; } + unlock(&onViewsStructuralModification); } Controller::SharedData Controller::m_instance; View* Controller::register_view(const std::string& name, View* v) { + lock(&m_instance.onViewsStructuralModification); + m_instance.allNamedViews.push_back(name); m_instance.allViews.push_back(v); + + unlock(&m_instance.onViewsStructuralModification); return v; } void Controller::unregister_view(View* v) { + lock(&m_instance.onViewsStructuralModification); + view_set_t::iterator it = std::find(m_instance.allViews.begin(), m_instance.allViews.end(), v); if (it != m_instance.allViews.end()) { @@ -87,12 +111,16 @@ void Controller::unregister_view(View* v) m_instance.allNamedViews.erase(m_instance.allNamedViews.begin() + d); m_instance.allViews.erase(m_instance.allViews.begin() + d); } + + unlock(&m_instance.onViewsStructuralModification); } View* Controller::unregister_view(const std::string& name) { View* view = nullptr; + lock(&m_instance.onViewsStructuralModification); + view_name_set_t::iterator it = std::find(m_instance.allNamedViews.begin(), m_instance.allNamedViews.end(), name); if (it != m_instance.allNamedViews.end()) { @@ -101,6 +129,7 @@ View* Controller::unregister_view(const std::string& name) m_instance.allNamedViews.erase(m_instance.allNamedViews.begin() + d); m_instance.allViews.erase(m_instance.allViews.begin() + d); } + unlock(&m_instance.onViewsStructuralModification); return view; } @@ -109,12 +138,14 @@ View* Controller::look_for_view(const std::string& name) { View* view = nullptr; + lock(&m_instance.onViewsStructuralModification); view_name_set_t::iterator it = std::find(m_instance.allNamedViews.begin(), m_instance.allNamedViews.end(), name); if (it != m_instance.allNamedViews.end()) { size_t d = std::distance(m_instance.allNamedViews.begin(), it); view = *(m_instance.allViews.begin() + d); } + unlock(&m_instance.onViewsStructuralModification); return view; } @@ -129,26 +160,36 @@ Controller::~Controller() ScicosID Controller::createObject(kind_t k) { + lock(&m_instance.onModelStructuralModification); ScicosID uid = m_instance.model.createObject(k); + unlock(&m_instance.onModelStructuralModification); + lock(&m_instance.onViewsStructuralModification); for (view_set_t::iterator iter = m_instance.allViews.begin(); iter != m_instance.allViews.end(); ++iter) { (*iter)->objectCreated(uid, k); } + unlock(&m_instance.onViewsStructuralModification); return uid; } unsigned Controller::referenceObject(const ScicosID uid) const { + lock(&m_instance.onModelStructuralModification); + unsigned refCount = m_instance.model.referenceObject(uid); REF_PRINT(uid, refCount); - auto o = getObject(uid); + auto o = m_instance.model.getObject(uid); + unlock(&m_instance.onModelStructuralModification); + + lock(&m_instance.onViewsStructuralModification); for (view_set_t::iterator iter = m_instance.allViews.begin(); iter != m_instance.allViews.end(); ++iter) { (*iter)->objectReferenced(uid, o->kind(), refCount); } + unlock(&m_instance.onViewsStructuralModification); return refCount; } @@ -161,7 +202,9 @@ void Controller::deleteObject(ScicosID uid) return; } - auto initial = getObject(uid); + lock(&m_instance.onModelStructuralModification); + + auto initial = m_instance.model.getObject(uid); if (initial == nullptr) { // defensive programming @@ -171,15 +214,18 @@ void Controller::deleteObject(ScicosID uid) // if this object has been referenced somewhere else do not delete it but decrement the reference counter unsigned& refCount = m_instance.model.referenceCount(uid); + unlock(&m_instance.onModelStructuralModification); if (refCount > 0) { --refCount; UNREF_PRINT(uid, refCount); + lock(&m_instance.onViewsStructuralModification); for (view_set_t::iterator iter = m_instance.allViews.begin(); iter != m_instance.allViews.end(); ++iter) { (*iter)->objectUnreferenced(uid, k, refCount); } + unlock(&m_instance.onViewsStructuralModification); return; } @@ -231,28 +277,32 @@ void Controller::deleteObject(ScicosID uid) } // delete the object + lock(&m_instance.onModelStructuralModification); m_instance.model.deleteObject(uid); + unlock(&m_instance.onModelStructuralModification); + lock(&m_instance.onViewsStructuralModification); for (view_set_t::iterator iter = m_instance.allViews.begin(); iter != m_instance.allViews.end(); ++iter) { (*iter)->objectDeleted(uid, k); } + unlock(&m_instance.onViewsStructuralModification); } void Controller::unlinkVector(ScicosID uid, kind_t k, object_properties_t uid_prop, object_properties_t ref_prop) { ScicosID v; - getObjectProperty(uid, k, uid_prop, v); + m_instance.model.getObjectProperty(uid, k, uid_prop, v); if (v != 0) { - auto o = getObject(v); + auto o = m_instance.model.getObject(v); if (o == nullptr) { return; } std::vector children; - getObjectProperty(o->id(), o->kind(), ref_prop, children); + m_instance.model.getObjectProperty(o->id(), o->kind(), ref_prop, children); std::vector::iterator it = std::find(children.begin(), children.end(), uid); if (it != children.end()) @@ -260,19 +310,19 @@ void Controller::unlinkVector(ScicosID uid, kind_t k, object_properties_t uid_pr children.erase(it); } - setObjectProperty(o->id(), o->kind(), ref_prop, children); + m_instance.model.setObjectProperty(o->id(), o->kind(), ref_prop, children); } } void Controller::unlink(ScicosID uid, kind_t k, object_properties_t uid_prop, object_properties_t ref_prop) { std::vector v; - getObjectProperty(uid, k, uid_prop, v); + m_instance.model.getObjectProperty(uid, k, uid_prop, v); for (const ScicosID id : v) { if (id != 0) { - auto o = getObject(id); + auto o = m_instance.model.getObject(id); if (o == nullptr) { continue; @@ -280,10 +330,10 @@ void Controller::unlink(ScicosID uid, kind_t k, object_properties_t uid_prop, ob // Find which end of the link is connected to the port ScicosID oppositeRef; - getObjectProperty(o->id(), o->kind(), ref_prop, oppositeRef); + m_instance.model.getObjectProperty(o->id(), o->kind(), ref_prop, oppositeRef); if (oppositeRef == uid) { - setObjectProperty(o->id(), o->kind(), ref_prop, ScicosID()); + m_instance.model.setObjectProperty(o->id(), o->kind(), ref_prop, ScicosID()); } } } @@ -292,19 +342,35 @@ void Controller::unlink(ScicosID uid, kind_t k, object_properties_t uid_prop, ob void Controller::deleteVector(ScicosID uid, kind_t k, object_properties_t uid_prop) { std::vector children; - getObjectProperty(uid, k, uid_prop, children); + m_instance.model.getObjectProperty(uid, k, uid_prop, children); for (ScicosID id : children) { - deleteObject(id); + m_instance.model.deleteObject(id); + } +} + +template +void Controller::cloneProperties(model::BaseObject* initial, ScicosID clone) +{ + for (int i = 0; i < MAX_OBJECT_PROPERTIES; ++i) + { + enum object_properties_t p = static_cast(i); + + T value; + bool status = m_instance.model.getObjectProperty(initial->id(), initial->kind(), p, value); + if (status) + { + m_instance.model.setObjectProperty(clone, initial->kind(), p, value); + } } } ScicosID Controller::cloneObject(std::map& mapped, ScicosID uid, bool cloneChildren, bool clonePorts) { - auto initial = getObject(uid); + auto initial = m_instance.model.getObject(uid); const kind_t k = initial->kind(); - ScicosID o = createObject(k); + ScicosID o = m_instance.model.createObject(k); mapped.insert(std::make_pair(uid, o)); // Get then set all properties per type that do not manage ScicosID @@ -361,14 +427,13 @@ ScicosID Controller::cloneObject(std::map& mapped, ScicosID deepClone(mapped, uid, o, k, SOURCE_BLOCK, false); deepCloneVector(mapped, uid, o, k, CONNECTED_SIGNALS, false); } - return o; } void Controller::deepClone(std::map& mapped, ScicosID uid, ScicosID clone, kind_t k, object_properties_t p, bool cloneIfNotFound) { ScicosID v; - getObjectProperty(uid, k, p, v); + m_instance.model.getObjectProperty(uid, k, p, v); ScicosID cloned = 0; @@ -396,13 +461,13 @@ void Controller::deepClone(std::map& mapped, ScicosID uid, S } } - setObjectProperty(clone, k, p, cloned); + m_instance.model.setObjectProperty(clone, k, p, cloned); } void Controller::deepCloneVector(std::map& mapped, ScicosID uid, ScicosID clone, kind_t k, object_properties_t p, bool cloneIfNotFound) { std::vector v; - getObjectProperty(uid, k, p, v); + m_instance.model.getObjectProperty(uid, k, p, v); std::vector cloned; cloned.reserve(v.size()); @@ -441,29 +506,45 @@ void Controller::deepCloneVector(std::map& mapped, ScicosID } } - setObjectProperty(clone, k, p, cloned); + m_instance.model.setObjectProperty(clone, k, p, cloned); } ScicosID Controller::cloneObject(ScicosID uid, bool cloneChildren, bool clonePorts) { + lock(&m_instance.onModelStructuralModification); + std::map mapped; ScicosID clone = cloneObject(mapped, uid, cloneChildren, clonePorts); CLONE_PRINT(uid, clone); + + unlock(&m_instance.onModelStructuralModification); return clone; } kind_t Controller::getKind(ScicosID uid) const { - return m_instance.model.getKind(uid); + lock(&m_instance.onModelStructuralModification); + + kind_t kind = m_instance.model.getKind(uid); + + unlock(&m_instance.onModelStructuralModification); + return kind; } std::vector Controller::getAll(kind_t k) const { - return m_instance.model.getAll(k); + lock(&m_instance.onModelStructuralModification); + + auto vec = m_instance.model.getAll(k); + + unlock(&m_instance.onModelStructuralModification); + return vec; } void Controller::sortAndFillKind(std::vector& uids, std::vector& kinds) { + lock(&m_instance.onModelStructuralModification); + // create a container of pair struct local_pair { @@ -475,7 +556,7 @@ void Controller::sortAndFillKind(std::vector& uids, std::vector& // fill it for (size_t i = 0; i < uids.size(); ++i) { - container[i] = { uids[i], getKind(uids[i]) }; + container[i] = { uids[i], m_instance.model.getKind(uids[i]) }; } // sort according to the kinds @@ -487,18 +568,23 @@ void Controller::sortAndFillKind(std::vector& uids, std::vector& // move things back uids.clear(); kinds.reserve(uids.capacity()); - for (const auto& v : container) + for (const auto & v : container) { uids.push_back(v.first); kinds.push_back(v.second); } + + unlock(&m_instance.onModelStructuralModification); } model::BaseObject* Controller::getObject(ScicosID uid) const { - return m_instance.model.getObject(uid); + lock(&m_instance.onModelStructuralModification); + model::BaseObject* o = m_instance.model.getObject(uid); + unlock(&m_instance.onModelStructuralModification); + return o; } } /* namespace org_scilab_modules_scicos */