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 b42d5b4823b0f82f8478a3174fb7e61d0e3645ce..20e877ca766b53b07eee2220e71e52d39a0975a4 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
    */
   /** @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();
 
   /** @brief Cancel the current Action if running */
   virtual void cancel();


@@ -170,9 +170,9 @@ public:
 
 protected:
   StateSet* state_set_;
 
 protected:
   StateSet* state_set_;

-  int refcount_ = 1;

 
 private:
 
 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 */
   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 b21dbbade15a9156e860f0e315da68760a753f3c..bac3a436610351e9210aedbfb9f17a78dd46ac98 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()
 {
 
 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_);
 }
 
   xbt_free(category_);
 }
 


@@ -137,16 +148,6 @@ int Action::unref()
 {
   refcount_--;
   if (not refcount_) {
 {
   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;
   }
     delete this;
     return 1;
   }

diff --git a/src/surf/cpu_ti.cpp b/src/surf/cpu_ti.cpp
index 6cf864c64d1d260675f67c31997a50c1369ffb46..c2b3eb6daf31a3c6daaf3ce67730be6f097ab80d 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);
 }
 {
   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);
 }
 
   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()
 }
 
 void CpuTiAction::cancel()

diff --git a/src/surf/cpu_ti.hpp b/src/surf/cpu_ti.hpp
index ec4806d8bcba6e5866b4ab9b842e081b0ef8f837..375de18577e8aef91bf8af0970ee1600392cf2fa 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);
   friend class CpuTi;
 public:
   CpuTiAction(CpuTiModel *model, double cost, bool failed, CpuTi *cpu);

+  ~CpuTiAction();

 
   void set_state(simgrid::kernel::resource::Action::State state) override;
 
   void set_state(simgrid::kernel::resource::Action::State state) override;

-  int unref() override;

   void cancel() override;
   void suspend() override;
   void resume() 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 c42e0e1c4197b4e0f5565f8f1e65a61cb5162907..3821b8a361300636ac61c0e87a0618ce5961ea1f 100644 (file)
--- a/src/surf/network_ns3.cpp
+++ b/src/surf/network_ns3.cpp
@@ -350,19 +350,6 @@ bool NetworkNS3Action::isSuspended()
   return false;
 }
 
   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 f8e3e3c84d2504d650bd82df5ce3b4c05ae4eaba..dbf99ea794228e9f7d9a9a4d9f7e0802642fe429 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;
   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;
   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 a613ff30d62aad0202361ced1019960c2957636f..b6700367cb193a52a2556e0721b0ea8fdf5e4ddb 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 90de6a9f6ea30ed251e6ed41c4c6b72466556c4a..03d43d204ecd3461b4ebed355853e82afe76f25e 100644 (file)
--- a/src/surf/ptask_L07.hpp
+++ b/src/surf/ptask_L07.hpp
@@ -113,8 +113,6 @@ public:
 
   void updateBound();
 
 
   void updateBound();
 

-  int unref() override;
-

   std::vector<s4u::Host*>* hostList_ = new std::vector<s4u::Host*>();
   double *computationAmount_;
   double *communicationAmount_;
   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 647e2b738634d1dcc377dd5366fb5f98ea4b30ba..47234a01f83170163326bed1c85618a8946590e5 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();
 }
 
   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);
 void StorageN11Action::cancel()
 {
   set_state(Action::State::failed);

diff --git a/src/surf/storage_n11.hpp b/src/surf/storage_n11.hpp
index 2df40f38a67320397290726038d6d8059bd08080..1e4d3426ab4752cbca582b1fd6a1a33f7caf8a8c 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;
   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;
   void cancel() override;
   void resume() override;
   bool isSuspended() override;