Make SIMIX_process_from_PID much faster

It was a linear search in a swag, that's now using a std::map that are
usually implemented with Red/Black trees.

This patch is more complex that it should because currently, the
model-checker needs to read the list of processes from the remote
process. We cannot do that from a std::map because it would require to
understand the private implementation of the std::map, which is
system/compiler dependent.

Instead, in MC mode, we copy the whole list of processes from the map
into a dynar that is easier to read remotely. This hack should
obviously not remain as is.

Instead, we should make sure that the MCer does not need to have the
whole list of processes (even if it could read one or two given
processes on need). It seems that this list is mainly used for the
computation of the list of enabled transitions. A promising idea is to
move that computation to the MCed that has all needed information. The
precomputed data could be made available to the MCer that would only
have to read the result, which constitutes much less info to read
remotely.

diff --git a/src/mc/Process.cpp b/src/mc/Process.cpp
index 30a45ff..fd35b52 100644 (file)
--- a/src/mc/Process.cpp
+++ b/src/mc/Process.cpp
@@ -499,7 +499,7 @@ const void *Process::read_bytes(void* buffer, std::size_t size,
   }
 
   if (pread_whole(this->memory_file, buffer, size, address.address()) < 0)
-    xbt_die("Read from process %lli failed", (long long) this->pid_);
+    xbt_die("Read at %p from process %lli failed", (void*)address.address(), (long long)this->pid_);
   return buffer;
 }
 

diff --git a/src/mc/mc_base.cpp b/src/mc/mc_base.cpp
index ee4462f..96fee34 100644 (file)
--- a/src/mc/mc_base.cpp
+++ b/src/mc/mc_base.cpp
@@ -71,6 +71,12 @@ void wait_for_requests(void)
         SIMIX_simcall_handle(req, 0);
     }
   }
