From b6393c619656e4aa71ebb6735ebdc3d39017466e Mon Sep 17 00:00:00 2001 From: Phil Genera Date: Wed, 3 Jun 2020 19:40:10 +0000 Subject: [PATCH 01/21] New eds_speed_tests and temporary complexity annotations in upstream_impl. Signed-off-by: Phil Genera --- source/common/upstream/upstream_impl.cc | 12 ++++++ test/common/upstream/eds_speed_test.cc | 57 ++++++++++++++++++++----- 2 files changed, 59 insertions(+), 10 deletions(-) diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index a78a331cb9cc0..f936a5743a092 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -1337,12 +1337,15 @@ bool BaseDynamicClusterImpl::updateDynamicHostList(const HostVector& new_hosts, std::unordered_set existing_hosts_for_current_priority( current_priority_hosts.size()); HostVector final_hosts; + // this entire function: O(n), due to this loop. for (const HostSharedPtr& host : new_hosts) { + // O(1) if (updated_hosts.count(host->address()->asString())) { continue; } // To match a new host with an existing host means comparing their addresses. + // O(1) auto existing_host = all_hosts.find(host->address()->asString()); const bool existing_host_found = existing_host != all_hosts.end(); @@ -1378,6 +1381,7 @@ bool BaseDynamicClusterImpl::updateDynamicHostList(const HostVector& new_hosts, // Did metadata change? bool metadata_changed = true; + // O(sizeof(metadata())) if (host->metadata() && existing_host->second->metadata()) { metadata_changed = !Protobuf::util::MessageDifferencer::Equivalent( *host->metadata(), *existing_host->second->metadata()); @@ -1406,7 +1410,9 @@ bool BaseDynamicClusterImpl::updateDynamicHostList(const HostVector& new_hosts, } existing_host->second->weight(host->weight()); + // O(1) final_hosts.push_back(existing_host->second); + // O(1) updated_hosts[existing_host->second->address()->asString()] = existing_host->second; } else { if (host->weight() > max_host_weight) { @@ -1424,6 +1430,7 @@ bool BaseDynamicClusterImpl::updateDynamicHostList(const HostVector& new_hosts, } } + // O(1) updated_hosts[host->address()->asString()] = host; final_hosts.push_back(host); hosts_added_to_current_priority.push_back(host); @@ -1432,10 +1439,13 @@ bool BaseDynamicClusterImpl::updateDynamicHostList(const HostVector& new_hosts, // Remove hosts from current_priority_hosts that were matched to an existing host in the previous // loop. + // this entire block: O(n^3) (worst case I am not going to type a phi or whatever) for (auto itr = current_priority_hosts.begin(); itr != current_priority_hosts.end();) { + // O(n)! auto existing_itr = existing_hosts_for_current_priority.find((*itr)->address()->asString()); if (existing_itr != existing_hosts_for_current_priority.end()) { + // O(n)! existing_hosts_for_current_priority.erase(existing_itr); itr = current_priority_hosts.erase(itr); } else { @@ -1459,6 +1469,7 @@ bool BaseDynamicClusterImpl::updateDynamicHostList(const HostVector& new_hosts, const bool dont_remove_healthy_hosts = health_checker_ != nullptr && !info()->drainConnectionsOnHostRemoval(); if (!current_priority_hosts.empty() && dont_remove_healthy_hosts) { + // this whole block: O(n^2) for (auto i = current_priority_hosts.begin(); i != current_priority_hosts.end();) { if (!((*i)->healthFlagGet(Host::HealthFlag::FAILED_ACTIVE_HC) || (*i)->healthFlagGet(Host::HealthFlag::FAILED_EDS_HEALTH))) { @@ -1469,6 +1480,7 @@ bool BaseDynamicClusterImpl::updateDynamicHostList(const HostVector& new_hosts, final_hosts.push_back(*i); updated_hosts[(*i)->address()->asString()] = *i; (*i)->healthFlagSet(Host::HealthFlag::PENDING_DYNAMIC_REMOVAL); + // O(n)! i = current_priority_hosts.erase(i); } else { i++; diff --git a/test/common/upstream/eds_speed_test.cc b/test/common/upstream/eds_speed_test.cc index e02f16a086f82..0645d10694177 100644 --- a/test/common/upstream/eds_speed_test.cc +++ b/test/common/upstream/eds_speed_test.cc @@ -52,11 +52,7 @@ class EdsSpeedTest { cluster_->initialize([this] { initialized_ = true; }); } - // Set up an EDS config with multiple priorities, localities, weights and make sure - // they are loaded and reloaded as expected. - void priorityAndLocalityWeightedHelper(bool ignore_unknown_dynamic_fields, int num_hosts) { - envoy::config::endpoint::v3::ClusterLoadAssignment cluster_load_assignment; - cluster_load_assignment.set_cluster_name("fare"); + void resetClusterHelper() { resetCluster(R"EOF( name: name connect_timeout: 0.25s @@ -70,6 +66,14 @@ class EdsSpeedTest { refresh_delay: 1s )EOF", Envoy::Upstream::Cluster::InitializePhase::Secondary); + } + + // Set up an EDS config with multiple priorities, localities, weights and make sure + // they are loaded as expected. + void priorityAndLocalityWeightedHelper(bool ignore_unknown_dynamic_fields, int num_hosts, + bool healthy) { + envoy::config::endpoint::v3::ClusterLoadAssignment cluster_load_assignment; + cluster_load_assignment.set_cluster_name("fare"); // Add a whole bunch of hosts in a single place: auto* endpoints = cluster_load_assignment.add_endpoints(); @@ -82,10 +86,14 @@ class EdsSpeedTest { uint32_t port = 1000; for (int i = 0; i < num_hosts; ++i) { - auto* socket_address = endpoints->add_lb_endpoints() - ->mutable_endpoint() - ->mutable_address() - ->mutable_socket_address(); + auto* lb_endpoint = endpoints->add_lb_endpoints(); + if (healthy) { + lb_endpoint->set_health_status(envoy::config::core::v3::HEALTHY); + } else { + lb_endpoint->set_health_status(envoy::config::core::v3::UNHEALTHY); + } + auto* endpoint = lb_endpoint->mutable_endpoint(); + auto* socket_address = endpoint->mutable_address()->mutable_socket_address(); socket_address->set_address("10.0.1." + std::to_string(i / 60000)); socket_address->set_port_value((port + i) % 60000); } @@ -127,8 +135,37 @@ static void priorityAndLocalityWeighted(benchmark::State& state) { Envoy::Logger::Logger::DEFAULT_LOG_FORMAT, lock, false); for (auto _ : state) { Envoy::Upstream::EdsSpeedTest speed_test; - speed_test.priorityAndLocalityWeightedHelper(state.range(0), state.range(1)); + speed_test.resetClusterHelper(); + speed_test.priorityAndLocalityWeightedHelper(state.range(0), state.range(1), true); } } BENCHMARK(priorityAndLocalityWeighted)->Ranges({{false, true}, {2000, 100000}}); + +static void duplicateUpdate(benchmark::State& state) { + Envoy::Thread::MutexBasicLockable lock; + Envoy::Logger::Context logging_state(spdlog::level::warn, + Envoy::Logger::Logger::DEFAULT_LOG_FORMAT, lock, false); + for (auto _ : state) { + Envoy::Upstream::EdsSpeedTest speed_test; + speed_test.resetClusterHelper(); + speed_test.priorityAndLocalityWeightedHelper(true, state.range(0), true); + speed_test.priorityAndLocalityWeightedHelper(true, state.range(0), true); + } +} + +BENCHMARK(duplicateUpdate)->Range(2000, 100000); + +static void healthOnlyUpdate(benchmark::State& state) { + Envoy::Thread::MutexBasicLockable lock; + Envoy::Logger::Context logging_state(spdlog::level::warn, + Envoy::Logger::Logger::DEFAULT_LOG_FORMAT, lock, false); + for (auto _ : state) { + Envoy::Upstream::EdsSpeedTest speed_test; + speed_test.resetClusterHelper(); + speed_test.priorityAndLocalityWeightedHelper(true, state.range(0), true); + speed_test.priorityAndLocalityWeightedHelper(true, state.range(0), false); + } +} + +BENCHMARK(healthOnlyUpdate)->Range(2000, 100000); From 779aa74e017f8bc44ab6a86900d4623ec455b797 Mon Sep 17 00:00:00 2001 From: Phil Genera Date: Thu, 4 Jun 2020 15:27:56 +0000 Subject: [PATCH 02/21] Remove N^2 behavior in updateDynamicHostList, write a benchmark for it. Signed-off-by: Phil Genera --- source/common/upstream/upstream_impl.cc | 31 +++++++++---------------- test/common/upstream/eds_speed_test.cc | 15 +++++------- 2 files changed, 17 insertions(+), 29 deletions(-) diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index f936a5743a092..d58525c506941 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -1327,9 +1327,7 @@ bool BaseDynamicClusterImpl::updateDynamicHostList(const HostVector& new_hosts, bool hosts_changed = false; // Go through and see if the list we have is different from what we just got. If it is, we make a - // new host list and raise a change notification. This uses an N^2 search given that this does not - // happen very often and the list sizes should be small (see - // https://github.com/envoyproxy/envoy/issues/2874). We also check for duplicates here. It's + // new host list and raise a change notification. We also check for duplicates here. It's // possible for DNS to return the same address multiple times, and a bad EDS implementation could // do the same thing. @@ -1337,15 +1335,12 @@ bool BaseDynamicClusterImpl::updateDynamicHostList(const HostVector& new_hosts, std::unordered_set existing_hosts_for_current_priority( current_priority_hosts.size()); HostVector final_hosts; - // this entire function: O(n), due to this loop. for (const HostSharedPtr& host : new_hosts) { - // O(1) if (updated_hosts.count(host->address()->asString())) { continue; } // To match a new host with an existing host means comparing their addresses. - // O(1) auto existing_host = all_hosts.find(host->address()->asString()); const bool existing_host_found = existing_host != all_hosts.end(); @@ -1410,9 +1405,7 @@ bool BaseDynamicClusterImpl::updateDynamicHostList(const HostVector& new_hosts, } existing_host->second->weight(host->weight()); - // O(1) final_hosts.push_back(existing_host->second); - // O(1) updated_hosts[existing_host->second->address()->asString()] = existing_host->second; } else { if (host->weight() > max_host_weight) { @@ -1430,7 +1423,6 @@ bool BaseDynamicClusterImpl::updateDynamicHostList(const HostVector& new_hosts, } } - // O(1) updated_hosts[host->address()->asString()] = host; final_hosts.push_back(host); hosts_added_to_current_priority.push_back(host); @@ -1439,19 +1431,18 @@ bool BaseDynamicClusterImpl::updateDynamicHostList(const HostVector& new_hosts, // Remove hosts from current_priority_hosts that were matched to an existing host in the previous // loop. - // this entire block: O(n^3) (worst case I am not going to type a phi or whatever) - for (auto itr = current_priority_hosts.begin(); itr != current_priority_hosts.end();) { - // O(n)! - auto existing_itr = existing_hosts_for_current_priority.find((*itr)->address()->asString()); + auto current_erase_from = current_priority_hosts.end(); + for (auto i = current_priority_hosts.begin(); i != current_erase_from;) { + auto existing_itr = existing_hosts_for_current_priority.find((*i)->address()->asString()); if (existing_itr != existing_hosts_for_current_priority.end()) { - // O(n)! existing_hosts_for_current_priority.erase(existing_itr); - itr = current_priority_hosts.erase(itr); + *i = *(--current_erase_from); } else { - itr++; + i++; } } + current_priority_hosts.erase(current_erase_from, current_priority_hosts.end()); // If we saw existing hosts during this iteration from a different priority, then we've moved // a host from another priority into this one, so we should mark the priority as having changed. @@ -1469,8 +1460,8 @@ bool BaseDynamicClusterImpl::updateDynamicHostList(const HostVector& new_hosts, const bool dont_remove_healthy_hosts = health_checker_ != nullptr && !info()->drainConnectionsOnHostRemoval(); if (!current_priority_hosts.empty() && dont_remove_healthy_hosts) { - // this whole block: O(n^2) - for (auto i = current_priority_hosts.begin(); i != current_priority_hosts.end();) { + auto erase_from_itr = current_priority_hosts.end(); + for (auto i = current_priority_hosts.begin(); i != erase_from_itr;) { if (!((*i)->healthFlagGet(Host::HealthFlag::FAILED_ACTIVE_HC) || (*i)->healthFlagGet(Host::HealthFlag::FAILED_EDS_HEALTH))) { if ((*i)->weight() > max_host_weight) { @@ -1480,12 +1471,12 @@ bool BaseDynamicClusterImpl::updateDynamicHostList(const HostVector& new_hosts, final_hosts.push_back(*i); updated_hosts[(*i)->address()->asString()] = *i; (*i)->healthFlagSet(Host::HealthFlag::PENDING_DYNAMIC_REMOVAL); - // O(n)! - i = current_priority_hosts.erase(i); + *i = *(--erase_from_itr); } else { i++; } } + current_priority_hosts.erase(erase_from_itr, current_priority_hosts.end()); } // At this point we've accounted for all the new hosts as well the hosts that previously diff --git a/test/common/upstream/eds_speed_test.cc b/test/common/upstream/eds_speed_test.cc index 0645d10694177..8790475d6648f 100644 --- a/test/common/upstream/eds_speed_test.cc +++ b/test/common/upstream/eds_speed_test.cc @@ -48,11 +48,6 @@ class EdsSpeedTest { } void initialize() { - EXPECT_CALL(*cm_.subscription_factory_.subscription_, start(_)); - cluster_->initialize([this] { initialized_ = true; }); - } - - void resetClusterHelper() { resetCluster(R"EOF( name: name connect_timeout: 0.25s @@ -66,6 +61,9 @@ class EdsSpeedTest { refresh_delay: 1s )EOF", Envoy::Upstream::Cluster::InitializePhase::Secondary); + + EXPECT_CALL(*cm_.subscription_factory_.subscription_, start(_)); + cluster_->initialize([this] { initialized_ = true; }); } // Set up an EDS config with multiple priorities, localities, weights and make sure @@ -101,7 +99,6 @@ class EdsSpeedTest { // this is what we're actually testing: validation_visitor_.setSkipValidation(ignore_unknown_dynamic_fields); - initialize(); Protobuf::RepeatedPtrField resources; resources.Add()->PackFrom(cluster_load_assignment); eds_callbacks_->onConfigUpdate(resources, ""); @@ -135,7 +132,7 @@ static void priorityAndLocalityWeighted(benchmark::State& state) { Envoy::Logger::Logger::DEFAULT_LOG_FORMAT, lock, false); for (auto _ : state) { Envoy::Upstream::EdsSpeedTest speed_test; - speed_test.resetClusterHelper(); + speed_test.initialize(); speed_test.priorityAndLocalityWeightedHelper(state.range(0), state.range(1), true); } } @@ -148,7 +145,7 @@ static void duplicateUpdate(benchmark::State& state) { Envoy::Logger::Logger::DEFAULT_LOG_FORMAT, lock, false); for (auto _ : state) { Envoy::Upstream::EdsSpeedTest speed_test; - speed_test.resetClusterHelper(); + speed_test.initialize(); speed_test.priorityAndLocalityWeightedHelper(true, state.range(0), true); speed_test.priorityAndLocalityWeightedHelper(true, state.range(0), true); } @@ -162,7 +159,7 @@ static void healthOnlyUpdate(benchmark::State& state) { Envoy::Logger::Logger::DEFAULT_LOG_FORMAT, lock, false); for (auto _ : state) { Envoy::Upstream::EdsSpeedTest speed_test; - speed_test.resetClusterHelper(); + speed_test.initialize(); speed_test.priorityAndLocalityWeightedHelper(true, state.range(0), true); speed_test.priorityAndLocalityWeightedHelper(true, state.range(0), false); } From 5005fcff4735c098b3ead1cbecda062567f09aab Mon Sep 17 00:00:00 2001 From: Phil Genera Date: Thu, 4 Jun 2020 15:45:57 +0000 Subject: [PATCH 03/21] Run pre-push hooks Signed-off-by: Phil Genera From de4eeb7a0cf9860b5af3f9ee2681074b086ee6be Mon Sep 17 00:00:00 2001 From: Phil Genera Date: Thu, 4 Jun 2020 18:13:43 +0000 Subject: [PATCH 04/21] Remove a note I missed in the prior pass Signed-off-by: Phil Genera --- source/common/upstream/upstream_impl.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index d58525c506941..da991f115452d 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -1376,7 +1376,6 @@ bool BaseDynamicClusterImpl::updateDynamicHostList(const HostVector& new_hosts, // Did metadata change? bool metadata_changed = true; - // O(sizeof(metadata())) if (host->metadata() && existing_host->second->metadata()) { metadata_changed = !Protobuf::util::MessageDifferencer::Equivalent( *host->metadata(), *existing_host->second->metadata()); From 46a176e37c1c5ed20656960be8b45362b26511fe Mon Sep 17 00:00:00 2001 From: Phil Genera Date: Fri, 5 Jun 2020 19:20:00 +0000 Subject: [PATCH 05/21] Respond to (simple) review comments Signed-off-by: Phil Genera --- source/common/upstream/upstream_impl.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index da991f115452d..72b118c102def 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -1438,7 +1438,7 @@ bool BaseDynamicClusterImpl::updateDynamicHostList(const HostVector& new_hosts, existing_hosts_for_current_priority.erase(existing_itr); *i = *(--current_erase_from); } else { - i++; + ++i; } } current_priority_hosts.erase(current_erase_from, current_priority_hosts.end()); @@ -1472,7 +1472,7 @@ bool BaseDynamicClusterImpl::updateDynamicHostList(const HostVector& new_hosts, (*i)->healthFlagSet(Host::HealthFlag::PENDING_DYNAMIC_REMOVAL); *i = *(--erase_from_itr); } else { - i++; + ++i; } } current_priority_hosts.erase(erase_from_itr, current_priority_hosts.end()); From f846f8f5541aad113ca9bb796316eff3768bffd3 Mon Sep 17 00:00:00 2001 From: Phil Genera Date: Fri, 12 Jun 2020 15:55:48 +0000 Subject: [PATCH 06/21] Respond to reivew comments, fix eds_speed_test. Signed-off-by: Phil Genera --- source/common/common/utility.h | 28 +++++++++++++++ source/common/upstream/upstream_impl.cc | 48 ++++++++++++------------- test/common/common/utility_test.cc | 25 +++++++++++++ test/common/upstream/eds_speed_test.cc | 15 ++++---- 4 files changed, 84 insertions(+), 32 deletions(-) diff --git a/source/common/common/utility.h b/source/common/common/utility.h index 8cab7c8a47c9b..5b4e3c833bd52 100644 --- a/source/common/common/utility.h +++ b/source/common/common/utility.h @@ -709,4 +709,32 @@ class InlineString : public InlineStorage { char data_[]; }; +/** + * Utilities for working with containers. + */ +class Containers { +public: + /** + * Remove elements that match the predicate from the list by swapping them to + * the end and truncating. Iterates over c once, order is not preserved. + * + * @param c The container on which to operate + * @param predicate A function which will be called once for every entry in c, + * which returns true if that entry should be removed. + */ + template + static void removeMatchingElements(Container& c, std::function predicate) { + auto erase_from = c.end(); + for (auto i = c.begin(); i != erase_from;) { + if (predicate(*i)) { + *i = *(--erase_from); + } else { + ++i; + } + } + + c.erase(erase_from, c.end()); + } +}; + } // namespace Envoy diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 72b118c102def..f96dcbee43a4c 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -1430,18 +1430,18 @@ bool BaseDynamicClusterImpl::updateDynamicHostList(const HostVector& new_hosts, // Remove hosts from current_priority_hosts that were matched to an existing host in the previous // loop. - auto current_erase_from = current_priority_hosts.end(); - for (auto i = current_priority_hosts.begin(); i != current_erase_from;) { - auto existing_itr = existing_hosts_for_current_priority.find((*i)->address()->asString()); + std::function predicate = + [&existing_hosts_for_current_priority](const HostSharedPtr& p) mutable { + auto existing_itr = existing_hosts_for_current_priority.find(p->address()->asString()); - if (existing_itr != existing_hosts_for_current_priority.end()) { - existing_hosts_for_current_priority.erase(existing_itr); - *i = *(--current_erase_from); - } else { - ++i; - } - } - current_priority_hosts.erase(current_erase_from, current_priority_hosts.end()); + if (existing_itr != existing_hosts_for_current_priority.end()) { + existing_hosts_for_current_priority.erase(existing_itr); + return true; + } + + return false; + }; + Containers::removeMatchingElements(current_priority_hosts, predicate); // If we saw existing hosts during this iteration from a different priority, then we've moved // a host from another priority into this one, so we should mark the priority as having changed. @@ -1459,24 +1459,22 @@ bool BaseDynamicClusterImpl::updateDynamicHostList(const HostVector& new_hosts, const bool dont_remove_healthy_hosts = health_checker_ != nullptr && !info()->drainConnectionsOnHostRemoval(); if (!current_priority_hosts.empty() && dont_remove_healthy_hosts) { - auto erase_from_itr = current_priority_hosts.end(); - for (auto i = current_priority_hosts.begin(); i != erase_from_itr;) { - if (!((*i)->healthFlagGet(Host::HealthFlag::FAILED_ACTIVE_HC) || - (*i)->healthFlagGet(Host::HealthFlag::FAILED_EDS_HEALTH))) { - if ((*i)->weight() > max_host_weight) { - max_host_weight = (*i)->weight(); + predicate = [&updated_hosts, &final_hosts, &max_host_weight](const HostSharedPtr& p) mutable { + if (!(p->healthFlagGet(Host::HealthFlag::FAILED_ACTIVE_HC) || + p->healthFlagGet(Host::HealthFlag::FAILED_EDS_HEALTH))) { + if (p->weight() > max_host_weight) { + max_host_weight = p->weight(); } - final_hosts.push_back(*i); - updated_hosts[(*i)->address()->asString()] = *i; - (*i)->healthFlagSet(Host::HealthFlag::PENDING_DYNAMIC_REMOVAL); - *i = *(--erase_from_itr); - } else { - ++i; + final_hosts.push_back(p); + updated_hosts[p->address()->asString()] = p; + p->healthFlagSet(Host::HealthFlag::PENDING_DYNAMIC_REMOVAL); + return true; } - } - current_priority_hosts.erase(erase_from_itr, current_priority_hosts.end()); + return false; + }; } + Containers::removeMatchingElements(current_priority_hosts, predicate); // At this point we've accounted for all the new hosts as well the hosts that previously // existed in this priority. diff --git a/test/common/common/utility_test.cc b/test/common/common/utility_test.cc index bb326989b94ff..fbc72ce9cf628 100644 --- a/test/common/common/utility_test.cc +++ b/test/common/common/utility_test.cc @@ -875,4 +875,29 @@ TEST(InlineStorageTest, InlineString) { EXPECT_EQ("Hello, world!", hello->toString()); } +TEST(ContainerRemoveElementsTest, Containers) { + auto l = StringUtil::splitToken("one,two,three", ","); + std::function onep = [](const absl::string_view& s) { + return "one" == s; + }; + std::function truep = + [](ABSL_ATTRIBUTE_UNUSED const absl::string_view& s) { return true; }; + std::function falsep = + [](ABSL_ATTRIBUTE_UNUSED const absl::string_view& s) { return false; }; + + Containers::removeMatchingElements(l, falsep); + // nothing is removed: + EXPECT_EQ(3, l.size()); + + Containers::removeMatchingElements(l, onep); + // one element is removed: + EXPECT_EQ(2, l.size()); + // and the last element is now first: + EXPECT_EQ("three", l[0]); + + Containers::removeMatchingElements(l, truep); + // everything is removed: + EXPECT_EQ(0, l.size()); +} + } // namespace Envoy diff --git a/test/common/upstream/eds_speed_test.cc b/test/common/upstream/eds_speed_test.cc index bf486aeb4f348..a2eee8568a3b1 100644 --- a/test/common/upstream/eds_speed_test.cc +++ b/test/common/upstream/eds_speed_test.cc @@ -143,7 +143,8 @@ static void priorityAndLocalityWeighted(benchmark::State& state) { for (auto _ : state) { Envoy::Upstream::EdsSpeedTest speed_test(state); speed_test.initialize(); - speed_test.priorityAndLocalityWeightedHelper(state.range(0), state.range(1), state.range(2), true); + speed_test.priorityAndLocalityWeightedHelper(state.range(0), state.range(1), state.range(2), + true); } } @@ -154,14 +155,14 @@ static void duplicateUpdate(benchmark::State& state) { Envoy::Logger::Context logging_state(spdlog::level::warn, Envoy::Logger::Logger::DEFAULT_LOG_FORMAT, lock, false); for (auto _ : state) { - Envoy::Upstream::EdsSpeedTest speed_test; + Envoy::Upstream::EdsSpeedTest speed_test(state); speed_test.initialize(); - speed_test.priorityAndLocalityWeightedHelper(state.range(0), true, state.range(1), true); - speed_test.priorityAndLocalityWeightedHelper(state.range(0), true, state.range(1), true); + speed_test.priorityAndLocalityWeightedHelper(false, true, state.range(0), true); + speed_test.priorityAndLocalityWeightedHelper(false, true, state.range(0), true); } } -BENCHMARK(duplicateUpdate)->Range({false, true}, 2000, 100000); +BENCHMARK(duplicateUpdate)->Range(2000, 100000); static void healthOnlyUpdate(benchmark::State& state) { Envoy::Thread::MutexBasicLockable lock; @@ -170,8 +171,8 @@ static void healthOnlyUpdate(benchmark::State& state) { for (auto _ : state) { Envoy::Upstream::EdsSpeedTest speed_test(state); speed_test.initialize(); - speed_test.priorityAndLocalityWeightedHelper(state.range(0), true, state.range(1), true); - speed_test.priorityAndLocalityWeightedHelper(state.range(0), true, state.range(1), false); + speed_test.priorityAndLocalityWeightedHelper(false, true, state.range(0), true); + speed_test.priorityAndLocalityWeightedHelper(false, true, state.range(0), false); } } From 6d08d004734a39b1ed78c632bf95310a4e7a03d2 Mon Sep 17 00:00:00 2001 From: Phil Genera Date: Wed, 17 Jun 2020 13:13:43 +0000 Subject: [PATCH 07/21] review comments, fix multiple calls to grpc initializers Signed-off-by: Phil Genera --- source/common/common/utility.h | 2 +- test/common/common/utility_test.cc | 31 ++++++++++++++++++-------- test/common/upstream/eds_speed_test.cc | 6 ++--- 3 files changed, 26 insertions(+), 13 deletions(-) diff --git a/source/common/common/utility.h b/source/common/common/utility.h index 5b4e3c833bd52..03040eaffe848 100644 --- a/source/common/common/utility.h +++ b/source/common/common/utility.h @@ -723,7 +723,7 @@ class Containers { * which returns true if that entry should be removed. */ template - static void removeMatchingElements(Container& c, std::function predicate) { + static void removeMatchingElements(Container& c, std::function predicate) { auto erase_from = c.end(); for (auto i = c.begin(); i != erase_from;) { if (predicate(*i)) { diff --git a/test/common/common/utility_test.cc b/test/common/common/utility_test.cc index fbc72ce9cf628..35df2ee2d3003 100644 --- a/test/common/common/utility_test.cc +++ b/test/common/common/utility_test.cc @@ -877,27 +877,40 @@ TEST(InlineStorageTest, InlineString) { TEST(ContainerRemoveElementsTest, Containers) { auto l = StringUtil::splitToken("one,two,three", ","); - std::function onep = [](const absl::string_view& s) { - return "one" == s; + std::function onep = [](absl::string_view s) { return "one" == s; }; + std::function threep = [](absl::string_view s) { return "three" == s; }; + std::function truep = [](ABSL_ATTRIBUTE_UNUSED absl::string_view s) { + return true; + }; + std::function falsep = [](ABSL_ATTRIBUTE_UNUSED absl::string_view s) { + return false; }; - std::function truep = - [](ABSL_ATTRIBUTE_UNUSED const absl::string_view& s) { return true; }; - std::function falsep = - [](ABSL_ATTRIBUTE_UNUSED const absl::string_view& s) { return false; }; Containers::removeMatchingElements(l, falsep); // nothing is removed: EXPECT_EQ(3, l.size()); + Containers::removeMatchingElements(l, threep); + // one element is removed from the end: + EXPECT_EQ(2, l.size()); + EXPECT_EQ("two", l[1]); + Containers::removeMatchingElements(l, onep); // one element is removed: - EXPECT_EQ(2, l.size()); - // and the last element is now first: - EXPECT_EQ("three", l[0]); + EXPECT_EQ(1, l.size()); + // and the last element is now first (and only): + EXPECT_EQ("two", l[0]); Containers::removeMatchingElements(l, truep); // everything is removed: EXPECT_EQ(0, l.size()); } +TEST(ContainerRemoveElementsListTest, Containers) { + std::list l = {1, 2, 3, 4, 5, 6, 7, 8, 9, 10}; + std::function evens = [](int i) { return i % 2 == 0; }; + Containers::removeMatchingElements(l, evens); + EXPECT_EQ(5, l.size()); +} + } // namespace Envoy diff --git a/test/common/upstream/eds_speed_test.cc b/test/common/upstream/eds_speed_test.cc index 5ecae7a831780..3ee4e8de020ed 100644 --- a/test/common/upstream/eds_speed_test.cc +++ b/test/common/upstream/eds_speed_test.cc @@ -60,6 +60,8 @@ class EdsSpeedTest { EXPECT_CALL(*cm_.subscription_factory_.subscription_, start(_)); cluster_->initialize([this] { initialized_ = true; }); + EXPECT_CALL(*async_client_, startRaw(_, _, _, _)).WillOnce(testing::Return(&async_stream_)); + subscription_->start({"fare"}); } void resetCluster(const std::string& yaml_config, Cluster::InitializePhase initialize_phase) { @@ -85,6 +87,7 @@ class EdsSpeedTest { void priorityAndLocalityWeightedHelper(bool ignore_unknown_dynamic_fields, size_t num_hosts, bool healthy) { state_.PauseTiming(); + envoy::config::endpoint::v3::ClusterLoadAssignment cluster_load_assignment; cluster_load_assignment.set_cluster_name("fare"); @@ -124,9 +127,6 @@ class EdsSpeedTest { ""); resource->set_type_url("type.googleapis.com/envoy.api.v2.ClusterLoadAssignment"); } - EXPECT_CALL(*async_client_, startRaw(_, _, _, _)) - .WillRepeatedly(testing::Return(&async_stream_)); - subscription_->start({"fare"}); state_.ResumeTiming(); grpc_mux_->grpcStreamForTest().onReceiveMessage(std::move(response)); ASSERT(cluster_->prioritySet().hostSetsPerPriority()[1]->hostsPerLocality().get()[0].size() == From 7cccabb9e414d22235cca32b453eea3a981711d4 Mon Sep 17 00:00:00 2001 From: Phil Genera Date: Wed, 17 Jun 2020 14:32:15 +0000 Subject: [PATCH 08/21] respond to review comments Signed-off-by: Phil Genera --- test/common/common/utility_test.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/common/common/utility_test.cc b/test/common/common/utility_test.cc index 35df2ee2d3003..cbac5af1addc9 100644 --- a/test/common/common/utility_test.cc +++ b/test/common/common/utility_test.cc @@ -909,8 +909,10 @@ TEST(ContainerRemoveElementsTest, Containers) { TEST(ContainerRemoveElementsListTest, Containers) { std::list l = {1, 2, 3, 4, 5, 6, 7, 8, 9, 10}; std::function evens = [](int i) { return i % 2 == 0; }; + Containers::removeMatchingElements(l, evens); - EXPECT_EQ(5, l.size()); + // since the swap-and-erase happens iteratively, the list is very reordered + EXPECT_EQ((std::list{1, 9, 3, 7, 5}), l); } } // namespace Envoy From 4d1acad6cfce413928bad03d3f668b7c9eddcfd3 Mon Sep 17 00:00:00 2001 From: Phil Genera Date: Wed, 17 Jun 2020 15:51:47 +0000 Subject: [PATCH 09/21] Solve the mystery of c++ templates. Signed-off-by: Phil Genera --- source/common/common/utility.h | 6 ++- source/common/upstream/upstream_impl.cc | 50 ++++++++++++------------- test/common/common/utility_test.cc | 19 ++++------ 3 files changed, 36 insertions(+), 39 deletions(-) diff --git a/source/common/common/utility.h b/source/common/common/utility.h index 03040eaffe848..159991ae17eac 100644 --- a/source/common/common/utility.h +++ b/source/common/common/utility.h @@ -722,8 +722,10 @@ class Containers { * @param predicate A function which will be called once for every entry in c, * which returns true if that entry should be removed. */ - template - static void removeMatchingElements(Container& c, std::function predicate) { + template + static void + removeMatchingElements(Container& c, + std::function predicate) { auto erase_from = c.end(); for (auto i = c.begin(); i != erase_from;) { if (predicate(*i)) { diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index f96dcbee43a4c..449dbe6ef81d8 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -1430,18 +1430,17 @@ bool BaseDynamicClusterImpl::updateDynamicHostList(const HostVector& new_hosts, // Remove hosts from current_priority_hosts that were matched to an existing host in the previous // loop. - std::function predicate = - [&existing_hosts_for_current_priority](const HostSharedPtr& p) mutable { - auto existing_itr = existing_hosts_for_current_priority.find(p->address()->asString()); + Containers::removeMatchingElements(current_priority_hosts, [&existing_hosts_for_current_priority]( + const HostSharedPtr& p) mutable { + auto existing_itr = existing_hosts_for_current_priority.find(p->address()->asString()); - if (existing_itr != existing_hosts_for_current_priority.end()) { - existing_hosts_for_current_priority.erase(existing_itr); - return true; - } + if (existing_itr != existing_hosts_for_current_priority.end()) { + existing_hosts_for_current_priority.erase(existing_itr); + return true; + } - return false; - }; - Containers::removeMatchingElements(current_priority_hosts, predicate); + return false; + }); // If we saw existing hosts during this iteration from a different priority, then we've moved // a host from another priority into this one, so we should mark the priority as having changed. @@ -1459,22 +1458,23 @@ bool BaseDynamicClusterImpl::updateDynamicHostList(const HostVector& new_hosts, const bool dont_remove_healthy_hosts = health_checker_ != nullptr && !info()->drainConnectionsOnHostRemoval(); if (!current_priority_hosts.empty() && dont_remove_healthy_hosts) { - predicate = [&updated_hosts, &final_hosts, &max_host_weight](const HostSharedPtr& p) mutable { - if (!(p->healthFlagGet(Host::HealthFlag::FAILED_ACTIVE_HC) || - p->healthFlagGet(Host::HealthFlag::FAILED_EDS_HEALTH))) { - if (p->weight() > max_host_weight) { - max_host_weight = p->weight(); - } - - final_hosts.push_back(p); - updated_hosts[p->address()->asString()] = p; - p->healthFlagSet(Host::HealthFlag::PENDING_DYNAMIC_REMOVAL); - return true; - } - return false; - }; + Containers::removeMatchingElements( + current_priority_hosts, + [&updated_hosts, &final_hosts, &max_host_weight](const HostSharedPtr& p) mutable { + if (!(p->healthFlagGet(Host::HealthFlag::FAILED_ACTIVE_HC) || + p->healthFlagGet(Host::HealthFlag::FAILED_EDS_HEALTH))) { + if (p->weight() > max_host_weight) { + max_host_weight = p->weight(); + } + + final_hosts.push_back(p); + updated_hosts[p->address()->asString()] = p; + p->healthFlagSet(Host::HealthFlag::PENDING_DYNAMIC_REMOVAL); + return true; + } + return false; + }); } - Containers::removeMatchingElements(current_priority_hosts, predicate); // At this point we've accounted for all the new hosts as well the hosts that previously // existed in this priority. diff --git a/test/common/common/utility_test.cc b/test/common/common/utility_test.cc index cbac5af1addc9..d87612ddd6bce 100644 --- a/test/common/common/utility_test.cc +++ b/test/common/common/utility_test.cc @@ -877,16 +877,11 @@ TEST(InlineStorageTest, InlineString) { TEST(ContainerRemoveElementsTest, Containers) { auto l = StringUtil::splitToken("one,two,three", ","); - std::function onep = [](absl::string_view s) { return "one" == s; }; - std::function threep = [](absl::string_view s) { return "three" == s; }; - std::function truep = [](ABSL_ATTRIBUTE_UNUSED absl::string_view s) { - return true; - }; - std::function falsep = [](ABSL_ATTRIBUTE_UNUSED absl::string_view s) { - return false; - }; + auto onep = [](absl::string_view s) { return "one" == s; }; + auto threep = [](absl::string_view s) { return "three" == s; }; - Containers::removeMatchingElements(l, falsep); + Containers::removeMatchingElements( + l, [](ABSL_ATTRIBUTE_UNUSED absl::string_view s) { return false; }); // nothing is removed: EXPECT_EQ(3, l.size()); @@ -901,16 +896,16 @@ TEST(ContainerRemoveElementsTest, Containers) { // and the last element is now first (and only): EXPECT_EQ("two", l[0]); - Containers::removeMatchingElements(l, truep); + Containers::removeMatchingElements( + l, [](ABSL_ATTRIBUTE_UNUSED absl::string_view s) { return true; }); // everything is removed: EXPECT_EQ(0, l.size()); } TEST(ContainerRemoveElementsListTest, Containers) { std::list l = {1, 2, 3, 4, 5, 6, 7, 8, 9, 10}; - std::function evens = [](int i) { return i % 2 == 0; }; - Containers::removeMatchingElements(l, evens); + Containers::removeMatchingElements(l, [](int i) { return i % 2 == 0; }); // since the swap-and-erase happens iteratively, the list is very reordered EXPECT_EQ((std::list{1, 9, 3, 7, 5}), l); } From d8929c33c1636e6daeb38621675831a0f91fc741 Mon Sep 17 00:00:00 2001 From: Phil Genera Date: Fri, 19 Jun 2020 14:07:42 +0000 Subject: [PATCH 10/21] std::remove_if instead of building it myself Signed-off-by: Phil Genera --- source/common/common/utility.h | 30 --------------------- source/common/upstream/upstream_impl.cc | 26 ++++++++++-------- test/common/common/utility_test.cc | 35 ------------------------- 3 files changed, 15 insertions(+), 76 deletions(-) diff --git a/source/common/common/utility.h b/source/common/common/utility.h index 159991ae17eac..8cab7c8a47c9b 100644 --- a/source/common/common/utility.h +++ b/source/common/common/utility.h @@ -709,34 +709,4 @@ class InlineString : public InlineStorage { char data_[]; }; -/** - * Utilities for working with containers. - */ -class Containers { -public: - /** - * Remove elements that match the predicate from the list by swapping them to - * the end and truncating. Iterates over c once, order is not preserved. - * - * @param c The container on which to operate - * @param predicate A function which will be called once for every entry in c, - * which returns true if that entry should be removed. - */ - template - static void - removeMatchingElements(Container& c, - std::function predicate) { - auto erase_from = c.end(); - for (auto i = c.begin(); i != erase_from;) { - if (predicate(*i)) { - *i = *(--erase_from); - } else { - ++i; - } - } - - c.erase(erase_from, c.end()); - } -}; - } // namespace Envoy diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 449dbe6ef81d8..e335354f036c2 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -1430,17 +1430,20 @@ bool BaseDynamicClusterImpl::updateDynamicHostList(const HostVector& new_hosts, // Remove hosts from current_priority_hosts that were matched to an existing host in the previous // loop. - Containers::removeMatchingElements(current_priority_hosts, [&existing_hosts_for_current_priority]( - const HostSharedPtr& p) mutable { - auto existing_itr = existing_hosts_for_current_priority.find(p->address()->asString()); + auto erase_from = + std::remove_if(current_priority_hosts.begin(), current_priority_hosts.end(), + [&existing_hosts_for_current_priority](const HostSharedPtr& p) mutable { + auto existing_itr = + existing_hosts_for_current_priority.find(p->address()->asString()); - if (existing_itr != existing_hosts_for_current_priority.end()) { - existing_hosts_for_current_priority.erase(existing_itr); - return true; - } + if (existing_itr != existing_hosts_for_current_priority.end()) { + existing_hosts_for_current_priority.erase(existing_itr); + return true; + } - return false; - }); + return false; + }); + current_priority_hosts.erase(erase_from, current_priority_hosts.end()); // If we saw existing hosts during this iteration from a different priority, then we've moved // a host from another priority into this one, so we should mark the priority as having changed. @@ -1458,8 +1461,8 @@ bool BaseDynamicClusterImpl::updateDynamicHostList(const HostVector& new_hosts, const bool dont_remove_healthy_hosts = health_checker_ != nullptr && !info()->drainConnectionsOnHostRemoval(); if (!current_priority_hosts.empty() && dont_remove_healthy_hosts) { - Containers::removeMatchingElements( - current_priority_hosts, + erase_from = std::remove_if( + current_priority_hosts.begin(), current_priority_hosts.end(), [&updated_hosts, &final_hosts, &max_host_weight](const HostSharedPtr& p) mutable { if (!(p->healthFlagGet(Host::HealthFlag::FAILED_ACTIVE_HC) || p->healthFlagGet(Host::HealthFlag::FAILED_EDS_HEALTH))) { @@ -1474,6 +1477,7 @@ bool BaseDynamicClusterImpl::updateDynamicHostList(const HostVector& new_hosts, } return false; }); + current_priority_hosts.erase(erase_from, current_priority_hosts.end()); } // At this point we've accounted for all the new hosts as well the hosts that previously diff --git a/test/common/common/utility_test.cc b/test/common/common/utility_test.cc index d87612ddd6bce..bb326989b94ff 100644 --- a/test/common/common/utility_test.cc +++ b/test/common/common/utility_test.cc @@ -875,39 +875,4 @@ TEST(InlineStorageTest, InlineString) { EXPECT_EQ("Hello, world!", hello->toString()); } -TEST(ContainerRemoveElementsTest, Containers) { - auto l = StringUtil::splitToken("one,two,three", ","); - auto onep = [](absl::string_view s) { return "one" == s; }; - auto threep = [](absl::string_view s) { return "three" == s; }; - - Containers::removeMatchingElements( - l, [](ABSL_ATTRIBUTE_UNUSED absl::string_view s) { return false; }); - // nothing is removed: - EXPECT_EQ(3, l.size()); - - Containers::removeMatchingElements(l, threep); - // one element is removed from the end: - EXPECT_EQ(2, l.size()); - EXPECT_EQ("two", l[1]); - - Containers::removeMatchingElements(l, onep); - // one element is removed: - EXPECT_EQ(1, l.size()); - // and the last element is now first (and only): - EXPECT_EQ("two", l[0]); - - Containers::removeMatchingElements( - l, [](ABSL_ATTRIBUTE_UNUSED absl::string_view s) { return true; }); - // everything is removed: - EXPECT_EQ(0, l.size()); -} - -TEST(ContainerRemoveElementsListTest, Containers) { - std::list l = {1, 2, 3, 4, 5, 6, 7, 8, 9, 10}; - - Containers::removeMatchingElements(l, [](int i) { return i % 2 == 0; }); - // since the swap-and-erase happens iteratively, the list is very reordered - EXPECT_EQ((std::list{1, 9, 3, 7, 5}), l); -} - } // namespace Envoy From b21f4e1552d84b13e4eaf8f0929d18de385a8acd Mon Sep 17 00:00:00 2001 From: Phil Genera Date: Fri, 19 Jun 2020 17:20:17 +0000 Subject: [PATCH 11/21] respond to review comments: longer test timeout. Signed-off-by: Phil Genera --- test/common/upstream/BUILD | 1 + test/common/upstream/eds_speed_test.cc | 8 +++++--- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/test/common/upstream/BUILD b/test/common/upstream/BUILD index fc6bc5d966efa..ed7619c090200 100644 --- a/test/common/upstream/BUILD +++ b/test/common/upstream/BUILD @@ -127,6 +127,7 @@ envoy_cc_benchmark_binary( envoy_benchmark_test( name = "eds_speed_test_benchmark_test", + size = "large", benchmark_binary = "eds_speed_test", ) diff --git a/test/common/upstream/eds_speed_test.cc b/test/common/upstream/eds_speed_test.cc index 3ee4e8de020ed..2e5c5c7d2d845 100644 --- a/test/common/upstream/eds_speed_test.cc +++ b/test/common/upstream/eds_speed_test.cc @@ -172,7 +172,9 @@ static void priorityAndLocalityWeighted(benchmark::State& state) { } } -BENCHMARK(priorityAndLocalityWeighted)->Ranges({{false, true}, {false, true}, {2000, 100000}}); +BENCHMARK(priorityAndLocalityWeighted) + ->Ranges({{false, true}, {false, true}, {2000, 100000}}) + ->Unit(benchmark::kMillisecond); static void duplicateUpdate(benchmark::State& state) { Envoy::Thread::MutexBasicLockable lock; @@ -185,7 +187,7 @@ static void duplicateUpdate(benchmark::State& state) { } } -BENCHMARK(duplicateUpdate)->Range(2000, 100000); +BENCHMARK(duplicateUpdate)->Range(2000, 100000)->Unit(benchmark::kMillisecond); static void healthOnlyUpdate(benchmark::State& state) { Envoy::Thread::MutexBasicLockable lock; @@ -198,4 +200,4 @@ static void healthOnlyUpdate(benchmark::State& state) { } } -BENCHMARK(healthOnlyUpdate)->Range(2000, 100000); +BENCHMARK(healthOnlyUpdate)->Range(2000, 100000)->Unit(benchmark::kMillisecond); From 5fb29ebfa4871a5495d00bddbfdc0708ff575edc Mon Sep 17 00:00:00 2001 From: Phil Genera Date: Mon, 22 Jun 2020 18:34:45 +0000 Subject: [PATCH 12/21] Respond to revievw comments, decrease benchmark iterations Signed-off-by: Phil Genera --- source/common/upstream/upstream_impl.cc | 34 ++++++++++++------------- test/common/upstream/eds_speed_test.cc | 13 +++++++--- 2 files changed, 27 insertions(+), 20 deletions(-) diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index e335354f036c2..2a42439f6b832 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -1432,7 +1432,7 @@ bool BaseDynamicClusterImpl::updateDynamicHostList(const HostVector& new_hosts, // loop. auto erase_from = std::remove_if(current_priority_hosts.begin(), current_priority_hosts.end(), - [&existing_hosts_for_current_priority](const HostSharedPtr& p) mutable { + [&existing_hosts_for_current_priority](const HostSharedPtr& p) { auto existing_itr = existing_hosts_for_current_priority.find(p->address()->asString()); @@ -1461,22 +1461,22 @@ bool BaseDynamicClusterImpl::updateDynamicHostList(const HostVector& new_hosts, const bool dont_remove_healthy_hosts = health_checker_ != nullptr && !info()->drainConnectionsOnHostRemoval(); if (!current_priority_hosts.empty() && dont_remove_healthy_hosts) { - erase_from = std::remove_if( - current_priority_hosts.begin(), current_priority_hosts.end(), - [&updated_hosts, &final_hosts, &max_host_weight](const HostSharedPtr& p) mutable { - if (!(p->healthFlagGet(Host::HealthFlag::FAILED_ACTIVE_HC) || - p->healthFlagGet(Host::HealthFlag::FAILED_EDS_HEALTH))) { - if (p->weight() > max_host_weight) { - max_host_weight = p->weight(); - } - - final_hosts.push_back(p); - updated_hosts[p->address()->asString()] = p; - p->healthFlagSet(Host::HealthFlag::PENDING_DYNAMIC_REMOVAL); - return true; - } - return false; - }); + erase_from = + std::remove_if(current_priority_hosts.begin(), current_priority_hosts.end(), + [&updated_hosts, &final_hosts, &max_host_weight](const HostSharedPtr& p) { + if (!(p->healthFlagGet(Host::HealthFlag::FAILED_ACTIVE_HC) || + p->healthFlagGet(Host::HealthFlag::FAILED_EDS_HEALTH))) { + if (p->weight() > max_host_weight) { + max_host_weight = p->weight(); + } + + final_hosts.push_back(p); + updated_hosts[p->address()->asString()] = p; + p->healthFlagSet(Host::HealthFlag::PENDING_DYNAMIC_REMOVAL); + return true; + } + return false; + }); current_priority_hosts.erase(erase_from, current_priority_hosts.end()); } diff --git a/test/common/upstream/eds_speed_test.cc b/test/common/upstream/eds_speed_test.cc index 2e5c5c7d2d845..778051032c351 100644 --- a/test/common/upstream/eds_speed_test.cc +++ b/test/common/upstream/eds_speed_test.cc @@ -172,8 +172,11 @@ static void priorityAndLocalityWeighted(benchmark::State& state) { } } +// The endpoint count (1, 2) is intentionally very low to save on test runtime +// if you're not actually interested in this benchmark; (1000, 100000) has been +// used for measuring impactful changes. BENCHMARK(priorityAndLocalityWeighted) - ->Ranges({{false, true}, {false, true}, {2000, 100000}}) + ->Ranges({{false, true}, {false, true}, {1, 2}}) ->Unit(benchmark::kMillisecond); static void duplicateUpdate(benchmark::State& state) { @@ -187,7 +190,11 @@ static void duplicateUpdate(benchmark::State& state) { } } -BENCHMARK(duplicateUpdate)->Range(2000, 100000)->Unit(benchmark::kMillisecond); +// The endpoint count (100) here and elsewhere is intentionally low to save on +// resources in continous testing, if you're trying to quantify new changes, +// Range(2000, 100000) is a reasonable starting point. +BENCHMARK(duplicateUpdate)->Arg(100)->Unit(benchmark::kMillisecond); + static void healthOnlyUpdate(benchmark::State& state) { Envoy::Thread::MutexBasicLockable lock; @@ -200,4 +207,4 @@ static void healthOnlyUpdate(benchmark::State& state) { } } -BENCHMARK(healthOnlyUpdate)->Range(2000, 100000)->Unit(benchmark::kMillisecond); +BENCHMARK(healthOnlyUpdate)->Arg(100)->Unit(benchmark::kMillisecond); From 3661e8f877ad43412dfed33bb5f89c768dec319f Mon Sep 17 00:00:00 2001 From: Phil Genera Date: Mon, 22 Jun 2020 18:35:46 +0000 Subject: [PATCH 13/21] check_format.py fix Signed-off-by: Phil Genera --- test/common/upstream/eds_speed_test.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/test/common/upstream/eds_speed_test.cc b/test/common/upstream/eds_speed_test.cc index 778051032c351..f11f987395d0c 100644 --- a/test/common/upstream/eds_speed_test.cc +++ b/test/common/upstream/eds_speed_test.cc @@ -195,7 +195,6 @@ static void duplicateUpdate(benchmark::State& state) { // Range(2000, 100000) is a reasonable starting point. BENCHMARK(duplicateUpdate)->Arg(100)->Unit(benchmark::kMillisecond); - static void healthOnlyUpdate(benchmark::State& state) { Envoy::Thread::MutexBasicLockable lock; Envoy::Logger::Context logging_state(spdlog::level::warn, From 6dd6560e8b9291c0717bce75b49fcf0cd85c731a Mon Sep 17 00:00:00 2001 From: Phil Genera Date: Mon, 22 Jun 2020 18:38:15 +0000 Subject: [PATCH 14/21] appease spellchecker Signed-off-by: Phil Genera --- test/common/upstream/eds_speed_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/common/upstream/eds_speed_test.cc b/test/common/upstream/eds_speed_test.cc index f11f987395d0c..11cb33bb203f0 100644 --- a/test/common/upstream/eds_speed_test.cc +++ b/test/common/upstream/eds_speed_test.cc @@ -174,7 +174,7 @@ static void priorityAndLocalityWeighted(benchmark::State& state) { // The endpoint count (1, 2) is intentionally very low to save on test runtime // if you're not actually interested in this benchmark; (1000, 100000) has been -// used for measuring impactful changes. +// used for measuring changes. BENCHMARK(priorityAndLocalityWeighted) ->Ranges({{false, true}, {false, true}, {1, 2}}) ->Unit(benchmark::kMillisecond); @@ -191,7 +191,7 @@ static void duplicateUpdate(benchmark::State& state) { } // The endpoint count (100) here and elsewhere is intentionally low to save on -// resources in continous testing, if you're trying to quantify new changes, +// resources in continuous testing, if you're trying to quantify new changes, // Range(2000, 100000) is a reasonable starting point. BENCHMARK(duplicateUpdate)->Arg(100)->Unit(benchmark::kMillisecond); From 5f4a5a71f4aefe1bdeaea2a26a1e3189843f771b Mon Sep 17 00:00:00 2001 From: Phil Genera Date: Sun, 28 Jun 2020 01:17:45 +0000 Subject: [PATCH 15/21] Add a flag to skip (and lie about) slow benchmarks. Signed-off-by: Phil Genera --- bazel/test_for_benchmark_wrapper.sh | 6 +++-- test/benchmark/BUILD | 1 + test/benchmark/main.cc | 26 +++++++++++++++++---- test/common/upstream/BUILD | 1 - test/common/upstream/eds_speed_test.cc | 32 +++++++++++++++----------- 5 files changed, 45 insertions(+), 21 deletions(-) diff --git a/bazel/test_for_benchmark_wrapper.sh b/bazel/test_for_benchmark_wrapper.sh index 7c1dc7a1def62..37de6d0d0d810 100755 --- a/bazel/test_for_benchmark_wrapper.sh +++ b/bazel/test_for_benchmark_wrapper.sh @@ -1,4 +1,6 @@ #!/bin/bash -# Set the benchmark time to 0 to just verify that the benchmark runs to completion. -"${TEST_SRCDIR}/envoy/$@" --benchmark_min_time=0 +# Set the benchmark time to 0 to just verify that the benchmark runs to +# completion. We're interacting with two different flag parsers, so the order +# of flags and the -- matters. +"${TEST_SRCDIR}/envoy/$@" --skip_expensive_benchmarks -- --benchmark_min_time=0 diff --git a/test/benchmark/BUILD b/test/benchmark/BUILD index afba86c9dd220..06d31f0243c7c 100644 --- a/test/benchmark/BUILD +++ b/test/benchmark/BUILD @@ -13,6 +13,7 @@ envoy_cc_test_library( srcs = ["main.cc"], external_deps = [ "benchmark", + "tclap", ], deps = [ "//test/test_common:environment_lib", diff --git a/test/benchmark/main.cc b/test/benchmark/main.cc index 7afdf85e6558d..487bece4c7bcb 100644 --- a/test/benchmark/main.cc +++ b/test/benchmark/main.cc @@ -3,14 +3,32 @@ #include "test/test_common/environment.h" #include "benchmark/benchmark.h" +#include "tclap/CmdLine.h" -// Boilerplate main(), which discovers benchmarks and runs them. +bool g_skip_expensive_benchmarks = false; + +// Boilerplate main(), which discovers benchmarks and runs them. This uses two +// different flag parsers, so the order of flags matters: flags defined here +// must be passed first, and flags defined in benchmark::Initialize second, +// seperated by --. int main(int argc, char** argv) { Envoy::TestEnvironment::initializeTestMain(argv[0]); - benchmark::Initialize(&argc, argv); - if (benchmark::ReportUnrecognizedArguments(argc, argv)) { - return 1; + TCLAP::CmdLine cmd("envoy-benchmark-test", ' ', "0.1"); + TCLAP::SwitchArg skip_switch("s", "skip_expensive_benchmarks", + "skip or minimize expensive benchmarks", cmd, false); + + cmd.setExceptionHandling(false); + try { + cmd.parse(argc, argv); + } catch (const TCLAP::ExitException& e) { + // parse() throws an ExitException with status 0 after printing the output + // for --help and --version. + return 0; } + + g_skip_expensive_benchmarks = skip_switch.getValue(); + + benchmark::Initialize(&argc, argv); benchmark::RunSpecifiedBenchmarks(); } diff --git a/test/common/upstream/BUILD b/test/common/upstream/BUILD index ed7619c090200..fc6bc5d966efa 100644 --- a/test/common/upstream/BUILD +++ b/test/common/upstream/BUILD @@ -127,7 +127,6 @@ envoy_cc_benchmark_binary( envoy_benchmark_test( name = "eds_speed_test_benchmark_test", - size = "large", benchmark_binary = "eds_speed_test", ) diff --git a/test/common/upstream/eds_speed_test.cc b/test/common/upstream/eds_speed_test.cc index 11cb33bb203f0..ff8c0a57c7608 100644 --- a/test/common/upstream/eds_speed_test.cc +++ b/test/common/upstream/eds_speed_test.cc @@ -162,38 +162,40 @@ class EdsSpeedTest { } // namespace Upstream } // namespace Envoy +extern bool g_skip_expensive_benchmarks; + static void priorityAndLocalityWeighted(benchmark::State& state) { Envoy::Thread::MutexBasicLockable lock; Envoy::Logger::Context logging_state(spdlog::level::warn, Envoy::Logger::Logger::DEFAULT_LOG_FORMAT, lock, false); for (auto _ : state) { Envoy::Upstream::EdsSpeedTest speed_test(state, state.range(0)); - speed_test.priorityAndLocalityWeightedHelper(state.range(1), state.range(2), true); + // if we've been instructed to skip tests, only run once no matter the argument: + uint32_t endpoints = g_skip_expensive_benchmarks ? 1 : state.range(2); + + speed_test.priorityAndLocalityWeightedHelper(state.range(1), endpoints, true); } } -// The endpoint count (1, 2) is intentionally very low to save on test runtime -// if you're not actually interested in this benchmark; (1000, 100000) has been -// used for measuring changes. BENCHMARK(priorityAndLocalityWeighted) - ->Ranges({{false, true}, {false, true}, {1, 2}}) + ->Ranges({{false, true}, {false, true}, {1, 100000}}) ->Unit(benchmark::kMillisecond); static void duplicateUpdate(benchmark::State& state) { Envoy::Thread::MutexBasicLockable lock; Envoy::Logger::Context logging_state(spdlog::level::warn, Envoy::Logger::Logger::DEFAULT_LOG_FORMAT, lock, false); + for (auto _ : state) { Envoy::Upstream::EdsSpeedTest speed_test(state, false); - speed_test.priorityAndLocalityWeightedHelper(true, state.range(0), true); - speed_test.priorityAndLocalityWeightedHelper(true, state.range(0), true); + uint32_t endpoints = g_skip_expensive_benchmarks ? 1 : state.range(0); + + speed_test.priorityAndLocalityWeightedHelper(true, endpoints, true); + speed_test.priorityAndLocalityWeightedHelper(true, endpoints, true); } } -// The endpoint count (100) here and elsewhere is intentionally low to save on -// resources in continuous testing, if you're trying to quantify new changes, -// Range(2000, 100000) is a reasonable starting point. -BENCHMARK(duplicateUpdate)->Arg(100)->Unit(benchmark::kMillisecond); +BENCHMARK(duplicateUpdate)->Range(1, 100000)->Unit(benchmark::kMillisecond); static void healthOnlyUpdate(benchmark::State& state) { Envoy::Thread::MutexBasicLockable lock; @@ -201,9 +203,11 @@ static void healthOnlyUpdate(benchmark::State& state) { Envoy::Logger::Logger::DEFAULT_LOG_FORMAT, lock, false); for (auto _ : state) { Envoy::Upstream::EdsSpeedTest speed_test(state, false); - speed_test.priorityAndLocalityWeightedHelper(true, state.range(0), true); - speed_test.priorityAndLocalityWeightedHelper(true, state.range(0), false); + uint32_t endpoints = g_skip_expensive_benchmarks ? 1 : state.range(0); + + speed_test.priorityAndLocalityWeightedHelper(true, endpoints, true); + speed_test.priorityAndLocalityWeightedHelper(true, endpoints, false); } } -BENCHMARK(healthOnlyUpdate)->Arg(100)->Unit(benchmark::kMillisecond); +BENCHMARK(healthOnlyUpdate)->Range(1, 100000)->Unit(benchmark::kMillisecond); From db737b99a3acbf7dff0e8775fbddf0455ca103ed Mon Sep 17 00:00:00 2001 From: Phil Genera Date: Sun, 28 Jun 2020 01:19:44 +0000 Subject: [PATCH 16/21] spelling Signed-off-by: Phil Genera --- test/benchmark/main.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/benchmark/main.cc b/test/benchmark/main.cc index 487bece4c7bcb..08490d6fa259c 100644 --- a/test/benchmark/main.cc +++ b/test/benchmark/main.cc @@ -10,7 +10,7 @@ bool g_skip_expensive_benchmarks = false; // Boilerplate main(), which discovers benchmarks and runs them. This uses two // different flag parsers, so the order of flags matters: flags defined here // must be passed first, and flags defined in benchmark::Initialize second, -// seperated by --. +// separated by --. int main(int argc, char** argv) { Envoy::TestEnvironment::initializeTestMain(argv[0]); From 3c37bc62d037ee3219c3578c8b356d9b046c3883 Mon Sep 17 00:00:00 2001 From: Phil Genera Date: Mon, 29 Jun 2020 16:00:48 +0000 Subject: [PATCH 17/21] response to review comments Signed-off-by: Phil Genera --- test/benchmark/BUILD | 1 + test/benchmark/main.cc | 10 ++++++++-- test/benchmark/main.h | 7 +++++++ test/common/upstream/eds_speed_test.cc | 9 ++++----- 4 files changed, 20 insertions(+), 7 deletions(-) create mode 100644 test/benchmark/main.h diff --git a/test/benchmark/BUILD b/test/benchmark/BUILD index 06d31f0243c7c..fa01e3b1ce638 100644 --- a/test/benchmark/BUILD +++ b/test/benchmark/BUILD @@ -11,6 +11,7 @@ envoy_package() envoy_cc_test_library( name = "main", srcs = ["main.cc"], + hdrs = ["main.h"], external_deps = [ "benchmark", "tclap", diff --git a/test/benchmark/main.cc b/test/benchmark/main.cc index 08490d6fa259c..0c3ca4408b3ba 100644 --- a/test/benchmark/main.cc +++ b/test/benchmark/main.cc @@ -1,19 +1,23 @@ // NOLINT(namespace-envoy) // This is an Envoy driver for benchmarks. +#include "test/benchmark/main.h" + #include "test/test_common/environment.h" #include "benchmark/benchmark.h" #include "tclap/CmdLine.h" -bool g_skip_expensive_benchmarks = false; +bool skip_expensive_benchmarks = false; // Boilerplate main(), which discovers benchmarks and runs them. This uses two // different flag parsers, so the order of flags matters: flags defined here // must be passed first, and flags defined in benchmark::Initialize second, // separated by --. +// TODO(pgenera): convert this to abseil/flags/ when benchmark also adopts abseil. int main(int argc, char** argv) { Envoy::TestEnvironment::initializeTestMain(argv[0]); + // NOLINTNEXTLINE(clang-analyzer-optin.cplusplus.VirtualCall) TCLAP::CmdLine cmd("envoy-benchmark-test", ' ', "0.1"); TCLAP::SwitchArg skip_switch("s", "skip_expensive_benchmarks", "skip or minimize expensive benchmarks", cmd, false); @@ -27,8 +31,10 @@ int main(int argc, char** argv) { return 0; } - g_skip_expensive_benchmarks = skip_switch.getValue(); + skip_expensive_benchmarks = skip_switch.getValue(); benchmark::Initialize(&argc, argv); benchmark::RunSpecifiedBenchmarks(); } + +bool SkipExpensiveBenchmarks() { return skip_expensive_benchmarks; } diff --git a/test/benchmark/main.h b/test/benchmark/main.h new file mode 100644 index 0000000000000..e08741e20f7f8 --- /dev/null +++ b/test/benchmark/main.h @@ -0,0 +1,7 @@ +#pragma once +// NOLINT(namespace-envoy) + +/** + * Benchmarks can use this to skip or hurry through long-running tests in CI. + */ +bool SkipExpensiveBenchmarks(); diff --git a/test/common/upstream/eds_speed_test.cc b/test/common/upstream/eds_speed_test.cc index ff8c0a57c7608..f7ccd9a4a9b77 100644 --- a/test/common/upstream/eds_speed_test.cc +++ b/test/common/upstream/eds_speed_test.cc @@ -16,6 +16,7 @@ #include "server/transport_socket_config_impl.h" +#include "test/benchmark/main.h" #include "test/common/upstream/utility.h" #include "test/mocks/local_info/mocks.h" #include "test/mocks/protobuf/mocks.h" @@ -162,8 +163,6 @@ class EdsSpeedTest { } // namespace Upstream } // namespace Envoy -extern bool g_skip_expensive_benchmarks; - static void priorityAndLocalityWeighted(benchmark::State& state) { Envoy::Thread::MutexBasicLockable lock; Envoy::Logger::Context logging_state(spdlog::level::warn, @@ -171,7 +170,7 @@ static void priorityAndLocalityWeighted(benchmark::State& state) { for (auto _ : state) { Envoy::Upstream::EdsSpeedTest speed_test(state, state.range(0)); // if we've been instructed to skip tests, only run once no matter the argument: - uint32_t endpoints = g_skip_expensive_benchmarks ? 1 : state.range(2); + uint32_t endpoints = SkipExpensiveBenchmarks() ? 1 : state.range(2); speed_test.priorityAndLocalityWeightedHelper(state.range(1), endpoints, true); } @@ -188,7 +187,7 @@ static void duplicateUpdate(benchmark::State& state) { for (auto _ : state) { Envoy::Upstream::EdsSpeedTest speed_test(state, false); - uint32_t endpoints = g_skip_expensive_benchmarks ? 1 : state.range(0); + uint32_t endpoints = SkipExpensiveBenchmarks() ? 1 : state.range(0); speed_test.priorityAndLocalityWeightedHelper(true, endpoints, true); speed_test.priorityAndLocalityWeightedHelper(true, endpoints, true); @@ -203,7 +202,7 @@ static void healthOnlyUpdate(benchmark::State& state) { Envoy::Logger::Logger::DEFAULT_LOG_FORMAT, lock, false); for (auto _ : state) { Envoy::Upstream::EdsSpeedTest speed_test(state, false); - uint32_t endpoints = g_skip_expensive_benchmarks ? 1 : state.range(0); + uint32_t endpoints = SkipExpensiveBenchmarks() ? 1 : state.range(0); speed_test.priorityAndLocalityWeightedHelper(true, endpoints, true); speed_test.priorityAndLocalityWeightedHelper(true, endpoints, false); From e99517dd3072211ac031f1d39898fcaf0d60bb62 Mon Sep 17 00:00:00 2001 From: Phil Genera Date: Mon, 29 Jun 2020 16:35:28 +0000 Subject: [PATCH 18/21] respond to comments Signed-off-by: Phil Genera --- test/benchmark/main.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/benchmark/main.cc b/test/benchmark/main.cc index 0c3ca4408b3ba..a785f1a51056b 100644 --- a/test/benchmark/main.cc +++ b/test/benchmark/main.cc @@ -7,7 +7,7 @@ #include "benchmark/benchmark.h" #include "tclap/CmdLine.h" -bool skip_expensive_benchmarks = false; +static bool skip_expensive_benchmarks = false; // Boilerplate main(), which discovers benchmarks and runs them. This uses two // different flag parsers, so the order of flags matters: flags defined here From 60e769f6e9d378411918515f41e68941678376a0 Mon Sep 17 00:00:00 2001 From: Phil Genera Date: Mon, 6 Jul 2020 13:00:24 +0000 Subject: [PATCH 19/21] respond to review comments Signed-off-by: Phil Genera --- test/benchmark/main.cc | 2 +- test/benchmark/main.h | 2 +- test/common/upstream/eds_speed_test.cc | 6 +++--- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/test/benchmark/main.cc b/test/benchmark/main.cc index a785f1a51056b..9226703d42b83 100644 --- a/test/benchmark/main.cc +++ b/test/benchmark/main.cc @@ -37,4 +37,4 @@ int main(int argc, char** argv) { benchmark::RunSpecifiedBenchmarks(); } -bool SkipExpensiveBenchmarks() { return skip_expensive_benchmarks; } +bool skipExpensiveBenchmarks() { return skip_expensive_benchmarks; } diff --git a/test/benchmark/main.h b/test/benchmark/main.h index e08741e20f7f8..06b33fda3c2c6 100644 --- a/test/benchmark/main.h +++ b/test/benchmark/main.h @@ -4,4 +4,4 @@ /** * Benchmarks can use this to skip or hurry through long-running tests in CI. */ -bool SkipExpensiveBenchmarks(); +bool skipExpensiveBenchmarks(); diff --git a/test/common/upstream/eds_speed_test.cc b/test/common/upstream/eds_speed_test.cc index f7ccd9a4a9b77..ab2cbfe98983c 100644 --- a/test/common/upstream/eds_speed_test.cc +++ b/test/common/upstream/eds_speed_test.cc @@ -170,7 +170,7 @@ static void priorityAndLocalityWeighted(benchmark::State& state) { for (auto _ : state) { Envoy::Upstream::EdsSpeedTest speed_test(state, state.range(0)); // if we've been instructed to skip tests, only run once no matter the argument: - uint32_t endpoints = SkipExpensiveBenchmarks() ? 1 : state.range(2); + uint32_t endpoints = skipExpensiveBenchmarks() ? 1 : state.range(2); speed_test.priorityAndLocalityWeightedHelper(state.range(1), endpoints, true); } @@ -187,7 +187,7 @@ static void duplicateUpdate(benchmark::State& state) { for (auto _ : state) { Envoy::Upstream::EdsSpeedTest speed_test(state, false); - uint32_t endpoints = SkipExpensiveBenchmarks() ? 1 : state.range(0); + uint32_t endpoints = skipExpensiveBenchmarks() ? 1 : state.range(0); speed_test.priorityAndLocalityWeightedHelper(true, endpoints, true); speed_test.priorityAndLocalityWeightedHelper(true, endpoints, true); @@ -202,7 +202,7 @@ static void healthOnlyUpdate(benchmark::State& state) { Envoy::Logger::Logger::DEFAULT_LOG_FORMAT, lock, false); for (auto _ : state) { Envoy::Upstream::EdsSpeedTest speed_test(state, false); - uint32_t endpoints = SkipExpensiveBenchmarks() ? 1 : state.range(0); + uint32_t endpoints = skipExpensiveBenchmarks() ? 1 : state.range(0); speed_test.priorityAndLocalityWeightedHelper(true, endpoints, true); speed_test.priorityAndLocalityWeightedHelper(true, endpoints, false); From 558423af26974fb0f44447d9b7a9ba3892890878 Mon Sep 17 00:00:00 2001 From: Phil Genera Date: Wed, 8 Jul 2020 18:32:01 +0000 Subject: [PATCH 20/21] respond to review comments Signed-off-by: Phil Genera --- test/benchmark/main.cc | 2 +- test/benchmark/main.h | 8 +++++++- test/common/upstream/eds_speed_test.cc | 13 ++++++++----- 3 files changed, 16 insertions(+), 7 deletions(-) diff --git a/test/benchmark/main.cc b/test/benchmark/main.cc index 9226703d42b83..6c23c1031a6c4 100644 --- a/test/benchmark/main.cc +++ b/test/benchmark/main.cc @@ -37,4 +37,4 @@ int main(int argc, char** argv) { benchmark::RunSpecifiedBenchmarks(); } -bool skipExpensiveBenchmarks() { return skip_expensive_benchmarks; } +bool Envoy::benchmark::skipExpensiveBenchmarks() { return skip_expensive_benchmarks; } diff --git a/test/benchmark/main.h b/test/benchmark/main.h index 06b33fda3c2c6..efb6797a74ef2 100644 --- a/test/benchmark/main.h +++ b/test/benchmark/main.h @@ -1,7 +1,13 @@ #pragma once -// NOLINT(namespace-envoy) /** * Benchmarks can use this to skip or hurry through long-running tests in CI. */ + +namespace Envoy { +namespace benchmark { + bool skipExpensiveBenchmarks(); + +} +} // namespace Envoy diff --git a/test/common/upstream/eds_speed_test.cc b/test/common/upstream/eds_speed_test.cc index cab2d0bb82a30..173ac81a90157 100644 --- a/test/common/upstream/eds_speed_test.cc +++ b/test/common/upstream/eds_speed_test.cc @@ -28,12 +28,15 @@ #include "benchmark/benchmark.h" +using ::benchmark::State; +using Envoy::benchmark::skipExpensiveBenchmarks; + namespace Envoy { namespace Upstream { class EdsSpeedTest { public: - EdsSpeedTest(benchmark::State& state, bool v2_config) + EdsSpeedTest(State& state, bool v2_config) : state_(state), v2_config_(v2_config), type_url_(v2_config_ ? "type.googleapis.com/envoy.api.v2.ClusterLoadAssignment" @@ -134,7 +137,7 @@ class EdsSpeedTest { num_hosts); } - benchmark::State& state_; + State& state_; const bool v2_config_; const std::string type_url_; bool initialized_{}; @@ -165,7 +168,7 @@ class EdsSpeedTest { } // namespace Upstream } // namespace Envoy -static void priorityAndLocalityWeighted(benchmark::State& state) { +static void priorityAndLocalityWeighted(State& state) { Envoy::Thread::MutexBasicLockable lock; Envoy::Logger::Context logging_state(spdlog::level::warn, Envoy::Logger::Logger::DEFAULT_LOG_FORMAT, lock, false); @@ -182,7 +185,7 @@ BENCHMARK(priorityAndLocalityWeighted) ->Ranges({{false, true}, {false, true}, {1, 100000}}) ->Unit(benchmark::kMillisecond); -static void duplicateUpdate(benchmark::State& state) { +static void duplicateUpdate(State& state) { Envoy::Thread::MutexBasicLockable lock; Envoy::Logger::Context logging_state(spdlog::level::warn, Envoy::Logger::Logger::DEFAULT_LOG_FORMAT, lock, false); @@ -198,7 +201,7 @@ static void duplicateUpdate(benchmark::State& state) { BENCHMARK(duplicateUpdate)->Range(1, 100000)->Unit(benchmark::kMillisecond); -static void healthOnlyUpdate(benchmark::State& state) { +static void healthOnlyUpdate(State& state) { Envoy::Thread::MutexBasicLockable lock; Envoy::Logger::Context logging_state(spdlog::level::warn, Envoy::Logger::Logger::DEFAULT_LOG_FORMAT, lock, false); From 2b689d187b01d7e18265295be40baabaa1b56f69 Mon Sep 17 00:00:00 2001 From: Phil Genera Date: Wed, 8 Jul 2020 20:13:57 +0000 Subject: [PATCH 21/21] Kick CI Signed-off-by: Phil Genera