Repository: mesos Updated Branches: refs/heads/master 7b785c75b -> abf11a951
Optimized the generation of metrics snapshots. Profiling of metrics generation revealed a large amount of time spent in map operations. This patch does three things to mitigate this: * Stores the metrics as an ordered map so that we only pay the price of sorting when the metric is first added. * Makes use of vectors instead of maps for intermediate objects, which eliminates the need for another intermediate object. * Hints when inserting into the returned map, reducing the cost of insertion into that ordered container. Review: https://reviews.apache.org/r/67871/ Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/abf11a95 Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/abf11a95 Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/abf11a95 Branch: refs/heads/master Commit: abf11a951d177182d0c3a4d40ae7825101778739 Parents: 7b785c7 Author: Greg Mann <g...@mesosphere.io> Authored: Mon Jul 16 09:11:46 2018 -0700 Committer: Greg Mann <gregorywm...@gmail.com> Committed: Tue Jul 17 13:27:00 2018 -0700 ---------------------------------------------------------------------- .../include/process/metrics/metrics.hpp | 8 +- 3rdparty/libprocess/src/metrics/metrics.cpp | 82 ++++++++++++-------- 2 files changed, 55 insertions(+), 35 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/abf11a95/3rdparty/libprocess/include/process/metrics/metrics.hpp ---------------------------------------------------------------------- diff --git a/3rdparty/libprocess/include/process/metrics/metrics.hpp b/3rdparty/libprocess/include/process/metrics/metrics.hpp index f9b7202..3dc6bbf 100644 --- a/3rdparty/libprocess/include/process/metrics/metrics.hpp +++ b/3rdparty/libprocess/include/process/metrics/metrics.hpp @@ -25,7 +25,6 @@ #include <process/metrics/metric.hpp> -#include <stout/hashmap.hpp> #include <stout/nothing.hpp> #include <stout/option.hpp> @@ -71,11 +70,12 @@ private: // capture with C++14. Future<std::map<std::string, double>> __snapshot( const Option<Duration>& timeout, - hashmap<std::string, Future<double>>&& metrics, - hashmap<std::string, Option<Statistics<double>>>&& statistics); + std::vector<std::string>&& keys, + std::vector<Future<double>>&& metrics, + std::vector<Option<Statistics<double>>>&& statistics); // The Owned<Metric> is an explicit copy of the Metric passed to 'add'. - hashmap<std::string, Owned<Metric>> metrics; + std::map<std::string, Owned<Metric>> metrics; // Used to rate limit the snapshot endpoint. Option<Owned<RateLimiter>> limiter; http://git-wip-us.apache.org/repos/asf/mesos/blob/abf11a95/3rdparty/libprocess/src/metrics/metrics.cpp ---------------------------------------------------------------------- diff --git a/3rdparty/libprocess/src/metrics/metrics.cpp b/3rdparty/libprocess/src/metrics/metrics.cpp index 4883c9a..e2fdbc7 100644 --- a/3rdparty/libprocess/src/metrics/metrics.cpp +++ b/3rdparty/libprocess/src/metrics/metrics.cpp @@ -28,7 +28,6 @@ #include <stout/duration.hpp> #include <stout/error.hpp> #include <stout/foreach.hpp> -#include <stout/hashmap.hpp> #include <stout/numify.hpp> #include <stout/option.hpp> #include <stout/os.hpp> @@ -121,7 +120,7 @@ string MetricsProcess::help() Future<Nothing> MetricsProcess::add(Owned<Metric> metric) { - if (metrics.contains(metric->name())) { + if (metrics.count(metric->name()) > 0) { return Failure("Metric '" + metric->name() + "' was already added"); } @@ -132,7 +131,7 @@ Future<Nothing> MetricsProcess::add(Owned<Metric> metric) Future<Nothing> MetricsProcess::remove(const string& name) { - if (!metrics.contains(name)) { + if (metrics.count(name) == 0) { return Failure("Metric '" + name + "' not found"); } @@ -145,29 +144,44 @@ Future<Nothing> MetricsProcess::remove(const string& name) Future<map<string, double>> MetricsProcess::snapshot( const Option<Duration>& timeout) { - hashmap<string, Future<double>> futures; - hashmap<string, Option<Statistics<double>>> statistics; - - foreachkey (const string& name, metrics) { - const Owned<Metric>& metric = metrics.at(name); - futures[name] = metric->value(); - // TODO(dhamon): It would be nice to compute these asynchronously. - statistics[name] = metric->statistics(); + // To avoid creating a new vector when calling `await()` below, we use three + // ordered vectors, where the Nth key in `keys` is associated with the Nth + // items in each of `futures` and `statistics`. + vector<string> keys; + vector<Future<double>> futures; + vector<Option<Statistics<double>>> statistics; + + keys.reserve(metrics.size()); + futures.reserve(metrics.size()); + statistics.reserve(metrics.size()); + + for (auto iter = metrics.begin(); iter != metrics.end(); ++iter) { + keys.emplace_back(iter->first); + futures.emplace_back(iter->second->value()); + statistics.emplace_back(iter->second->statistics()); } Future<Nothing> timedout = after(timeout.getOrElse(Duration::max())); - vector<Future<double>> values = futures.values(); - // Return the response once it finishes or we time out. - return select<Nothing>({ + // + // NOTE: We assign the result of `select()` to a local variable to ensure that + // the `await()` call in this expression is evaluated before the call to + // `std::move(futures)` in the subsequent expression. Otherwise, it's possible + // that the `move()` could be evaluated first, causing an empty vector to be + // passed into `await()`. + Future<Future<Nothing>> waited = + select<Nothing>({ timedout, - await(std::move(values)).then([]{ return Nothing(); }) }) + await(futures).then([]{ return Nothing(); }) }); + + return waited .onAny([=]() mutable { timedout.discard(); }) // Don't accumulate timers. .then(defer(self(), &Self::__snapshot, timeout, + std::move(keys), std::move(futures), std::move(statistics))); } @@ -209,34 +223,40 @@ Future<http::Response> MetricsProcess::_snapshot( Future<map<string, double>> MetricsProcess::__snapshot( const Option<Duration>& timeout, - hashmap<string, Future<double>>&& metrics, - hashmap<string, Option<Statistics<double>>>&& statistics) + vector<string>&& keys, + vector<Future<double>>&& metrics, + vector<Option<Statistics<double>>>&& statistics) { map<string, double> snapshot; - foreachpair (const string& key, const Future<double>& value, metrics) { + for (size_t i = 0; i < metrics.size(); ++i) { // TODO(dhamon): Maybe add the failure message for this metric to the // response if value.isFailed(). + const string& key = keys[i]; + const Future<double>& value = metrics[i]; + if (value.isPending()) { CHECK_SOME(timeout); VLOG(1) << "Exceeded timeout of " << timeout.get() << " when attempting to get metric '" << key << "'"; } else if (value.isReady()) { - snapshot[key] = value.get(); + snapshot.emplace_hint(snapshot.end(), key, value.get()); } - Option<Statistics<double>> statistics_ = statistics.get(key).get(); - - if (statistics_.isSome()) { - snapshot[key + "/count"] = static_cast<double>(statistics_->count); - snapshot[key + "/min"] = statistics_->min; - snapshot[key + "/max"] = statistics_->max; - snapshot[key + "/p50"] = statistics_->p50; - snapshot[key + "/p90"] = statistics_->p90; - snapshot[key + "/p95"] = statistics_->p95; - snapshot[key + "/p99"] = statistics_->p99; - snapshot[key + "/p999"] = statistics_->p999; - snapshot[key + "/p9999"] = statistics_->p9999; + if (statistics[i].isSome()) { + Statistics<double>& statistics_ = statistics[i].get(); + snapshot.emplace_hint( + snapshot.end(), + key + "/count", + static_cast<double>(statistics_.count)); + snapshot.emplace_hint(snapshot.end(), key + "/max", statistics_.max); + snapshot.emplace_hint(snapshot.end(), key + "/min", statistics_.min); + snapshot.emplace_hint(snapshot.end(), key + "/p50", statistics_.p50); + snapshot.emplace_hint(snapshot.end(), key + "/p90", statistics_.p90); + snapshot.emplace_hint(snapshot.end(), key + "/p95", statistics_.p95); + snapshot.emplace_hint(snapshot.end(), key + "/p99", statistics_.p99); + snapshot.emplace_hint(snapshot.end(), key + "/p999", statistics_.p999); + snapshot.emplace_hint(snapshot.end(), key + "/p9999", statistics_.p9999); } }