From 79739657970ffcf609c1e90f6112e8f5a8583801 Mon Sep 17 00:00:00 2001 From: Martin Quinson Date: Sun, 25 Mar 2018 22:41:11 +0200 Subject: [PATCH 1/1] 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. --- include/simgrid/kernel/resource/Action.hpp | 4 ++-- src/kernel/resource/Action.cpp | 21 +++++++++-------- src/surf/cpu_ti.cpp | 27 ++++++++-------------- src/surf/cpu_ti.hpp | 2 +- src/surf/network_ns3.cpp | 13 ----------- src/surf/network_ns3.hpp | 1 - src/surf/ptask_L07.cpp | 14 ----------- src/surf/ptask_L07.hpp | 2 -- src/surf/storage_n11.cpp | 15 ------------ src/surf/storage_n11.hpp | 1 - 10 files changed, 23 insertions(+), 77 deletions(-) diff --git a/include/simgrid/kernel/resource/Action.hpp b/include/simgrid/kernel/resource/Action.hpp index b42d5b4823..20e877ca76 100644 --- 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 b21dbbade1..bac3a43661 100644 --- 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 6cf864c64d..c2b3eb6daf 100644 --- 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 ec4806d8bc..375de18577 100644 --- 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 c42e0e1c41..3821b8a361 100644 --- 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 f8e3e3c84d..dbf99ea794 100644 --- 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 links() override; diff --git a/src/surf/ptask_L07.cpp b/src/surf/ptask_L07.cpp index a613ff30d6..b6700367cb 100644 --- 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 90de6a9f6e..03d43d204e 100644 --- a/src/surf/ptask_L07.hpp +++ b/src/surf/ptask_L07.hpp @@ -113,8 +113,6 @@ public: void updateBound(); - int unref() override; - std::vector* hostList_ = new std::vector(); double *computationAmount_; double *communicationAmount_; diff --git a/src/surf/storage_n11.cpp b/src/surf/storage_n11.cpp index 647e2b7386..47234a01f8 100644 --- 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 2df40f38a6..1e4d3426ab 100644 --- 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; -- 2.20.1