cleanup Action refcounting

Previously, subclasses did override unref() to do some cleanups on
destruction. This required to have refcount_ protected for them to
mess with it.

Instead, the cleanups go to the various destructors, where they
belong. The refcounting can be made private to Action as it should.

diff --git a/include/simgrid/kernel/resource/Action.hpp b/include/simgrid/kernel/resource/Action.hpp
index b42d5b4..20e877c 100644 (file)
--- a/include/simgrid/kernel/resource/Action.hpp
+++ b/include/simgrid/kernel/resource/Action.hpp
@@ -133,7 +133,7 @@ public:
   /** @brief Unref that action (and destroy it if refcount reaches 0)
    *  @return true if the action was destroyed and false if someone still has references on it
    */
-  virtual int unref();
+  int unref();
 
   /** @brief Cancel the current Action if running */
   virtual void cancel();
@@ -170,9 +170,9 @@ public:
 
 protected:
   StateSet* state_set_;
-  int refcount_ = 1;
 
 private:
+  int refcount_            = 1;
   double sharing_priority_ = 1.0;             /**< priority (1.0 by default) */
   double max_duration_   = NO_MAX_DURATION; /*< max_duration (may fluctuate until the task is completed) */
   double remains_;           /**< How much of that cost remains to be done in the currently running task */

diff --git a/src/kernel/resource/Action.cpp b/src/kernel/resource/Action.cpp
index b21dbba..bac3a43 100644 (file)
--- a/src/kernel/resource/Action.cpp
+++ b/src/kernel/resource/Action.cpp
@@ -32,6 +32,17 @@ Action::Action(simgrid::kernel::resource::Model* model, double cost, bool failed
 
 Action::~Action()
 {
+  if (state_set_hook_.is_linked())
+    simgrid::xbt::intrusive_erase(*state_set_, *this);
+  if (getVariable())
+    get_model()->getMaxminSystem()->variable_free(getVariable());
+  if (get_model()->getUpdateMechanism() == UM_LAZY) {
+    /* remove from heap */
+    heapRemove(get_model()->getActionHeap());
+    if (modified_set_hook_.is_linked())
+      simgrid::xbt::intrusive_erase(*get_model()->getModifiedSet(), *this);
+  }
+
   xbt_free(category_);
 }
 
@@ -137,16 +148,6 @@ int Action::unref()
 {
   refcount_--;
   if (not refcount_) {
-    if (state_set_hook_.is_linked())
-      simgrid::xbt::intrusive_erase(*state_set_, *this);
-    if (getVariable())
-      get_model()->getMaxminSystem()->variable_free(getVariable());
-    if (get_model()->getUpdateMechanism() == UM_LAZY) {
-      /* remove from heap */
-      heapRemove(get_model()->getActionHeap());
-      if (modified_set_hook_.is_linked())
-        simgrid::xbt::intrusive_erase(*get_model()->getModifiedSet(), *this);
-    }
     delete this;
     return 1;
   }

diff --git a/src/surf/cpu_ti.cpp b/src/surf/cpu_ti.cpp
index 6cf864c..c2b3eb6 100644 (file)
--- a/src/surf/cpu_ti.cpp
+++ b/src/surf/cpu_ti.cpp
@@ -625,29 +625,20 @@ CpuTiAction::CpuTiAction(CpuTiModel *model_, double cost, bool failed, CpuTi *cp
 {
   cpu_->modified(true);
 }
-
-void CpuTiAction::set_state(Action::State state)
+CpuTiAction::~CpuTiAction()
 {
-  CpuAction::set_state(state);
+  /* remove from action_set */
+  if (action_ti_hook.is_linked())
+    simgrid::xbt::intrusive_erase(cpu_->actionSet_, *this);
+  /* remove from heap */
+  heapRemove(get_model()->getActionHeap());
   cpu_->modified(true);
 }
 
-int CpuTiAction::unref()
+void CpuTiAction::set_state(Action::State state)
 {
-  refcount_--;
-  if (not refcount_) {
-    if (state_set_hook_.is_linked())
-      simgrid::xbt::intrusive_erase(*get_state_set(), *this);
-    /* remove from action_set */
-    if (action_ti_hook.is_linked())
-      simgrid::xbt::intrusive_erase(cpu_->actionSet_, *this);
-    /* remove from heap */
-    heapRemove(get_model()->getActionHeap());
-    cpu_->modified(true);
-    delete this;
-    return 1;
-  }
-  return 0;
+  CpuAction::set_state(state);
+  cpu_->modified(true);
 }
 
 void CpuTiAction::cancel()

diff --git a/src/surf/cpu_ti.hpp b/src/surf/cpu_ti.hpp
index ec4806d..375de18 100644 (file)
--- a/src/surf/cpu_ti.hpp
+++ b/src/surf/cpu_ti.hpp
@@ -83,9 +83,9 @@ class CpuTiAction: public CpuAction {
   friend class CpuTi;
 public:
   CpuTiAction(CpuTiModel *model, double cost, bool failed, CpuTi *cpu);
+  ~CpuTiAction();
 
   void set_state(simgrid::kernel::resource::Action::State state) override;
-  int unref() override;
   void cancel() override;
   void suspend() override;
   void resume() override;

diff --git a/src/surf/network_ns3.cpp b/src/surf/network_ns3.cpp
index c42e0e1..3821b8a 100644 (file)
--- a/src/surf/network_ns3.cpp
+++ b/src/surf/network_ns3.cpp
@@ -350,19 +350,6 @@ bool NetworkNS3Action::isSuspended()
   return false;
 }
 
-int NetworkNS3Action::unref()
-{
-  refcount_--;
-  if (not refcount_) {
-    if (state_set_hook_.is_linked())
-      simgrid::xbt::intrusive_erase(*state_set_, *this);
-    XBT_DEBUG ("Removing action %p", this);
-    delete this;
-    return 1;
-  }
-  return 0;
-}
-
 }
 }
 

diff --git a/src/surf/network_ns3.hpp b/src/surf/network_ns3.hpp
index f8e3e3c..dbf99ea 100644 (file)
--- a/src/surf/network_ns3.hpp
+++ b/src/surf/network_ns3.hpp
@@ -48,7 +48,6 @@ public:
   NetworkNS3Action(kernel::resource::Model* model, double cost, s4u::Host* src, s4u::Host* dst);
 
   bool isSuspended() override;
-  int unref() override;
   void suspend() override;
   void resume() override;
   std::list<LinkImpl*> links() override;

diff --git a/src/surf/ptask_L07.cpp b/src/surf/ptask_L07.cpp
index a613ff3..b670036 100644 (file)
--- a/src/surf/ptask_L07.cpp
+++ b/src/surf/ptask_L07.cpp
@@ -407,19 +407,5 @@ void L07Action::updateBound()
   }
 }
 
