From 3c70dc4c287ae79687c38dea772eeaa546b7b6c6 Mon Sep 17 00:00:00 2001 From: Dmitry Rozhkov Date: Wed, 3 Mar 2021 00:05:59 +0200 Subject: [PATCH 1/2] upstream: avoid quadratic time complexity in server initialization (#15237) Currently ClusterManagerInitHelper.secondary_init_clusters_ is a list. Upon every insert the list gets searched for an already added cluster with the same name. Since in normal circumstances all new clusters have unique names the worst case scenario is triggered and all elements of the list are checked sequentially upon every cluster added. Replace the list with a hash map. Signed-off-by: Dmitry Rozhkov --- .../common/upstream/cluster_manager_impl.cc | 32 ++++++++----------- source/common/upstream/cluster_manager_impl.h | 4 +-- .../upstream/cluster_manager_impl_test.cc | 7 ++++ 3 files changed, 22 insertions(+), 21 deletions(-) diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index aa360d1db3ced..04b466e67a93c 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -3,7 +3,6 @@ #include #include #include -#include #include #include #include @@ -67,20 +66,12 @@ void ClusterManagerInitHelper::addCluster(ClusterManagerCluster& cm_cluster) { Cluster& cluster = cm_cluster.cluster(); if (cluster.initializePhase() == Cluster::InitializePhase::Primary) { // Remove the previous cluster before the cluster object is destroyed. - primary_init_clusters_.remove_if( - [name_to_remove = cluster.info()->name()](ClusterManagerCluster* cluster_iter) { - return cluster_iter->cluster().info()->name() == name_to_remove; - }); - primary_init_clusters_.push_back(&cm_cluster); + primary_init_clusters_.insert_or_assign(cm_cluster.cluster().info()->name(), &cm_cluster); cluster.initialize(initialize_cb); } else { ASSERT(cluster.initializePhase() == Cluster::InitializePhase::Secondary); // Remove the previous cluster before the cluster object is destroyed. - secondary_init_clusters_.remove_if( - [name_to_remove = cluster.info()->name()](ClusterManagerCluster* cluster_iter) { - return cluster_iter->cluster().info()->name() == name_to_remove; - }); - secondary_init_clusters_.push_back(&cm_cluster); + secondary_init_clusters_.insert_or_assign(cm_cluster.cluster().info()->name(), &cm_cluster); if (started_secondary_initialize_) { // This can happen if we get a second CDS update that adds new clusters after we have // already started secondary init. In this case, just immediately initialize. @@ -105,17 +96,20 @@ void ClusterManagerInitHelper::removeCluster(ClusterManagerCluster& cluster) { // There is a remote edge case where we can remove a cluster via CDS that has not yet been // initialized. When called via the remove cluster API this code catches that case. - std::list* cluster_list; + absl::flat_hash_map* cluster_map; if (cluster.cluster().initializePhase() == Cluster::InitializePhase::Primary) { - cluster_list = &primary_init_clusters_; + cluster_map = &primary_init_clusters_; } else { ASSERT(cluster.cluster().initializePhase() == Cluster::InitializePhase::Secondary); - cluster_list = &secondary_init_clusters_; + cluster_map = &secondary_init_clusters_; } // It is possible that the cluster we are removing has already been initialized, and is not - // present in the initializer list. If so, this is fine. - cluster_list->remove(&cluster); + // present in the initializer map. If so, this is fine. + auto iter = cluster_map->find(cluster.cluster().info()->name()); + if (iter != cluster_map->end() && iter->second == &cluster) { + cluster_map->erase(iter); + } ENVOY_LOG(debug, "cm init: init complete: cluster={} primary={} secondary={}", cluster.cluster().info()->name(), primary_init_clusters_.size(), secondary_init_clusters_.size()); @@ -124,13 +118,13 @@ void ClusterManagerInitHelper::removeCluster(ClusterManagerCluster& cluster) { void ClusterManagerInitHelper::initializeSecondaryClusters() { started_secondary_initialize_ = true; - // Cluster::initialize() method can modify the list of secondary_init_clusters_ to remove + // Cluster::initialize() method can modify the map of secondary_init_clusters_ to remove // the item currently being initialized, so we eschew range-based-for and do this complicated // dance to increment the iterator before calling initialize. for (auto iter = secondary_init_clusters_.begin(); iter != secondary_init_clusters_.end();) { - ClusterManagerCluster* cluster = *iter; + ClusterManagerCluster* cluster = iter->second; + ENVOY_LOG(debug, "initializing secondary cluster {}", iter->first); ++iter; - ENVOY_LOG(debug, "initializing secondary cluster {}", cluster->cluster().info()->name()); cluster->cluster().initialize([cluster, this] { onClusterInit(*cluster); }); } } diff --git a/source/common/upstream/cluster_manager_impl.h b/source/common/upstream/cluster_manager_impl.h index 655616cf37e12..5f7fa6cdceedc 100644 --- a/source/common/upstream/cluster_manager_impl.h +++ b/source/common/upstream/cluster_manager_impl.h @@ -188,8 +188,8 @@ class ClusterManagerInitHelper : Logger::Loggable { CdsApi* cds_{}; ClusterManager::PrimaryClustersReadyCallback primary_clusters_initialized_callback_; ClusterManager::InitializationCompleteCallback initialized_callback_; - std::list primary_init_clusters_; - std::list secondary_init_clusters_; + absl::flat_hash_map primary_init_clusters_; + absl::flat_hash_map secondary_init_clusters_; State state_{State::Loading}; bool started_secondary_initialize_{}; }; diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index 08b3d01968e8c..cd8e167b29a78 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -3318,6 +3318,13 @@ TEST_F(ClusterManagerInitHelperTest, UpdateAlreadyInitialized) { init_helper_.startInitializingSecondaryClusters(); } +TEST_F(ClusterManagerInitHelperTest, RemoveUnknown) { + InSequence s; + + NiceMock cluster; + init_helper_.removeCluster(cluster); +} + // If secondary clusters initialization triggered outside of CdsApiImpl::onConfigUpdate()'s // callback flows, sending ClusterLoadAssignment should not be paused before calling // ClusterManagerInitHelper::maybeFinishInitialize(). This case tests that From 4b30e58158f20d6cf835853c70ea4a085d728791 Mon Sep 17 00:00:00 2001 From: Dmitry Rozhkov Date: Mon, 8 Mar 2021 09:42:03 +0000 Subject: [PATCH 2/2] add comment on CDS update Signed-off-by: Dmitry Rozhkov --- source/common/upstream/cluster_manager_impl.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index 04b466e67a93c..dc731e6038b3b 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -105,7 +105,9 @@ void ClusterManagerInitHelper::removeCluster(ClusterManagerCluster& cluster) { } // It is possible that the cluster we are removing has already been initialized, and is not - // present in the initializer map. If so, this is fine. + // present in the initializer map. If so, this is fine as a CDS update may happen for a + // cluster with the same name. See the case "UpdateAlreadyInitialized" of the + // target //test/common/upstream:cluster_manager_impl_test. auto iter = cluster_map->find(cluster.cluster().info()->name()); if (iter != cluster_map->end() && iter->second == &cluster) { cluster_map->erase(iter);