Replace std::unordered_map/set with absl containers (envoyproxy#11879)

- Replace with absl::node_hash_map/set - Primarily for performance optimizations and to root out any assumptions made about iteration order in tests or otherwise (the replacement absl containers have a non-deterministic iteration order - absl::node_hash_map/set should be drop-in replacements for std::unordered_map/set - Note that a future refactor should reevaluate and move to absl::flat_hash_map/set where possible for memory optimizations - Add format check to disallow future usage of std::unordered_map/set - Small changes made where absl containers required it or tests needed to be modified for correctness Additional Description: - There may be an issue we should open with abseil about `emplace` and `try_emplace` when attempting to do in-place construction - When a constructor throws an exception, as far as I can tell the c++ language standard says the container should not be affected, however this does not seem to be the case for the absl containers so their guarantees are not the same (though they may be intended to have the same guarantees) - //test/server:overload_manager_impl_test demonstrates this - see abseil/abseil-cpp#388 Risk Level: Low, absl::node_hash_map/set should be drop-in replacements for std::unordered_map/set though this may shake loose more assumptions in tests over time we weren't able to catch locally Testing: Small changes to unit tests, repeatedly run on Windows and Linux/clang Docs Changes: N/A Release Notes: N/A Fixes envoyproxy#11825 Signed-off-by: Sunjay Bhatia <[email protected]> Co-authored-by: William A Rowe Jr <[email protected]>
ruimya · Jul 28, 2020 · cf2df8c · cf2df8c
1 parent 8f668e9
commit cf2df8c
Show file tree

Hide file tree

Showing 174 changed files with 494 additions and 442 deletions.
diff --git a/include/envoy/grpc/status.h b/include/envoy/grpc/status.h
@@ -9,8 +9,6 @@ class Status {
 public:
   using GrpcStatus = int64_t;
 
-  // If this enum is changed, then the std::unordered_map in Envoy::Grpc::Utility::nameToGrpcStatus
-  // located at: //source/common/access_log/grpc/status.cc must also be changed.
   enum WellKnownGrpcStatus {
     // The RPC completed successfully.
     Ok = 0,

diff --git a/include/envoy/http/BUILD b/include/envoy/http/BUILD
@@ -128,6 +128,7 @@ envoy_cc_library(
 envoy_cc_library(
     name = "metadata_interface",
     hdrs = ["metadata_interface.h"],
+    external_deps = ["abseil_node_hash_map"],
 )
 
 envoy_cc_library(

diff --git a/include/envoy/http/header_map.h b/include/envoy/http/header_map.h
@@ -6,7 +6,6 @@
 #include <iostream>
 #include <memory>
 #include <string>
-#include <unordered_set>
 #include <vector>
 
 #include "envoy/common/pure.h"

diff --git a/include/envoy/http/metadata_interface.h b/include/envoy/http/metadata_interface.h
@@ -3,9 +3,10 @@
 #include <functional>
 #include <iostream>
 #include <memory>
-#include <unordered_map>
 #include <vector>
 
+#include "absl/container/node_hash_map.h"
+
 namespace Envoy {
 namespace Http {
 
@@ -20,7 +21,7 @@ constexpr uint8_t END_METADATA_FLAG = 0x4;
 // TODO(soya3129): Respect max_frame_size after nghttp2 #1250 is resolved.
 constexpr uint64_t METADATA_MAX_PAYLOAD_SIZE = 16384;
 
-using UnorderedStringMap = std::unordered_map<std::string, std::string>;
+using UnorderedStringMap = absl::node_hash_map<std::string, std::string>;
 
 class MetadataMap : public UnorderedStringMap {
 public:

diff --git a/include/envoy/runtime/BUILD b/include/envoy/runtime/BUILD
@@ -11,7 +11,10 @@ envoy_package()
 envoy_cc_library(
     name = "runtime_interface",
     hdrs = ["runtime.h"],
-    external_deps = ["abseil_optional"],
+    external_deps = [
+        "abseil_node_hash_map",
+        "abseil_optional",
+    ],
     deps = [
         "//include/envoy/stats:stats_interface",
         "//include/envoy/thread_local:thread_local_interface",

diff --git a/include/envoy/runtime/runtime.h b/include/envoy/runtime/runtime.h
@@ -5,7 +5,6 @@
 #include <limits>
 #include <memory>
 #include <string>
-#include <unordered_map>
 #include <vector>
 
 #include "envoy/common/pure.h"
@@ -17,6 +16,7 @@
 #include "common/singleton/threadsafe_singleton.h"
 
 #include "absl/container/flat_hash_map.h"
+#include "absl/container/node_hash_map.h"
 #include "absl/types/optional.h"
 
 namespace Envoy {
@@ -253,7 +253,7 @@ class Loader {
    * a key, use an empty string as the value.
    * @param values the values to merge
    */
-  virtual void mergeValues(const std::unordered_map<std::string, std::string>& values) PURE;
+  virtual void mergeValues(const absl::node_hash_map<std::string, std::string>& values) PURE;
 
   /**
    * Initiate all RTDS subscriptions. The `on_done` callback is invoked when all RTDS requests

diff --git a/include/envoy/server/overload_manager.h b/include/envoy/server/overload_manager.h
@@ -1,7 +1,6 @@
 #pragma once
 
 #include <string>
-#include <unordered_map>
 
 #include "envoy/common/pure.h"
 #include "envoy/thread_local/thread_local.h"

diff --git a/include/envoy/upstream/BUILD b/include/envoy/upstream/BUILD
@@ -11,6 +11,9 @@ envoy_package()
 envoy_cc_library(
     name = "cluster_manager_interface",
     hdrs = ["cluster_manager.h"],
+    external_deps = [
+        "abseil_node_hash_map",
+    ],
     deps = [
         ":health_checker_interface",
         ":load_balancer_interface",

diff --git a/include/envoy/upstream/cluster_manager.h b/include/envoy/upstream/cluster_manager.h
@@ -4,7 +4,6 @@
 #include <functional>
 #include <memory>
 #include <string>
-#include <unordered_map>
 
 #include "envoy/access_log/access_log.h"
 #include "envoy/api/api.h"
@@ -32,6 +31,9 @@
 #include "envoy/upstream/thread_local_cluster.h"
 #include "envoy/upstream/upstream.h"
 
+#include "absl/container/flat_hash_set.h"
+#include "absl/container/node_hash_map.h"
+
 namespace Envoy {
 namespace Upstream {
 
@@ -123,15 +125,15 @@ class ClusterManager {
   virtual void
   initializeSecondaryClusters(const envoy::config::bootstrap::v3::Bootstrap& bootstrap) PURE;
 
-  using ClusterInfoMap = std::unordered_map<std::string, std::reference_wrapper<const Cluster>>;
+  using ClusterInfoMap = absl::node_hash_map<std::string, std::reference_wrapper<const Cluster>>;
 
   /**
    * @return ClusterInfoMap all current clusters. These are the primary (not thread local)
    * clusters which should only be used for stats/admin.
    */
   virtual ClusterInfoMap clusters() PURE;
 
-  using ClusterSet = std::unordered_set<std::string>;
+  using ClusterSet = absl::flat_hash_set<std::string>;
 
   /**
    * @return const ClusterSet& providing the cluster names that are eligible as

diff --git a/include/envoy/upstream/upstream.h b/include/envoy/upstream/upstream.h
@@ -211,7 +211,7 @@ using HostVector = std::vector<HostSharedPtr>;
 using HealthyHostVector = Phantom<HostVector, Healthy>;
 using DegradedHostVector = Phantom<HostVector, Degraded>;
 using ExcludedHostVector = Phantom<HostVector, Excluded>;
-using HostMap = std::unordered_map<std::string, Upstream::HostSharedPtr>;
+using HostMap = absl::node_hash_map<std::string, Upstream::HostSharedPtr>;
 using HostVectorSharedPtr = std::shared_ptr<HostVector>;
 using HostVectorConstSharedPtr = std::shared_ptr<const HostVector>;
 
@@ -221,7 +221,7 @@ using ExcludedHostVectorConstSharedPtr = std::shared_ptr<const ExcludedHostVecto
 
 using HostListPtr = std::unique_ptr<HostVector>;
 using LocalityWeightsMap =
-    std::unordered_map<envoy::config::core::v3::Locality, uint32_t, LocalityHash, LocalityEqualTo>;
+    absl::node_hash_map<envoy::config::core::v3::Locality, uint32_t, LocalityHash, LocalityEqualTo>;
 using PriorityState = std::vector<std::pair<HostListPtr, LocalityWeightsMap>>;
 
 /**

diff --git a/source/common/access_log/BUILD b/source/common/access_log/BUILD
@@ -12,7 +12,9 @@ envoy_cc_library(
     name = "access_log_lib",
     srcs = ["access_log_impl.cc"],
     hdrs = ["access_log_impl.h"],
-    external_deps = ["abseil_hash"],
+    external_deps = [
+        "abseil_hash",
+    ],
     deps = [
         "//include/envoy/access_log:access_log_interface",
         "//include/envoy/config:typed_config_interface",

diff --git a/source/common/access_log/access_log_impl.h b/source/common/access_log/access_log_impl.h
@@ -2,7 +2,6 @@
 
 #include <cstdint>
 #include <string>
-#include <unordered_set>
 #include <vector>
 
 #include "envoy/access_log/access_log.h"
@@ -17,6 +16,7 @@
 #include "common/http/header_utility.h"
 #include "common/protobuf/protobuf.h"
 
+#include "absl/container/node_hash_set.h"
 #include "absl/hash/hash.h"
 
 namespace Envoy {
@@ -207,7 +207,7 @@ class ResponseFlagFilter : public Filter {
 class GrpcStatusFilter : public Filter {
 public:
   using GrpcStatusHashSet =
-      std::unordered_set<Grpc::Status::GrpcStatus, absl::Hash<Grpc::Status::GrpcStatus>>;
+      absl::node_hash_set<Grpc::Status::GrpcStatus, absl::Hash<Grpc::Status::GrpcStatus>>;
 
   GrpcStatusFilter(const envoy::config::accesslog::v3::GrpcStatusFilter& config);
 

diff --git a/source/common/access_log/access_log_manager_impl.h b/source/common/access_log/access_log_manager_impl.h
@@ -1,7 +1,6 @@
 #pragma once
 
 #include <string>
-#include <unordered_map>
 
 #include "envoy/access_log/access_log.h"
 #include "envoy/api/api.h"
@@ -14,6 +13,8 @@
 #include "common/common/logger.h"
 #include "common/common/thread.h"
 
+#include "absl/container/node_hash_map.h"
+
 namespace Envoy {
 
 #define ACCESS_LOG_FILE_STATS(COUNTER, GAUGE)                                                      \
@@ -51,7 +52,7 @@ class AccessLogManagerImpl : public AccessLogManager, Logger::Loggable<Logger::I
   Event::Dispatcher& dispatcher_;
   Thread::BasicLockable& lock_;
   AccessLogFileStats file_stats_;
-  std::unordered_map<std::string, AccessLogFileSharedPtr> access_logs_;
+  absl::node_hash_map<std::string, AccessLogFileSharedPtr> access_logs_;
 };
 
 /**

diff --git a/source/common/common/BUILD b/source/common/common/BUILD
@@ -347,6 +347,7 @@ envoy_cc_library(
     name = "utility_lib",
     srcs = ["utility.cc"],
     hdrs = ["utility.h"],
+    external_deps = ["abseil_node_hash_map"],
     deps = [
         ":assert_lib",
         ":hash_lib",

diff --git a/source/common/common/hash.h b/source/common/common/hash.h
@@ -1,8 +1,6 @@
 #pragma once
 
 #include <string>
-#include <unordered_map>
-#include <unordered_set>
 
 #include "absl/container/flat_hash_map.h"
 #include "absl/container/flat_hash_set.h"

diff --git a/source/common/common/perf_annotation.h b/source/common/common/perf_annotation.h
@@ -4,11 +4,11 @@
 
 #include <chrono>
 #include <cstdint>
-#include <unordered_map>
 
 #include "common/common/thread.h"
 #include "common/common/utility.h"
 
+#include "absl/container/node_hash_map.h"
 #include "absl/strings/string_view.h"
 
 // Performance Annotation system, enabled with
@@ -139,7 +139,7 @@ class PerfAnnotationContext {
     }
   };
 
-  using DurationStatsMap = std::unordered_map<CategoryDescription, DurationStats, Hash>;
+  using DurationStatsMap = absl::node_hash_map<CategoryDescription, DurationStats, Hash>;
 
   // Maps {category, description} to DurationStats.
 #if PERF_THREAD_SAFE

diff --git a/source/common/common/utility.cc b/source/common/common/utility.cc
@@ -15,6 +15,7 @@
 #include "common/common/hash.h"
 #include "common/singleton/const_singleton.h"
 
+#include "absl/container/node_hash_map.h"
 #include "absl/strings/ascii.h"
 #include "absl/strings/match.h"
 #include "absl/strings/str_join.h"
@@ -85,7 +86,7 @@ std::string DateFormatter::fromTime(const SystemTime& time) const {
       SpecifierOffsets specifier_offsets;
     };
     // A map is used to keep different formatted format strings at a given second.
-    std::unordered_map<std::string, const Formatted> formatted;
+    absl::node_hash_map<std::string, const Formatted> formatted;
   };
   static thread_local CachedTime cached_time;
 
@@ -101,9 +102,11 @@ std::string DateFormatter::fromTime(const SystemTime& time) const {
     // Remove all the expired cached items.
     for (auto it = cached_time.formatted.cbegin(); it != cached_time.formatted.cend();) {
       if (it->second.epoch_time_seconds != epoch_time_seconds) {
-        it = cached_time.formatted.erase(it);
+        auto next_it = std::next(it);
+        cached_time.formatted.erase(it);
+        it = next_it;
       } else {
-        it++;
+        ++it;
       }
     }
 

diff --git a/source/common/common/utility.h b/source/common/common/utility.h
@@ -5,7 +5,6 @@
 #include <set>
 #include <sstream>
 #include <string>
-#include <unordered_set>
 #include <vector>
 
 #include "envoy/common/interval_set.h"

diff --git a/source/common/config/config_provider_impl.h b/source/common/config/config_provider_impl.h
@@ -391,10 +391,10 @@ class ConfigProviderManagerImplBase : public ConfigProviderManager, public Singl
 protected:
   // Ordered set for deterministic config dump output.
   using ConfigProviderSet = std::set<ConfigProvider*>;
-  using ConfigProviderMap = std::unordered_map<ConfigProviderInstanceType,
-                                               std::unique_ptr<ConfigProviderSet>, EnumClassHash>;
+  using ConfigProviderMap = absl::node_hash_map<ConfigProviderInstanceType,
+                                                std::unique_ptr<ConfigProviderSet>, EnumClassHash>;
   using ConfigSubscriptionMap =
-      std::unordered_map<uint64_t, std::weak_ptr<ConfigSubscriptionCommonBase>>;
+      absl::node_hash_map<uint64_t, std::weak_ptr<ConfigSubscriptionCommonBase>>;
 
   ConfigProviderManagerImplBase(Server::Admin& admin, const std::string& config_name);
 

diff --git a/source/common/config/delta_subscription_state.h b/source/common/config/delta_subscription_state.h
@@ -13,6 +13,8 @@
 #include "common/config/pausable_ack_queue.h"
 #include "common/config/watch_map.h"
 
+#include "absl/container/node_hash_map.h"
+
 namespace Envoy {
 namespace Config {
 
@@ -81,7 +83,7 @@ class DeltaSubscriptionState : public Logger::Loggable<Logger::Id::config> {
   // names we are currently interested in. Those in the waitingForServer state currently don't have
   // any version for that resource: we need to inform the server if we lose interest in them, but we
   // also need to *not* include them in the initial_resource_versions map upon a reconnect.
-  std::unordered_map<std::string, ResourceVersion> resource_versions_;
+  absl::node_hash_map<std::string, ResourceVersion> resource_versions_;
   // The keys of resource_versions_. Only tracked separately because std::map does not provide an
   // iterator into just its keys, e.g. for use in std::set_difference.
   std::set<std::string> resource_names_;
@@ -94,8 +96,8 @@ class DeltaSubscriptionState : public Logger::Loggable<Logger::Id::config> {
   bool any_request_sent_yet_in_current_stream_{};
 
   // Tracks changes in our subscription interest since the previous DeltaDiscoveryRequest we sent.
-  // Can't use unordered_set due to ordering issues in gTest expectation matching.
-  // Feel free to change to unordered if you can figure out how to make it work.
+  // TODO: Can't use absl::flat_hash_set due to ordering issues in gTest expectation matching.
+  // Feel free to change to an unordered container once we figure out how to make it work.
   std::set<std::string> names_added_;
   std::set<std::string> names_removed_;
 };

diff --git a/source/common/config/grpc_mux_impl.cc b/source/common/config/grpc_mux_impl.cc
@@ -1,7 +1,5 @@
 #include "common/config/grpc_mux_impl.h"
 
-#include <unordered_set>
-
 #include "envoy/service/discovery/v3/discovery.pb.h"
 
 #include "common/config/decoded_resource_impl.h"
@@ -11,6 +9,7 @@
 #include "common/protobuf/protobuf.h"
 
 #include "absl/container/btree_map.h"
+#include "absl/container/node_hash_set.h"
 
 namespace Envoy {
 namespace Config {
@@ -36,7 +35,7 @@ void GrpcMuxImpl::sendDiscoveryRequest(const std::string& type_url) {
   request.mutable_resource_names()->Clear();
 
   // Maintain a set to avoid dupes.
-  std::unordered_set<std::string> resources;
+  absl::node_hash_set<std::string> resources;
   for (const auto* watch : api_state.watches_) {
     for (const std::string& resource : watch->resources_) {
       if (resources.count(resource) == 0) {

diff --git a/source/common/config/grpc_mux_impl.h b/source/common/config/grpc_mux_impl.h
@@ -3,7 +3,6 @@
 #include <cstdint>
 #include <memory>
 #include <queue>
-#include <unordered_map>
 
 #include "envoy/api/v2/discovery.pb.h"
 #include "envoy/common/random_generator.h"
@@ -21,6 +20,8 @@
 #include "common/config/grpc_stream.h"
 #include "common/config/utility.h"
 
+#include "absl/container/node_hash_map.h"
+
 namespace Envoy {
 namespace Config {
 /**
@@ -131,7 +132,7 @@ class GrpcMuxImpl : public GrpcMux,
   const LocalInfo::LocalInfo& local_info_;
   const bool skip_subsequent_node_;
   bool first_stream_request_;
-  std::unordered_map<std::string, ApiState> api_state_;
+  absl::node_hash_map<std::string, ApiState> api_state_;
   // Envoy's dependency ordering.
   std::list<std::string> subscriptions_;