From: Bruno Donassolo <bruno.donassolo@inria.fr>
Date: Thu, 18 Mar 2021 09:49:30 +0000 (+0100)
Subject: Refactor Host and HostImpl
X-Git-Tag: v3.28~517
X-Git-Url: http://bilbo.iut-bm.univ-fcomte.fr/pub/gitweb/simgrid.git/commitdiff_plain/9b4e5a12b9e11e584c2c5af4d96e38b8220a72b9

Refactor Host and HostImpl

- Going towards empty s4u interfaces
- s4u::Host is created at HostImpl directly as for s4u::Link and
LinkImpl.
- VirtualMachines are still an exception. The user creates directly the
s4u::VirtualMachine object
---

diff --git a/include/simgrid/s4u/Engine.hpp b/include/simgrid/s4u/Engine.hpp
index 883aa08282..9013733677 100644
--- a/include/simgrid/s4u/Engine.hpp
+++ b/include/simgrid/s4u/Engine.hpp
@@ -85,6 +85,7 @@ public:
 
 protected:
 #ifndef DOXYGEN
+  friend surf::HostImpl;
   friend Host;
   friend Link;
   friend Disk;
diff --git a/include/simgrid/s4u/Host.hpp b/include/simgrid/s4u/Host.hpp
index 7ebb16d2eb..b23efc3c63 100644
--- a/include/simgrid/s4u/Host.hpp
+++ b/include/simgrid/s4u/Host.hpp
@@ -40,9 +40,13 @@ class XBT_PUBLIC Host : public xbt::Extendable<Host> {
   friend vm::VMModel;            // Use the pimpl_cpu to compute the VM sharing
   friend vm::VirtualMachineImpl; // creates the the pimpl_cpu
   friend kernel::routing::NetZoneImpl;
+  friend surf::HostImpl; // call destructor from private implementation
+
+  // The private implementation, that never changes
+  surf::HostImpl* const pimpl_;
 
 public:
-  explicit Host(const std::string& name);
+  explicit Host(surf::HostImpl* pimpl) : pimpl_(pimpl) {}
 
 protected:
   virtual ~Host(); // Call destroy() instead of manually deleting it.
@@ -75,9 +79,9 @@ public:
   static Host* current();
 
   /** Retrieves the name of that host as a C++ string */
-  xbt::string const& get_name() const { return name_; }
+  xbt::string const& get_name() const;
   /** Retrieves the name of that host as a C string */
-  const char* get_cname() const { return name_.c_str(); }
+  const char* get_cname() const;
 
   kernel::routing::NetPoint* get_netpoint() const { return pimpl_netpoint_; }
 
@@ -183,17 +187,15 @@ public:
 
   /** Block the calling actor on an execution located on the called host (with explicit priority) */
   void execute(double flops, double priority) const;
+  surf::HostImpl* get_impl() const { return pimpl_; }
 
 private:
-  xbt::string name_{"noname"};
   kernel::routing::NetPoint* pimpl_netpoint_ = nullptr;
 
 public:
 #ifndef DOXYGEN
   /** DO NOT USE DIRECTLY (@todo: these should be protected, once our code is clean) */
   kernel::resource::Cpu* pimpl_cpu = nullptr;
-  // TODO, this could be a unique_ptr
-  surf::HostImpl* pimpl_ = nullptr;
 #endif
 };
 } // namespace s4u
