From 92d87d991948c97b02c5a4aec11e9fbc429ce4c6 Mon Sep 17 00:00:00 2001 From: SUTER Frederic Date: Tue, 14 Sep 2021 15:49:19 +0200 Subject: [PATCH] move maestro to EngineImpl. breaks a unit-test --- include/simgrid/s4u/Engine.hpp | 1 + src/kernel/EngineImpl.cpp | 90 ++++++++++++++------------- src/kernel/EngineImpl.hpp | 14 ++++- src/kernel/actor/ActorImpl.cpp | 16 ++--- src/kernel/context/ContextThread.cpp | 4 +- src/plugins/vm/s4u_VirtualMachine.cpp | 2 +- src/s4u/s4u_Engine.cpp | 16 ++--- src/simix/popping_bodies.cpp | 3 +- src/simix/simcalls.py | 3 +- src/simix/smx_global.cpp | 2 +- src/simix/smx_private.hpp | 10 --- 11 files changed, 85 insertions(+), 76 deletions(-) diff --git a/include/simgrid/s4u/Engine.hpp b/include/simgrid/s4u/Engine.hpp index a494fa05a2..fdffcc1558 100644 --- a/include/simgrid/s4u/Engine.hpp +++ b/include/simgrid/s4u/Engine.hpp @@ -50,6 +50,7 @@ public: static double get_clock(); /** @brief Retrieve the engine singleton */ static s4u::Engine* get_instance(); + static s4u::Engine* get_instance(int* argc, char** argv); void load_platform(const std::string& platf) const; diff --git a/src/kernel/EngineImpl.cpp b/src/kernel/EngineImpl.cpp index 3a2ba74c26..c1b511a5e9 100644 --- a/src/kernel/EngineImpl.cpp +++ b/src/kernel/EngineImpl.cpp @@ -34,32 +34,64 @@ EngineImpl* EngineImpl::instance_ = nullptr; /* That singleton is awful too. */ config::Flag cfg_breakpoint{"debug/breakpoint", "When non-negative, raise a SIGTRAP after given (simulated) time", -1.0}; + +EngineImpl::EngineImpl(int* argc, char** argv) +{ + EngineImpl::instance_ = this; +} + +EngineImpl::~EngineImpl() +{ + /* Since hosts_ is a std::map, the hosts are destroyed in the lexicographic order, which ensures that the output is + * reproducible. + */ + while (not hosts_.empty()) + hosts_.begin()->second->destroy(); + + /* Also delete the other data */ + delete netzone_root_; + for (auto const& kv : netpoints_) + delete kv.second; + + for (auto const& kv : links_) + if (kv.second) + kv.second->destroy(); + + for (auto const& kv : mailboxes_) + delete kv.second; + + /* Free the remaining data structures */ +#if SIMGRID_HAVE_MC + xbt_dynar_free(&actors_vector_); + xbt_dynar_free(&dead_actors_vector_); +#endif + /* clear models before freeing handle, network models can use external callback defined in the handle */ + models_prio_.clear(); +} + void EngineImpl::shutdown() { static bool already_cleaned_up = false; if (already_cleaned_up) return; // to avoid double cleaning by java and C already_cleaned_up = true; - if (not EngineImpl::instance_) { - simix_global->destroy_maestro(); + if (not instance_) { simix_global->destroy_context_factory(); return; // Nothing more to shutdown } XBT_DEBUG("EngineImpl::shutdown() called. Simulation's over."); - - /* Kill all actors (but maestro) */ - simix_global->get_maestro()->kill_all(); - instance_->run_all_actors(); - instance_->empty_trash(); - if (instance_->has_actors_to_run() && simgrid_get_clock() <= 0.0) { XBT_CRITICAL(" "); - XBT_CRITICAL("The time is still 0, and you still have %lu processes ready to run.", - instance_->get_actor_to_run_count()); + XBT_CRITICAL("The time is still 0, and you still have processes ready to run."); XBT_CRITICAL("It seems that you forgot to run the simulation that you setup."); xbt_die("Bailing out to avoid that stop-before-start madness. Please fix your code."); } + /* Kill all actors (but maestro) */ + instance_->maestro_->kill_all(); + instance_->run_all_actors(); + instance_->empty_trash(); + #if HAVE_SMPI if (not instance_->actor_list_.empty()) { if (smpi_process()->initialized()) { @@ -72,7 +104,7 @@ void EngineImpl::shutdown() #endif /* Let's free maestro now */ - simix_global->destroy_maestro(); + instance_->destroy_maestro(); /* Finish context module and SURF */ simix_global->destroy_context_factory(); @@ -84,38 +116,10 @@ void EngineImpl::shutdown() tmgr_finalize(); sg_platf_exit(); - simgrid::s4u::Engine::shutdown(); simix_global = nullptr; -} - -EngineImpl::~EngineImpl() -{ - /* Since hosts_ is a std::map, the hosts are destroyed in the lexicographic order, which ensures that the output is - * reproducible. - */ - while (not hosts_.empty()) - hosts_.begin()->second->destroy(); - - /* Also delete the other data */ - delete netzone_root_; - for (auto const& kv : netpoints_) - delete kv.second; - - for (auto const& kv : links_) - if (kv.second) - kv.second->destroy(); - - for (auto const& kv : mailboxes_) - delete kv.second; - - /* Free the remaining data structures */ -#if SIMGRID_HAVE_MC - xbt_dynar_free(&actors_vector_); - xbt_dynar_free(&dead_actors_vector_); -#endif - /* clear models before freeing handle, network models can use external callback defined in the handle */ - models_prio_.clear(); + delete instance_; + instance_ = nullptr; } void EngineImpl::load_platform(const std::string& platf) @@ -429,7 +433,7 @@ void EngineImpl::run() if (actor_list_.size() == daemons_.size()) for (auto const& dmon : daemons_) { XBT_DEBUG("Kill %s", dmon->get_cname()); - simix_global->get_maestro()->kill(dmon); + maestro_->kill(dmon); } } @@ -469,7 +473,7 @@ void EngineImpl::run() simgrid::s4u::Engine::on_deadlock(); for (auto const& kv : actor_list_) { XBT_DEBUG("Kill %s", kv.second->get_cname()); - simix_global->get_maestro()->kill(kv.second); + maestro_->kill(kv.second); } } } while (time > -1.0 || has_actors_to_run()); diff --git a/src/kernel/EngineImpl.hpp b/src/kernel/EngineImpl.hpp index a775191b4e..136faf40f8 100644 --- a/src/kernel/EngineImpl.hpp +++ b/src/kernel/EngineImpl.hpp @@ -71,12 +71,13 @@ class EngineImpl { std::mutex mutex_; static EngineImpl* instance_; + actor::ActorImpl* maestro_ = nullptr; std::unique_ptr> platf_handle_; //!< handle for platform library friend s4u::Engine; public: - EngineImpl() = default; + explicit EngineImpl(int* argc, char** argv); /* Currently, only one instance is allowed to exist. This is why you can't copy or move it */ #ifndef DOXYGEN @@ -91,6 +92,14 @@ public: void register_function(const std::string& name, const actor::ActorCodeFactory& code); void register_default(const actor::ActorCodeFactory& code); + bool is_maestro(const actor::ActorImpl* actor) const { return actor == maestro_; } + void set_maestro(actor::ActorImpl* actor) { maestro_ = actor; } + actor::ActorImpl* get_maestro() const { return maestro_; } + void destroy_maestro() + { + delete maestro_; + maestro_ = nullptr; + } /** * @brief Add a model to engine list * @@ -103,7 +112,8 @@ public: /** @brief Get list of all models managed by this engine */ const std::vector& get_all_models() const { return models_; } - static EngineImpl* get_instance() { return simgrid::s4u::Engine::get_instance()->pimpl; } + static EngineImpl* get_instance() { return s4u::Engine::get_instance()->pimpl; } + static EngineImpl* get_instance(int* argc, char** argv) { return s4u::Engine::get_instance(argc, argv)->pimpl; } actor::ActorCodeFactory get_function(const std::string& name) { diff --git a/src/kernel/actor/ActorImpl.cpp b/src/kernel/actor/ActorImpl.cpp index 587e847362..4a59ebdcf7 100644 --- a/src/kernel/actor/ActorImpl.cpp +++ b/src/kernel/actor/ActorImpl.cpp @@ -74,7 +74,7 @@ ActorImpl::ActorImpl(xbt::string name, s4u::Host* host) : host_(host), name_(std ActorImpl::~ActorImpl() { - if (simix_global != nullptr && not simix_global->is_maestro(this)) + if (simix_global != nullptr && not EngineImpl::get_instance()->is_maestro(this)) s4u::Actor::on_destruction(*get_ciface()); } @@ -179,7 +179,7 @@ void ActorImpl::cleanup() XBT_DEBUG("%s@%s(%ld) should not run anymore", get_cname(), get_host()->get_cname(), get_pid()); - if (simix_global->is_maestro(this)) /* Do not cleanup maestro */ + if (EngineImpl::get_instance()->is_maestro(this)) /* Do not cleanup maestro */ return; XBT_DEBUG("Cleanup actor %s (%p), waiting synchro %p", get_cname(), this, waiting_synchro_.get()); @@ -236,7 +236,7 @@ void ActorImpl::exit() void ActorImpl::kill(ActorImpl* actor) const { - xbt_assert(not simix_global->is_maestro(actor), "Killing maestro is a rather bad idea"); + xbt_assert(not EngineImpl::get_instance()->is_maestro(actor), "Killing maestro is a rather bad idea"); if (actor->finished_) { XBT_DEBUG("Ignoring request to kill actor %s@%s that is already dead", actor->get_cname(), actor->host_->get_cname()); @@ -335,7 +335,7 @@ void ActorImpl::undaemonize() s4u::Actor* ActorImpl::restart() { - xbt_assert(not simix_global->is_maestro(this), "Restarting maestro is not supported"); + xbt_assert(not EngineImpl::get_instance()->is_maestro(this), "Restarting maestro is not supported"); XBT_DEBUG("Restarting actor %s on %s", get_cname(), host_->get_cname()); @@ -429,11 +429,11 @@ void ActorImpl::throw_exception(std::exception_ptr e) void ActorImpl::simcall_answer() { - if (not simix_global->is_maestro(this)) { + auto* engine = EngineImpl::get_instance(); + if (not engine->is_maestro(this)) { XBT_DEBUG("Answer simcall %s issued by %s (%p)", SIMIX_simcall_name(simcall_), get_cname(), this); xbt_assert(simcall_.call_ != simix::Simcall::NONE); simcall_.call_ = simix::Simcall::NONE; - auto* engine = EngineImpl::get_instance(); const auto& actors_to_run = engine->get_actors_to_run(); xbt_assert(not XBT_LOG_ISENABLED(simix_process, xbt_log_priority_debug) || std::find(begin(actors_to_run), end(actors_to_run), this) == end(actors_to_run), @@ -518,8 +518,8 @@ void create_maestro(const std::function& code) maestro->context_.reset(simix_global->get_context_factory()->create_maestro(ActorCode(code), maestro)); } - maestro->simcall_.issuer_ = maestro; - simix_global->set_maestro(maestro); + maestro->simcall_.issuer_ = maestro; + EngineImpl::get_instance()->set_maestro(maestro); } } // namespace actor diff --git a/src/kernel/context/ContextThread.cpp b/src/kernel/context/ContextThread.cpp index a29203fae6..4399d82482 100644 --- a/src/kernel/context/ContextThread.cpp +++ b/src/kernel/context/ContextThread.cpp @@ -157,7 +157,7 @@ void ThreadContext::suspend() void ThreadContext::attach_start() { // We're breaking the layers here by depending on the upper layer: - auto* maestro = static_cast(simix_global->get_maestro()->context_.get()); + auto* maestro = static_cast(EngineImpl::get_instance()->get_maestro()->context_.get()); maestro->begin_.release(); xbt_assert(not this->is_maestro()); this->start(); @@ -168,7 +168,7 @@ void ThreadContext::attach_stop() xbt_assert(not this->is_maestro()); this->yield(); - auto* maestro = static_cast(simix_global->get_maestro()->context_.get()); + auto* maestro = static_cast(EngineImpl::get_instance()->get_maestro()->context_.get()); maestro->end_.acquire(); Context::set_current(nullptr); diff --git a/src/plugins/vm/s4u_VirtualMachine.cpp b/src/plugins/vm/s4u_VirtualMachine.cpp index 9eadb22a37..486a75b4cb 100644 --- a/src/plugins/vm/s4u_VirtualMachine.cpp +++ b/src/plugins/vm/s4u_VirtualMachine.cpp @@ -132,7 +132,7 @@ void VirtualMachine::destroy() }); }; - if (this_actor::get_host() == this) { + if (not this_actor::is_maestro() && this_actor::get_host() == this) { XBT_VERB("Launch another actor on physical host %s to destroy my own VM: %s", get_pm()->get_cname(), get_cname()); simgrid::s4u::Actor::create(get_cname() + std::string("-destroy"), get_pm(), destroy_code); simgrid::s4u::this_actor::yield(); diff --git a/src/s4u/s4u_Engine.cpp b/src/s4u/s4u_Engine.cpp index 7ec9933c75..75193315fd 100644 --- a/src/s4u/s4u_Engine.cpp +++ b/src/s4u/s4u_Engine.cpp @@ -42,34 +42,37 @@ void Engine::initialize(int* argc, char** argv) { xbt_assert(Engine::instance_ == nullptr, "It is currently forbidden to create more than one instance of s4u::Engine"); Engine::instance_ = this; - kernel::EngineImpl::instance_ = pimpl; instr::init(); SIMIX_global_init(argc, argv); } -Engine::Engine(std::string name) : pimpl(new kernel::EngineImpl()) +Engine::Engine(std::string name) : pimpl(new kernel::EngineImpl(nullptr, nullptr)) { int argc = 1; char* argv = &name[0]; initialize(&argc, &argv); } -Engine::Engine(int* argc, char** argv) : pimpl(new kernel::EngineImpl()) +Engine::Engine(int* argc, char** argv) : pimpl(new kernel::EngineImpl(argc, argv)) { initialize(argc, argv); } Engine::~Engine() { - delete pimpl; + pimpl->shutdown(); Engine::instance_ = nullptr; } /** @brief Retrieve the engine singleton */ Engine* Engine::get_instance() +{ + return get_instance(nullptr, nullptr); +} +Engine* Engine::get_instance(int* argc, char** argv) { if (Engine::instance_ == nullptr) { - auto e = new Engine(nullptr, nullptr); + auto e = new Engine(argc, argv); xbt_assert(Engine::instance_ == e); } return Engine::instance_; @@ -78,7 +81,6 @@ Engine* Engine::get_instance() void Engine::shutdown() { delete Engine::instance_; - Engine::instance_ = nullptr; } double Engine::get_clock() @@ -456,7 +458,7 @@ Engine* Engine::set_default_comm_data_copy_callback(void (*callback)(kernel::act /* **************************** Public C interface *************************** */ void simgrid_init(int* argc, char** argv) { - simgrid::s4u::Engine e(argc, argv); + static simgrid::s4u::Engine e(argc, argv); } void simgrid_load_platform(const char* file) { diff --git a/src/simix/popping_bodies.cpp b/src/simix/popping_bodies.cpp index c3b3d94bad..4b4a0843d0 100644 --- a/src/simix/popping_bodies.cpp +++ b/src/simix/popping_bodies.cpp @@ -15,6 +15,7 @@ */ #include "smx_private.hpp" +#include "src/kernel/EngineImpl.hpp" #include "src/mc/mc_forward.hpp" #include "xbt/ex.h" #include @@ -31,7 +32,7 @@ inline static R simcall(Simcall call, T const&... t) { smx_actor_t self = SIMIX_process_self(); simgrid::simix::marshal(&self->simcall_, call, t...); - if (not simix_global->is_maestro(self)) { + if (not simgrid::kernel::EngineImpl::get_instance()->is_maestro(self)) { XBT_DEBUG("Yield process '%s' on simcall %s", self->get_cname(), SIMIX_simcall_name(self->simcall_)); self->yield(); } else { diff --git a/src/simix/simcalls.py b/src/simix/simcalls.py index fb346f8d8f..4d1a9ba3f8 100755 --- a/src/simix/simcalls.py +++ b/src/simix/simcalls.py @@ -364,6 +364,7 @@ if __name__ == '__main__': fd = header('popping_bodies.cpp') fd.write('#include "smx_private.hpp"\n') fd.write('#include "src/mc/mc_forward.hpp"\n') + fd.write('#include "src/kernel/EngineImpl.hpp"\n') fd.write('#include "xbt/ex.h"\n') fd.write('#include \n') fd.write('#include \n') @@ -380,7 +381,7 @@ inline static R simcall(Simcall call, T const&... t) { smx_actor_t self = SIMIX_process_self(); simgrid::simix::marshal(&self->simcall_, call, t...); - if (not simix_global->is_maestro(self)) { + if (not simgrid::kernel::EngineImpl::get_instance()->is_maestro(self)) { XBT_DEBUG("Yield process '%s' on simcall %s", self->get_cname(), SIMIX_simcall_name(self->simcall_)); self->yield(); } else { diff --git a/src/simix/smx_global.cpp b/src/simix/smx_global.cpp index 5e14abbee1..144f2784b5 100644 --- a/src/simix/smx_global.cpp +++ b/src/simix/smx_global.cpp @@ -205,5 +205,5 @@ int SIMIX_is_maestro() if (simix_global == nullptr) // SimDag return true; const simgrid::kernel::actor::ActorImpl* self = SIMIX_process_self(); - return self == nullptr || simix_global->is_maestro(self); + return self == nullptr || simgrid::kernel::EngineImpl::get_instance()->is_maestro(self); } diff --git a/src/simix/smx_private.hpp b/src/simix/smx_private.hpp index f0d915ec5b..e9485988df 100644 --- a/src/simix/smx_private.hpp +++ b/src/simix/smx_private.hpp @@ -17,18 +17,8 @@ namespace simix { class Global { kernel::context::ContextFactory* context_factory_ = nullptr; - kernel::actor::ActorImpl* maestro_ = nullptr; public: - bool is_maestro(const kernel::actor::ActorImpl* actor) const { return actor == maestro_; } - void set_maestro(kernel::actor::ActorImpl* actor) { maestro_ = actor; } - kernel::actor::ActorImpl* get_maestro() const { return maestro_; } - void destroy_maestro() - { - delete maestro_; - maestro_ = nullptr; - } - kernel::context::ContextFactory* get_context_factory() const { return context_factory_; } void set_context_factory(kernel::context::ContextFactory* factory) { context_factory_ = factory; } bool has_context_factory() const { return context_factory_ != nullptr; } -- 2.20.1