From dbef7c255d1e1bebb88d6986817557648289aec8 Mon Sep 17 00:00:00 2001 From: Martin Quinson Date: Thu, 19 Jan 2017 02:43:58 +0100 Subject: [PATCH] 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. --- src/mc/Process.cpp | 2 +- src/mc/mc_base.cpp | 6 ++++++ src/mc/mc_global.cpp | 7 ++++--- src/mc/mc_smx.cpp | 25 ++++++++++++++++++++++++- src/mc/remote/Channel.cpp | 1 + src/mc/remote/Channel.hpp | 1 + src/mc/remote/Client.cpp | 33 +++++++++++++++------------------ src/simix/ActorImpl.cpp | 39 +++++++++++++++------------------------ src/simix/smx_global.cpp | 20 +++++++------------- src/simix/smx_private.h | 14 +++++++++++++- 10 files changed, 87 insertions(+), 61 deletions(-) diff --git a/src/mc/Process.cpp b/src/mc/Process.cpp index 30a45ff7c9..fd35b52b85 100644 --- 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 ee4462fd94..96fee34cff 100644 --- 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 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 8418b96de3..e62b9c659f 100644 --- 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 86303fc333..ff841f7d45 100644 --- 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& target, + simgrid::mc::RemotePtr 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 simix_global = this->read(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 30ccdcd74e..6c4000dae3 100644 --- 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 ac4fd0c11e..f3e2ce9bbc 100644 --- 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 typename std::enable_if(), 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 2b25c63917..7cc5127094 100644 --- 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 a0e300ded6..5fefcd31d1 100644 --- 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 (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 8503d4cb1f..78512e6795 100644 --- 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 (@): \""); - 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 85d2dfa15a..d8fff8b9ae 100644 --- a/src/simix/smx_private.h +++ b/src/simix/smx_private.h @@ -10,6 +10,8 @@ #include #include "src/kernel/context/Context.hpp" +#include + /********************************** 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 process_list = std::map(); +#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; -- 2.20.1