diff --git a/src/kernel/actor/ActorImpl.cpp b/src/kernel/actor/ActorImpl.cpp
index 0db05cc41e..3cfa311990 100644
--- a/src/kernel/actor/ActorImpl.cpp
+++ b/src/kernel/actor/ActorImpl.cpp
@@ -115,7 +115,7 @@ ActorImplPtr ActorImpl::attach(const std::string& name, void* data, s4u::Host* h
     actor->set_properties(*properties);
 
   /* Add the actor to it's host actor list */
-  host->pimpl_->add_actor(actor);
+  host->get_impl()->add_actor(actor);
 
   /* Now insert it in the global actor list and in the actors to run list */
   simix_global->process_list[actor->get_pid()] = actor;
@@ -152,7 +152,7 @@ void ActorImpl::cleanup_from_simix()
   const std::lock_guard<std::mutex> lock(simix_global->mutex);
   simix_global->process_list.erase(pid_);
   if (host_ && host_actor_list_hook.is_linked())
-    host_->pimpl_->remove_actor(this);
+    host_->get_impl()->remove_actor(this);
   if (not smx_destroy_list_hook.is_linked()) {
 #if SIMGRID_HAVE_MC
     xbt_dynar_push_as(simix_global->dead_actors_vector, ActorImpl*, this);
@@ -464,9 +464,9 @@ void ActorImpl::simcall_answer()
 
 void ActorImpl::set_host(s4u::Host* dest)
 {
-  host_->pimpl_->remove_actor(this);
+  host_->get_impl()->remove_actor(this);
   host_ = dest;
-  dest->pimpl_->add_actor(this);
+  dest->get_impl()->add_actor(this);
 }
 
 ActorImplPtr ActorImpl::init(const std::string& name, s4u::Host* host) const
@@ -498,7 +498,7 @@ ActorImpl* ActorImpl::start(const ActorCode& code)
   XBT_DEBUG("Start context '%s'", get_cname());
 
   /* Add the actor to its host's actor list */
-  host_->pimpl_->add_actor(this);
+  host_->get_impl()->add_actor(this);
   simix_global->process_list[pid_] = this;
 
   /* Now insert it in the global actor list and in the actor to run list */
diff --git a/src/kernel/routing/NetZoneImpl.cpp b/src/kernel/routing/NetZoneImpl.cpp
index da4070ff64..034a0bb950 100644
--- a/src/kernel/routing/NetZoneImpl.cpp
+++ b/src/kernel/routing/NetZoneImpl.cpp
@@ -93,7 +93,7 @@ s4u::Host* NetZoneImpl::create_host(const std::string& name, const std::vector<d
   if (hierarchy_ == RoutingMode::unset)
     hierarchy_ = RoutingMode::base;
 
-  auto* res = new s4u::Host(name);
+  auto* res = (new surf::HostImpl(name))->get_iface();
   res->set_netpoint((new NetPoint(name, NetPoint::Type::Host))->set_englobing_zone(this));
 
   cpu_model_pm_->create_cpu(res, speed_per_pstate)->set_core_count(core_amount)->seal();
diff --git a/src/plugins/vm/VirtualMachineImpl.cpp b/src/plugins/vm/VirtualMachineImpl.cpp
index 4c35264706..01790ffff0 100644
--- a/src/plugins/vm/VirtualMachineImpl.cpp
+++ b/src/plugins/vm/VirtualMachineImpl.cpp
@@ -173,13 +173,12 @@ double VMModel::next_occurring_event(double now)
  * Resource *
  ************/
 
-VirtualMachineImpl::VirtualMachineImpl(simgrid::s4u::VirtualMachine* piface, simgrid::s4u::Host* host_PM,
-                                       int core_amount, size_t ramsize)
-    : HostImpl(piface), physical_host_(host_PM), core_amount_(core_amount), ramsize_(ramsize)
+VirtualMachineImpl::VirtualMachineImpl(const std::string& name, s4u::VirtualMachine* piface,
+                                       simgrid::s4u::Host* host_PM, int core_amount, size_t ramsize)
+    : HostImpl(name, piface), piface_(piface), physical_host_(host_PM), core_amount_(core_amount), ramsize_(ramsize)
 {
   /* Register this VM to the list of all VMs */
   allVms_.push_back(piface);
-
   /* We create cpu_action corresponding to a VM process on the host operating system. */
   /* TODO: we have to periodically input GUESTOS_NOISE to the system? how ?
    * The value for GUESTOS_NOISE corresponds to the cost of the global action associated to the VM.  It corresponds to
@@ -190,7 +189,7 @@ VirtualMachineImpl::VirtualMachineImpl(simgrid::s4u::VirtualMachine* piface, sim
   // It's empty for now, so it should not request resources in the PM
   update_action_weight();
 
-  XBT_VERB("Create VM(%s)@PM(%s)", piface->get_cname(), physical_host_->get_cname());
+  XBT_VERB("Create VM(%s)@PM(%s)", name.c_str(), physical_host_->get_cname());
   on_creation(*this);
 }
 
diff --git a/src/plugins/vm/VirtualMachineImpl.hpp b/src/plugins/vm/VirtualMachineImpl.hpp
index 2bddd02a10..e3056d23f8 100644
--- a/src/plugins/vm/VirtualMachineImpl.hpp
+++ b/src/plugins/vm/VirtualMachineImpl.hpp
@@ -38,7 +38,8 @@ public:
 
   static std::deque<s4u::VirtualMachine*> allVms_;
 
-  explicit VirtualMachineImpl(s4u::VirtualMachine* piface, s4u::Host* host, int core_amount, size_t ramsize);
+  explicit VirtualMachineImpl(const std::string& name, s4u::VirtualMachine* piface, s4u::Host* host, int core_amount,
+                              size_t ramsize);
   ~VirtualMachineImpl() override;
 
   virtual void suspend(kernel::actor::ActorImpl* issuer);
@@ -59,6 +60,9 @@ public:
   unsigned int get_core_amount() const { return core_amount_; }
   kernel::resource::Action* get_action() const { return action_; }
 
+  const s4u::VirtualMachine* get_iface() const override { return piface_; }
+  s4u::VirtualMachine* get_iface() override { return piface_; }
+
   virtual void set_bound(double bound);
 
   void update_action_weight();
@@ -71,6 +75,7 @@ public:
   bool is_migrating() const { return is_migrating_; }
 
 private:
+  s4u::VirtualMachine* piface_;
   kernel::resource::Action* action_ = nullptr;
   unsigned int active_execs_        = 0;
   s4u::Host* physical_host_;
@@ -100,7 +105,7 @@ public:
     return nullptr;
   };
 };
-}
-}
+} // namespace vm
+} // namespace simgrid
 
 #endif /* VM_INTERFACE_HPP_ */
diff --git a/src/plugins/vm/s4u_VirtualMachine.cpp b/src/plugins/vm/s4u_VirtualMachine.cpp
index 3b412c8890..7450b709cd 100644
--- a/src/plugins/vm/s4u_VirtualMachine.cpp
+++ b/src/plugins/vm/s4u_VirtualMachine.cpp
@@ -30,7 +30,8 @@ VirtualMachine::VirtualMachine(const std::string& name, s4u::Host* physical_host
 }
 
 VirtualMachine::VirtualMachine(const std::string& name, s4u::Host* physical_host, int core_amount, size_t ramsize)
-    : Host(name), pimpl_vm_(new vm::VirtualMachineImpl(this, physical_host, core_amount, ramsize))
+    : Host(new vm::VirtualMachineImpl(name, this, physical_host, core_amount, ramsize))
+    , pimpl_vm_(dynamic_cast<vm::VirtualMachineImpl*>(Host::get_impl()))
 {
   XBT_DEBUG("Create VM %s", get_cname());
 
@@ -42,7 +43,12 @@ VirtualMachine::VirtualMachine(const std::string& name, s4u::Host* physical_host
   for (int i = 0; i < physical_host->get_pstate_count(); i++)
     speeds.push_back(physical_host->get_pstate_speed(i));
 
-  physical_host->get_netpoint()->get_englobing_zone()->get_cpu_vm_model()->create_cpu(this, speeds)->set_core_count(core_amount)->seal();
+  physical_host->get_netpoint()
+      ->get_englobing_zone()
+      ->get_cpu_vm_model()
+      ->create_cpu(this, speeds)
+      ->set_core_count(core_amount)
+      ->seal();
   if (physical_host->get_pstate() != 0)
     set_pstate(physical_host->get_pstate());
 
@@ -124,7 +130,8 @@ void VirtualMachine::destroy()
   shutdown();
 
   /* Then, destroy the VM object */
-  Host::destroy();
+  get_impl()->destroy();
+  delete this;
 }
 
 simgrid::s4u::Host* VirtualMachine::get_pm() const
@@ -185,8 +192,8 @@ VirtualMachine* VirtualMachine::set_bound(double bound)
   return this;
 }
 
-} // namespace simgrid
 } // namespace s4u
+} // namespace simgrid
 
 /* **************************** Public C interface *************************** */
 
@@ -280,12 +287,13 @@ void sg_vm_resume(sg_vm_t vm)
 /** @brief Immediately kills all processes within the given VM.
  *
  @beginrst
- 
+
  The memory allocated by these actors is leaked, unless you used :cpp:func:`simgrid::s4u::Actor::on_exit`.
-  
+
  @endrst
- * 
- * No extra delay occurs by default. You may let your actor sleep by a specific amount to simulate any extra delay that you want.
+ *
+ * No extra delay occurs by default. You may let your actor sleep by a specific amount to simulate any extra delay that
+ you want.
  */
 void sg_vm_shutdown(sg_vm_t vm)
 {
diff --git a/src/s4u/s4u_Actor.cpp b/src/s4u/s4u_Actor.cpp
index d7a20c2439..944b97c2f8 100644
--- a/src/s4u/s4u_Actor.cpp
+++ b/src/s4u/s4u_Actor.cpp
@@ -133,7 +133,7 @@ void Actor::set_auto_restart(bool autorestart)
 
     auto* arg = new kernel::actor::ProcessArg(pimpl_->get_host(), pimpl_);
     XBT_DEBUG("Adding %s to the actors_at_boot_ list of Host %s", arg->name.c_str(), arg->host->get_cname());
-    pimpl_->get_host()->pimpl_->add_actor_at_boot(arg);
+    pimpl_->get_host()->get_impl()->add_actor_at_boot(arg);
   });
 }
 
