You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mesos.apache.org by bb...@apache.org on 2019/09/23 12:41:15 UTC
[mesos] 01/02: Fixed inefficient `hashmap` access patterns.
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 <bb...@apache.org>
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());