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:14 UTC
[mesos] branch master updated (9f1d38f -> b7d4e7a)
This is an automated email from the ASF dual-hosted git repository.
bbannier pushed a change to branch master
in repository https://gitbox.apache.org/repos/asf/mesos.git.
from 9f1d38f Introduced new names for SSL-related libprocess flags.
new 06bcb35 Fixed inefficient `hashmap` access patterns.
new b7d4e7a Fixed inefficient `hashmap` access patterns.
The 2 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
Summary of changes:
3rdparty/libprocess/src/metrics/metrics.cpp | 2 +-
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 +-
14 files changed, 35 insertions(+), 34 deletions(-)
[mesos] 02/02: Fixed inefficient `hashmap` access patterns.
Posted by bb...@apache.org.
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 b7d4e7aff3e34c724945f47b9b967e078f9f46a1
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/71520
---
3rdparty/libprocess/src/metrics/metrics.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/3rdparty/libprocess/src/metrics/metrics.cpp b/3rdparty/libprocess/src/metrics/metrics.cpp
index 623d44a..66e78b3 100644
--- a/3rdparty/libprocess/src/metrics/metrics.cpp
+++ b/3rdparty/libprocess/src/metrics/metrics.cpp
@@ -196,7 +196,7 @@ Future<http::Response> MetricsProcess::_snapshot(
Option<Duration> timeout;
if (request.url.query.contains("timeout")) {
- string parameter = request.url.query.get("timeout").get();
+ string parameter = request.url.query.at("timeout");
Try<Duration> duration = Duration::parse(parameter);
[mesos] 01/02: Fixed inefficient `hashmap` access patterns.
Posted by bb...@apache.org.
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());