+#if HAVE_MC
+  xbt_dynar_reset(simix_global->actors_vector);
+  for (std::pair<int, smx_actor_t> kv : simix_global->process_list) {
+    xbt_dynar_push_as(simix_global->actors_vector, smx_actor_t, kv.second);
+  }
+#endif
 }
 
 /** @brief returns if there this transition can proceed in a finite amount of time

diff --git a/src/mc/mc_global.cpp b/src/mc/mc_global.cpp
index 8418b96..e62b9c6 100644 (file)
--- a/src/mc/mc_global.cpp
+++ b/src/mc/mc_global.cpp
@@ -97,9 +97,10 @@ void MC_run()
   simgrid::mc::processes_time.resize(SIMIX_process_get_maxpid());
   MC_ignore_heap(simgrid::mc::processes_time.data(),
     simgrid::mc::processes_time.size() * sizeof(simgrid::mc::processes_time[0]));
-  smx_actor_t process;
-  xbt_swag_foreach(process, simix_global->process_list)
-    MC_ignore_heap(&(process->process_hookup), sizeof(process->process_hookup));
+  for (auto kv : simix_global->process_list) {
+    smx_actor_t actor = kv.second;
+    MC_ignore_heap(&(actor->process_hookup), sizeof(actor->process_hookup));
+  }
   simgrid::mc::Client::get()->mainLoop();
   simgrid::mc::processes_time.clear();
 }

diff --git a/src/mc/mc_smx.cpp b/src/mc/mc_smx.cpp
index 86303fc..ff841f7 100644 (file)
--- a/src/mc/mc_smx.cpp
+++ b/src/mc/mc_smx.cpp
@@ -71,6 +71,29 @@ static void MC_process_refresh_simix_process_list(simgrid::mc::Process* process,
   assert(i == swag.count);
 }
 
+static void MC_process_refresh_simix_actor_dynar(simgrid::mc::Process* process,
+                                                 std::vector<simgrid::mc::ActorInformation>& target,
+                                                 simgrid::mc::RemotePtr<s_xbt_dynar_t> remote_dynar)
+{
+  target.clear();
+
+  s_xbt_dynar_t dynar;
+  process->read_bytes(&dynar, sizeof(dynar), remote_dynar);
+
+  smx_actor_t* data = (smx_actor_t*)malloc(dynar.elmsize * dynar.used);
+  process->read_bytes(data, dynar.elmsize * dynar.used, dynar.data);
+
+  // Load each element of the vector from the MCed process:
+  for (unsigned int i = 0; i < dynar.used; ++i) {
+
+    simgrid::mc::ActorInformation info;
+    info.address  = data[i];
+    info.hostname = nullptr;
+    process->read_bytes(&info.copy, sizeof(info.copy), remote(data[i]));
+    target.push_back(std::move(info));
+  }
+  free(data);
+}
 namespace simgrid {
 namespace mc {
 
@@ -96,7 +119,7 @@ void Process::refresh_simix()
   Remote<simgrid::simix::Global> simix_global =
     this->read<simgrid::simix::Global>(simix_global_p);
 
-  MC_process_refresh_simix_process_list(this, this->smx_actors_infos, remote(simix_global.getBuffer()->process_list));
+  MC_process_refresh_simix_actor_dynar(this, this->smx_actors_infos, remote(simix_global.getBuffer()->actors_vector));
   MC_process_refresh_simix_process_list(this, this->smx_dead_actors_infos,
                                         remote(simix_global.getBuffer()->process_to_destroy));
 

diff --git a/src/mc/remote/Channel.cpp b/src/mc/remote/Channel.cpp
index 30ccdcd..6c4000d 100644 (file)
--- a/src/mc/remote/Channel.cpp
+++ b/src/mc/remote/Channel.cpp
@@ -25,6 +25,7 @@ Channel::~Channel()
     close(this->socket_);
 }
 
+/** @brief Send a message; returns 0 on success or errno on failure */
 int Channel::send(const void* message, size_t size) const
 {
   XBT_DEBUG("Send %s", MC_message_type_name(*(e_mc_message_type*)message));

diff --git a/src/mc/remote/Channel.hpp b/src/mc/remote/Channel.hpp
index ac4fd0c..f3e2ce9 100644 (file)
--- a/src/mc/remote/Channel.hpp
+++ b/src/mc/remote/Channel.hpp
@@ -53,6 +53,7 @@ public:
     s_mc_message message = {type};
     return this->send(&message, sizeof(message));
   }