-int L07Action::unref()
-{
-  refcount_--;
-  if (not refcount_) {
-    if (state_set_hook_.is_linked())
-      simgrid::xbt::intrusive_erase(*state_set_, *this);
-    if (getVariable())
-      get_model()->getMaxminSystem()->variable_free(getVariable());
-    delete this;
-    return 1;
-  }
-  return 0;
-}
-
 }
 }

diff --git a/src/surf/ptask_L07.hpp b/src/surf/ptask_L07.hpp
index 90de6a9..03d43d2 100644 (file)
--- a/src/surf/ptask_L07.hpp
+++ b/src/surf/ptask_L07.hpp
@@ -113,8 +113,6 @@ public:
 
   void updateBound();
 
-  int unref() override;
-
   std::vector<s4u::Host*>* hostList_ = new std::vector<s4u::Host*>();
   double *computationAmount_;
   double *communicationAmount_;

diff --git a/src/surf/storage_n11.cpp b/src/surf/storage_n11.cpp
index 647e2b7..47234a0 100644 (file)
--- a/src/surf/storage_n11.cpp
+++ b/src/surf/storage_n11.cpp
@@ -139,21 +139,6 @@ StorageN11Action::StorageN11Action(kernel::resource::Model* model, double cost,
   XBT_OUT();
 }
 
-int StorageN11Action::unref()
-{
-  refcount_--;
-  if (not refcount_) {
-    if (state_set_hook_.is_linked())
-      simgrid::xbt::intrusive_erase(*state_set_, *this);
-    if (getVariable())
-      get_model()->getMaxminSystem()->variable_free(getVariable());
-    xbt_free(get_category());
-    delete this;
-    return 1;
-  }
-  return 0;
-}
-
 void StorageN11Action::cancel()
 {
   set_state(Action::State::failed);

diff --git a/src/surf/storage_n11.hpp b/src/surf/storage_n11.hpp
index 2df40f3..1e4d342 100644 (file)
--- a/src/surf/storage_n11.hpp
+++ b/src/surf/storage_n11.hpp
@@ -55,7 +55,6 @@ public:
   StorageN11Action(kernel::resource::Model* model, double cost, bool failed, StorageImpl* storage,
                    e_surf_action_storage_type_t type);
   void suspend() override;
-  int unref() override;
   void cancel() override;
   void resume() override;
   bool isSuspended() override;