This is an automated email from the ASF dual-hosted git repository. bbannier pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/mesos.git
commit 06bcb3581d0f20993bb2d0a37989bfb21eb97be7 Author: Benjamin Bannier <[email protected]> AuthorDate: Thu Sep 19 15:23:56 2019 +0200 Fixed inefficient `hashmap` access patterns. This patch fixes some inefficient access patterns around `hashmap::get`. Since this function returns an `Option` it can be used as a shorthand for a `contains` check and subsequent creation of a value (`Option` always contains a value). It was never not intended and is inefficient as `contains` itself (e.g., via `hashmap::get::isSome`), and for cases where only access to parts of the value in the `hashmap` is required (e.g., to access a member of an optional value). In both these cases we neither want nor need to create a temporary, and should instead either just use `contains`, or access the value with `hashmap::at` after a `contains` check; otherwise we might spend a lot of time creating unnecessary temporary values. This patch fixes some easily identifiable cases found from skimming the result of the following clang-query command: match cxxMemberCallExpr( on(hasType(cxxRecordDecl(hasName("hashmap")))), unless( hasParent(cxxBindTemporaryExpr( hasParent(materializeTemporaryExpr( hasParent(exprWithCleanups( hasParent(varDecl())))))))), callee(cxxMethodDecl(hasName("get")))) This most probably misses a lot of cases. Given how easy it is to misuse `hashmap::get` we should consider whether it makes sense to get rid of it completely in lieu of an inlined form trading some additional lookups for temporary avoidance, Option<X> x = map.contains(k) ? Some(map.at(k)) : Option<X>::none(); Review: https://reviews.apache.org/r/71519 --- src/exec/exec.cpp | 2 +- src/executor/executor.cpp | 2 +- src/files/files.cpp | 8 ++++---- src/master/master.cpp | 8 ++++---- src/master/metrics.cpp | 14 +++++++------- src/slave/containerizer/fetcher.cpp | 4 ++-- src/slave/containerizer/mesos/isolators/posix.hpp | 4 ++-- src/slave/containerizer/mesos/launcher.cpp | 2 +- src/slave/containerizer/mesos/provisioner/provisioner.cpp | 6 +++--- src/slave/slave.cpp | 9 +++++---- src/state/log.cpp | 2 +- src/tests/slave_recovery_tests.cpp | 4 ++-- src/tests/task_status_update_manager_tests.cpp | 2 +- 13 files changed, 34 insertions(+), 33 deletions(-) diff --git a/src/exec/exec.cpp b/src/exec/exec.cpp index 67e082e..69e5e24 100644 --- a/src/exec/exec.cpp +++ b/src/exec/exec.cpp @@ -755,7 +755,7 @@ Status MesosExecutorDriver::start() hashmap<string, string> env(environment); // Check if this is local (for example, for testing). - local = env.get("MESOS_LOCAL").isSome(); + local = env.contains("MESOS_LOCAL"); // Get slave PID from environment. value = env.get("MESOS_SLAVE_PID"); diff --git a/src/executor/executor.cpp b/src/executor/executor.cpp index 664a2f1..b412603 100644 --- a/src/executor/executor.cpp +++ b/src/executor/executor.cpp @@ -210,7 +210,7 @@ public: hashmap<string, string> env(mesosEnvironment); // Check if this is local (for example, for testing). - local = env.get("MESOS_LOCAL").isSome(); + local = env.contains("MESOS_LOCAL"); Option<string> value; diff --git a/src/files/files.cpp b/src/files/files.cpp index f200d5e..091061a 100644 --- a/src/files/files.cpp +++ b/src/files/files.cpp @@ -511,8 +511,8 @@ Future<http::Response> FilesProcess::__read( off_t offset = -1; - if (request.url.query.get("offset").isSome()) { - Try<off_t> result = numify<off_t>(request.url.query.get("offset").get()); + if (request.url.query.contains("offset")) { + Try<off_t> result = numify<off_t>(request.url.query.at("offset")); if (result.isError()) { return BadRequest("Failed to parse offset: " + result.error() + ".\n"); @@ -528,9 +528,9 @@ Future<http::Response> FilesProcess::__read( Option<size_t> length; - if (request.url.query.get("length").isSome()) { + if (request.url.query.contains("length")) { Try<ssize_t> result = numify<ssize_t>( - request.url.query.get("length").get()); + request.url.query.at("length")); if (result.isError()) { return BadRequest("Failed to parse length: " + result.error() + ".\n"); diff --git a/src/master/master.cpp b/src/master/master.cpp index a2c289a..e5bc493 100644 --- a/src/master/master.cpp +++ b/src/master/master.cpp @@ -1463,7 +1463,7 @@ void Master::consume(MessageEvent&& event) // If the framework has a principal, the counter must exist. CHECK(metrics->frameworks.contains(principal.get())); Counter messages_received = - metrics->frameworks.get(principal.get()).get()->messages_received; + metrics->frameworks.at(principal.get())->messages_received; ++messages_received; } @@ -1611,7 +1611,7 @@ void Master::_consume(MessageEvent&& event) // this principal. if (principal.isSome() && metrics->frameworks.contains(principal.get())) { Counter messages_processed = - metrics->frameworks.get(principal.get()).get()->messages_processed; + metrics->frameworks.at(principal.get())->messages_processed; ++messages_processed; } } @@ -12506,7 +12506,7 @@ void Master::_apply( const UUID resourceVersion = resourceProviderId.isNone() ? slave->resourceVersion.get() - : slave->resourceProviders.get(resourceProviderId.get())->resourceVersion; + : slave->resourceProviders.at(resourceProviderId.get()).resourceVersion; Operation* operation = new Operation(protobuf::createOperation( operationInfo, @@ -13741,7 +13741,7 @@ bool Slave::hasExecutor(const FrameworkID& frameworkId, const ExecutorID& executorId) const { return executors.contains(frameworkId) && - executors.get(frameworkId)->contains(executorId); + executors.at(frameworkId).contains(executorId); } diff --git a/src/master/metrics.cpp b/src/master/metrics.cpp index 20d7231..7973485 100644 --- a/src/master/metrics.cpp +++ b/src/master/metrics.cpp @@ -864,8 +864,8 @@ void FrameworkMetrics::incrementCall(const scheduler::Call::Type& callType) { CHECK(call_types.contains(callType)); - call_types.get(callType).get()++; - calls++; + ++call_types.at(callType); + ++calls; } @@ -873,10 +873,10 @@ void FrameworkMetrics::incrementTaskState(const TaskState& state) { if (protobuf::isTerminalState(state)) { CHECK(terminal_task_states.contains(state)); - terminal_task_states.get(state).get()++; + ++terminal_task_states.at(state); } else { CHECK(active_task_states.contains(state)); - active_task_states.get(state).get() += 1; + ++active_task_states.at(state); } } @@ -885,7 +885,7 @@ void FrameworkMetrics::decrementActiveTaskState(const TaskState& state) { CHECK(active_task_states.contains(state)); - active_task_states.get(state).get() -= 1; + active_task_states.at(state) -= 1; } @@ -893,8 +893,8 @@ void FrameworkMetrics::incrementOperation(const Offer::Operation& operation) { CHECK(operation_types.contains(operation.type())); - operation_types.get(operation.type()).get()++; - operations++; + ++operation_types.at(operation.type()); + ++operations; } diff --git a/src/slave/containerizer/fetcher.cpp b/src/slave/containerizer/fetcher.cpp index 8ae789a..3a5a74b 100644 --- a/src/slave/containerizer/fetcher.cpp +++ b/src/slave/containerizer/fetcher.cpp @@ -945,7 +945,7 @@ void FetcherProcess::kill(const ContainerID& containerId) if (subprocessPids.contains(containerId)) { VLOG(1) << "Killing the fetcher for container '" << containerId << "'"; // Best effort kill the entire fetcher tree. - os::killtree(subprocessPids.get(containerId).get(), SIGKILL); + os::killtree(subprocessPids.at(containerId), SIGKILL); subprocessPids.erase(containerId); } @@ -1057,7 +1057,7 @@ bool FetcherProcess::Cache::contains( const string& uri) const { const string key = cacheKey(user, uri); - return table.get(key).isSome(); + return table.contains(key); } diff --git a/src/slave/containerizer/mesos/isolators/posix.hpp b/src/slave/containerizer/mesos/isolators/posix.hpp index 1ff942c..f8de3d2 100644 --- a/src/slave/containerizer/mesos/isolators/posix.hpp +++ b/src/slave/containerizer/mesos/isolators/posix.hpp @@ -161,7 +161,7 @@ public: // Use 'mesos-usage' but only request 'cpus_' values. Try<ResourceStatistics> usage = - mesos::internal::usage(pids.get(containerId).get(), false, true); + mesos::internal::usage(pids.at(containerId), false, true); if (usage.isError()) { return process::Failure(usage.error()); } @@ -195,7 +195,7 @@ public: // Use 'mesos-usage' but only request 'mem_' values. Try<ResourceStatistics> usage = - mesos::internal::usage(pids.get(containerId).get(), true, false); + mesos::internal::usage(pids.at(containerId), true, false); if (usage.isError()) { return process::Failure(usage.error()); } diff --git a/src/slave/containerizer/mesos/launcher.cpp b/src/slave/containerizer/mesos/launcher.cpp index 413cc62..a5b4990 100644 --- a/src/slave/containerizer/mesos/launcher.cpp +++ b/src/slave/containerizer/mesos/launcher.cpp @@ -165,7 +165,7 @@ Future<Nothing> SubprocessLauncher::destroy(const ContainerID& containerId) return Nothing(); } - pid_t pid = pids.get(containerId).get(); + pid_t pid = pids.at(containerId); // Kill all processes in the session and process group. os::killtree(pid, SIGKILL, true, true); diff --git a/src/slave/containerizer/mesos/provisioner/provisioner.cpp b/src/slave/containerizer/mesos/provisioner/provisioner.cpp index bf3908d..3d0b291 100644 --- a/src/slave/containerizer/mesos/provisioner/provisioner.cpp +++ b/src/slave/containerizer/mesos/provisioner/provisioner.cpp @@ -517,7 +517,7 @@ Future<ProvisionInfo> ProvisionerProcess::provision( } // Get and then provision image layers from the store. - return stores.get(image.type()).get()->get(image, defaultBackend) + return stores.at(image.type())->get(image, defaultBackend) .then(defer( self(), &Self::_provision, @@ -570,7 +570,7 @@ Future<ProvisionInfo> ProvisionerProcess::_provision( containerId, backend); - return backends.get(backend).get()->provision( + return backends.at(backend)->provision( imageInfo.layers, rootfs, backendDir) @@ -704,7 +704,7 @@ Future<bool> ProvisionerProcess::_destroy( << "' for container " << containerId; futures.push_back( - backends.get(backend).get()->destroy(rootfs, backendDir)); + backends.at(backend)->destroy(rootfs, backendDir)); } } diff --git a/src/slave/slave.cpp b/src/slave/slave.cpp index 96890d3..3839a12 100644 --- a/src/slave/slave.cpp +++ b/src/slave/slave.cpp @@ -7918,8 +7918,9 @@ Future<Nothing> Slave::__recoverOperations( if (operation->latest_status().state() == OPERATION_PENDING) { // The agent failed over before the checkpoint of the // `OPERATION_FINISHED` update completed. - CHECK(state->streams.get(operationUuid).isNone() || - state->streams.get(operationUuid)->isNone()); + CHECK( + !state->streams.contains(operationUuid) || + state->streams.at(operationUuid).isNone()); Option<OperationID> operationId = operation->info().has_id() @@ -9491,13 +9492,13 @@ Future<bool> Slave::authorizeSandboxAccess( ObjectApprover::Object object; if (frameworks.contains(frameworkId)) { - Framework* framework = frameworks.get(frameworkId).get(); + Framework* framework = frameworks.at(frameworkId); object.framework_info = &(framework->info); if (framework->executors.contains(executorId)) { object.executor_info = - &(framework->executors.get(executorId).get()->info); + &(framework->executors.at(executorId)->info); } } diff --git a/src/state/log.cpp b/src/state/log.cpp index d3bf2cc..8ca2869 100644 --- a/src/state/log.cpp +++ b/src/state/log.cpp @@ -566,7 +566,7 @@ Future<bool> LogStorageProcess::___set( // use the returned position (i.e., do nothing). if (diffs > 0) { CHECK(snapshots.contains(entry.name())); - position = snapshots.get(entry.name())->position; + position = snapshots.at(entry.name()).position; } Snapshot snapshot(position.get(), entry, diffs); diff --git a/src/tests/slave_recovery_tests.cpp b/src/tests/slave_recovery_tests.cpp index d99752a..b23162d 100644 --- a/src/tests/slave_recovery_tests.cpp +++ b/src/tests/slave_recovery_tests.cpp @@ -1497,7 +1497,7 @@ TYPED_TEST(SlaveRecoveryTest, RecoverTerminatedHTTPExecutor) ASSERT_TRUE(state->slave->frameworks.contains(frameworkId.get())); slave::state::FrameworkState frameworkState = - state->slave->frameworks.get(frameworkId.get()).get(); + state->slave->frameworks.at(frameworkId.get()); ASSERT_EQ(1u, frameworkState.executors.size()); @@ -4012,7 +4012,7 @@ TYPED_TEST(SlaveRecoveryTest, ReconcileTasksMissingFromSlave) ASSERT_TRUE(state->slave->frameworks.contains(frameworkId.get())); slave::state::FrameworkState frameworkState = - state->slave->frameworks.get(frameworkId.get()).get(); + state->slave->frameworks.at(frameworkId.get()); ASSERT_EQ(1u, frameworkState.executors.size()); diff --git a/src/tests/task_status_update_manager_tests.cpp b/src/tests/task_status_update_manager_tests.cpp index 0c8b058..25cc8ef 100644 --- a/src/tests/task_status_update_manager_tests.cpp +++ b/src/tests/task_status_update_manager_tests.cpp @@ -158,7 +158,7 @@ TEST_F_TEMP_DISABLED_ON_WINDOWS( ASSERT_TRUE(state->slave->frameworks.contains(frameworkId.get())); slave::state::FrameworkState frameworkState = - state->slave->frameworks.get(frameworkId.get()).get(); + state->slave->frameworks.at(frameworkId.get()); ASSERT_EQ(1u, frameworkState.executors.size());