diff --git a/src/s4u/s4u_Host.cpp b/src/s4u/s4u_Host.cpp
index f7b9a43fad..2689b56341 100644
--- a/src/s4u/s4u_Host.cpp
+++ b/src/s4u/s4u_Host.cpp
@@ -32,13 +32,6 @@ xbt::signal<void(Host const&)> Host::on_destruction;
 xbt::signal<void(Host const&)> Host::on_state_change;
 xbt::signal<void(Host const&)> Host::on_speed_change;
 
-Host::Host(const std::string& name) : name_(name)
-{
-  xbt_assert(Host::by_name_or_null(name_) == nullptr, "Refusing to create a second host named '%s'.", get_cname());
-  Engine::get_instance()->host_register(name_, this);
-  new surf::HostImpl(this);
-}
-
 Host* Host::set_netpoint(kernel::routing::NetPoint* netpoint)
 {
   pimpl_netpoint_ = netpoint;
@@ -47,7 +40,6 @@ Host* Host::set_netpoint(kernel::routing::NetPoint* netpoint)
 
 Host::~Host()
 {
-  delete pimpl_;
   if (pimpl_netpoint_ != nullptr) // not removed yet by a children class
     Engine::get_instance()->netpoint_unregister(pimpl_netpoint_);
   delete pimpl_cpu;
@@ -62,9 +54,7 @@ Host::~Host()
  */
 void Host::destroy()
 {
-  on_destruction(*this);
-  Engine::get_instance()->host_unregister(std::string(name_));
-  delete this;
+  kernel::actor::simcall([this] { this->pimpl_->destroy(); });
 }
 
 Host* Host::by_name(const std::string& name)
@@ -84,6 +74,16 @@ Host* Host::current()
   return self->get_host();
 }
 
+xbt::string const& Host::get_name() const
+{
+  return this->pimpl_->get_name();
+}
+
+const char* Host::get_cname() const
+{
+  return this->pimpl_->get_cname();
+}
+
 void Host::turn_on()
 {
   if (not is_on()) {
diff --git a/src/smpi/mpi/smpi_comm.cpp b/src/smpi/mpi/smpi_comm.cpp
index 8f5397263c..84751b080d 100644
--- a/src/smpi/mpi/smpi_comm.cpp
+++ b/src/smpi/mpi/smpi_comm.cpp
@@ -356,7 +356,7 @@ MPI_Comm Comm::find_intra_comm(int * leader){
   //get the indices of all processes sharing the same simix host
   int intra_comm_size     = 0;
   int min_index           = INT_MAX; // the minimum index will be the leader
-  sg_host_self()->pimpl_->foreach_actor([this, &intra_comm_size, &min_index](auto& actor) {
+  sg_host_self()->get_impl()->foreach_actor([this, &intra_comm_size, &min_index](auto& actor) {
     int index = actor.get_pid();
     if (this->group()->rank(actor.get_ciface()) != MPI_UNDEFINED) { // Is this process in the current group?
       intra_comm_size++;
@@ -367,7 +367,7 @@ MPI_Comm Comm::find_intra_comm(int * leader){
   XBT_DEBUG("number of processes deployed on my node : %d", intra_comm_size);
   auto* group_intra = new Group(intra_comm_size);
   int i = 0;
-  sg_host_self()->pimpl_->foreach_actor([this, group_intra, &i](auto& actor) {
+  sg_host_self()->get_impl()->foreach_actor([this, group_intra, &i](auto& actor) {
     if (this->group()->rank(actor.get_ciface()) != MPI_UNDEFINED) {
       group_intra->set_mapping(actor.get_ciface(), i);
       i++;
diff --git a/src/surf/HostImpl.cpp b/src/surf/HostImpl.cpp
index d6547d5c81..9b8ec04df4 100644
--- a/src/surf/HostImpl.cpp
+++ b/src/surf/HostImpl.cpp
@@ -3,6 +3,8 @@
 /* This program is free software; you can redistribute it and/or modify it
  * under the terms of the license (GNU LGPL) which comes with this package. */
 
+#include "simgrid/s4u/Engine.hpp"
+#include "simgrid/s4u/Host.hpp"
 #include "src/plugins/vm/VirtualMachineImpl.hpp"
 #include "src/simix/smx_private.hpp"
 
@@ -23,11 +25,16 @@ namespace surf {
 /************
  * Resource *
  ************/
-HostImpl::HostImpl(s4u::Host* host) : piface_(host)
+HostImpl::HostImpl(const std::string& name, s4u::Host* piface) : piface_(this), name_(name)
 {
-  /* The VM wants to reinstall a new HostImpl, but we don't want to leak the previously existing one */
-  delete piface_->pimpl_;
-  piface_->pimpl_ = this;
+  xbt_assert(s4u::Host::by_name_or_null(name_) == nullptr, "Refusing to create a second host named '%s'.", get_cname());
+  s4u::Engine::get_instance()->host_register(name_, piface);
+}
+
+HostImpl::HostImpl(const std::string& name) : piface_(this), name_(name)
+{
+  xbt_assert(s4u::Host::by_name_or_null(name_) == nullptr, "Refusing to create a second host named '%s'.", get_cname());
+  s4u::Engine::get_instance()->host_register(name_, &piface_);
 }
 
 HostImpl::~HostImpl()
@@ -49,6 +56,17 @@ HostImpl::~HostImpl()
     d->destroy();
 }
 
+/** @brief Fire the required callbacks and destroy the object
+ *
+ * Don't delete directly a Host, call h->destroy() instead.
+ */
+void HostImpl::destroy()
+{
+  s4u::Host::on_destruction(*this->get_iface());
+  s4u::Engine::get_instance()->host_unregister(std::string(name_));
+  delete this;
+}
+
 /** Re-starts all the actors that are marked as restartable.
  *
  * Weird things will happen if you turn on a host that is already on. S4U is fool-proof, not this.
@@ -132,6 +150,5 @@ void HostImpl::remove_disk(const std::string& disk_name)
     position++;
   }
 }
-
 } // namespace surf
 } // namespace simgrid
diff --git a/src/surf/HostImpl.hpp b/src/surf/HostImpl.hpp
index a28ef17b05..2429c3d91b 100644
--- a/src/surf/HostImpl.hpp
+++ b/src/surf/HostImpl.hpp
@@ -48,20 +48,32 @@ class XBT_PRIVATE HostImpl : public xbt::PropertyHolder {
 
   ActorList actor_list_;
   std::vector<kernel::actor::ProcessArg*> actors_at_boot_;
-  s4u::Host* piface_ = nullptr; // we must have a pointer there because the VM wants to change the piface in its ctor
+  s4u::Host piface_;
   std::vector<kernel::resource::DiskImpl*> disks_;
+  xbt::string name_{"noname"};
+
+protected:
+  virtual ~HostImpl(); // Use destroy() instead of this destructor.
+  HostImpl(const std::string& name, s4u::Host* piface);
 
 public:
   friend simgrid::vm::VirtualMachineImpl;
-  explicit HostImpl(s4u::Host* host);
-  virtual ~HostImpl();
+  explicit HostImpl(const std::string& name);
+
+  void destroy(); // Must be called instead of the destructor
 
   std::vector<s4u::Disk*> get_disks() const;
   void set_disks(const std::vector<kernel::resource::DiskImpl*>& disks, s4u::Host* host);
   void add_disk(const s4u::Disk* disk);
   void remove_disk(const std::string& disk_name);
 
-  s4u::Host* get_iface() const { return piface_; }
+  virtual const s4u::Host* get_iface() const { return &piface_; }
+  virtual s4u::Host* get_iface() { return &piface_; }
+
+  /** Retrieves the name of that host as a C++ string */
+  xbt::string const& get_name() const { return name_; }
+  /** Retrieves the name of that host as a C string */
+  const char* get_cname() const { return name_.c_str(); }
 
   void turn_on() const;
   void turn_off(const kernel::actor::ActorImpl* issuer);
@@ -77,7 +89,7 @@ public:
       function(actor);
   }
 };
-}
-}
+} // namespace surf
+} // namespace simgrid
 
 #endif /* SURF_HOST_INTERFACE_HPP */
diff --git a/src/surf/sg_platf.cpp b/src/surf/sg_platf.cpp
index 2c22bfbdae..a1304dd59d 100644
--- a/src/surf/sg_platf.cpp
+++ b/src/surf/sg_platf.cpp
@@ -74,7 +74,7 @@ void sg_platf_new_host(const simgrid::kernel::routing::HostCreationArgs* args)
     delete args->properties;
   }
 
-  host->pimpl_->set_disks(args->disks, host);
+  host->get_impl()->set_disks(args->disks, host);
 
   /* Change from the defaults */
   host->set_state_profile(args->state_trace)->set_speed_profile(args->speed_trace);
@@ -381,7 +381,7 @@ void sg_platf_new_actor(simgrid::kernel::routing::ActorCreationArgs* actor)
   auto* arg =
       new simgrid::kernel::actor::ProcessArg(actor_name, code, nullptr, host, kill_time, properties, auto_restart);
 
-  host->pimpl_->add_actor_at_boot(arg);
+  host->get_impl()->add_actor_at_boot(arg);
 
   if (start_time > SIMIX_get_clock()) {
     arg = new simgrid::kernel::actor::ProcessArg(actor_name, code, nullptr, host, kill_time, properties, auto_restart);