Xcos MVC: use std::atomic to be thread safe 00/17500/2
Clément DAVID [Mon, 23 Nov 2015 13:36:25 +0000 (14:36 +0100)]
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

scilab/modules/scicos/includes/Controller.hxx
scilab/modules/scicos/src/cpp/Controller.cpp

index 53d35c6..583d5dd 100644 (file)
@@ -13,6 +13,7 @@
 #ifndef CONTROLLER_HXX_
 #define CONTROLLER_HXX_
 
+#include <atomic>
 #include <string>
 #include <vector>
 #include <map>
@@ -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<typename T>
     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<typename T>
     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<ScicosID, ScicosID>& mapped, ScicosID uid, bool cloneChildren, bool clonePorts);
-
     template<typename T>
-    void cloneProperties(model::BaseObject* initial, ScicosID clone)
-    {
-        for (int i = 0; i < MAX_OBJECT_PROPERTIES; ++i)
-        {
-            enum object_properties_t p = static_cast<enum object_properties_t>(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<ScicosID, ScicosID>& mapped, ScicosID uid, ScicosID clone, kind_t k, object_properties_t p, bool cloneIfNotFound);
     void deepCloneVector(std::map<ScicosID, ScicosID>& 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);
index ad8f013..635c50c 100644 (file)
@@ -10,6 +10,7 @@
  *
  */
 
+#include <atomic>
 #include <string>
 #include <vector>
 #include <map>
 #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<ScicosID> children;
-        getObjectProperty(o->id(), o->kind(), ref_prop, children);
+        m_instance.model.getObjectProperty(o->id(), o->kind(), ref_prop, children);
 
         std::vector<ScicosID>::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<ScicosID> 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<ScicosID> 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<typename T>
+void Controller::cloneProperties(model::BaseObject* initial, ScicosID clone)
+{
+    for (int i = 0; i < MAX_OBJECT_PROPERTIES; ++i)
+    {
+        enum object_properties_t p = static_cast<enum object_properties_t>(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<ScicosID, ScicosID>& 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<ScicosID, ScicosID>& 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<ScicosID, ScicosID>& 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<ScicosID, ScicosID>& mapped, ScicosID uid, S
         }
     }
 
-    setObjectProperty(clone, k, p, cloned);
+    m_instance.model.setObjectProperty(clone, k, p, cloned);
 }
 
 void Controller::deepCloneVector(std::map<ScicosID, ScicosID>& mapped, ScicosID uid, ScicosID clone, kind_t k, object_properties_t p, bool cloneIfNotFound)
 {
     std::vector<ScicosID> v;
-    getObjectProperty(uid, k, p, v);
+    m_instance.model.getObjectProperty(uid, k, p, v);
 
     std::vector<ScicosID> cloned;
     cloned.reserve(v.size());
@@ -441,29 +506,45 @@ void Controller::deepCloneVector(std::map<ScicosID, ScicosID>& 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<ScicosID, ScicosID> 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<ScicosID> 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<ScicosID>& uids, std::vector<int>& kinds)
 {
+    lock(&m_instance.onModelStructuralModification);
+
     // create a container of pair
     struct local_pair
     {
@@ -475,7 +556,7 @@ void Controller::sortAndFillKind(std::vector<ScicosID>& uids, std::vector<int>&
     // 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<ScicosID>& uids, std::vector<int>&
     // 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 */