+  /** @brief Send a message; returns 0 on success or errno on failure */
   template <class M> typename std::enable_if<messageType<M>(), int>::type send(M const& m) const
   {
     return this->send(&m, sizeof(M));

diff --git a/src/mc/remote/Client.cpp b/src/mc/remote/Client.cpp
index 2b25c63..7cc5127 100644 (file)
--- a/src/mc/remote/Client.cpp
+++ b/src/mc/remote/Client.cpp
@@ -39,7 +39,7 @@ Client* Client::initialize()
 {
   // We are not in MC mode:
   // TODO, handle this more gracefully.
-  if (!getenv(MC_ENV_SOCKET_FD))
+  if (!std::getenv(MC_ENV_SOCKET_FD))
     return nullptr;
 
   // Do not break if we are called multiple times:
@@ -89,13 +89,13 @@ void Client::handleMessages()
     XBT_DEBUG("Waiting messages from model-checker");
 
     char message_buffer[MC_MESSAGE_LENGTH];
-    ssize_t s;
+    ssize_t received_size;
 
-    if ((s = channel_.receive(&message_buffer, sizeof(message_buffer))) < 0)
+    if ((received_size = channel_.receive(&message_buffer, sizeof(message_buffer))) < 0)
       xbt_die("Could not receive commands from the model-checker");
 
     s_mc_message_t message;
-    if ((size_t)s < sizeof(message))
+    if ((size_t)received_size < sizeof(message))
       xbt_die("Received message is too small");
     memcpy(&message, message_buffer, sizeof(message));
     switch (message.type) {
@@ -103,22 +103,20 @@ void Client::handleMessages()
       case MC_MESSAGE_DEADLOCK_CHECK: {
         // Check deadlock:
         bool deadlock = false;
-        smx_actor_t actor;
-        if (xbt_swag_size(simix_global->process_list)) {
+        if (!simix_global->process_list.empty()) {
           deadlock = true;
-          xbt_swag_foreach(actor, simix_global->process_list) if (simgrid::mc::actor_is_enabled(actor))
-          {
-            deadlock = false;
-            break;
-          }
+          for (auto kv : simix_global->process_list)
+            if (simgrid::mc::actor_is_enabled(kv.second)) {
+              deadlock = false;
+              break;
+            }
         }
 
         // Send result:
         s_mc_int_message_t answer;
         answer.type  = MC_MESSAGE_DEADLOCK_CHECK_REPLY;
         answer.value = deadlock;
-        if (channel_.send(answer))
-          xbt_die("Could not send response");
+        xbt_assert(channel_.send(answer) == 0, "Could not send response");
       } break;
 
       case MC_MESSAGE_CONTINUE:
@@ -126,7 +124,7 @@ void Client::handleMessages()
 
       case MC_MESSAGE_SIMCALL_HANDLE: {
         s_mc_simcall_handle_message_t message;
-        if (s != sizeof(message))
+        if (received_size != sizeof(message))
           xbt_die("Unexpected size for SIMCALL_HANDLE");
         memcpy(&message, message_buffer, sizeof(message));
         smx_actor_t process = SIMIX_process_from_PID(message.pid);
@@ -139,7 +137,7 @@ void Client::handleMessages()
 
       case MC_MESSAGE_RESTORE: {
         s_mc_restore_message_t message;
-        if (s != sizeof(message))
+        if (received_size != sizeof(message))
           xbt_die("Unexpected size for SIMCALL_HANDLE");
         memcpy(&message, message_buffer, sizeof(message));
 #if HAVE_SMPI
@@ -157,8 +155,7 @@ void Client::mainLoop(void)
 {
   while (1) {
     simgrid::mc::wait_for_requests();
-    if (channel_.send(MC_MESSAGE_WAITING))
-      xbt_die("Could not send WAITING mesage to model-checker");
+    xbt_assert(channel_.send(MC_MESSAGE_WAITING) == 0, "Could not send WAITING message to model-checker");
     this->handleMessages();
   }
 }
@@ -208,7 +205,7 @@ void Client::unignoreHeap(void* address, std::size_t size)
   message.addr = (std::uintptr_t)address;
   message.size = size;
   if (channel_.send(message))
-    xbt_die("Could not send IGNORE_HEAP mesasge to model-checker");
+    xbt_die("Could not send IGNORE_HEAP message to model-checker");
 }
 
 void Client::declareSymbol(const char* name, int* value)

diff --git a/src/simix/ActorImpl.cpp b/src/simix/ActorImpl.cpp
index a0e300d..5fefcd3 100644 (file)
--- a/src/simix/ActorImpl.cpp
+++ b/src/simix/ActorImpl.cpp
@@ -133,7 +133,7 @@ void SIMIX_process_cleanup(smx_actor_t process)
   }
 
   XBT_DEBUG("%p should not be run anymore",process);
-  xbt_swag_remove(process, simix_global->process_list);
+  simix_global->process_list.erase(process->pid);
   if (process->host)
     xbt_swag_remove(process, sg_host_simix(process->host)->process_list);
   xbt_swag_insert(process, simix_global->process_to_destroy);
@@ -279,7 +279,7 @@ smx_actor_t SIMIX_process_create(
     XBT_DEBUG("Start context '%s'", process->name.c_str());
 
     /* Now insert it in the global process list and in the process to run list */
-    xbt_swag_insert(process, simix_global->process_list);
+    simix_global->process_list[process->pid] = process;
     XBT_DEBUG("Inserting %s(%s) in the to_run list", process->cname(), host->cname());
     xbt_dynar_push_as(simix_global->process_to_run, smx_actor_t, process);
 
@@ -357,7 +357,7 @@ smx_actor_t SIMIX_process_attach(
   xbt_swag_insert(process, sg_host_simix(host)->process_list);
 
   /* Now insert it in the global process list and in the process to run list */
-  xbt_swag_insert(process, simix_global->process_list);
+  simix_global->process_list[process->pid] = process;
   XBT_DEBUG("Inserting %s(%s) in the to_run list", process->cname(), host->cname());
   xbt_dynar_push_as(simix_global->process_to_run, smx_actor_t, process);
 
@@ -540,13 +540,9 @@ void simcall_HANDLER_process_killall(smx_simcall_t simcall, int reset_pid) {
  */
 void SIMIX_process_killall(smx_actor_t issuer, int reset_pid)
 {
-  smx_actor_t p = nullptr;
-
-  while ((p = (smx_actor_t) xbt_swag_extract(simix_global->process_list))) {
-    if (p != issuer) {
-      SIMIX_process_kill(p,issuer);
-    }
-  }
+  for (auto kv : simix_global->process_list)
+    if (kv.second != issuer)
+      SIMIX_process_kill(kv.second, issuer);
 
   if (reset_pid > 0)
     simix_process_maxpid = reset_pid;
@@ -630,7 +626,7 @@ int SIMIX_process_get_maxpid() {
 
 int SIMIX_process_count()
 {
-  return xbt_swag_size(simix_global->process_list);
+  return simix_global->process_list.size();
 }
 
 int SIMIX_process_get_PID(smx_actor_t self)
@@ -681,11 +677,9 @@ const char* SIMIX_process_self_get_name() {
 
 smx_actor_t SIMIX_process_get_by_name(const char* name)
 {
-  smx_actor_t proc;
-  xbt_swag_foreach(proc, simix_global->process_list) {
-    if (proc->name == name)
-      return proc;
-  }
+  for (auto kv : simix_global->process_list)
+    if (kv.second->name == name)
+      return kv.second;
   return nullptr;
 }
 
@@ -874,19 +868,16 @@ xbt_dynar_t SIMIX_process_get_runnable()
  */
 smx_actor_t SIMIX_process_from_PID(int PID)
 {
-  smx_actor_t proc;
-  xbt_swag_foreach(proc, simix_global->process_list) {
-   if (proc->pid == static_cast<unsigned long> (PID))
-    return proc;
-  }
-  return nullptr;
+  if (simix_global->process_list.find(PID) == simix_global->process_list.end())
+    return nullptr;
+  return simix_global->process_list.at(PID);
 }
 
 /** @brief returns a dynar containing all currently existing processes */
 xbt_dynar_t SIMIX_processes_as_dynar() {
-  smx_actor_t proc;
   xbt_dynar_t res = xbt_dynar_new(sizeof(smx_actor_t),nullptr);
-  xbt_swag_foreach(proc, simix_global->process_list) {
+  for (auto kv : simix_global->process_list) {
+    smx_actor_t proc = kv.second;
     xbt_dynar_push(res,&proc);
   }
   return res;

diff --git a/src/simix/smx_global.cpp b/src/simix/smx_global.cpp
index 8503d4c..78512e6 100644 (file)
--- a/src/simix/smx_global.cpp
+++ b/src/simix/smx_global.cpp
@@ -206,7 +206,6 @@ void SIMIX_global_init(int *argc, char **argv)
     simgrid::simix::ActorImpl proc;
     simix_global->process_to_run = xbt_dynar_new(sizeof(smx_actor_t), nullptr);
     simix_global->process_that_ran = xbt_dynar_new(sizeof(smx_actor_t), nullptr);
-    simix_global->process_list = xbt_swag_new(xbt_swag_offset(proc, process_hookup));
     simix_global->process_to_destroy = xbt_swag_new(xbt_swag_offset(proc, destroy_hookup));
     simix_global->maestro_process = nullptr;
     simix_global->create_process_function = &SIMIX_process_create;
@@ -300,8 +299,7 @@ void SIMIX_clean()
   xbt_dynar_free(&simix_global->process_to_run);
   xbt_dynar_free(&simix_global->process_that_ran);
   xbt_swag_free(simix_global->process_to_destroy);
-  xbt_swag_free(simix_global->process_list);
-  simix_global->process_list = nullptr;
+  simix_global->process_list.clear();
   simix_global->process_to_destroy = nullptr;
 
   xbt_os_mutex_destroy(simix_global->mutex);
@@ -516,7 +514,7 @@ void SIMIX_run()
     }
 
     time = SIMIX_timer_next();
-    if (time > -1.0 || xbt_swag_size(simix_global->process_list) != 0) {
+    if (time > -1.0 || simix_global->process_list.empty() == false) {
       XBT_DEBUG("Calling surf_solve");
       time = surf_solve(time);
       XBT_DEBUG("Moving time ahead : %g", time);
@@ -548,12 +546,12 @@ void SIMIX_run()
     XBT_DEBUG("### time %f, empty %d", time, xbt_dynar_is_empty(simix_global->process_to_run));
 
     if (xbt_dynar_is_empty(simix_global->process_to_run) &&
-        xbt_swag_size(simix_global->process_list) != 0)
+        !simix_global->process_list.empty())
     simgrid::simix::onDeadlock();
 
   } while (time > -1.0 || !xbt_dynar_is_empty(simix_global->process_to_run));
 
-  if (xbt_swag_size(simix_global->process_list) != 0) {
+  if (simix_global->process_list.size() != 0) {
 
     TRACE_end();
 
@@ -637,17 +635,13 @@ void SIMIX_function_register_process_cleanup(void_pfn_smxprocess_t function)
 
 void SIMIX_display_process_status()
 {
-  if (simix_global->process_list == nullptr) {
-    return;
-  }
-
-  smx_actor_t process = nullptr;
-  int nbprocess = xbt_swag_size(simix_global->process_list);
+  int nbprocess = simix_global->process_list.size();
 
   XBT_INFO("%d processes are still running, waiting for something.", nbprocess);
   /*  List the process and their state */
   XBT_INFO("Legend of the following listing: \"Process <pid> (<name>@<host>): <status>\"");
-  xbt_swag_foreach(process, simix_global->process_list) {
+  for (auto kv : simix_global->process_list) {
+    smx_actor_t process = kv.second;
 
     if (process->waiting_synchro) {
 

diff --git a/src/simix/smx_private.h b/src/simix/smx_private.h
index 85d2dfa..d8fff8b 100644 (file)
--- a/src/simix/smx_private.h
+++ b/src/simix/smx_private.h
@@ -10,6 +10,8 @@
 #include <signal.h>
 #include "src/kernel/context/Context.hpp"
 
+#include <map>
+
 /********************************** Simix Global ******************************/
 
 namespace simgrid {
@@ -20,7 +22,17 @@ public:
   smx_context_factory_t context_factory = nullptr;
   xbt_dynar_t process_to_run = nullptr;
   xbt_dynar_t process_that_ran = nullptr;
-  xbt_swag_t process_list = nullptr;
+  std::map<int, smx_actor_t> process_list = std::map<int, smx_actor_t>();
+#if HAVE_MC
+  /* MCer cannot read the std::map above in the remote process, so we copy the info it needs in a dynar.
+   * FIXME: This is supposed to be a temporary hack.
+   * A better solution would be to change the split between MCer and MCed, where the responsibility
+   *   to compute the list of the enabled transitions goes to the MCed.
+   * That way, the MCer would not need to have the list of actors on its side.
+   * These info could be published by the MCed to the MCer in a way inspired of vd.so
+   */
+  xbt_dynar_t actors_vector = xbt_dynar_new(sizeof(smx_actor_t), nullptr);
+#endif
   xbt_swag_t process_to_destroy = nullptr;
   smx_actor_t maestro_process = nullptr;