From 34ecb8a7a22aa675c9621b648af4cae2c282483a Mon Sep 17 00:00:00 2001 From: wbpcode Date: Mon, 12 Jun 2023 13:54:39 +0000 Subject: [PATCH 01/36] common inline map registry: complete initial version Signed-off-by: wbpcode --- envoy/common/BUILD | 15 + envoy/common/inline_map_registry.h | 402 ++++++++++++++++++ test/common/common/BUILD | 12 + .../common/inline_map_registry_speed_test.cc | 74 ++++ 4 files changed, 503 insertions(+) create mode 100644 envoy/common/inline_map_registry.h create mode 100644 test/common/common/inline_map_registry_speed_test.cc diff --git a/envoy/common/BUILD b/envoy/common/BUILD index 1a71c7f609689..8823ce31eb269 100644 --- a/envoy/common/BUILD +++ b/envoy/common/BUILD @@ -121,3 +121,18 @@ envoy_cc_library( "//source/common/common:utility_lib", ], ) + +envoy_cc_library( + name = "inline_map_registry", + hdrs = [ + "inline_map_registry.h", + ], + external_deps = [ + "abseil_node_hash_map", + ], + deps = [ + "//source/common/common:assert_lib", + "//source/common/common:macros", + "//source/common/common:utility_lib", + ], +) diff --git a/envoy/common/inline_map_registry.h b/envoy/common/inline_map_registry.h new file mode 100644 index 0000000000000..3c8a0da9bd40f --- /dev/null +++ b/envoy/common/inline_map_registry.h @@ -0,0 +1,402 @@ +#pragma once + +#include "source/common/common/assert.h" +#include "source/common/common/macros.h" +#include "source/common/common/utility.h" + +#include "absl/container/flat_hash_map.h" + +namespace Envoy { + +class UntypedInlineHandle { +public: + UntypedInlineHandle(uint64_t inline_scope_id, uint64_t inline_entry_id, + absl::string_view inline_entry_key) + : inline_scope_id_(inline_scope_id), inline_entry_id_(inline_entry_id), + inline_entry_key_(inline_entry_key) {} + + uint64_t inlineScopeId() const { return inline_scope_id_; } + + uint64_t inlineEntryId() const { return inline_entry_id_; } + + absl::string_view inlineEntryKey() const { return inline_entry_key_; } + +private: + uint64_t inline_scope_id_{}; + uint64_t inline_entry_id_{}; + absl::string_view inline_entry_key_; +}; + +/** + * Information about the registry. This is used to print out the registry debug information. + */ +struct InlineMapRegistryDebugInfo { + // Registry scope id for the registry, this is generated dynamically by the + // InlineMapRegistryManager. + uint64_t registry_scope_id; + // Registry scope name for the registry, typically short string from + // scope type. + absl::string_view registry_scope_name; + // All inline keys registered for the registry. + std::vector registry_inline_keys; +}; + +class InlineMapRegistryManager { +public: + static const std::vector& registryInfos() { + static bool finalized = false; + + // Call finalize() to ensure that all finalizers are called. + if (!finalized) { + finalize(); + finalized = true; + } + + return mutableRegistryInfos(); + } + + /** + * Generate a scope id for the given scope and register a finalizer to be called when + * InlineMapRegistry::finalize() is called. + * + * NOTE: This is called by InlineMapRegistry::scopeId() and should never be called + * manually. + */ + template static uint64_t generateScopeId(); + +private: + using Finalizer = std::function; + + /** + * Call all finalizers. This should only be called once, after all registrations are + * complete. + */ + static void finalize() { + while (!mutableFinalizers().empty()) { + mutableRegistryInfos().push_back(mutableFinalizers().back()()); + mutableFinalizers().pop_back(); + } + } + + static std::vector& mutableRegistryInfos() { + MUTABLE_CONSTRUCT_ON_FIRST_USE(std::vector); + } + + static std::vector& mutableFinalizers() { + MUTABLE_CONSTRUCT_ON_FIRST_USE(std::vector); + } +}; + +template class InlineMapRegistry { +public: + class InlineHandle { + public: + uint64_t inlineEntryId() const { return inline_entry_id_; } + + absl::string_view inlineEntryKey() const { return inline_entry_key_; } + + private: + friend class InlineMapRegistry; + + // This constructor should only be called by InlineMapRegistry. + InlineHandle(uint64_t inline_entry_id, absl::string_view inline_entry_key) + : inline_entry_id_(inline_entry_id), inline_entry_key_(inline_entry_key) {} + + uint64_t inline_entry_id_{}; + absl::string_view inline_entry_key_; + }; + + using RegistrationMap = absl::flat_hash_map; + + template class InlineMap : public InlineStorage { + public: + using TPtr = std::unique_ptr; + using RawT = T*; + using ConstRawT = const T*; + using HashMap = absl::flat_hash_map; + + // Get the entry for the given key. If the key is not found, return nullptr. + RawT lookup(absl::string_view key) const { + // Compute the hash value for the key and avoid computing it again in the lookup. + const size_t hash_value = absl::Hash()(key); + + if (auto entry_id = staticLookup(key, hash_value); entry_id.has_value()) { + return inline_entries_[*entry_id]; + } + + if (auto it = map_.find(key, hash_value); it != map_.end()) { + return it->second.get(); + } + + return nullptr; + } + + // Get the entry for the given handle. If the handle is not valid, return nullptr. + RawT lookup(InlineHandle handle) const { + ASSERT(handle.inlineEntryId() < InlineMapRegistry::registrationMapSize()); + return inline_entries_[handle.inlineEntryId()]; + } + + // Get the entry for the given untyped handle. If the handle is not valid, return nullptr. + RawT lookup(UntypedInlineHandle handle) const { + // If the scope id does not match, the handle is not valid. + if (handle.inlineScopeId() != InlineMapRegistry::scopeId()) { + return nullptr; + } + + // If the entry id is valid, it is an inline entry. + if (handle.inlineEntryId() < InlineMapRegistry::registrationMapSize()) { + return inline_entries_[handle.inlineEntryId()]; + } + + // Otherwise, try normal map entry. + if (auto it = map_.find(handle.inlineEntryKey()); it != map_.end()) { + return it->second.get(); + } + return nullptr; + } + + // Set the entry for the given key. If the key is already present, overwrite it. + void insert(absl::string_view key, TPtr value) { + // Compute the hash value for the key and avoid computing it again in the lookup. + const size_t hash_value = absl::Hash()(key); + + if (auto entry_id = staticLookup(key, hash_value); entry_id.has_value()) { + resetInlineMapEntry(*entry_id, std::move(value)); + } else { + map_[key] = std::move(value); + } + } + + // Set the entry for the given handle. If the handle is not valid, do nothing. + void insert(InlineHandle handle, TPtr value) { + ASSERT(handle.inlineEntryId() < InlineMapRegistry::registrationMapSize()); + resetInlineMapEntry(handle.inlineEntryId(), std::move(value)); + } + + // Set the entry for the given untyped handle. If the handle is not valid, do nothing. + void insert(UntypedInlineHandle handle, TPtr value) { + // If the scope id does not match, the handle is not valid. + if (handle.inlineScopeId() != InlineMapRegistry::scopeId()) { + return; + } + + // If the entry id is valid, it is an inline entry. + if (handle.inlineEntryId() < InlineMapRegistry::registrationMapSize()) { + resetInlineMapEntry(handle.inlineEntryId(), std::move(value)); + return; + } + + // Otherwise, try normal map entry. + map_[handle.inlineEntryKey()] = std::move(value); + } + + // Remove the entry for the given key. If the key is not found, do nothing. + void remove(absl::string_view key) { + // Compute the hash value for the key and avoid computing it again in the lookup. + const size_t hash_value = absl::Hash()(key); + + if (auto entry_id = staticLookup(key, hash_value); entry_id.has_value()) { + resetInlineMapEntry(*entry_id); + } else { + map_.erase(key); + } + } + + // Remove the entry for the given handle. If the handle is not valid, do nothing. + void remove(InlineHandle handle) { resetInlineMapEntry(handle.inlineEntryId()); } + + // Remove the entry for the given untyped handle. If the handle is not valid, do nothing. + void remove(UntypedInlineHandle handle) { + // If the scope id does not match, the handle is not valid. + if (handle.inlineScopeId() != InlineMapRegistry::scopeId()) { + return; + } + + // If the entry id is valid, it is an inline entry. + if (handle.inlineEntryId() < InlineMapRegistry::registrationMapSize()) { + resetInlineMapEntry(handle.inlineEntryId()); + return; + } + + // Otherwise, try normal map entry. + map_.erase(handle.inlineEntryKey()); + } + + void iterate(std::function callback) const { + for (const auto& entry : map_) { + if (!callback(entry.first, entry.second.get())) { + return; + } + } + + for (const auto& id : InlineMapRegistry::registrationMap()) { + ASSERT(id.second.inlineEntryId() < InlineMapRegistry::registrationMapSize()); + + auto entry = inline_entries_[id.second.inlineEntryId()]; + if (entry == nullptr) { + continue; + } + + if (!callback(id.second.inlineEntryKey(), entry.get())) { + return; + } + } + } + + static std::unique_ptr createInlineMap() { + return std::unique_ptr( + new ((InlineMapRegistry::registrationMapSize() * sizeof(TPtr))) InlineMap()); + } + + private: + InlineMap() { + memset(inline_entries_, 0, InlineMapRegistry::registrationMapSize() * sizeof(TPtr)); + } + + void resetInlineMapEntry(uint64_t inline_entry_id, TPtr new_entry = nullptr) { + ASSERT(inline_entry_id < InlineMapRegistry::registrationMapSize()); + + if (auto entry = inline_entries_[inline_entry_id]; entry != nullptr) { + delete entry; + } + inline_entries_[inline_entry_id] = new_entry.release(); + } + + absl::optional staticLookup(absl::string_view key, size_t hash_value) const { + if (auto iter = InlineMapRegistry::registrationMap().find(key, hash_value); + iter != InlineMapRegistry::registrationMap().end()) { + return iter->second.inlineEntryId(); + } + return absl::nullopt; + } + + HashMap map_; + RawT inline_entries_[]; + }; + + /** + * Register a custom inline key. May only be called before finalized(). + */ + static InlineHandle registerInlineKey(absl::string_view key) { + RELEASE_ASSERT(!mutableFinalized(), "Cannot register inline key after finalized()"); + + // Initialize the custom inline key registrations. If this is the first time this function + // is called, this will generate a scope id and register a finalizer to the manager. + // Otherwise, this will be a no-op. + initialize(); + + // If the key is already registered, return the existing handle. + if (auto it = mutableRegistrationMap().find(key); it != mutableRegistrationMap().end()) { + return it->second; + } + + InlineHandle handle{mutableRegistrationMap().size(), key}; + mutableRegistrationMap().emplace(key, handle); + return handle; + } + + /** + * Fetch all registered key. May only be called after finalized(). + */ + static const RegistrationMap& registrationMap() { + ASSERT(mutableFinalized(), "Cannot fetch registration map before finalized()"); + return mutableRegistrationMap(); + } + + /** + * Fetch the number of registered keys. This will call finalize() automatically to ensure that + * no further changes are allowed. + */ + static uint64_t registrationMapSize() { + static uint64_t size = []() { + finalize(); + return mutableRegistrationMap().size(); + }(); + + return size; + } + + /** + * Get the scope id for this registry. When called for the first time, this will generate a + * id and register a finalizer to the InlineMapRegistryManager. + */ + static uint64_t scopeId() { + // Scope id is generated on first use. Static ensures that it is only generated once. + static uint64_t scope_id = InlineMapRegistryManager::generateScopeId(); + return scope_id; + } + +private: + friend class InlineMapRegistryManager; + + /** + * Finalize the custom inline key registrations. No further changes are allowed after this + * point. This guaranteed that all map created by the process have the same variable size and + * custom registrations. This function could be called multiple times safely and only the first + * call will have effect. + * + * NOTE: This should only be called by InlineMapRegistryManager with the finalizer or be called + * in the registrationMapSize. + */ + static InlineMapRegistryDebugInfo finalize() { + // Initialize the custom inline key registrations. If the initialize() or scopeId() is never + // called before, this call here will generate a scope id and register a finalizer to the + // manager to ensure that the InlineMapRegistryManager always could get all the debug info. + // If the initialize() or scopeId() is already called before, this will be a no-op. + initialize(); + + // Mark the registry as finalized to ensure that no further changes are allowed. + mutableFinalized() = true; + return debugInfo(); + } + + /** + * Initialize the custom inline key registrations. This may be called multiple times but only + * the first call will have effect and the rest will be no-op. + */ + static void initialize() { + // Call scopeId() to ensure that the scope is initialized. + scopeId(); + } + + static bool& mutableFinalized() { MUTABLE_CONSTRUCT_ON_FIRST_USE(bool); } + + static RegistrationMap& mutableRegistrationMap() { + MUTABLE_CONSTRUCT_ON_FIRST_USE(RegistrationMap); + } + + static InlineMapRegistryDebugInfo debugInfo() { + RELEASE_ASSERT(mutableFinalized(), "Cannot fetch debug info before finalized()"); + + std::vector all_inline_keys; + all_inline_keys.reserve(mutableRegistrationMap().size()); + for (const auto& entry : mutableRegistrationMap()) { + all_inline_keys.push_back(absl::string_view(entry.first)); + } + + return {scopeId(), Scope::name(), std::move(all_inline_keys)}; + } +}; + +template uint64_t InlineMapRegistryManager::generateScopeId() { + // This function should never be called multiple times for same scope. + static bool initialized = false; + RELEASE_ASSERT(!initialized, + "InlineMapRegistryManager::generateScopeId() called twice for same scope"); + + const uint64_t scope_id = mutableFinalizers().size(); + mutableFinalizers().push_back([scope_id]() -> InlineMapRegistryDebugInfo { + using Registry = InlineMapRegistry; + ASSERT(Registry::scopeId() == scope_id); + return Registry::finalize(); + }); + + // Mark the scope as initialized and the RELEASE_ASSERT will crash Envoy + // if this function is called twice. + initialized = true; + + return scope_id; +} + +} // namespace Envoy diff --git a/test/common/common/BUILD b/test/common/common/BUILD index 74f86504f154a..d428f710af9a7 100644 --- a/test/common/common/BUILD +++ b/test/common/common/BUILD @@ -492,3 +492,15 @@ envoy_cc_test( "//source/common/common:random_generator_lib", ], ) + +envoy_cc_benchmark_binary( + name = "inline_map_registry_speed_test", + srcs = ["inline_map_registry_speed_test.cc"], + external_deps = ["benchmark"], + deps = ["//envoy/common:inline_map_registry"], +) + +envoy_benchmark_test( + name = "inline_map_registry_speed_test_benchmark_test", + benchmark_binary = "inline_map_registry_speed_test", +) diff --git a/test/common/common/inline_map_registry_speed_test.cc b/test/common/common/inline_map_registry_speed_test.cc new file mode 100644 index 0000000000000..441f73186b3d1 --- /dev/null +++ b/test/common/common/inline_map_registry_speed_test.cc @@ -0,0 +1,74 @@ +#include "envoy/common/inline_map_registry.h" + +#include "benchmark/benchmark.h" +#include + +namespace Envoy { +namespace { + +class BenchmarkTestScope { +public: + static absl::string_view name() { return "BenchmarkTestScope"; } +}; + +static std::vector normal_keys = {}; + +static std::vector::InlineHandle> initializeRegistry() { + std::vector::InlineHandle> handles; + + for (size_t i = 0; i < 200; ++i) { + std::string key = "key_" + std::to_string(i); + normal_keys.push_back(key); + handles.push_back(InlineMapRegistry::registerInlineKey(key)); + } + + // Force the inline map registry to be finalized. + InlineMapRegistryManager::registryInfos(); + + return handles; +} + +static std::vector::InlineHandle> inline_handles = + initializeRegistry(); + +// NOLINTNEXTLINE(readability-identifier-naming) +static void InlineMapFind(benchmark::State& state) { + size_t map_type = state.range(0); + size_t used_inline_handles = state.range(1); + + absl::flat_hash_map> normal_map; + auto inline_map = + InlineMapRegistry::InlineMap::createInlineMap(); + + for (size_t i = 0; i < 200; ++i) { + normal_map[normal_keys[i]] = std::make_unique("value_" + std::to_string(i)); + inline_map->insert(normal_keys[i], std::make_unique("value_" + std::to_string(i))); + } + + if (map_type == 0) { + for (auto _ : state) { // NOLINT + for (size_t i = 0; i < 200; ++i) { + normal_map.find(normal_keys[i]); + } + } + } else { + if (used_inline_handles == 0) { + for (auto _ : state) { // NOLINT + for (size_t i = 0; i < 200; ++i) { + inline_map->lookup(normal_keys[i]); + } + } + } else { + for (auto _ : state) { // NOLINT + for (size_t i = 0; i < 200; ++i) { + inline_map->lookup(inline_handles[i]); + } + } + } + } +} + +BENCHMARK(InlineMapFind)->Args({0, 0})->Args({1, 0})->Args({1, 1}); + +} // namespace +} // namespace Envoy From 1548f415d38b380a9f54e9beae47b40ab877a411 Mon Sep 17 00:00:00 2001 From: wbpcode Date: Tue, 13 Jun 2023 04:01:46 +0000 Subject: [PATCH 02/36] minor update Signed-off-by: wbpcode --- envoy/common/inline_map_registry.h | 27 +++--- .../common/inline_map_registry_speed_test.cc | 83 ++++++++++++++----- 2 files changed, 74 insertions(+), 36 deletions(-) diff --git a/envoy/common/inline_map_registry.h b/envoy/common/inline_map_registry.h index 3c8a0da9bd40f..e21ed0e989538 100644 --- a/envoy/common/inline_map_registry.h +++ b/envoy/common/inline_map_registry.h @@ -44,14 +44,8 @@ struct InlineMapRegistryDebugInfo { class InlineMapRegistryManager { public: static const std::vector& registryInfos() { - static bool finalized = false; - // Call finalize() to ensure that all finalizers are called. - if (!finalized) { - finalize(); - finalized = true; - } - + finalize(); return mutableRegistryInfos(); } @@ -67,6 +61,11 @@ class InlineMapRegistryManager { private: using Finalizer = std::function; + static uint64_t nextScopeId() { + static uint64_t next_scope_id = 0; + return next_scope_id++; + } + /** * Call all finalizers. This should only be called once, after all registrations are * complete. @@ -107,6 +106,7 @@ template class InlineMapRegistry { }; using RegistrationMap = absl::flat_hash_map; + using Hash = absl::container_internal::hash_default_hash; template class InlineMap : public InlineStorage { public: @@ -118,16 +118,17 @@ template class InlineMapRegistry { // Get the entry for the given key. If the key is not found, return nullptr. RawT lookup(absl::string_view key) const { // Compute the hash value for the key and avoid computing it again in the lookup. - const size_t hash_value = absl::Hash()(key); - - if (auto entry_id = staticLookup(key, hash_value); entry_id.has_value()) { - return inline_entries_[*entry_id]; - } + const size_t hash_value = InlineMapRegistry::Hash()(key); + // Because the normal string view key is used here, try the normal map entry first. if (auto it = map_.find(key, hash_value); it != map_.end()) { return it->second.get(); } + if (auto entry_id = staticLookup(key, hash_value); entry_id.has_value()) { + return inline_entries_[*entry_id]; + } + return nullptr; } @@ -385,7 +386,7 @@ template uint64_t InlineMapRegistryManager::generateScopeId() { RELEASE_ASSERT(!initialized, "InlineMapRegistryManager::generateScopeId() called twice for same scope"); - const uint64_t scope_id = mutableFinalizers().size(); + const uint64_t scope_id = nextScopeId(); mutableFinalizers().push_back([scope_id]() -> InlineMapRegistryDebugInfo { using Registry = InlineMapRegistry; ASSERT(Registry::scopeId() == scope_id); diff --git a/test/common/common/inline_map_registry_speed_test.cc b/test/common/common/inline_map_registry_speed_test.cc index 441f73186b3d1..d35b38e572934 100644 --- a/test/common/common/inline_map_registry_speed_test.cc +++ b/test/common/common/inline_map_registry_speed_test.cc @@ -6,69 +6,106 @@ namespace Envoy { namespace { -class BenchmarkTestScope { +template class BenchmarkTestScope { public: - static absl::string_view name() { return "BenchmarkTestScope"; } -}; + static absl::string_view name() { + static const std::string name = "BenchmarkTestScope_KeySize_" + std::to_string(N); + return name; + } -static std::vector normal_keys = {}; + static std::vector::InlineHandle> + initializeRegistry() { + std::vector::InlineHandle> handles; -static std::vector::InlineHandle> initializeRegistry() { - std::vector::InlineHandle> handles; + // Force the inline map registry to be initialized and be added to the registry manager. + InlineMapRegistry::scopeId(); - for (size_t i = 0; i < 200; ++i) { - std::string key = "key_" + std::to_string(i); - normal_keys.push_back(key); - handles.push_back(InlineMapRegistry::registerInlineKey(key)); + for (size_t i = 0; i < N; ++i) { + std::string key = "key_" + std::to_string(i); + handles.push_back(InlineMapRegistry::registerInlineKey(key)); + } + + // Force the inline map registry to be finalized. + InlineMapRegistryManager::registryInfos(); + + return handles; } - // Force the inline map registry to be finalized. - InlineMapRegistryManager::registryInfos(); + static std::vector::InlineHandle> inlineHandles() { + static const std::vector::InlineHandle> + inline_handles = initializeRegistry(); + return inline_handles; + } +}; - return handles; +template static std::vector getNormalKeys() { + static const std::vector normal_keys = []() { + std::vector keys; + for (size_t i = 0; i < N; ++i) { + keys.push_back("key_" + std::to_string(i)); + } + return keys; + }(); + return normal_keys; } -static std::vector::InlineHandle> inline_handles = - initializeRegistry(); - // NOLINTNEXTLINE(readability-identifier-naming) -static void InlineMapFind(benchmark::State& state) { +static void BM_InlineMapFind(benchmark::State& state) { size_t map_type = state.range(0); size_t used_inline_handles = state.range(1); + auto normal_keys = getNormalKeys<200>(); + auto inline_handles_200 = BenchmarkTestScope<200>::inlineHandles(); + auto inline_hanldes_0 = BenchmarkTestScope<0>::inlineHandles(); + absl::flat_hash_map> normal_map; - auto inline_map = - InlineMapRegistry::InlineMap::createInlineMap(); + auto inline_map_200 = + InlineMapRegistry>::InlineMap::createInlineMap(); + auto inline_map_0 = + InlineMapRegistry>::InlineMap::createInlineMap(); for (size_t i = 0; i < 200; ++i) { normal_map[normal_keys[i]] = std::make_unique("value_" + std::to_string(i)); - inline_map->insert(normal_keys[i], std::make_unique("value_" + std::to_string(i))); + inline_map_200->insert(normal_keys[i], + std::make_unique("value_" + std::to_string(i))); + inline_map_0->insert(normal_keys[i], + std::make_unique("value_" + std::to_string(i))); } if (map_type == 0) { + // Normal map. for (auto _ : state) { // NOLINT for (size_t i = 0; i < 200; ++i) { normal_map.find(normal_keys[i]); } } + } else if (map_type == 1) { + // Inline map without any inline keys. + for (auto _ : state) { // NOLINT + for (size_t i = 0; i < 200; ++i) { + inline_map_0->lookup(normal_keys[i]); + } + } } else { + // Inline map with 200 inline keys. if (used_inline_handles == 0) { for (auto _ : state) { // NOLINT for (size_t i = 0; i < 200; ++i) { - inline_map->lookup(normal_keys[i]); + inline_map_200->lookup(normal_keys[i]); } } } else { for (auto _ : state) { // NOLINT for (size_t i = 0; i < 200; ++i) { - inline_map->lookup(inline_handles[i]); + inline_map_200->lookup(inline_handles_200[i]); } } } } } -BENCHMARK(InlineMapFind)->Args({0, 0})->Args({1, 0})->Args({1, 1}); +// Additional `{0, 0}` is used to avoid the effect of the first run. +BENCHMARK(BM_InlineMapFind)->Args({0, 0})->Args({0, 0})->Args({1, 0})->Args({2, 0})->Args({2, 1}); } // namespace } // namespace Envoy From 02ac4b34058adbaf1e189e6996b90310cbb3ef43 Mon Sep 17 00:00:00 2001 From: wbpcode Date: Tue, 13 Jun 2023 05:36:02 +0000 Subject: [PATCH 03/36] fix format Signed-off-by: wbpcode --- test/common/common/inline_map_registry_speed_test.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/common/common/inline_map_registry_speed_test.cc b/test/common/common/inline_map_registry_speed_test.cc index d35b38e572934..079e0ad202190 100644 --- a/test/common/common/inline_map_registry_speed_test.cc +++ b/test/common/common/inline_map_registry_speed_test.cc @@ -1,7 +1,8 @@ +#include + #include "envoy/common/inline_map_registry.h" #include "benchmark/benchmark.h" -#include namespace Envoy { namespace { From 4f7817a00b2e3b08b2deb5a90402c32638a76c10 Mon Sep 17 00:00:00 2001 From: wbpcode Date: Wed, 14 Jun 2023 08:17:46 +0000 Subject: [PATCH 04/36] fix lifetime problem and add unit test and add more detailed comments Signed-off-by: wbpcode --- envoy/common/inline_map_registry.h | 187 +++++++++++++----- test/common/common/BUILD | 17 +- .../common/inline_map_registry_speed_test.cc | 42 +--- .../common/common/inline_map_registry_test.cc | 112 +++++++++++ .../common/inline_map_registry_test_scope.h | 40 ++++ 5 files changed, 308 insertions(+), 90 deletions(-) create mode 100644 test/common/common/inline_map_registry_test.cc create mode 100644 test/common/common/inline_map_registry_test_scope.h diff --git a/envoy/common/inline_map_registry.h b/envoy/common/inline_map_registry.h index e21ed0e989538..75d7563ea2313 100644 --- a/envoy/common/inline_map_registry.h +++ b/envoy/common/inline_map_registry.h @@ -5,6 +5,10 @@ #include "source/common/common/utility.h" #include "absl/container/flat_hash_map.h" +#include "absl/container/node_hash_set.h" + +#include +#include namespace Envoy { @@ -86,6 +90,39 @@ class InlineMapRegistryManager { } }; +/** + * This template could be used as an alternative of normal hash map to store the key/value pairs. + * But by this template, you can register some frequently used keys as inline keys and get the + * handle for the key. Then you can use the handle to access the key/value pair in the map without + * even one time hash searching. + * + * This is useful when the key is frequently used and the key is known at compile time or bootstrap + * time. You can get superior performance by using the inline handle. + * This template also provides the interface to access the key/value pair by the normal key and the + * interface has similar searching performance as the normal hash map. But the insertion and removal + * by the normal key is slower than the normal hash map. + * + * The Scope template parameter is used to distinguish different inline map registry. Different + * Scope will have different inline key registrations and different scope id. + * + * These is usage example: + * + * // Define a scope type. + * class ExampleScope { + * public: + * static absl::string_view name() { return "ExampleScope"; } + * }; + * + * // Register the inline key. We should never do this after bootstrapping. + * static auto inline_handle = InlineMapRegistry::registerInlineKey("inline_key"); + * + * // Create the inline map. + * auto inline_map = InlineMapRegistry::InlineMap::createInlineMap(); + * + * // Get value by inline handle. + * inline_map->insert(inline_handle, std::make_unique("value")); + * EXPECT_EQ(*inline_map->lookup(inline_handle), "value"); + */ template class InlineMapRegistry { public: class InlineHandle { @@ -105,57 +142,33 @@ template class InlineMapRegistry { absl::string_view inline_entry_key_; }; - using RegistrationMap = absl::flat_hash_map; + // This is the Hash used for registration map/set and underlay hash map. using Hash = absl::container_internal::hash_default_hash; + using RegistrationMap = absl::flat_hash_map; + + // Node hash set is used to store the registration keys because it's entries have stable address + // and it is safe to reference the key in the InlineHandle or RegistrationMap. + using RegistrationSet = absl::node_hash_set; + template class InlineMap : public InlineStorage { public: using TPtr = std::unique_ptr; using RawT = T*; using ConstRawT = const T*; - using HashMap = absl::flat_hash_map; + using UnderlayHashMap = absl::flat_hash_map; // Get the entry for the given key. If the key is not found, return nullptr. - RawT lookup(absl::string_view key) const { - // Compute the hash value for the key and avoid computing it again in the lookup. - const size_t hash_value = InlineMapRegistry::Hash()(key); - - // Because the normal string view key is used here, try the normal map entry first. - if (auto it = map_.find(key, hash_value); it != map_.end()) { - return it->second.get(); - } - - if (auto entry_id = staticLookup(key, hash_value); entry_id.has_value()) { - return inline_entries_[*entry_id]; - } - - return nullptr; - } + ConstRawT lookup(absl::string_view key) const { return lookupImpl(key); } + RawT lookup(absl::string_view key) { return lookupImpl(key); } // Get the entry for the given handle. If the handle is not valid, return nullptr. - RawT lookup(InlineHandle handle) const { - ASSERT(handle.inlineEntryId() < InlineMapRegistry::registrationMapSize()); - return inline_entries_[handle.inlineEntryId()]; - } + ConstRawT lookup(InlineHandle handle) const { return lookupImpl(handle); } + RawT lookup(InlineHandle handle) { return lookupImpl(handle); } // Get the entry for the given untyped handle. If the handle is not valid, return nullptr. - RawT lookup(UntypedInlineHandle handle) const { - // If the scope id does not match, the handle is not valid. - if (handle.inlineScopeId() != InlineMapRegistry::scopeId()) { - return nullptr; - } - - // If the entry id is valid, it is an inline entry. - if (handle.inlineEntryId() < InlineMapRegistry::registrationMapSize()) { - return inline_entries_[handle.inlineEntryId()]; - } - - // Otherwise, try normal map entry. - if (auto it = map_.find(handle.inlineEntryKey()); it != map_.end()) { - return it->second.get(); - } - return nullptr; - } + ConstRawT lookup(UntypedInlineHandle handle) const { return lookupImpl(handle); } + RawT lookup(UntypedInlineHandle handle) { return lookupImpl(handle); } // Set the entry for the given key. If the key is already present, overwrite it. void insert(absl::string_view key, TPtr value) { @@ -165,7 +178,7 @@ template class InlineMapRegistry { if (auto entry_id = staticLookup(key, hash_value); entry_id.has_value()) { resetInlineMapEntry(*entry_id, std::move(value)); } else { - map_[key] = std::move(value); + normal_entries_[key] = std::move(value); } } @@ -189,7 +202,7 @@ template class InlineMapRegistry { } // Otherwise, try normal map entry. - map_[handle.inlineEntryKey()] = std::move(value); + normal_entries_[handle.inlineEntryKey()] = std::move(value); } // Remove the entry for the given key. If the key is not found, do nothing. @@ -200,7 +213,7 @@ template class InlineMapRegistry { if (auto entry_id = staticLookup(key, hash_value); entry_id.has_value()) { resetInlineMapEntry(*entry_id); } else { - map_.erase(key); + normal_entries_.erase(key); } } @@ -221,30 +234,32 @@ template class InlineMapRegistry { } // Otherwise, try normal map entry. - map_.erase(handle.inlineEntryKey()); + normal_entries_.erase(handle.inlineEntryKey()); } void iterate(std::function callback) const { - for (const auto& entry : map_) { + for (const auto& entry : normal_entries_) { if (!callback(entry.first, entry.second.get())) { return; } } for (const auto& id : InlineMapRegistry::registrationMap()) { - ASSERT(id.second.inlineEntryId() < InlineMapRegistry::registrationMapSize()); + ASSERT(id.second < InlineMapRegistry::registrationMapSize()); - auto entry = inline_entries_[id.second.inlineEntryId()]; + auto entry = inline_entries_[id.second]; if (entry == nullptr) { continue; } - if (!callback(id.second.inlineEntryKey(), entry.get())) { + if (!callback(id.first, entry.get())) { return; } } } + uint64_t size() const { return normal_entries_.size() + inline_entries_size_; } + static std::unique_ptr createInlineMap() { return std::unique_ptr( new ((InlineMapRegistry::registrationMapSize() * sizeof(TPtr))) InlineMap()); @@ -255,24 +270,76 @@ template class InlineMapRegistry { memset(inline_entries_, 0, InlineMapRegistry::registrationMapSize() * sizeof(TPtr)); } + RawT lookupImpl(absl::string_view key) const { + // Compute the hash value for the key and avoid computing it again in the lookup. + const size_t hash_value = InlineMapRegistry::Hash()(key); + + // Because the normal string view key is used here, try the normal map entry first. + if (auto it = normal_entries_.find(key, hash_value); it != normal_entries_.end()) { + return it->second.get(); + } + + if (auto entry_id = staticLookup(key, hash_value); entry_id.has_value()) { + return inline_entries_[*entry_id]; + } + + return nullptr; + } + + RawT lookupImpl(InlineHandle handle) const { + ASSERT(handle.inlineEntryId() < InlineMapRegistry::registrationMapSize()); + return inline_entries_[handle.inlineEntryId()]; + } + + // Get the entry for the given untyped handle. If the handle is not valid, return nullptr. + RawT lookupImpl(UntypedInlineHandle handle) const { + // If the scope id does not match, the handle is not valid. + if (handle.inlineScopeId() != InlineMapRegistry::scopeId()) { + return nullptr; + } + + // If the entry id is valid, it is an inline entry. + if (handle.inlineEntryId() < InlineMapRegistry::registrationMapSize()) { + return inline_entries_[handle.inlineEntryId()]; + } + + // Otherwise, try normal map entry. + if (auto it = normal_entries_.find(handle.inlineEntryKey()); it != normal_entries_.end()) { + return it->second.get(); + } + return nullptr; + } + void resetInlineMapEntry(uint64_t inline_entry_id, TPtr new_entry = nullptr) { ASSERT(inline_entry_id < InlineMapRegistry::registrationMapSize()); - if (auto entry = inline_entries_[inline_entry_id]; entry != nullptr) { + // Remove and delete the old valid entry. + ASSERT(inline_entries_size_ > 0); + --inline_entries_size_; delete entry; } + + if (new_entry != nullptr) { + // Append the new valid entry. + ++inline_entries_size_; + } + inline_entries_[inline_entry_id] = new_entry.release(); } absl::optional staticLookup(absl::string_view key, size_t hash_value) const { if (auto iter = InlineMapRegistry::registrationMap().find(key, hash_value); iter != InlineMapRegistry::registrationMap().end()) { - return iter->second.inlineEntryId(); + return iter->second; } return absl::nullopt; } - HashMap map_; + // This is the underlay hash map for the normal entries. + UnderlayHashMap normal_entries_; + + uint64_t inline_entries_size_{}; + // This should be the last member of the class and no member should be added after this. RawT inline_entries_[]; }; @@ -289,12 +356,22 @@ template class InlineMapRegistry { // If the key is already registered, return the existing handle. if (auto it = mutableRegistrationMap().find(key); it != mutableRegistrationMap().end()) { - return it->second; + // It is safe to reference the key here because the key is stored in the node hash set. + return InlineHandle(it->second, it->first); } - InlineHandle handle{mutableRegistrationMap().size(), key}; - mutableRegistrationMap().emplace(key, handle); - return handle; + // If the key is not registered, create a new handle for it. + + const uint64_t entry_id = mutableRegistrationMap().size(); + + // Insert the key to the node hash set and keep the reference of the key in the + // inline handle and registration map. + auto result = mutableRegistrationSet().emplace(key); + RELEASE_ASSERT(result.second, "The key is already registered and this should never happen"); + + // It is safe to reference the key here because the key is stored in the node hash set. + mutableRegistrationMap().emplace(absl::string_view(*result.first), entry_id); + return InlineHandle(entry_id, absl::string_view(*result.first)); } /** @@ -367,6 +444,10 @@ template class InlineMapRegistry { MUTABLE_CONSTRUCT_ON_FIRST_USE(RegistrationMap); } + static RegistrationSet& mutableRegistrationSet() { + MUTABLE_CONSTRUCT_ON_FIRST_USE(RegistrationSet); + } + static InlineMapRegistryDebugInfo debugInfo() { RELEASE_ASSERT(mutableFinalized(), "Cannot fetch debug info before finalized()"); diff --git a/test/common/common/BUILD b/test/common/common/BUILD index d428f710af9a7..11d9bd06bc1d6 100644 --- a/test/common/common/BUILD +++ b/test/common/common/BUILD @@ -4,6 +4,7 @@ load( "envoy_cc_benchmark_binary", "envoy_cc_fuzz_test", "envoy_cc_test", + "envoy_cc_test_library", "envoy_package", "envoy_select_boringssl", ) @@ -493,11 +494,25 @@ envoy_cc_test( ], ) +envoy_cc_test_library( + name = "inline_map_registry_test_scope_lib", + hdrs = ["inline_map_registry_test_scope.h"], + deps = [ + "//envoy/common:inline_map_registry", + ], +) + +envoy_cc_test( + name = "inline_map_registry_test", + srcs = ["inline_map_registry_test.cc"], + deps = [":inline_map_registry_test_scope_lib"], +) + envoy_cc_benchmark_binary( name = "inline_map_registry_speed_test", srcs = ["inline_map_registry_speed_test.cc"], external_deps = ["benchmark"], - deps = ["//envoy/common:inline_map_registry"], + deps = [":inline_map_registry_test_scope_lib"], ) envoy_benchmark_test( diff --git a/test/common/common/inline_map_registry_speed_test.cc b/test/common/common/inline_map_registry_speed_test.cc index 079e0ad202190..d5b7846124c91 100644 --- a/test/common/common/inline_map_registry_speed_test.cc +++ b/test/common/common/inline_map_registry_speed_test.cc @@ -2,43 +2,13 @@ #include "envoy/common/inline_map_registry.h" +#include "test/common/common/inline_map_registry_test_scope.h" + #include "benchmark/benchmark.h" namespace Envoy { namespace { -template class BenchmarkTestScope { -public: - static absl::string_view name() { - static const std::string name = "BenchmarkTestScope_KeySize_" + std::to_string(N); - return name; - } - - static std::vector::InlineHandle> - initializeRegistry() { - std::vector::InlineHandle> handles; - - // Force the inline map registry to be initialized and be added to the registry manager. - InlineMapRegistry::scopeId(); - - for (size_t i = 0; i < N; ++i) { - std::string key = "key_" + std::to_string(i); - handles.push_back(InlineMapRegistry::registerInlineKey(key)); - } - - // Force the inline map registry to be finalized. - InlineMapRegistryManager::registryInfos(); - - return handles; - } - - static std::vector::InlineHandle> inlineHandles() { - static const std::vector::InlineHandle> - inline_handles = initializeRegistry(); - return inline_handles; - } -}; - template static std::vector getNormalKeys() { static const std::vector normal_keys = []() { std::vector keys; @@ -56,14 +26,14 @@ static void BM_InlineMapFind(benchmark::State& state) { size_t used_inline_handles = state.range(1); auto normal_keys = getNormalKeys<200>(); - auto inline_handles_200 = BenchmarkTestScope<200>::inlineHandles(); - auto inline_hanldes_0 = BenchmarkTestScope<0>::inlineHandles(); + auto inline_handles_200 = InlineMapRegistryTestScope<200>::inlineHandles(); + auto inline_hanldes_0 = InlineMapRegistryTestScope<0>::inlineHandles(); absl::flat_hash_map> normal_map; auto inline_map_200 = - InlineMapRegistry>::InlineMap::createInlineMap(); + InlineMapRegistry>::InlineMap::createInlineMap(); auto inline_map_0 = - InlineMapRegistry>::InlineMap::createInlineMap(); + InlineMapRegistry>::InlineMap::createInlineMap(); for (size_t i = 0; i < 200; ++i) { normal_map[normal_keys[i]] = std::make_unique("value_" + std::to_string(i)); diff --git a/test/common/common/inline_map_registry_test.cc b/test/common/common/inline_map_registry_test.cc new file mode 100644 index 0000000000000..d2adf85c47ccf --- /dev/null +++ b/test/common/common/inline_map_registry_test.cc @@ -0,0 +1,112 @@ +#include "test/common/common/inline_map_registry_test_scope.h" + +#include "gtest/gtest.h" + +namespace Envoy { +namespace { + +TEST(InlineMapWithZeroInlineKey, InlineMapWithZeroInlineKeyTest) { + auto map = + InlineMapRegistry>::InlineMap::createInlineMap(); + + EXPECT_EQ(InlineMapRegistryManager::registryInfos().size(), 1); + EXPECT_EQ(InlineMapRegistryManager::registryInfos()[0].registry_scope_name, + InlineMapRegistryTestScope<0>::name()); + + // Insert keys. + for (size_t i = 0; i < 200; ++i) { + map->insert("key_" + std::to_string(i), + std::make_unique("value_" + std::to_string(i))); + } + + // Lookup keys. + for (size_t i = 0; i < 200; ++i) { + EXPECT_EQ(*map->lookup("key_" + std::to_string(i)), "value_" + std::to_string(i)); + } + + // Looup by untyped inline handle. + for (size_t i = 0; i < 200; ++i) { + const std::string key = "key_" + std::to_string(i); + UntypedInlineHandle handle(InlineMapRegistry>::scopeId(), i, key); + EXPECT_EQ(*map->lookup(handle), "value_" + std::to_string(i)); + } + + // Lookup non-existing keys. + EXPECT_EQ(map->lookup("non_existing_key"), nullptr); + + // remove keys by untyped inline handle. + for (size_t i = 0; i < 100; ++i) { + const std::string key = "key_" + std::to_string(i); + UntypedInlineHandle handle(InlineMapRegistry>::scopeId(), i, key); + map->remove(handle); + } + + // Remove keys. + for (size_t i = 0; i < 200; ++i) { + map->remove("key_" + std::to_string(i)); + } +} + +TEST(InlineMapWith20InlineKey, InlineMapWith20InlineKeyTest) { + + auto inline_hanldes = InlineMapRegistryTestScope<20>::inlineHandles(); + + auto map = + InlineMapRegistry>::InlineMap::createInlineMap(); + + // Insert keys. + for (size_t i = 0; i < 200; ++i) { + map->insert("key_" + std::to_string(i), + std::make_unique("value_" + std::to_string(i))); + } + + // Lookup keys. + for (size_t i = 0; i < 200; ++i) { + EXPECT_EQ(*map->lookup("key_" + std::to_string(i)), "value_" + std::to_string(i)); + } + + // Lookup by untyped inline handle. + for (size_t i = 0; i < 200; ++i) { + const std::string key = "key_" + std::to_string(i); + UntypedInlineHandle handle(InlineMapRegistry>::scopeId(), i, + key); + EXPECT_EQ(*map->lookup(handle), "value_" + std::to_string(i)); + } + + // Lookup by typed inline handle. + for (size_t i = 0; i < inline_hanldes.size(); ++i) { + const std::string key = "key_" + std::to_string(i); + auto handle = inline_hanldes[i]; + EXPECT_EQ(handle.inlineEntryKey(), key); + EXPECT_EQ(handle.inlineEntryId(), i); + EXPECT_EQ(*map->lookup(handle), "value_" + std::to_string(i)); + } + + // Lookup non-existing keys. + EXPECT_EQ(map->lookup("non_existing_key"), nullptr); + + // Remove keys by typed inline handle. + for (size_t i = 0; i < 10; ++i) { + const std::string key = "key_" + std::to_string(i); + auto handle = inline_hanldes[i]; + map->remove(handle); + } + + // Remove keys by untyped inline handle. + for (size_t i = 10; i < 30; ++i) { + const std::string key = "key_" + std::to_string(i); + UntypedInlineHandle handle(InlineMapRegistry>::scopeId(), i, + key); + map->remove(handle); + } + + // Remove keys. + for (size_t i = 30; i < 200; ++i) { + map->remove("key_" + std::to_string(i)); + } + + EXPECT_EQ(map->size(), 0); +} + +} // namespace +} // namespace Envoy diff --git a/test/common/common/inline_map_registry_test_scope.h b/test/common/common/inline_map_registry_test_scope.h new file mode 100644 index 0000000000000..2042623414da8 --- /dev/null +++ b/test/common/common/inline_map_registry_test_scope.h @@ -0,0 +1,40 @@ +#pragma once + +#include "envoy/common/inline_map_registry.h" + +namespace Envoy { + +template class InlineMapRegistryTestScope { +public: + static absl::string_view name() { + static const std::string name = "BenchmarkTestScope_KeySize_" + std::to_string(N); + return name; + } + + static std::vector::InlineHandle> + initializeRegistry() { + std::vector::InlineHandle> handles; + + // Force the inline map registry to be initialized and be added to the registry manager. + InlineMapRegistry::scopeId(); + + for (size_t i = 0; i < N; ++i) { + std::string key = "key_" + std::to_string(i); + handles.push_back(InlineMapRegistry::registerInlineKey(key)); + } + + // Force the inline map registry to be finalized. + InlineMapRegistryManager::registryInfos(); + + return handles; + } + + static std::vector::InlineHandle> + inlineHandles() { + static const std::vector::InlineHandle> + inline_handles = initializeRegistry(); + return inline_handles; + } +}; + +} // namespace Envoy From e190a5549c2691d17f0ee0b4b6163590af345d79 Mon Sep 17 00:00:00 2001 From: wbpcode Date: Wed, 14 Jun 2023 08:44:21 +0000 Subject: [PATCH 05/36] fix memory leak Signed-off-by: wbpcode --- envoy/common/inline_map_registry.h | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/envoy/common/inline_map_registry.h b/envoy/common/inline_map_registry.h index 75d7563ea2313..811ede4f9ec12 100644 --- a/envoy/common/inline_map_registry.h +++ b/envoy/common/inline_map_registry.h @@ -270,6 +270,16 @@ template class InlineMapRegistry { memset(inline_entries_, 0, InlineMapRegistry::registrationMapSize() * sizeof(TPtr)); } + ~InlineMap() { + for (uint64_t i = 0; i < InlineMapRegistry::registrationMapSize(); ++i) { + auto entry = inline_entries_[i]; + if (entry != nullptr) { + delete entry; + } + } + memset(inline_entries_, 0, InlineMapRegistry::registrationMapSize() * sizeof(TPtr)); + } + RawT lookupImpl(absl::string_view key) const { // Compute the hash value for the key and avoid computing it again in the lookup. const size_t hash_value = InlineMapRegistry::Hash()(key); From ba3c8745770026818f307c449ffa54e726623b93 Mon Sep 17 00:00:00 2001 From: wbpcode Date: Wed, 14 Jun 2023 09:11:58 +0000 Subject: [PATCH 06/36] fix format Signed-off-by: wbpcode --- envoy/common/inline_map_registry.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/envoy/common/inline_map_registry.h b/envoy/common/inline_map_registry.h index 811ede4f9ec12..6ee1545d720bc 100644 --- a/envoy/common/inline_map_registry.h +++ b/envoy/common/inline_map_registry.h @@ -1,5 +1,8 @@ #pragma once +#include +#include + #include "source/common/common/assert.h" #include "source/common/common/macros.h" #include "source/common/common/utility.h" @@ -7,9 +10,6 @@ #include "absl/container/flat_hash_map.h" #include "absl/container/node_hash_set.h" -#include -#include - namespace Envoy { class UntypedInlineHandle { From e10bcd88c6c9dc030ac60677a3d129c40ad58b50 Mon Sep 17 00:00:00 2001 From: wbpcode Date: Wed, 14 Jun 2023 09:27:05 +0000 Subject: [PATCH 07/36] update comment and new function Signed-off-by: wbpcode --- envoy/common/inline_map_registry.h | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/envoy/common/inline_map_registry.h b/envoy/common/inline_map_registry.h index 6ee1545d720bc..e52c8a9ca6afe 100644 --- a/envoy/common/inline_map_registry.h +++ b/envoy/common/inline_map_registry.h @@ -96,8 +96,11 @@ class InlineMapRegistryManager { * handle for the key. Then you can use the handle to access the key/value pair in the map without * even one time hash searching. * - * This is useful when the key is frequently used and the key is known at compile time or bootstrap - * time. You can get superior performance by using the inline handle. + * This is useful when some keys are frequently used and the keys are known at compile time or + * bootstrap time. You can get superior performance by using the inline handle. For example, the + * filter state uses always use the filter name as the key and the filter name is known at compile + * time. By using the inline handle, the filter state could get the key/value pair without any hash. + * * This template also provides the interface to access the key/value pair by the normal key and the * interface has similar searching performance as the normal hash map. But the insertion and removal * by the normal key is slower than the normal hash map. @@ -384,6 +387,23 @@ template class InlineMapRegistry { return InlineHandle(entry_id, absl::string_view(*result.first)); } + /** + * Fetch the inline handle for the given key. May only be called after finalized(). This should + * be used to get the inline handle for the key registered by registerInlineKey(). This function + * could used to determine if the key is registered or not at runtime or xDS config loading time + * and decide if the key should be used as inline key or normal key. + */ + absl::optional getInlineHandle(absl::string_view key) { + ASSERT(mutableFinalized(), "Cannot get inline handle before finalized()"); + + if (auto it = mutableRegistrationMap().find(key); it != mutableRegistrationMap().end()) { + // It is safe to reference the key here because the key is stored in the node hash set. + return InlineHandle(it->second, it->first); + } + + return absl::nullopt; + } + /** * Fetch all registered key. May only be called after finalized(). */ From 43e3e0eb55c9b57d24cbc29a2f0cf76cd97c6745 Mon Sep 17 00:00:00 2001 From: wbpcode Date: Wed, 14 Jun 2023 09:30:51 +0000 Subject: [PATCH 08/36] update comment Signed-off-by: wbpcode --- envoy/common/inline_map_registry.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/envoy/common/inline_map_registry.h b/envoy/common/inline_map_registry.h index e52c8a9ca6afe..32aa290def1be 100644 --- a/envoy/common/inline_map_registry.h +++ b/envoy/common/inline_map_registry.h @@ -98,8 +98,8 @@ class InlineMapRegistryManager { * * This is useful when some keys are frequently used and the keys are known at compile time or * bootstrap time. You can get superior performance by using the inline handle. For example, the - * filter state uses always use the filter name as the key and the filter name is known at compile - * time. By using the inline handle, the filter state could get the key/value pair without any hash. + * filter state always uses the filter name as the key and the filter name is known at compile time. + * By using the inline handle, the filter state could get the key/value pair without any hash. * * This template also provides the interface to access the key/value pair by the normal key and the * interface has similar searching performance as the normal hash map. But the insertion and removal From 1666aea2cc170e1aa78c4bbbc9982821e8b25e00 Mon Sep 17 00:00:00 2001 From: wbpcode Date: Wed, 14 Jun 2023 09:50:57 +0000 Subject: [PATCH 09/36] fix format Signed-off-by: wbpcode --- test/common/common/inline_map_registry_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/common/common/inline_map_registry_test.cc b/test/common/common/inline_map_registry_test.cc index d2adf85c47ccf..96916d5dadfdd 100644 --- a/test/common/common/inline_map_registry_test.cc +++ b/test/common/common/inline_map_registry_test.cc @@ -24,7 +24,7 @@ TEST(InlineMapWithZeroInlineKey, InlineMapWithZeroInlineKeyTest) { EXPECT_EQ(*map->lookup("key_" + std::to_string(i)), "value_" + std::to_string(i)); } - // Looup by untyped inline handle. + // Lookup by untyped inline handle. for (size_t i = 0; i < 200; ++i) { const std::string key = "key_" + std::to_string(i); UntypedInlineHandle handle(InlineMapRegistry>::scopeId(), i, key); From e0eb5280e2a769da0cf36ba72ed6b0343a931e1a Mon Sep 17 00:00:00 2001 From: wbpcode Date: Wed, 14 Jun 2023 10:57:01 +0000 Subject: [PATCH 10/36] fix test Signed-off-by: wbpcode --- envoy/common/inline_map_registry.h | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/envoy/common/inline_map_registry.h b/envoy/common/inline_map_registry.h index 32aa290def1be..7f7876a5e9d7f 100644 --- a/envoy/common/inline_map_registry.h +++ b/envoy/common/inline_map_registry.h @@ -268,11 +268,6 @@ template class InlineMapRegistry { new ((InlineMapRegistry::registrationMapSize() * sizeof(TPtr))) InlineMap()); } - private: - InlineMap() { - memset(inline_entries_, 0, InlineMapRegistry::registrationMapSize() * sizeof(TPtr)); - } - ~InlineMap() { for (uint64_t i = 0; i < InlineMapRegistry::registrationMapSize(); ++i) { auto entry = inline_entries_[i]; @@ -283,6 +278,11 @@ template class InlineMapRegistry { memset(inline_entries_, 0, InlineMapRegistry::registrationMapSize() * sizeof(TPtr)); } + private: + InlineMap() { + memset(inline_entries_, 0, InlineMapRegistry::registrationMapSize() * sizeof(TPtr)); + } + RawT lookupImpl(absl::string_view key) const { // Compute the hash value for the key and avoid computing it again in the lookup. const size_t hash_value = InlineMapRegistry::Hash()(key); From 9579aed2692ca5887a3f2a36999aecda2a91ceb5 Mon Sep 17 00:00:00 2001 From: wbpcode Date: Fri, 16 Jun 2023 02:54:35 +0000 Subject: [PATCH 11/36] address comments Signed-off-by: wbpcode --- envoy/common/inline_map_registry.h | 29 +++++++++---------- .../common/inline_map_registry_speed_test.cc | 7 ++--- .../common/inline_map_registry_test_scope.h | 11 ++++--- 3 files changed, 22 insertions(+), 25 deletions(-) diff --git a/envoy/common/inline_map_registry.h b/envoy/common/inline_map_registry.h index 7f7876a5e9d7f..5af30627e0dc8 100644 --- a/envoy/common/inline_map_registry.h +++ b/envoy/common/inline_map_registry.h @@ -116,8 +116,9 @@ class InlineMapRegistryManager { * static absl::string_view name() { return "ExampleScope"; } * }; * - * // Register the inline key. We should never do this after bootstrapping. - * static auto inline_handle = InlineMapRegistry::registerInlineKey("inline_key"); + * // Register the inline key. We should never do this after bootstrapping. Typically, we can + * // do this by define a global variable. + * auto inline_handle = InlineMapRegistry::registerInlineKey("inline_key"); * * // Create the inline map. * auto inline_map = InlineMapRegistry::InlineMap::createInlineMap(); @@ -157,21 +158,19 @@ template class InlineMapRegistry { template class InlineMap : public InlineStorage { public: using TPtr = std::unique_ptr; - using RawT = T*; - using ConstRawT = const T*; using UnderlayHashMap = absl::flat_hash_map; // Get the entry for the given key. If the key is not found, return nullptr. - ConstRawT lookup(absl::string_view key) const { return lookupImpl(key); } - RawT lookup(absl::string_view key) { return lookupImpl(key); } + const T* lookup(absl::string_view key) const { return lookupImpl(key); } + T* lookup(absl::string_view key) { return lookupImpl(key); } // Get the entry for the given handle. If the handle is not valid, return nullptr. - ConstRawT lookup(InlineHandle handle) const { return lookupImpl(handle); } - RawT lookup(InlineHandle handle) { return lookupImpl(handle); } + const T* lookup(InlineHandle handle) const { return lookupImpl(handle); } + T* lookup(InlineHandle handle) { return lookupImpl(handle); } // Get the entry for the given untyped handle. If the handle is not valid, return nullptr. - ConstRawT lookup(UntypedInlineHandle handle) const { return lookupImpl(handle); } - RawT lookup(UntypedInlineHandle handle) { return lookupImpl(handle); } + const T* lookup(UntypedInlineHandle handle) const { return lookupImpl(handle); } + T* lookup(UntypedInlineHandle handle) { return lookupImpl(handle); } // Set the entry for the given key. If the key is already present, overwrite it. void insert(absl::string_view key, TPtr value) { @@ -240,7 +239,7 @@ template class InlineMapRegistry { normal_entries_.erase(handle.inlineEntryKey()); } - void iterate(std::function callback) const { + void iterate(std::function callback) const { for (const auto& entry : normal_entries_) { if (!callback(entry.first, entry.second.get())) { return; @@ -283,7 +282,7 @@ template class InlineMapRegistry { memset(inline_entries_, 0, InlineMapRegistry::registrationMapSize() * sizeof(TPtr)); } - RawT lookupImpl(absl::string_view key) const { + T* lookupImpl(absl::string_view key) const { // Compute the hash value for the key and avoid computing it again in the lookup. const size_t hash_value = InlineMapRegistry::Hash()(key); @@ -299,13 +298,13 @@ template class InlineMapRegistry { return nullptr; } - RawT lookupImpl(InlineHandle handle) const { + T* lookupImpl(InlineHandle handle) const { ASSERT(handle.inlineEntryId() < InlineMapRegistry::registrationMapSize()); return inline_entries_[handle.inlineEntryId()]; } // Get the entry for the given untyped handle. If the handle is not valid, return nullptr. - RawT lookupImpl(UntypedInlineHandle handle) const { + T* lookupImpl(UntypedInlineHandle handle) const { // If the scope id does not match, the handle is not valid. if (handle.inlineScopeId() != InlineMapRegistry::scopeId()) { return nullptr; @@ -353,7 +352,7 @@ template class InlineMapRegistry { uint64_t inline_entries_size_{}; // This should be the last member of the class and no member should be added after this. - RawT inline_entries_[]; + T* inline_entries_[]; }; /** diff --git a/test/common/common/inline_map_registry_speed_test.cc b/test/common/common/inline_map_registry_speed_test.cc index d5b7846124c91..1f20e2753e406 100644 --- a/test/common/common/inline_map_registry_speed_test.cc +++ b/test/common/common/inline_map_registry_speed_test.cc @@ -9,15 +9,14 @@ namespace Envoy { namespace { -template static std::vector getNormalKeys() { - static const std::vector normal_keys = []() { +template static const std::vector& getNormalKeys() { + CONSTRUCT_ON_FIRST_USE(std::vector, []() { std::vector keys; for (size_t i = 0; i < N; ++i) { keys.push_back("key_" + std::to_string(i)); } return keys; - }(); - return normal_keys; + }()); } // NOLINTNEXTLINE(readability-identifier-naming) diff --git a/test/common/common/inline_map_registry_test_scope.h b/test/common/common/inline_map_registry_test_scope.h index 2042623414da8..af830f01e2d4d 100644 --- a/test/common/common/inline_map_registry_test_scope.h +++ b/test/common/common/inline_map_registry_test_scope.h @@ -7,8 +7,7 @@ namespace Envoy { template class InlineMapRegistryTestScope { public: static absl::string_view name() { - static const std::string name = "BenchmarkTestScope_KeySize_" + std::to_string(N); - return name; + CONSTRUCT_ON_FIRST_USE(std::string, "BenchmarkTestScope_KeySize_" + std::to_string(N)); } static std::vector::InlineHandle> @@ -29,11 +28,11 @@ template class InlineMapRegistryTestScope { return handles; } - static std::vector::InlineHandle> + static const std::vector::InlineHandle>& inlineHandles() { - static const std::vector::InlineHandle> - inline_handles = initializeRegistry(); - return inline_handles; + CONSTRUCT_ON_FIRST_USE( + std::vector::InlineHandle>, + initializeRegistry()); } }; From 6b4b1a874165e939005372f556c40b835c2e84ab Mon Sep 17 00:00:00 2001 From: wbpcode Date: Mon, 19 Jun 2023 11:32:07 +0000 Subject: [PATCH 12/36] remove unnecessary scope id temporarily Signed-off-by: wbpcode --- envoy/common/inline_map_registry.h | 143 ++++-------------- .../common/common/inline_map_registry_test.cc | 36 +---- 2 files changed, 30 insertions(+), 149 deletions(-) diff --git a/envoy/common/inline_map_registry.h b/envoy/common/inline_map_registry.h index 5af30627e0dc8..74a55d8ad352a 100644 --- a/envoy/common/inline_map_registry.h +++ b/envoy/common/inline_map_registry.h @@ -12,32 +12,10 @@ namespace Envoy { -class UntypedInlineHandle { -public: - UntypedInlineHandle(uint64_t inline_scope_id, uint64_t inline_entry_id, - absl::string_view inline_entry_key) - : inline_scope_id_(inline_scope_id), inline_entry_id_(inline_entry_id), - inline_entry_key_(inline_entry_key) {} - - uint64_t inlineScopeId() const { return inline_scope_id_; } - - uint64_t inlineEntryId() const { return inline_entry_id_; } - - absl::string_view inlineEntryKey() const { return inline_entry_key_; } - -private: - uint64_t inline_scope_id_{}; - uint64_t inline_entry_id_{}; - absl::string_view inline_entry_key_; -}; - /** * Information about the registry. This is used to print out the registry debug information. */ struct InlineMapRegistryDebugInfo { - // Registry scope id for the registry, this is generated dynamically by the - // InlineMapRegistryManager. - uint64_t registry_scope_id; // Registry scope name for the registry, typically short string from // scope type. absl::string_view registry_scope_name; @@ -45,43 +23,43 @@ struct InlineMapRegistryDebugInfo { std::vector registry_inline_keys; }; +/** + * Inline map registry manager that used to get all the inline map registries debug info. + * This is used to print out the inline map registries debug information and try to finalize + * all the inline map registries when the server is starting up. + */ class InlineMapRegistryManager { public: - static const std::vector& registryInfos() { + static const std::vector& registriesInfo() { // Call finalize() to ensure that all finalizers are called. finalize(); - return mutableRegistryInfos(); + return mutableRegistriesInfo(); } /** - * Generate a scope id for the given scope and register a finalizer to be called when - * InlineMapRegistry::finalize() is called. + * Register a finalizer to be called when InlineMapRegistry::finalize() is called. * - * NOTE: This is called by InlineMapRegistry::scopeId() and should never be called + * NOTE: This is called by InlineMapRegistry::finalize() and should never be called * manually. */ - template static uint64_t generateScopeId(); + template static bool registerRegistry(); private: using Finalizer = std::function; - static uint64_t nextScopeId() { - static uint64_t next_scope_id = 0; - return next_scope_id++; - } - /** * Call all finalizers. This should only be called once, after all registrations are * complete. */ static void finalize() { - while (!mutableFinalizers().empty()) { - mutableRegistryInfos().push_back(mutableFinalizers().back()()); - mutableFinalizers().pop_back(); + auto& registries_info = mutableRegistriesInfo(); + for (const auto& finalizer : mutableFinalizers()) { + registries_info.push_back(finalizer()); } + mutableFinalizers().clear(); } - static std::vector& mutableRegistryInfos() { + static std::vector& mutableRegistriesInfo() { MUTABLE_CONSTRUCT_ON_FIRST_USE(std::vector); } @@ -168,10 +146,6 @@ template class InlineMapRegistry { const T* lookup(InlineHandle handle) const { return lookupImpl(handle); } T* lookup(InlineHandle handle) { return lookupImpl(handle); } - // Get the entry for the given untyped handle. If the handle is not valid, return nullptr. - const T* lookup(UntypedInlineHandle handle) const { return lookupImpl(handle); } - T* lookup(UntypedInlineHandle handle) { return lookupImpl(handle); } - // Set the entry for the given key. If the key is already present, overwrite it. void insert(absl::string_view key, TPtr value) { // Compute the hash value for the key and avoid computing it again in the lookup. @@ -190,23 +164,6 @@ template class InlineMapRegistry { resetInlineMapEntry(handle.inlineEntryId(), std::move(value)); } - // Set the entry for the given untyped handle. If the handle is not valid, do nothing. - void insert(UntypedInlineHandle handle, TPtr value) { - // If the scope id does not match, the handle is not valid. - if (handle.inlineScopeId() != InlineMapRegistry::scopeId()) { - return; - } - - // If the entry id is valid, it is an inline entry. - if (handle.inlineEntryId() < InlineMapRegistry::registrationMapSize()) { - resetInlineMapEntry(handle.inlineEntryId(), std::move(value)); - return; - } - - // Otherwise, try normal map entry. - normal_entries_[handle.inlineEntryKey()] = std::move(value); - } - // Remove the entry for the given key. If the key is not found, do nothing. void remove(absl::string_view key) { // Compute the hash value for the key and avoid computing it again in the lookup. @@ -222,23 +179,6 @@ template class InlineMapRegistry { // Remove the entry for the given handle. If the handle is not valid, do nothing. void remove(InlineHandle handle) { resetInlineMapEntry(handle.inlineEntryId()); } - // Remove the entry for the given untyped handle. If the handle is not valid, do nothing. - void remove(UntypedInlineHandle handle) { - // If the scope id does not match, the handle is not valid. - if (handle.inlineScopeId() != InlineMapRegistry::scopeId()) { - return; - } - - // If the entry id is valid, it is an inline entry. - if (handle.inlineEntryId() < InlineMapRegistry::registrationMapSize()) { - resetInlineMapEntry(handle.inlineEntryId()); - return; - } - - // Otherwise, try normal map entry. - normal_entries_.erase(handle.inlineEntryKey()); - } - void iterate(std::function callback) const { for (const auto& entry : normal_entries_) { if (!callback(entry.first, entry.second.get())) { @@ -303,25 +243,6 @@ template class InlineMapRegistry { return inline_entries_[handle.inlineEntryId()]; } - // Get the entry for the given untyped handle. If the handle is not valid, return nullptr. - T* lookupImpl(UntypedInlineHandle handle) const { - // If the scope id does not match, the handle is not valid. - if (handle.inlineScopeId() != InlineMapRegistry::scopeId()) { - return nullptr; - } - - // If the entry id is valid, it is an inline entry. - if (handle.inlineEntryId() < InlineMapRegistry::registrationMapSize()) { - return inline_entries_[handle.inlineEntryId()]; - } - - // Otherwise, try normal map entry. - if (auto it = normal_entries_.find(handle.inlineEntryKey()); it != normal_entries_.end()) { - return it->second.get(); - } - return nullptr; - } - void resetInlineMapEntry(uint64_t inline_entry_id, TPtr new_entry = nullptr) { ASSERT(inline_entry_id < InlineMapRegistry::registrationMapSize()); if (auto entry = inline_entries_[inline_entry_id]; entry != nullptr) { @@ -424,16 +345,6 @@ template class InlineMapRegistry { return size; } - /** - * Get the scope id for this registry. When called for the first time, this will generate a - * id and register a finalizer to the InlineMapRegistryManager. - */ - static uint64_t scopeId() { - // Scope id is generated on first use. Static ensures that it is only generated once. - static uint64_t scope_id = InlineMapRegistryManager::generateScopeId(); - return scope_id; - } - private: friend class InlineMapRegistryManager; @@ -447,10 +358,10 @@ template class InlineMapRegistry { * in the registrationMapSize. */ static InlineMapRegistryDebugInfo finalize() { - // Initialize the custom inline key registrations. If the initialize() or scopeId() is never - // called before, this call here will generate a scope id and register a finalizer to the - // manager to ensure that the InlineMapRegistryManager always could get all the debug info. - // If the initialize() or scopeId() is already called before, this will be a no-op. + // Initialize the custom inline key registrations. If the initialize()is never be called before, + // this call here will register a finalizer to the manager to ensure + // that the InlineMapRegistryManager always could get all the debug info. If the initialize() is + // already called before, this will be a no-op. initialize(); // Mark the registry as finalized to ensure that no further changes are allowed. @@ -463,8 +374,10 @@ template class InlineMapRegistry { * the first call will have effect and the rest will be no-op. */ static void initialize() { - // Call scopeId() to ensure that the scope is initialized. - scopeId(); + // registerRegistry() will be called on first use. Static ensures the same scope only be + // registered once. + static bool initialized = InlineMapRegistryManager::registerRegistry(); + RELEASE_ASSERT(initialized, "InlineMapRegistryManager::registerRegistry() failed"); } static bool& mutableFinalized() { MUTABLE_CONSTRUCT_ON_FIRST_USE(bool); } @@ -486,20 +399,18 @@ template class InlineMapRegistry { all_inline_keys.push_back(absl::string_view(entry.first)); } - return {scopeId(), Scope::name(), std::move(all_inline_keys)}; + return {Scope::name(), std::move(all_inline_keys)}; } }; -template uint64_t InlineMapRegistryManager::generateScopeId() { +template bool InlineMapRegistryManager::registerRegistry() { // This function should never be called multiple times for same scope. static bool initialized = false; RELEASE_ASSERT(!initialized, "InlineMapRegistryManager::generateScopeId() called twice for same scope"); - const uint64_t scope_id = nextScopeId(); - mutableFinalizers().push_back([scope_id]() -> InlineMapRegistryDebugInfo { + mutableFinalizers().push_back([]() -> InlineMapRegistryDebugInfo { using Registry = InlineMapRegistry; - ASSERT(Registry::scopeId() == scope_id); return Registry::finalize(); }); @@ -507,7 +418,7 @@ template uint64_t InlineMapRegistryManager::generateScopeId() { // if this function is called twice. initialized = true; - return scope_id; + return true; } } // namespace Envoy diff --git a/test/common/common/inline_map_registry_test.cc b/test/common/common/inline_map_registry_test.cc index 96916d5dadfdd..44a4783180ac9 100644 --- a/test/common/common/inline_map_registry_test.cc +++ b/test/common/common/inline_map_registry_test.cc @@ -9,8 +9,8 @@ TEST(InlineMapWithZeroInlineKey, InlineMapWithZeroInlineKeyTest) { auto map = InlineMapRegistry>::InlineMap::createInlineMap(); - EXPECT_EQ(InlineMapRegistryManager::registryInfos().size(), 1); - EXPECT_EQ(InlineMapRegistryManager::registryInfos()[0].registry_scope_name, + EXPECT_EQ(InlineMapRegistryManager::registriesInfo().size(), 1); + EXPECT_EQ(InlineMapRegistryManager::registriesInfo()[0].registry_scope_name, InlineMapRegistryTestScope<0>::name()); // Insert keys. @@ -24,23 +24,9 @@ TEST(InlineMapWithZeroInlineKey, InlineMapWithZeroInlineKeyTest) { EXPECT_EQ(*map->lookup("key_" + std::to_string(i)), "value_" + std::to_string(i)); } - // Lookup by untyped inline handle. - for (size_t i = 0; i < 200; ++i) { - const std::string key = "key_" + std::to_string(i); - UntypedInlineHandle handle(InlineMapRegistry>::scopeId(), i, key); - EXPECT_EQ(*map->lookup(handle), "value_" + std::to_string(i)); - } - // Lookup non-existing keys. EXPECT_EQ(map->lookup("non_existing_key"), nullptr); - // remove keys by untyped inline handle. - for (size_t i = 0; i < 100; ++i) { - const std::string key = "key_" + std::to_string(i); - UntypedInlineHandle handle(InlineMapRegistry>::scopeId(), i, key); - map->remove(handle); - } - // Remove keys. for (size_t i = 0; i < 200; ++i) { map->remove("key_" + std::to_string(i)); @@ -65,14 +51,6 @@ TEST(InlineMapWith20InlineKey, InlineMapWith20InlineKeyTest) { EXPECT_EQ(*map->lookup("key_" + std::to_string(i)), "value_" + std::to_string(i)); } - // Lookup by untyped inline handle. - for (size_t i = 0; i < 200; ++i) { - const std::string key = "key_" + std::to_string(i); - UntypedInlineHandle handle(InlineMapRegistry>::scopeId(), i, - key); - EXPECT_EQ(*map->lookup(handle), "value_" + std::to_string(i)); - } - // Lookup by typed inline handle. for (size_t i = 0; i < inline_hanldes.size(); ++i) { const std::string key = "key_" + std::to_string(i); @@ -92,16 +70,8 @@ TEST(InlineMapWith20InlineKey, InlineMapWith20InlineKeyTest) { map->remove(handle); } - // Remove keys by untyped inline handle. - for (size_t i = 10; i < 30; ++i) { - const std::string key = "key_" + std::to_string(i); - UntypedInlineHandle handle(InlineMapRegistry>::scopeId(), i, - key); - map->remove(handle); - } - // Remove keys. - for (size_t i = 30; i < 200; ++i) { + for (size_t i = 10; i < 200; ++i) { map->remove("key_" + std::to_string(i)); } From 024409ee68345270d9c8ff09a09a2e1cbebe528e Mon Sep 17 00:00:00 2001 From: wbpcode Date: Mon, 19 Jun 2023 11:49:31 +0000 Subject: [PATCH 13/36] minor update Signed-off-by: wbpcode --- envoy/common/inline_map_registry.h | 22 +++++++++---------- .../common/inline_map_registry_test_scope.h | 4 ++-- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/envoy/common/inline_map_registry.h b/envoy/common/inline_map_registry.h index 74a55d8ad352a..5bf9437cc81a0 100644 --- a/envoy/common/inline_map_registry.h +++ b/envoy/common/inline_map_registry.h @@ -345,6 +345,17 @@ template class InlineMapRegistry { return size; } + /** + * Initialize the custom inline key registrations. This may be called multiple times but only + * the first call will have effect and the rest will be no-op. + */ + static void initialize() { + // registerRegistry() will be called on first use. Static ensures the same scope only be + // registered once. + static bool initialized = InlineMapRegistryManager::registerRegistry(); + RELEASE_ASSERT(initialized, "InlineMapRegistryManager::registerRegistry() failed"); + } + private: friend class InlineMapRegistryManager; @@ -369,17 +380,6 @@ template class InlineMapRegistry { return debugInfo(); } - /** - * Initialize the custom inline key registrations. This may be called multiple times but only - * the first call will have effect and the rest will be no-op. - */ - static void initialize() { - // registerRegistry() will be called on first use. Static ensures the same scope only be - // registered once. - static bool initialized = InlineMapRegistryManager::registerRegistry(); - RELEASE_ASSERT(initialized, "InlineMapRegistryManager::registerRegistry() failed"); - } - static bool& mutableFinalized() { MUTABLE_CONSTRUCT_ON_FIRST_USE(bool); } static RegistrationMap& mutableRegistrationMap() { diff --git a/test/common/common/inline_map_registry_test_scope.h b/test/common/common/inline_map_registry_test_scope.h index af830f01e2d4d..d181f2c1c3a2f 100644 --- a/test/common/common/inline_map_registry_test_scope.h +++ b/test/common/common/inline_map_registry_test_scope.h @@ -15,7 +15,7 @@ template class InlineMapRegistryTestScope { std::vector::InlineHandle> handles; // Force the inline map registry to be initialized and be added to the registry manager. - InlineMapRegistry::scopeId(); + InlineMapRegistry::initialize(); for (size_t i = 0; i < N; ++i) { std::string key = "key_" + std::to_string(i); @@ -23,7 +23,7 @@ template class InlineMapRegistryTestScope { } // Force the inline map registry to be finalized. - InlineMapRegistryManager::registryInfos(); + InlineMapRegistryManager::registriesInfo(); return handles; } From 15a8ed4a9d32b1f4bfeaa586691f864a6a901123 Mon Sep 17 00:00:00 2001 From: wbpcode Date: Tue, 20 Jun 2023 10:17:36 +0000 Subject: [PATCH 14/36] refactor and update the filter state example Signed-off-by: wbpcode --- envoy/common/BUILD | 15 - envoy/common/inline_map_registry.h | 424 ------------------ envoy/stream_info/BUILD | 1 + envoy/stream_info/filter_state.h | 79 ++++ source/common/common/BUILD | 15 + source/common/common/inline_map_registry.cc | 54 +++ source/common/common/inline_map_registry.h | 388 ++++++++++++++++ source/common/network/application_protocol.cc | 9 +- source/common/network/application_protocol.h | 2 +- .../common/network/filter_state_proxy_info.cc | 8 +- .../common/network/filter_state_proxy_info.h | 2 +- .../network/proxy_protocol_filter_state.cc | 8 +- .../network/proxy_protocol_filter_state.h | 2 +- source/common/network/upstream_server_name.cc | 9 +- source/common/network/upstream_server_name.h | 2 +- .../network/upstream_subject_alt_names.cc | 9 +- .../network/upstream_subject_alt_names.h | 2 +- source/common/router/debug_config.cc | 8 +- source/common/router/debug_config.h | 2 +- source/common/router/router.cc | 5 +- .../common/stream_info/filter_state_impl.cc | 109 ++--- source/common/stream_info/filter_state_impl.h | 113 ++++- source/server/server.cc | 9 + test/common/common/BUILD | 13 +- .../common/inline_map_registry_speed_test.cc | 37 +- .../common/common/inline_map_registry_test.cc | 33 +- .../common/inline_map_registry_test_scope.h | 39 -- 27 files changed, 770 insertions(+), 627 deletions(-) delete mode 100644 envoy/common/inline_map_registry.h create mode 100644 source/common/common/inline_map_registry.cc create mode 100644 source/common/common/inline_map_registry.h delete mode 100644 test/common/common/inline_map_registry_test_scope.h diff --git a/envoy/common/BUILD b/envoy/common/BUILD index 8823ce31eb269..1a71c7f609689 100644 --- a/envoy/common/BUILD +++ b/envoy/common/BUILD @@ -121,18 +121,3 @@ envoy_cc_library( "//source/common/common:utility_lib", ], ) - -envoy_cc_library( - name = "inline_map_registry", - hdrs = [ - "inline_map_registry.h", - ], - external_deps = [ - "abseil_node_hash_map", - ], - deps = [ - "//source/common/common:assert_lib", - "//source/common/common:macros", - "//source/common/common:utility_lib", - ], -) diff --git a/envoy/common/inline_map_registry.h b/envoy/common/inline_map_registry.h deleted file mode 100644 index 5bf9437cc81a0..0000000000000 --- a/envoy/common/inline_map_registry.h +++ /dev/null @@ -1,424 +0,0 @@ -#pragma once - -#include -#include - -#include "source/common/common/assert.h" -#include "source/common/common/macros.h" -#include "source/common/common/utility.h" - -#include "absl/container/flat_hash_map.h" -#include "absl/container/node_hash_set.h" - -namespace Envoy { - -/** - * Information about the registry. This is used to print out the registry debug information. - */ -struct InlineMapRegistryDebugInfo { - // Registry scope name for the registry, typically short string from - // scope type. - absl::string_view registry_scope_name; - // All inline keys registered for the registry. - std::vector registry_inline_keys; -}; - -/** - * Inline map registry manager that used to get all the inline map registries debug info. - * This is used to print out the inline map registries debug information and try to finalize - * all the inline map registries when the server is starting up. - */ -class InlineMapRegistryManager { -public: - static const std::vector& registriesInfo() { - // Call finalize() to ensure that all finalizers are called. - finalize(); - return mutableRegistriesInfo(); - } - - /** - * Register a finalizer to be called when InlineMapRegistry::finalize() is called. - * - * NOTE: This is called by InlineMapRegistry::finalize() and should never be called - * manually. - */ - template static bool registerRegistry(); - -private: - using Finalizer = std::function; - - /** - * Call all finalizers. This should only be called once, after all registrations are - * complete. - */ - static void finalize() { - auto& registries_info = mutableRegistriesInfo(); - for (const auto& finalizer : mutableFinalizers()) { - registries_info.push_back(finalizer()); - } - mutableFinalizers().clear(); - } - - static std::vector& mutableRegistriesInfo() { - MUTABLE_CONSTRUCT_ON_FIRST_USE(std::vector); - } - - static std::vector& mutableFinalizers() { - MUTABLE_CONSTRUCT_ON_FIRST_USE(std::vector); - } -}; - -/** - * This template could be used as an alternative of normal hash map to store the key/value pairs. - * But by this template, you can register some frequently used keys as inline keys and get the - * handle for the key. Then you can use the handle to access the key/value pair in the map without - * even one time hash searching. - * - * This is useful when some keys are frequently used and the keys are known at compile time or - * bootstrap time. You can get superior performance by using the inline handle. For example, the - * filter state always uses the filter name as the key and the filter name is known at compile time. - * By using the inline handle, the filter state could get the key/value pair without any hash. - * - * This template also provides the interface to access the key/value pair by the normal key and the - * interface has similar searching performance as the normal hash map. But the insertion and removal - * by the normal key is slower than the normal hash map. - * - * The Scope template parameter is used to distinguish different inline map registry. Different - * Scope will have different inline key registrations and different scope id. - * - * These is usage example: - * - * // Define a scope type. - * class ExampleScope { - * public: - * static absl::string_view name() { return "ExampleScope"; } - * }; - * - * // Register the inline key. We should never do this after bootstrapping. Typically, we can - * // do this by define a global variable. - * auto inline_handle = InlineMapRegistry::registerInlineKey("inline_key"); - * - * // Create the inline map. - * auto inline_map = InlineMapRegistry::InlineMap::createInlineMap(); - * - * // Get value by inline handle. - * inline_map->insert(inline_handle, std::make_unique("value")); - * EXPECT_EQ(*inline_map->lookup(inline_handle), "value"); - */ -template class InlineMapRegistry { -public: - class InlineHandle { - public: - uint64_t inlineEntryId() const { return inline_entry_id_; } - - absl::string_view inlineEntryKey() const { return inline_entry_key_; } - - private: - friend class InlineMapRegistry; - - // This constructor should only be called by InlineMapRegistry. - InlineHandle(uint64_t inline_entry_id, absl::string_view inline_entry_key) - : inline_entry_id_(inline_entry_id), inline_entry_key_(inline_entry_key) {} - - uint64_t inline_entry_id_{}; - absl::string_view inline_entry_key_; - }; - - // This is the Hash used for registration map/set and underlay hash map. - using Hash = absl::container_internal::hash_default_hash; - - using RegistrationMap = absl::flat_hash_map; - - // Node hash set is used to store the registration keys because it's entries have stable address - // and it is safe to reference the key in the InlineHandle or RegistrationMap. - using RegistrationSet = absl::node_hash_set; - - template class InlineMap : public InlineStorage { - public: - using TPtr = std::unique_ptr; - using UnderlayHashMap = absl::flat_hash_map; - - // Get the entry for the given key. If the key is not found, return nullptr. - const T* lookup(absl::string_view key) const { return lookupImpl(key); } - T* lookup(absl::string_view key) { return lookupImpl(key); } - - // Get the entry for the given handle. If the handle is not valid, return nullptr. - const T* lookup(InlineHandle handle) const { return lookupImpl(handle); } - T* lookup(InlineHandle handle) { return lookupImpl(handle); } - - // Set the entry for the given key. If the key is already present, overwrite it. - void insert(absl::string_view key, TPtr value) { - // Compute the hash value for the key and avoid computing it again in the lookup. - const size_t hash_value = absl::Hash()(key); - - if (auto entry_id = staticLookup(key, hash_value); entry_id.has_value()) { - resetInlineMapEntry(*entry_id, std::move(value)); - } else { - normal_entries_[key] = std::move(value); - } - } - - // Set the entry for the given handle. If the handle is not valid, do nothing. - void insert(InlineHandle handle, TPtr value) { - ASSERT(handle.inlineEntryId() < InlineMapRegistry::registrationMapSize()); - resetInlineMapEntry(handle.inlineEntryId(), std::move(value)); - } - - // Remove the entry for the given key. If the key is not found, do nothing. - void remove(absl::string_view key) { - // Compute the hash value for the key and avoid computing it again in the lookup. - const size_t hash_value = absl::Hash()(key); - - if (auto entry_id = staticLookup(key, hash_value); entry_id.has_value()) { - resetInlineMapEntry(*entry_id); - } else { - normal_entries_.erase(key); - } - } - - // Remove the entry for the given handle. If the handle is not valid, do nothing. - void remove(InlineHandle handle) { resetInlineMapEntry(handle.inlineEntryId()); } - - void iterate(std::function callback) const { - for (const auto& entry : normal_entries_) { - if (!callback(entry.first, entry.second.get())) { - return; - } - } - - for (const auto& id : InlineMapRegistry::registrationMap()) { - ASSERT(id.second < InlineMapRegistry::registrationMapSize()); - - auto entry = inline_entries_[id.second]; - if (entry == nullptr) { - continue; - } - - if (!callback(id.first, entry.get())) { - return; - } - } - } - - uint64_t size() const { return normal_entries_.size() + inline_entries_size_; } - - static std::unique_ptr createInlineMap() { - return std::unique_ptr( - new ((InlineMapRegistry::registrationMapSize() * sizeof(TPtr))) InlineMap()); - } - - ~InlineMap() { - for (uint64_t i = 0; i < InlineMapRegistry::registrationMapSize(); ++i) { - auto entry = inline_entries_[i]; - if (entry != nullptr) { - delete entry; - } - } - memset(inline_entries_, 0, InlineMapRegistry::registrationMapSize() * sizeof(TPtr)); - } - - private: - InlineMap() { - memset(inline_entries_, 0, InlineMapRegistry::registrationMapSize() * sizeof(TPtr)); - } - - T* lookupImpl(absl::string_view key) const { - // Compute the hash value for the key and avoid computing it again in the lookup. - const size_t hash_value = InlineMapRegistry::Hash()(key); - - // Because the normal string view key is used here, try the normal map entry first. - if (auto it = normal_entries_.find(key, hash_value); it != normal_entries_.end()) { - return it->second.get(); - } - - if (auto entry_id = staticLookup(key, hash_value); entry_id.has_value()) { - return inline_entries_[*entry_id]; - } - - return nullptr; - } - - T* lookupImpl(InlineHandle handle) const { - ASSERT(handle.inlineEntryId() < InlineMapRegistry::registrationMapSize()); - return inline_entries_[handle.inlineEntryId()]; - } - - void resetInlineMapEntry(uint64_t inline_entry_id, TPtr new_entry = nullptr) { - ASSERT(inline_entry_id < InlineMapRegistry::registrationMapSize()); - if (auto entry = inline_entries_[inline_entry_id]; entry != nullptr) { - // Remove and delete the old valid entry. - ASSERT(inline_entries_size_ > 0); - --inline_entries_size_; - delete entry; - } - - if (new_entry != nullptr) { - // Append the new valid entry. - ++inline_entries_size_; - } - - inline_entries_[inline_entry_id] = new_entry.release(); - } - - absl::optional staticLookup(absl::string_view key, size_t hash_value) const { - if (auto iter = InlineMapRegistry::registrationMap().find(key, hash_value); - iter != InlineMapRegistry::registrationMap().end()) { - return iter->second; - } - return absl::nullopt; - } - - // This is the underlay hash map for the normal entries. - UnderlayHashMap normal_entries_; - - uint64_t inline_entries_size_{}; - // This should be the last member of the class and no member should be added after this. - T* inline_entries_[]; - }; - - /** - * Register a custom inline key. May only be called before finalized(). - */ - static InlineHandle registerInlineKey(absl::string_view key) { - RELEASE_ASSERT(!mutableFinalized(), "Cannot register inline key after finalized()"); - - // Initialize the custom inline key registrations. If this is the first time this function - // is called, this will generate a scope id and register a finalizer to the manager. - // Otherwise, this will be a no-op. - initialize(); - - // If the key is already registered, return the existing handle. - if (auto it = mutableRegistrationMap().find(key); it != mutableRegistrationMap().end()) { - // It is safe to reference the key here because the key is stored in the node hash set. - return InlineHandle(it->second, it->first); - } - - // If the key is not registered, create a new handle for it. - - const uint64_t entry_id = mutableRegistrationMap().size(); - - // Insert the key to the node hash set and keep the reference of the key in the - // inline handle and registration map. - auto result = mutableRegistrationSet().emplace(key); - RELEASE_ASSERT(result.second, "The key is already registered and this should never happen"); - - // It is safe to reference the key here because the key is stored in the node hash set. - mutableRegistrationMap().emplace(absl::string_view(*result.first), entry_id); - return InlineHandle(entry_id, absl::string_view(*result.first)); - } - - /** - * Fetch the inline handle for the given key. May only be called after finalized(). This should - * be used to get the inline handle for the key registered by registerInlineKey(). This function - * could used to determine if the key is registered or not at runtime or xDS config loading time - * and decide if the key should be used as inline key or normal key. - */ - absl::optional getInlineHandle(absl::string_view key) { - ASSERT(mutableFinalized(), "Cannot get inline handle before finalized()"); - - if (auto it = mutableRegistrationMap().find(key); it != mutableRegistrationMap().end()) { - // It is safe to reference the key here because the key is stored in the node hash set. - return InlineHandle(it->second, it->first); - } - - return absl::nullopt; - } - - /** - * Fetch all registered key. May only be called after finalized(). - */ - static const RegistrationMap& registrationMap() { - ASSERT(mutableFinalized(), "Cannot fetch registration map before finalized()"); - return mutableRegistrationMap(); - } - - /** - * Fetch the number of registered keys. This will call finalize() automatically to ensure that - * no further changes are allowed. - */ - static uint64_t registrationMapSize() { - static uint64_t size = []() { - finalize(); - return mutableRegistrationMap().size(); - }(); - - return size; - } - - /** - * Initialize the custom inline key registrations. This may be called multiple times but only - * the first call will have effect and the rest will be no-op. - */ - static void initialize() { - // registerRegistry() will be called on first use. Static ensures the same scope only be - // registered once. - static bool initialized = InlineMapRegistryManager::registerRegistry(); - RELEASE_ASSERT(initialized, "InlineMapRegistryManager::registerRegistry() failed"); - } - -private: - friend class InlineMapRegistryManager; - - /** - * Finalize the custom inline key registrations. No further changes are allowed after this - * point. This guaranteed that all map created by the process have the same variable size and - * custom registrations. This function could be called multiple times safely and only the first - * call will have effect. - * - * NOTE: This should only be called by InlineMapRegistryManager with the finalizer or be called - * in the registrationMapSize. - */ - static InlineMapRegistryDebugInfo finalize() { - // Initialize the custom inline key registrations. If the initialize()is never be called before, - // this call here will register a finalizer to the manager to ensure - // that the InlineMapRegistryManager always could get all the debug info. If the initialize() is - // already called before, this will be a no-op. - initialize(); - - // Mark the registry as finalized to ensure that no further changes are allowed. - mutableFinalized() = true; - return debugInfo(); - } - - static bool& mutableFinalized() { MUTABLE_CONSTRUCT_ON_FIRST_USE(bool); } - - static RegistrationMap& mutableRegistrationMap() { - MUTABLE_CONSTRUCT_ON_FIRST_USE(RegistrationMap); - } - - static RegistrationSet& mutableRegistrationSet() { - MUTABLE_CONSTRUCT_ON_FIRST_USE(RegistrationSet); - } - - static InlineMapRegistryDebugInfo debugInfo() { - RELEASE_ASSERT(mutableFinalized(), "Cannot fetch debug info before finalized()"); - - std::vector all_inline_keys; - all_inline_keys.reserve(mutableRegistrationMap().size()); - for (const auto& entry : mutableRegistrationMap()) { - all_inline_keys.push_back(absl::string_view(entry.first)); - } - - return {Scope::name(), std::move(all_inline_keys)}; - } -}; - -template bool InlineMapRegistryManager::registerRegistry() { - // This function should never be called multiple times for same scope. - static bool initialized = false; - RELEASE_ASSERT(!initialized, - "InlineMapRegistryManager::generateScopeId() called twice for same scope"); - - mutableFinalizers().push_back([]() -> InlineMapRegistryDebugInfo { - using Registry = InlineMapRegistry; - return Registry::finalize(); - }); - - // Mark the scope as initialized and the RELEASE_ASSERT will crash Envoy - // if this function is called twice. - initialized = true; - - return true; -} - -} // namespace Envoy diff --git a/envoy/stream_info/BUILD b/envoy/stream_info/BUILD index 10c253696cacc..afa73ed99a172 100644 --- a/envoy/stream_info/BUILD +++ b/envoy/stream_info/BUILD @@ -34,6 +34,7 @@ envoy_cc_library( hdrs = ["filter_state.h"], external_deps = ["abseil_optional"], deps = [ + "//source/common/common:inline_map_registry", "//source/common/common:utility_lib", "//source/common/protobuf", ], diff --git a/envoy/stream_info/filter_state.h b/envoy/stream_info/filter_state.h index b119b4d776d48..9b861e1ea790f 100644 --- a/envoy/stream_info/filter_state.h +++ b/envoy/stream_info/filter_state.h @@ -6,6 +6,7 @@ #include "envoy/common/pure.h" #include "source/common/common/fmt.h" +#include "source/common/common/inline_map_registry.h" #include "source/common/common/utility.h" #include "source/common/protobuf/protobuf.h" @@ -15,6 +16,13 @@ namespace Envoy { namespace StreamInfo { +class FilterStateInlineMapScope { +public: + static absl::string_view name() { return "inline_map_scope_filter_state"; } +}; + +using InlineKey = InlineMapRegistry::InlineKey; + class FilterState; using FilterStateSharedPtr = std::shared_ptr; @@ -134,6 +142,25 @@ class FilterState { LifeSpan life_span = LifeSpan::FilterChain, StreamSharingMayImpactPooling stream_sharing = StreamSharingMayImpactPooling::None) PURE; + /** + * @param data_key the handle of the data being set. + * @param data an owning pointer to the data to be stored. + * @param state_type indicates whether the object is mutable or not. + * @param life_span indicates the life span of the object: bound to the filter chain, a + * request, or a connection. + * + * Note that it is an error to call setData() twice with the same + * data_name, if the existing object is immutable. Similarly, it is an + * error to call setData() with same data_name but different state_types + * (mutable and readOnly, or readOnly and mutable) or different life_span. + * This is to enforce a single authoritative source for each piece of + * data stored in FilterState. + */ + virtual void + setData(InlineKey data_key, std::shared_ptr data, StateType state_type, + LifeSpan life_span = LifeSpan::FilterChain, + StreamSharingMayImpactPooling stream_sharing = StreamSharingMayImpactPooling::None) PURE; + /** * @param data_name the name of the data being looked up (mutable/readonly). * @return a typed pointer to the stored data or nullptr if the data does not exist or the data @@ -143,12 +170,27 @@ class FilterState { return dynamic_cast(getDataReadOnlyGeneric(data_name)); } + /** + * @param data_key the handle of the data being looked up (mutable/readonly). + * @return a typed pointer to the stored data or nullptr if the data does not exist or the data + * type does not match the expected type. + */ + template const T* getDataReadOnly(InlineKey data_key) const { + return dynamic_cast(getDataReadOnlyGeneric(data_key)); + } + /** * @param data_name the name of the data being looked up (mutable/readonly). * @return a const pointer to the stored data or nullptr if the data does not exist. */ virtual const Object* getDataReadOnlyGeneric(absl::string_view data_name) const PURE; + /** + * @param data_key the handle of the data being looked up (mutable/readonly). + * @return a const pointer to the stored data or nullptr if the data does not exist. + */ + virtual const Object* getDataReadOnlyGeneric(InlineKey data_key) const PURE; + /** * @param data_name the name of the data being looked up (mutable/readonly). * @return a typed pointer to the stored data or nullptr if the data does not exist or the data @@ -158,18 +200,39 @@ class FilterState { return dynamic_cast(getDataMutableGeneric(data_name)); } + /** + * @param data_name the name of the data being looked up (mutable/readonly). + * @return a typed pointer to the stored data or nullptr if the data does not exist or the data + * type does not match the expected type. + */ + template T* getDataMutable(InlineKey data_key) { + return dynamic_cast(getDataMutableGeneric(data_key)); + } + /** * @param data_name the name of the data being looked up (mutable/readonly). * @return a pointer to the stored data or nullptr if the data does not exist. */ virtual Object* getDataMutableGeneric(absl::string_view data_name) PURE; + /** + * @param data_key the handle of the data being looked up (mutable/readonly). + * @return a pointer to the stored data or nullptr if the data does not exist. + */ + virtual Object* getDataMutableGeneric(InlineKey data_key) PURE; + /** * @param data_name the name of the data being looked up (mutable/readonly). * @return a shared pointer to the stored data or nullptr if the data does not exist. */ virtual std::shared_ptr getDataSharedMutableGeneric(absl::string_view data_name) PURE; + /** + * @param data_key the handle of the data being looked up (mutable/readonly). + * @return a shared pointer to the stored data or nullptr if the data does not exist. + */ + virtual std::shared_ptr getDataSharedMutableGeneric(InlineKey data_key) PURE; + /** * @param data_name the name of the data being probed. * @return Whether data of the type and name specified exists in the @@ -179,6 +242,15 @@ class FilterState { return getDataReadOnly(data_name) != nullptr; } + /** + * @param data_key the handle of the data being probed. + * @return Whether data of the type and name specified exists in the + * data store. + */ + template bool hasData(InlineKey data_key) const { + return getDataReadOnly(data_key) != nullptr; + } + /** * @param data_name the name of the data being probed. * @return Whether data of any type and the name specified exists in the @@ -186,6 +258,13 @@ class FilterState { */ virtual bool hasDataWithName(absl::string_view data_name) const PURE; + /** + * @param data_key the handle of the data being probed. + * @return Whether data of any type and the name specified exists in the + * data store. + */ + virtual bool hasDataWithHandle(InlineKey data_key) const PURE; + /** * @param life_span the LifeSpan above which data existence is checked. * @return whether data of any type exist with LifeSpan greater than life_span. diff --git a/source/common/common/BUILD b/source/common/common/BUILD index 7c00df4467577..2d48c6c0f77e8 100644 --- a/source/common/common/BUILD +++ b/source/common/common/BUILD @@ -577,3 +577,18 @@ envoy_pch_library( "@envoy_api//envoy/service/discovery/v3:pkg_cc_proto", ], ) + +envoy_cc_library( + name = "inline_map_registry", + srcs = [ + "inline_map_registry.cc", + ], + hdrs = [ + "inline_map_registry.h", + ], + deps = [ + "//source/common/common:assert_lib", + "//source/common/common:macros", + "//source/common/common:utility_lib", + ], +) diff --git a/source/common/common/inline_map_registry.cc b/source/common/common/inline_map_registry.cc new file mode 100644 index 0000000000000..349a275351430 --- /dev/null +++ b/source/common/common/inline_map_registry.cc @@ -0,0 +1,54 @@ +#include "source/common/common/inline_map_registry.h" + +namespace Envoy { + +std::atomic InlineMapRegistry::global_registry_id_{0}; + +InlineMapRegistry::InlineKey InlineMapRegistry::registerInlineKey(absl::string_view key) { + RELEASE_ASSERT(!finalized_, "Cannot register inline key after finalized()"); + + // If the key is already registered, return the existing handle. + if (auto it = registration_map_.find(key); it != registration_map_.end()) { + // It is safe to reference the key here because the key is stored in the node hash set. + return {registry_id_, it->second, it->first}; + } + + // If the key is not registered, create a new handle for it. + + const uint64_t inline_id = registration_map_.size(); + + // Insert the key to the list and keep the reference of the key in the inline handle and + // registration map. + auto result = registration_set_.emplace(registration_set_.end(), key); + + RELEASE_ASSERT(result.second, + "The key should not be registered repeatedly and this should not happen."); + + // It is safe to reference the key here because the key is stored in the node hash set. + registration_map_.emplace(absl::string_view(*result.first), inline_id); + return {registry_id_, inline_id, absl::string_view(*result.first)}; +} + +absl::optional +InlineMapRegistry::getInlineKey(absl::string_view key) const { + ASSERT(finalized_, "Cannot get inline handle before finalized()"); + + if (auto it = registration_map_.find(key); it != registration_map_.end()) { + // It is safe to reference the key here because the key is stored in the node hash set. + return InlineKey(registry_id_, it->second, it->first); + } + + return absl::nullopt; +} + +const InlineMapRegistry::RegistrationMap& InlineMapRegistry::registrationMap() const { + ASSERT(finalized_, "Cannot fetch registration map before finalized()"); + return registration_map_; +} + +uint64_t InlineMapRegistry::registrationMapSize() const { + ASSERT(finalized_, "Cannot fetch registration map before finalized()"); + return registration_map_.size(); +} + +} // namespace Envoy diff --git a/source/common/common/inline_map_registry.h b/source/common/common/inline_map_registry.h new file mode 100644 index 0000000000000..5fc538833c70b --- /dev/null +++ b/source/common/common/inline_map_registry.h @@ -0,0 +1,388 @@ +#pragma once + +#include +#include + +#include "source/common/common/assert.h" +#include "source/common/common/macros.h" +#include "source/common/common/utility.h" + +#include "absl/container/flat_hash_map.h" +#include "absl/container/node_hash_set.h" + +namespace Envoy { + +/** + * This class could be used as an alternative of normal hash map to store the key/value pairs. + * But by this class, you can register some frequently used keys as inline keys and get the + * handle for the key. Then you can use the handle to access the key/value pair in the map without + * even one time hash searching. + * + * This is useful when some keys are frequently used and the keys are known at compile time or + * bootstrap time. You can get superior performance by using the inline handle. For example, the + * filter state always uses the filter name as the key and the filter name is known at compile time. + * By using the inline handle, the filter state could get the key/value pair without any hash. + * + * This class also provides the interface to access the key/value pair by the normal key and the + * interface has similar searching performance as the normal hash map. But the insertion and removal + * by the normal key is slower than the normal hash map. + * + * + * These is usage example: + * + * // Define registry to store the inline keys. The registry should have lifetime that no shorter + * // than all inline maps created by the registry. + * InlineMapRegistry registry; + * + * // Register the inline key. We should never do this after bootstrapping. Typically, we can + * // do this by define a global variable. + * auto inline_handle = registry.registerInlineKey("inline_key"); + * + * // Finalize the registry. No further changes are allowed to the registry after this point. + * registry.finalize(); + * + * // Create the inline map. + * auto inline_map = registry.createInlineMap(); + * + * // Get value by inline handle. + * inline_map->insert(inline_handle, std::make_unique("value")); + * EXPECT_EQ(*inline_map->lookup(inline_handle), "value"); + */ +class InlineMapRegistry { +public: + /** + * This is the handle used to access the key/value pair in the inline map. The handle is + * lightweight and could be copied and passed by value. + */ + class InlineKey { + public: + /** + * Get the id of the inline map registry that the handle created by. This will be used + * to determine if the handle could be accepted by a inline map. + */ + uint64_t registryId() const { return registry_id_; } + + /** + * Get the id of the inline entry in the inline map. This could be used to access the + * key/value pair in the inline map. + */ + uint64_t inlineId() const { return inline_id_; } + + /** + * Get the key view of the inline key. + */ + absl::string_view inlineKey() const { return inline_key_; } + + private: + friend class InlineMapRegistry; + + // This constructor should only be called by InlineMapRegistry. + InlineKey(uint64_t registry_id, uint64_t inline_id, absl::string_view inline_key) + : registry_id_(registry_id), inline_id_(inline_id), inline_key_(inline_key) {} + + uint64_t registry_id_{}; + uint64_t inline_id_{}; + absl::string_view inline_key_; + }; + + // This is the Hash used for registration map/set and underlay hash map. + using Hash = absl::container_internal::hash_default_hash; + + using RegistrationMap = absl::flat_hash_map; + + // Set to store the inline keys. Because the node of the set have stable address, we can reference + // the key in the handle or registration map safely. + using RegistrationSet = absl::node_hash_set; + + template class InlineMap : public InlineStorage { + public: + using TPtr = std::unique_ptr; + using UnderlayHashMap = absl::flat_hash_map; + + // Get the entry for the given key. If the key is not found, return nullptr. + const T* lookup(absl::string_view key) const { return lookupImpl(key); } + T* lookup(absl::string_view key) { return lookupImpl(key); } + + // Get the entry for the given handle. If the handle is not valid, return nullptr. + const T* lookup(InlineKey handle) const { return lookupImpl(handle); } + T* lookup(InlineKey handle) { return lookupImpl(handle); } + + // Set the entry for the given key. If the key is already present, overwrite it. + void insert(absl::string_view key, TPtr value) { + // Compute the hash value for the key and avoid computing it again in the lookup. + const size_t hash_value = absl::Hash()(key); + + if (auto entry_id = inlineLookup(key, hash_value); entry_id.has_value()) { + resetInlineMapEntry(*entry_id, std::move(value)); + } else { + normal_entries_[key] = std::move(value); + } + } + + // Set the entry for the given handle. If the handle is not valid, do nothing. + void insert(InlineKey handle, TPtr value) { + if (handle.registryId() != registry_.registryId()) { + return; + } + + ASSERT(handle.inlineId() < registry_.registrationNum()); + resetInlineMapEntry(handle.inlineId(), std::move(value)); + } + + // Remove the entry for the given key. If the key is not found, do nothing. + void remove(absl::string_view key) { + // Compute the hash value for the key and avoid computing it again in the lookup. + const size_t hash_value = absl::Hash()(key); + + if (auto entry_id = inlineLookup(key, hash_value); entry_id.has_value()) { + resetInlineMapEntry(*entry_id); + } else { + normal_entries_.erase(key); + } + } + + // Remove the entry for the given handle. If the handle is not valid, do nothing. + void remove(InlineKey handle) { + if (handle.registryId() != registry_.registryId()) { + return; + } + resetInlineMapEntry(handle.inlineId()); + } + + void iterate(std::function callback) const { + for (const auto& entry : normal_entries_) { + if (!callback(entry.first, entry.second.get())) { + return; + } + } + + for (const auto& id : registry_.registrationMap()) { + ASSERT(id.second < registry_.registrationNum()); + + auto entry = inline_entries_[id.second]; + if (entry == nullptr) { + continue; + } + + if (!callback(id.first, entry.get())) { + return; + } + } + } + + uint64_t size() const { return normal_entries_.size() + inline_entries_size_; } + + bool empty() const { return size() == 0; } + + // Get the registry id that the inline map created by. + uint64_t registryId() const { return registry_.registryId(); } + + ~InlineMap() { + for (uint64_t i = 0; i < registry_.registrationNum(); ++i) { + auto entry = inline_entries_[i]; + if (entry != nullptr) { + delete entry; + } + } + memset(inline_entries_, 0, registry_.registrationNum() * sizeof(TPtr)); + } + + private: + friend class InlineMapRegistry; + + static std::unique_ptr create(InlineMapRegistry& registry) { + return std::unique_ptr>(new ( + (registry.registrationMap().size() * sizeof(InlineMap::TPtr))) InlineMap(registry)); + } + + InlineMap(InlineMapRegistry& registry) : registry_(registry) { + // Initialize the inline entries to nullptr. + memset(inline_entries_, 0, registry_.registrationNum() * sizeof(TPtr)); + } + + T* lookupImpl(absl::string_view key) const { + // Compute the hash value for the key and avoid computing it again in the lookup. + const size_t hash_value = InlineMapRegistry::Hash()(key); + + // Because the normal string view key is used here, try the normal map entry first. + if (auto it = normal_entries_.find(key, hash_value); it != normal_entries_.end()) { + return it->second.get(); + } + + if (auto entry_id = inlineLookup(key, hash_value); entry_id.has_value()) { + return inline_entries_[*entry_id]; + } + + return nullptr; + } + + T* lookupImpl(InlineKey handle) const { + if (handle.registryId() != registry_.registryId()) { + return nullptr; + } + + ASSERT(handle.inlineId() < registry_.registrationNum()); + return inline_entries_[handle.inlineId()]; + } + + void resetInlineMapEntry(uint64_t inline_entry_id, TPtr new_entry = nullptr) { + ASSERT(inline_entry_id < registry_.registrationNum()); + if (auto entry = inline_entries_[inline_entry_id]; entry != nullptr) { + // Remove and delete the old valid entry. + ASSERT(inline_entries_size_ > 0); + --inline_entries_size_; + delete entry; + } + + if (new_entry != nullptr) { + // Append the new valid entry. + ++inline_entries_size_; + } + + inline_entries_[inline_entry_id] = new_entry.release(); + } + + absl::optional inlineLookup(absl::string_view key, size_t hash_value) const { + const auto& map_ref = registry_.registrationMap(); + if (auto iter = map_ref.find(key, hash_value); iter != map_ref.end()) { + return iter->second; + } + return absl::nullopt; + } + + // This is the reference of the registry that the inline map created by. This is used to + // validate the inline handle validity and get the inline key set. + const InlineMapRegistry& registry_; + + // This is the underlay hash map for the normal entries. + UnderlayHashMap normal_entries_; + + uint64_t inline_entries_size_{}; + // This should be the last member of the class and no member should be added after this. + T* inline_entries_[]; + }; + + template using InlineMapPtr = std::unique_ptr>; + + InlineMapRegistry() : registry_id_(generateRegistryId()) {} + + /** + * Register a custom inline key. May only be called before finalized(). + */ + InlineKey registerInlineKey(absl::string_view key); + + /** + * Fetch the inline handle for the given key. May only be called after finalized(). This should + * be used to get the inline handle for the key registered by registerInlineKey(). This function + * could used to determine if the key is registered or not at runtime or xDS config loading time + * and decide if the key should be used as inline key or normal key. + */ + absl::optional getInlineKey(absl::string_view key) const; + + /** + * Get the registration map that contains all registered inline keys. May only be called after + * finalized(). + */ + const RegistrationMap& registrationMap() const; + + /** + * Get the registration set that contains all registered inline keys. May only be called after + * finalized(). + */ + const RegistrationSet& registrationSet() const { return registration_set_; } + + /** + * Get the number of the registrations. May only be called after finalized(). + */ + uint64_t registrationNum() const; + + uint64_t registryId() const { return registry_id_; } + + // Finalize this registry. No further changes are allowed after this point. This guaranteed that + // all map created by the process have the same variable size and custom registrations. This + // function could be called multiple times safely and only the first call will have effect. + void finalize() { finalized_ = true; } + + template InlineMapPtr createInlineMap() { + ASSERT(finalized_, "Cannot create inline map before finalized()"); + + // Call finalize() anyway to make sure that the registry is finalized and no any new inline + // keys could be registered. + finalize(); + return InlineMap::create(*this); + } + +private: + static std::atomic global_registry_id_; + static uint64_t generateRegistryId() { return global_registry_id_++; } + + const uint64_t registry_id_{}; + + bool finalized_{}; + RegistrationSet registration_set_; + RegistrationMap registration_map_; +}; + +/** + * Helper class to get inline map registry singleton based on the scope. If cross-modules inline map + * registry is necessary, this class should be used. + */ +class InlineMapRegistryHelper { +public: + using ManagedRegistries = absl::flat_hash_map; + + /** + * Get the inline map registry singleton based on the scope. + */ + template + static InlineMapRegistry::InlineKey registerInlinKey(absl::string_view key) { + return mutableRegistry().registerInlineKey(key); + } + + /** + * Get registered inline key by the key name. + */ + template + static absl::optional getInlineKey(absl::string_view key) { + return mutableRegistry().getInlineKey(key); + } + + /** + * Create inline map based on the scope. + */ + template static InlineMapRegistry::InlineMapPtr createInlineMap() { + return mutableRegistry().template createInlineMap(); + } + + /** + * Get all registries managed by this helper. This this helpful to finalize all registries and + * print the debug info of inline map registry. + */ + static const ManagedRegistries& registries() { return mutableRegistries(); } + + /** + * Finalize all registries managed by this helper. This should be called after all inline keys + * are registered. + */ + static void finalize() { + for (auto& registry : mutableRegistries()) { + registry.second->finalize(); + } + } + +private: + template static InlineMapRegistry& mutableRegistry() { + MUTABLE_CONSTRUCT_ON_FIRST_USE(InlineMapRegistry, []() { + auto* registry = new InlineMapRegistry(); + auto result = InlineMapRegistryHelper::mutableRegistries().emplace(Scope::name(), registry); + RELEASE_ASSERT(result.second, "Duplicate inline map registry for scope: " + Scope::name()); + return registry; + }()); + } + + static ManagedRegistries& mutableRegistries() { + MUTABLE_CONSTRUCT_ON_FIRST_USE(ManagedRegistries); + } +}; + +} // namespace Envoy diff --git a/source/common/network/application_protocol.cc b/source/common/network/application_protocol.cc index 6794194ac90d5..883413773c63e 100644 --- a/source/common/network/application_protocol.cc +++ b/source/common/network/application_protocol.cc @@ -5,8 +5,11 @@ namespace Envoy { namespace Network { -const std::string& ApplicationProtocols::key() { - CONSTRUCT_ON_FIRST_USE(std::string, "envoy.network.application_protocols"); -} +auto application_protocols_inline_key = + InlineMapRegistryHelper::registerInlinKey( + "envoy.network.application_protocols"); + +const StreamInfo::InlineKey ApplicationProtocols::key() { return application_protocols_inline_key; } + } // namespace Network } // namespace Envoy diff --git a/source/common/network/application_protocol.h b/source/common/network/application_protocol.h index 177e584a5eb55..0628a4d3f8b7b 100644 --- a/source/common/network/application_protocol.h +++ b/source/common/network/application_protocol.h @@ -14,7 +14,7 @@ class ApplicationProtocols : public StreamInfo::FilterState::Object { explicit ApplicationProtocols(const std::vector& application_protocols) : application_protocols_(application_protocols) {} const std::vector& value() const { return application_protocols_; } - static const std::string& key(); + static const StreamInfo::InlineKey key(); private: const std::vector application_protocols_; diff --git a/source/common/network/filter_state_proxy_info.cc b/source/common/network/filter_state_proxy_info.cc index 0f43ef5e17341..4db945e30e565 100644 --- a/source/common/network/filter_state_proxy_info.cc +++ b/source/common/network/filter_state_proxy_info.cc @@ -3,8 +3,12 @@ namespace Envoy { namespace Network { -const std::string& Http11ProxyInfoFilterState::key() { - CONSTRUCT_ON_FIRST_USE(std::string, "envoy.network.transport_socket.http_11_proxy.info"); +auto http11_proxy_info_inline_key = + InlineMapRegistryHelper::registerInlinKey( + "envoy.network.transport_socket.http_11_proxy.info"); + +const StreamInfo::InlineKey Http11ProxyInfoFilterState::key() { + return http11_proxy_info_inline_key; } } // namespace Network diff --git a/source/common/network/filter_state_proxy_info.h b/source/common/network/filter_state_proxy_info.h index e06beeccbd488..33521e753de6c 100644 --- a/source/common/network/filter_state_proxy_info.h +++ b/source/common/network/filter_state_proxy_info.h @@ -15,7 +15,7 @@ namespace Network { class Http11ProxyInfoFilterState : public StreamInfo::FilterState::Object { public: // Returns the key for looking up the Http11ProxyInfoFilterState in the FilterState. - static const std::string& key(); + static const StreamInfo::InlineKey key(); Http11ProxyInfoFilterState(absl::string_view hostname, Network::Address::InstanceConstSharedPtr address) diff --git a/source/common/network/proxy_protocol_filter_state.cc b/source/common/network/proxy_protocol_filter_state.cc index 37940bbad746a..9fd9af7e9714e 100644 --- a/source/common/network/proxy_protocol_filter_state.cc +++ b/source/common/network/proxy_protocol_filter_state.cc @@ -5,8 +5,12 @@ namespace Envoy { namespace Network { -const std::string& ProxyProtocolFilterState::key() { - CONSTRUCT_ON_FIRST_USE(std::string, "envoy.network.proxy_protocol_options"); +auto proxy_protocol_options_inline_key = + InlineMapRegistryHelper::registerInlinKey( + "envoy.network.proxy_protocol_options"); + +const StreamInfo::InlineKey ProxyProtocolFilterState::key() { + return proxy_protocol_options_inline_key; } } // namespace Network diff --git a/source/common/network/proxy_protocol_filter_state.h b/source/common/network/proxy_protocol_filter_state.h index 9cb35a9ee8781..83710c662eec6 100644 --- a/source/common/network/proxy_protocol_filter_state.h +++ b/source/common/network/proxy_protocol_filter_state.h @@ -13,7 +13,7 @@ class ProxyProtocolFilterState : public StreamInfo::FilterState::Object { public: ProxyProtocolFilterState(Network::ProxyProtocolData options) : options_(options) {} const Network::ProxyProtocolData& value() const { return options_; } - static const std::string& key(); + static const StreamInfo::InlineKey key(); private: const Network::ProxyProtocolData options_; diff --git a/source/common/network/upstream_server_name.cc b/source/common/network/upstream_server_name.cc index c6bc2559679eb..52bd8db4ec023 100644 --- a/source/common/network/upstream_server_name.cc +++ b/source/common/network/upstream_server_name.cc @@ -5,8 +5,11 @@ namespace Envoy { namespace Network { -const std::string& UpstreamServerName::key() { - CONSTRUCT_ON_FIRST_USE(std::string, "envoy.network.upstream_server_name"); -} +auto upstream_server_name_inline_key = + InlineMapRegistryHelper::registerInlinKey( + "envoy.network.upstream_server_name"); + +const StreamInfo::InlineKey UpstreamServerName::key() { return upstream_server_name_inline_key; } + } // namespace Network } // namespace Envoy diff --git a/source/common/network/upstream_server_name.h b/source/common/network/upstream_server_name.h index e50bb906af5c6..9f244f3b09c17 100644 --- a/source/common/network/upstream_server_name.h +++ b/source/common/network/upstream_server_name.h @@ -16,7 +16,7 @@ class UpstreamServerName : public StreamInfo::FilterState::Object { public: UpstreamServerName(absl::string_view server_name) : server_name_(server_name) {} const std::string& value() const { return server_name_; } - static const std::string& key(); + static const StreamInfo::InlineKey key(); private: const std::string server_name_; diff --git a/source/common/network/upstream_subject_alt_names.cc b/source/common/network/upstream_subject_alt_names.cc index df8aa9adb1842..e32698783d72c 100644 --- a/source/common/network/upstream_subject_alt_names.cc +++ b/source/common/network/upstream_subject_alt_names.cc @@ -5,8 +5,13 @@ namespace Envoy { namespace Network { -const std::string& UpstreamSubjectAltNames::key() { - CONSTRUCT_ON_FIRST_USE(std::string, "envoy.network.upstream_subject_alt_names"); +auto upstream_subject_alt_names_inline_key = + InlineMapRegistryHelper::registerInlinKey( + "envoy.network.upstream_subject_alt_names"); + +const StreamInfo::InlineKey UpstreamSubjectAltNames::key() { + return upstream_subject_alt_names_inline_key; } + } // namespace Network } // namespace Envoy diff --git a/source/common/network/upstream_subject_alt_names.h b/source/common/network/upstream_subject_alt_names.h index a7ed77905508d..4ac7ab5189982 100644 --- a/source/common/network/upstream_subject_alt_names.h +++ b/source/common/network/upstream_subject_alt_names.h @@ -14,7 +14,7 @@ class UpstreamSubjectAltNames : public StreamInfo::FilterState::Object { explicit UpstreamSubjectAltNames(const std::vector& upstream_subject_alt_names) : upstream_subject_alt_names_(upstream_subject_alt_names) {} const std::vector& value() const { return upstream_subject_alt_names_; } - static const std::string& key(); + static const StreamInfo::InlineKey key(); private: const std::vector upstream_subject_alt_names_; diff --git a/source/common/router/debug_config.cc b/source/common/router/debug_config.cc index 4b6d4f3b5cb41..1cf9f7b74c0a4 100644 --- a/source/common/router/debug_config.cc +++ b/source/common/router/debug_config.cc @@ -5,6 +5,10 @@ namespace Envoy { namespace Router { +auto debug_config_inline_key = + InlineMapRegistryHelper::registerInlinKey( + "envoy.router.debug_config"); + DebugConfig::DebugConfig(bool append_cluster, absl::optional cluster_header, bool append_upstream_host, absl::optional hostname_header, @@ -16,9 +20,7 @@ DebugConfig::DebugConfig(bool append_cluster, absl::optional( + "num_internal_redirects"); uint32_t getLength(const Buffer::Instance* instance) { return instance ? instance->length() : 0; } diff --git a/source/common/stream_info/filter_state_impl.cc b/source/common/stream_info/filter_state_impl.cc index 9a997de0ccee4..4eb70383bdb3b 100644 --- a/source/common/stream_info/filter_state_impl.cc +++ b/source/common/stream_info/filter_state_impl.cc @@ -8,119 +8,74 @@ namespace StreamInfo { void FilterStateImpl::setData(absl::string_view data_name, std::shared_ptr data, FilterState::StateType state_type, FilterState::LifeSpan life_span, StreamSharingMayImpactPooling stream_sharing) { - if (life_span > life_span_) { - if (hasDataWithNameInternally(data_name)) { - IS_ENVOY_BUG("FilterStateAccessViolation: FilterState::setData called twice with " - "conflicting life_span on the same data_name."); - return; - } - maybeCreateParent(ParentAccessMode::ReadWrite); - parent_->setData(data_name, data, state_type, life_span, stream_sharing); - return; - } - if (parent_ && parent_->hasDataWithName(data_name)) { - IS_ENVOY_BUG("FilterStateAccessViolation: FilterState::setData called twice with " - "conflicting life_span on the same data_name."); - return; - } - const auto& it = data_storage_.find(data_name); - if (it != data_storage_.end()) { - // We have another object with same data_name. Check for mutability - // violations namely: readonly data cannot be overwritten, mutable data - // cannot be overwritten by readonly data. - const FilterStateImpl::FilterObject* current = it->second.get(); - if (current->state_type_ == FilterState::StateType::ReadOnly) { - IS_ENVOY_BUG("FilterStateAccessViolation: FilterState::setData called twice on same " - "ReadOnly state."); - return; - } - - if (current->state_type_ != state_type) { - IS_ENVOY_BUG("FilterStateAccessViolation: FilterState::setData called twice with " - "different state types."); - return; - } - } + setDataInternal(data_name, data, state_type, life_span, stream_sharing); +} - std::unique_ptr filter_object(new FilterStateImpl::FilterObject()); - filter_object->data_ = data; - filter_object->state_type_ = state_type; - filter_object->stream_sharing_ = stream_sharing; - data_storage_[data_name] = std::move(filter_object); +void FilterStateImpl::setData(InlineKey data_key, std::shared_ptr data, + StateType state_type, LifeSpan life_span, + StreamSharingMayImpactPooling stream_sharing) { + setDataInternal(data_key, data, state_type, life_span, stream_sharing); } bool FilterStateImpl::hasDataWithName(absl::string_view data_name) const { - return hasDataWithNameInternally(data_name) || (parent_ && parent_->hasDataWithName(data_name)); + return hasDataInternal(data_name) || (parent_ && parent_->hasDataWithName(data_name)); +} +bool FilterStateImpl::hasDataWithHandle(InlineKey data_key) const { + return hasDataInternal(data_key) || (parent_ && parent_->hasDataWithHandle(data_key)); } const FilterState::Object* FilterStateImpl::getDataReadOnlyGeneric(absl::string_view data_name) const { - const auto it = data_storage_.find(data_name); - - if (it == data_storage_.end()) { - if (parent_) { - return parent_->getDataReadOnlyGeneric(data_name); - } - return nullptr; - } - - const FilterStateImpl::FilterObject* current = it->second.get(); - return current->data_.get(); + return getDataReadOnlyGenericInternal(data_name); +} +const FilterState::Object* FilterStateImpl::getDataReadOnlyGeneric(InlineKey data_key) const { + return getDataReadOnlyGenericInternal(data_key); } FilterState::Object* FilterStateImpl::getDataMutableGeneric(absl::string_view data_name) { return getDataSharedMutableGeneric(data_name).get(); } +FilterState::Object* FilterStateImpl::getDataMutableGeneric(InlineKey data_key) { + return getDataSharedMutableGeneric(data_key).get(); +} std::shared_ptr FilterStateImpl::getDataSharedMutableGeneric(absl::string_view data_name) { - const auto& it = data_storage_.find(data_name); - - if (it == data_storage_.end()) { - if (parent_) { - return parent_->getDataSharedMutableGeneric(data_name); - } - return nullptr; - } - - FilterStateImpl::FilterObject* current = it->second.get(); - if (current->state_type_ == FilterState::StateType::ReadOnly) { - IS_ENVOY_BUG("FilterStateAccessViolation: FilterState accessed immutable data as mutable."); - // To reduce the chances of a crash, allow the mutation in this case instead of returning a - // nullptr. - } - - return current->data_; + return getDataSharedMutableGenericInternal(data_name); +} +std::shared_ptr +FilterStateImpl::getDataSharedMutableGeneric(InlineKey data_key) { + return getDataSharedMutableGenericInternal(data_key); } bool FilterStateImpl::hasDataAtOrAboveLifeSpan(FilterState::LifeSpan life_span) const { if (life_span > life_span_) { return parent_ && parent_->hasDataAtOrAboveLifeSpan(life_span); } - return !data_storage_.empty() || (parent_ && parent_->hasDataAtOrAboveLifeSpan(life_span)); + return !data_storage_->empty() || (parent_ && parent_->hasDataAtOrAboveLifeSpan(life_span)); } FilterState::ObjectsPtr FilterStateImpl::objectsSharedWithUpstreamConnection() const { auto objects = parent_ ? parent_->objectsSharedWithUpstreamConnection() : std::make_unique(); - for (const auto& [name, object] : data_storage_) { + + data_storage_->iterate([&objects](absl::string_view name, FilterObject* object) -> bool { switch (object->stream_sharing_) { case StreamSharingMayImpactPooling::SharedWithUpstreamConnection: - objects->push_back({object->data_, object->state_type_, object->stream_sharing_, name}); + objects->push_back( + {object->data_, object->state_type_, object->stream_sharing_, std::string(name)}); break; case StreamSharingMayImpactPooling::SharedWithUpstreamConnectionOnce: - objects->push_back( - {object->data_, object->state_type_, StreamSharingMayImpactPooling::None, name}); + objects->push_back({object->data_, object->state_type_, StreamSharingMayImpactPooling::None, + std::string(name)}); break; default: break; } - } - return objects; -} + return true; + }); -bool FilterStateImpl::hasDataWithNameInternally(absl::string_view data_name) const { - return data_storage_.count(data_name) > 0; + return objects; } void FilterStateImpl::maybeCreateParent(ParentAccessMode parent_access_mode) { diff --git a/source/common/stream_info/filter_state_impl.h b/source/common/stream_info/filter_state_impl.h index b6c736ffc8470..72327751aae54 100644 --- a/source/common/stream_info/filter_state_impl.h +++ b/source/common/stream_info/filter_state_impl.h @@ -14,7 +14,10 @@ namespace StreamInfo { class FilterStateImpl : public FilterState { public: - FilterStateImpl(FilterState::LifeSpan life_span) : life_span_(life_span) { + FilterStateImpl(FilterState::LifeSpan life_span) + : life_span_(life_span), + data_storage_( + InlineMapRegistryHelper::createInlineMap()) { maybeCreateParent(ParentAccessMode::ReadOnly); } @@ -23,7 +26,9 @@ class FilterStateImpl : public FilterState { * @param life_span the life span this is handling. */ FilterStateImpl(FilterStateSharedPtr ancestor, FilterState::LifeSpan life_span) - : ancestor_(ancestor), life_span_(life_span) { + : ancestor_(ancestor), life_span_(life_span), + data_storage_( + InlineMapRegistryHelper::createInlineMap()) { maybeCreateParent(ParentAccessMode::ReadOnly); } @@ -35,7 +40,9 @@ class FilterStateImpl : public FilterState { * @param life_span the life span this is handling. */ FilterStateImpl(LazyCreateAncestor lazy_create_ancestor, FilterState::LifeSpan life_span) - : ancestor_(lazy_create_ancestor), life_span_(life_span) { + : ancestor_(lazy_create_ancestor), life_span_(life_span), + data_storage_( + InlineMapRegistryHelper::createInlineMap()) { maybeCreateParent(ParentAccessMode::ReadOnly); } @@ -44,10 +51,23 @@ class FilterStateImpl : public FilterState { absl::string_view data_name, std::shared_ptr data, FilterState::StateType state_type, FilterState::LifeSpan life_span = FilterState::LifeSpan::FilterChain, StreamSharingMayImpactPooling stream_sharing = StreamSharingMayImpactPooling::None) override; + void setData( + InlineKey data_key, std::shared_ptr data, StateType state_type, + LifeSpan life_span = LifeSpan::FilterChain, + StreamSharingMayImpactPooling stream_sharing = StreamSharingMayImpactPooling::None) override; + bool hasDataWithName(absl::string_view) const override; + bool hasDataWithHandle(InlineKey data_key) const override; + const Object* getDataReadOnlyGeneric(absl::string_view data_name) const override; + const Object* getDataReadOnlyGeneric(InlineKey data_key) const override; + Object* getDataMutableGeneric(absl::string_view data_name) override; + Object* getDataMutableGeneric(InlineKey data_key) override; + std::shared_ptr getDataSharedMutableGeneric(absl::string_view data_name) override; + std::shared_ptr getDataSharedMutableGeneric(InlineKey data_key) override; + bool hasDataAtOrAboveLifeSpan(FilterState::LifeSpan life_span) const override; FilterState::ObjectsPtr objectsSharedWithUpstreamConnection() const override; @@ -55,15 +75,98 @@ class FilterStateImpl : public FilterState { FilterStateSharedPtr parent() const override { return parent_; } private: + template + void setDataInternal(DataKeyType data_key, std::shared_ptr data, StateType state_type, + LifeSpan life_span, StreamSharingMayImpactPooling stream_sharing) { + + if (life_span > life_span_) { + if (hasDataWithNameInternally(data_key)) { + IS_ENVOY_BUG("FilterStateAccessViolation: FilterState::setData called twice with " + "conflicting life_span on the same data_name."); + return; + } + maybeCreateParent(ParentAccessMode::ReadWrite); + parent_->setData(data_key, data, state_type, life_span, stream_sharing); + return; + } + if (parent_ && parent_->hasDataWithName(data_key)) { + IS_ENVOY_BUG("FilterStateAccessViolation: FilterState::setData called twice with " + "conflicting life_span on the same data_name."); + return; + } + const auto* current = data_storage_->lookup(data_key); + if (current != nullptr) { + // We have another object with same data_name. Check for mutability + // violations namely: readonly data cannot be overwritten, mutable data + // cannot be overwritten by readonly data. + if (current->state_type_ == FilterState::StateType::ReadOnly) { + IS_ENVOY_BUG("FilterStateAccessViolation: FilterState::setData called twice on same " + "ReadOnly state."); + return; + } + + if (current->state_type_ != state_type) { + IS_ENVOY_BUG("FilterStateAccessViolation: FilterState::setData called twice with " + "different state types."); + return; + } + } + + std::unique_ptr filter_object( + new FilterStateImpl::FilterObject()); + filter_object->data_ = data; + filter_object->state_type_ = state_type; + filter_object->stream_sharing_ = stream_sharing; + data_storage_->insert(data_key, std::move(filter_object)); + } + // This only checks the local data_storage_ for data_name existence. - bool hasDataWithNameInternally(absl::string_view data_name) const; + template bool hasDataInternal(DataKeyType data_key) const { + return data_storage_->lookup(data_key) != nullptr; + } + + template + const FilterState::Object* getDataReadOnlyGenericInternal(DataKeyType data_key) const { + const auto* current = data_storage_->lookup(data_key); + + if (current == nullptr) { + if (parent_) { + return parent_->getDataReadOnlyGeneric(data_key); + } + return nullptr; + } + + return current->data_.get(); + } + + template + std::shared_ptr getDataSharedMutableGenericInternal(DataKeyType data_key) { + const auto* current = data_storage_->lookup(data_key); + + if (current == nullptr) { + if (parent_) { + return parent_->getDataSharedMutableGeneric(data_key); + } + return nullptr; + } + + if (current->state_type_ == FilterState::StateType::ReadOnly) { + IS_ENVOY_BUG("FilterStateAccessViolation: FilterState accessed immutable data as mutable."); + // To reduce the chances of a crash, allow the mutation in this case instead of returning a + // nullptr. + } + + return current->data_; + } + enum class ParentAccessMode { ReadOnly, ReadWrite }; void maybeCreateParent(ParentAccessMode parent_access_mode); absl::variant ancestor_; FilterStateSharedPtr parent_; const FilterState::LifeSpan life_span_; - absl::flat_hash_map> data_storage_; + + InlineMapRegistry::InlineMapPtr data_storage_; }; } // namespace StreamInfo diff --git a/source/server/server.cc b/source/server/server.cc index 8ed78eb8b139c..b3ee0b4a53b2b 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -27,6 +27,7 @@ #include "source/common/api/api_impl.h" #include "source/common/api/os_sys_calls_impl.h" #include "source/common/common/enum_to_int.h" +#include "source/common/common/inline_map_registry.h" #include "source/common/common/mutex_tracer_impl.h" #include "source/common/common/utility.h" #include "source/common/config/utility.h" @@ -473,6 +474,14 @@ void InstanceImpl::initialize(Network::Address::InstanceConstSharedPtr local_add absl::StrJoin(info.registered_headers_, ",")); } + // Finalize all managed inline map registries. + InlineMapRegistryHelper::finalize(); + ENVOY_LOG(info, "Inline map registry info:"); + for (const auto& registry : InlineMapRegistryHelper::registries()) { + ENVOY_LOG(info, " {}: {}", registry.first, + absl::StrJoin(registry.second->registrationSet(), ",")); + }; + // Initialize the regex engine and inject to singleton. // Needs to happen before stats store initialization because the stats // matcher config can include regexes. diff --git a/test/common/common/BUILD b/test/common/common/BUILD index 11d9bd06bc1d6..cd64a80ac23b7 100644 --- a/test/common/common/BUILD +++ b/test/common/common/BUILD @@ -4,7 +4,6 @@ load( "envoy_cc_benchmark_binary", "envoy_cc_fuzz_test", "envoy_cc_test", - "envoy_cc_test_library", "envoy_package", "envoy_select_boringssl", ) @@ -494,25 +493,17 @@ envoy_cc_test( ], ) -envoy_cc_test_library( - name = "inline_map_registry_test_scope_lib", - hdrs = ["inline_map_registry_test_scope.h"], - deps = [ - "//envoy/common:inline_map_registry", - ], -) - envoy_cc_test( name = "inline_map_registry_test", srcs = ["inline_map_registry_test.cc"], - deps = [":inline_map_registry_test_scope_lib"], + deps = ["//source/common/common:inline_map_registry"], ) envoy_cc_benchmark_binary( name = "inline_map_registry_speed_test", srcs = ["inline_map_registry_speed_test.cc"], external_deps = ["benchmark"], - deps = [":inline_map_registry_test_scope_lib"], + deps = ["//source/common/common:inline_map_registry"], ) envoy_benchmark_test( diff --git a/test/common/common/inline_map_registry_speed_test.cc b/test/common/common/inline_map_registry_speed_test.cc index 1f20e2753e406..b4eb3ee34941c 100644 --- a/test/common/common/inline_map_registry_speed_test.cc +++ b/test/common/common/inline_map_registry_speed_test.cc @@ -1,38 +1,35 @@ #include -#include "envoy/common/inline_map_registry.h" - -#include "test/common/common/inline_map_registry_test_scope.h" +#include "source/common/common/inline_map_registry.h" #include "benchmark/benchmark.h" namespace Envoy { namespace { -template static const std::vector& getNormalKeys() { - CONSTRUCT_ON_FIRST_USE(std::vector, []() { - std::vector keys; - for (size_t i = 0; i < N; ++i) { - keys.push_back("key_" + std::to_string(i)); - } - return keys; - }()); -} - // NOLINTNEXTLINE(readability-identifier-naming) static void BM_InlineMapFind(benchmark::State& state) { size_t map_type = state.range(0); size_t used_inline_handles = state.range(1); - auto normal_keys = getNormalKeys<200>(); - auto inline_handles_200 = InlineMapRegistryTestScope<200>::inlineHandles(); - auto inline_hanldes_0 = InlineMapRegistryTestScope<0>::inlineHandles(); + std::vector normal_keys; + + InlineMapRegistry registry_200; + // Create 200 inline keys. + std::vector inline_handles_200; + for (size_t i = 0; i < 200; ++i) { + const std::string key = "key_" + std::to_string(i); + inline_handles_200.push_back(registry_200.registerInlineKey(key)); + normal_keys.push_back(key); + } + InlineMapRegistry registry_0; + + registry_200.finalize(); + registry_0.finalize(); absl::flat_hash_map> normal_map; - auto inline_map_200 = - InlineMapRegistry>::InlineMap::createInlineMap(); - auto inline_map_0 = - InlineMapRegistry>::InlineMap::createInlineMap(); + auto inline_map_200 = registry_200.createInlineMap(); + auto inline_map_0 = registry_0.createInlineMap(); for (size_t i = 0; i < 200; ++i) { normal_map[normal_keys[i]] = std::make_unique("value_" + std::to_string(i)); diff --git a/test/common/common/inline_map_registry_test.cc b/test/common/common/inline_map_registry_test.cc index 44a4783180ac9..6ab1f26677d7e 100644 --- a/test/common/common/inline_map_registry_test.cc +++ b/test/common/common/inline_map_registry_test.cc @@ -1,4 +1,4 @@ -#include "test/common/common/inline_map_registry_test_scope.h" +#include "source/common/common/inline_map_registry.h" #include "gtest/gtest.h" @@ -6,12 +6,10 @@ namespace Envoy { namespace { TEST(InlineMapWithZeroInlineKey, InlineMapWithZeroInlineKeyTest) { - auto map = - InlineMapRegistry>::InlineMap::createInlineMap(); + InlineMapRegistry registry; + registry.finalize(); - EXPECT_EQ(InlineMapRegistryManager::registriesInfo().size(), 1); - EXPECT_EQ(InlineMapRegistryManager::registriesInfo()[0].registry_scope_name, - InlineMapRegistryTestScope<0>::name()); + auto map = registry.createInlineMap(); // Insert keys. for (size_t i = 0; i < 200; ++i) { @@ -34,11 +32,18 @@ TEST(InlineMapWithZeroInlineKey, InlineMapWithZeroInlineKeyTest) { } TEST(InlineMapWith20InlineKey, InlineMapWith20InlineKeyTest) { + InlineMapRegistry registry; - auto inline_hanldes = InlineMapRegistryTestScope<20>::inlineHandles(); + std::vector inline_keys; - auto map = - InlineMapRegistry>::InlineMap::createInlineMap(); + // Create 20 inline keys. + for (size_t i = 0; i < 20; ++i) { + inline_keys.push_back(registry.registerInlineKey("key_" + std::to_string(i))); + } + + registry.finalize(); + + auto map = registry.createInlineMap(); // Insert keys. for (size_t i = 0; i < 200; ++i) { @@ -52,11 +57,11 @@ TEST(InlineMapWith20InlineKey, InlineMapWith20InlineKeyTest) { } // Lookup by typed inline handle. - for (size_t i = 0; i < inline_hanldes.size(); ++i) { + for (size_t i = 0; i < inline_keys.size(); ++i) { const std::string key = "key_" + std::to_string(i); - auto handle = inline_hanldes[i]; - EXPECT_EQ(handle.inlineEntryKey(), key); - EXPECT_EQ(handle.inlineEntryId(), i); + auto handle = inline_keys[i]; + EXPECT_EQ(handle.inlineKey(), key); + EXPECT_EQ(handle.inlineId(), i); EXPECT_EQ(*map->lookup(handle), "value_" + std::to_string(i)); } @@ -66,7 +71,7 @@ TEST(InlineMapWith20InlineKey, InlineMapWith20InlineKeyTest) { // Remove keys by typed inline handle. for (size_t i = 0; i < 10; ++i) { const std::string key = "key_" + std::to_string(i); - auto handle = inline_hanldes[i]; + auto handle = inline_keys[i]; map->remove(handle); } diff --git a/test/common/common/inline_map_registry_test_scope.h b/test/common/common/inline_map_registry_test_scope.h deleted file mode 100644 index d181f2c1c3a2f..0000000000000 --- a/test/common/common/inline_map_registry_test_scope.h +++ /dev/null @@ -1,39 +0,0 @@ -#pragma once - -#include "envoy/common/inline_map_registry.h" - -namespace Envoy { - -template class InlineMapRegistryTestScope { -public: - static absl::string_view name() { - CONSTRUCT_ON_FIRST_USE(std::string, "BenchmarkTestScope_KeySize_" + std::to_string(N)); - } - - static std::vector::InlineHandle> - initializeRegistry() { - std::vector::InlineHandle> handles; - - // Force the inline map registry to be initialized and be added to the registry manager. - InlineMapRegistry::initialize(); - - for (size_t i = 0; i < N; ++i) { - std::string key = "key_" + std::to_string(i); - handles.push_back(InlineMapRegistry::registerInlineKey(key)); - } - - // Force the inline map registry to be finalized. - InlineMapRegistryManager::registriesInfo(); - - return handles; - } - - static const std::vector::InlineHandle>& - inlineHandles() { - CONSTRUCT_ON_FIRST_USE( - std::vector::InlineHandle>, - initializeRegistry()); - } -}; - -} // namespace Envoy From 1335bc1206346a58165d6605a22dae74de923ff1 Mon Sep 17 00:00:00 2001 From: wbpcode Date: Tue, 20 Jun 2023 10:53:09 +0000 Subject: [PATCH 15/36] fix build Signed-off-by: wbpcode --- source/common/common/BUILD | 3 +++ 1 file changed, 3 insertions(+) diff --git a/source/common/common/BUILD b/source/common/common/BUILD index 2d48c6c0f77e8..0d9528012d634 100644 --- a/source/common/common/BUILD +++ b/source/common/common/BUILD @@ -586,6 +586,9 @@ envoy_cc_library( hdrs = [ "inline_map_registry.h", ], + external_deps = [ + "abseil_node_hash_set", + ], deps = [ "//source/common/common:assert_lib", "//source/common/common:macros", From af121560c4828ffe965049cd8a7240d76227abee Mon Sep 17 00:00:00 2001 From: wbpcode Date: Tue, 20 Jun 2023 11:41:38 +0000 Subject: [PATCH 16/36] minor update Signed-off-by: wbpcode --- envoy/stream_info/filter_state.h | 12 ++++++++++-- source/common/common/inline_map_registry.cc | 4 ++-- source/common/common/inline_map_registry.h | 10 ++++++---- source/common/stream_info/filter_state_impl.cc | 7 +++++-- source/common/stream_info/filter_state_impl.h | 7 ++++--- 5 files changed, 27 insertions(+), 13 deletions(-) diff --git a/envoy/stream_info/filter_state.h b/envoy/stream_info/filter_state.h index 9b861e1ea790f..65d1dda29b4fe 100644 --- a/envoy/stream_info/filter_state.h +++ b/envoy/stream_info/filter_state.h @@ -256,14 +256,22 @@ class FilterState { * @return Whether data of any type and the name specified exists in the * data store. */ - virtual bool hasDataWithName(absl::string_view data_name) const PURE; + virtual bool hasDataGeneric(absl::string_view data_name) const PURE; /** * @param data_key the handle of the data being probed. * @return Whether data of any type and the name specified exists in the * data store. */ - virtual bool hasDataWithHandle(InlineKey data_key) const PURE; + virtual bool hasDataGeneric(InlineKey data_key) const PURE; + + /** + * @param data_name the name of the data being probed. + * @return Whether data of any type and the name specified exists in the + * data store. + * NOTE: Please use hasDataUnTyped() instead. + */ + virtual bool hasDataWithName(absl::string_view data_name) const PURE; /** * @param life_span the LifeSpan above which data existence is checked. diff --git a/source/common/common/inline_map_registry.cc b/source/common/common/inline_map_registry.cc index 349a275351430..59dbe19a51ac2 100644 --- a/source/common/common/inline_map_registry.cc +++ b/source/common/common/inline_map_registry.cc @@ -19,7 +19,7 @@ InlineMapRegistry::InlineKey InlineMapRegistry::registerInlineKey(absl::string_v // Insert the key to the list and keep the reference of the key in the inline handle and // registration map. - auto result = registration_set_.emplace(registration_set_.end(), key); + auto result = registration_set_.emplace(key); RELEASE_ASSERT(result.second, "The key should not be registered repeatedly and this should not happen."); @@ -46,7 +46,7 @@ const InlineMapRegistry::RegistrationMap& InlineMapRegistry::registrationMap() c return registration_map_; } -uint64_t InlineMapRegistry::registrationMapSize() const { +uint64_t InlineMapRegistry::registrationNum() const { ASSERT(finalized_, "Cannot fetch registration map before finalized()"); return registration_map_.size(); } diff --git a/source/common/common/inline_map_registry.h b/source/common/common/inline_map_registry.h index 5fc538833c70b..de90a3a748d7d 100644 --- a/source/common/common/inline_map_registry.h +++ b/source/common/common/inline_map_registry.h @@ -164,7 +164,7 @@ class InlineMapRegistry { continue; } - if (!callback(id.first, entry.get())) { + if (!callback(id.first, entry)) { return; } } @@ -372,12 +372,14 @@ class InlineMapRegistryHelper { private: template static InlineMapRegistry& mutableRegistry() { - MUTABLE_CONSTRUCT_ON_FIRST_USE(InlineMapRegistry, []() { + static InlineMapRegistry* registry = []() { auto* registry = new InlineMapRegistry(); auto result = InlineMapRegistryHelper::mutableRegistries().emplace(Scope::name(), registry); - RELEASE_ASSERT(result.second, "Duplicate inline map registry for scope: " + Scope::name()); + RELEASE_ASSERT(result.second, + absl::StrCat("Duplicate inline map registry for scope: ", Scope::name())); return registry; - }()); + }(); + return *registry; } static ManagedRegistries& mutableRegistries() { diff --git a/source/common/stream_info/filter_state_impl.cc b/source/common/stream_info/filter_state_impl.cc index 4eb70383bdb3b..3bab483dabef7 100644 --- a/source/common/stream_info/filter_state_impl.cc +++ b/source/common/stream_info/filter_state_impl.cc @@ -20,8 +20,11 @@ void FilterStateImpl::setData(InlineKey data_key, std::shared_ptr data, bool FilterStateImpl::hasDataWithName(absl::string_view data_name) const { return hasDataInternal(data_name) || (parent_ && parent_->hasDataWithName(data_name)); } -bool FilterStateImpl::hasDataWithHandle(InlineKey data_key) const { - return hasDataInternal(data_key) || (parent_ && parent_->hasDataWithHandle(data_key)); +bool FilterStateImpl::hasDataGeneric(absl::string_view data_name) const { + return hasDataInternal(data_name) || (parent_ && parent_->hasDataGeneric(data_name)); +} +bool FilterStateImpl::hasDataGeneric(InlineKey data_key) const { + return hasDataInternal(data_key) || (parent_ && parent_->hasDataGeneric(data_key)); } const FilterState::Object* diff --git a/source/common/stream_info/filter_state_impl.h b/source/common/stream_info/filter_state_impl.h index 72327751aae54..08cf9b216e647 100644 --- a/source/common/stream_info/filter_state_impl.h +++ b/source/common/stream_info/filter_state_impl.h @@ -57,7 +57,8 @@ class FilterStateImpl : public FilterState { StreamSharingMayImpactPooling stream_sharing = StreamSharingMayImpactPooling::None) override; bool hasDataWithName(absl::string_view) const override; - bool hasDataWithHandle(InlineKey data_key) const override; + bool hasDataGeneric(absl::string_view data_name) const override; + bool hasDataGeneric(InlineKey data_key) const override; const Object* getDataReadOnlyGeneric(absl::string_view data_name) const override; const Object* getDataReadOnlyGeneric(InlineKey data_key) const override; @@ -80,7 +81,7 @@ class FilterStateImpl : public FilterState { LifeSpan life_span, StreamSharingMayImpactPooling stream_sharing) { if (life_span > life_span_) { - if (hasDataWithNameInternally(data_key)) { + if (hasDataInternal(data_key)) { IS_ENVOY_BUG("FilterStateAccessViolation: FilterState::setData called twice with " "conflicting life_span on the same data_name."); return; @@ -89,7 +90,7 @@ class FilterStateImpl : public FilterState { parent_->setData(data_key, data, state_type, life_span, stream_sharing); return; } - if (parent_ && parent_->hasDataWithName(data_key)) { + if (parent_ && parent_->hasDataGeneric(data_key)) { IS_ENVOY_BUG("FilterStateAccessViolation: FilterState::setData called twice with " "conflicting life_span on the same data_name."); return; From 2073ab6001d68617193ffbb6368714430bb3068b Mon Sep 17 00:00:00 2001 From: wbpcode Date: Tue, 20 Jun 2023 12:17:42 +0000 Subject: [PATCH 17/36] minor fix after update filter state Signed-off-by: wbpcode --- source/common/router/router.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 7b57be639dd0a..4ea9084eb5bd6 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -551,7 +551,7 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers, absl::string_view sni_value = parsed_authority.host_; if (should_set_sni && upstream_http_protocol_options.value().auto_sni() && - !callbacks_->streamInfo().filterState()->hasDataWithName( + !callbacks_->streamInfo().filterState()->hasDataGeneric( Network::UpstreamServerName::key())) { callbacks_->streamInfo().filterState()->setData( Network::UpstreamServerName::key(), @@ -560,7 +560,7 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers, } if (upstream_http_protocol_options.value().auto_san_validation() && - !callbacks_->streamInfo().filterState()->hasDataWithName( + !callbacks_->streamInfo().filterState()->hasDataGeneric( Network::UpstreamSubjectAltNames::key())) { callbacks_->streamInfo().filterState()->setData( Network::UpstreamSubjectAltNames::key(), From a35e4bfe72db11ccb30d60dbd7d73848fd9097b3 Mon Sep 17 00:00:00 2001 From: wbpcode Date: Wed, 21 Jun 2023 01:36:58 +0000 Subject: [PATCH 18/36] fix test Signed-off-by: wbpcode --- test/common/http/conn_manager_impl_test_2.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/common/http/conn_manager_impl_test_2.cc b/test/common/http/conn_manager_impl_test_2.cc index 5daf2b1a45e6a..051498ed3c22c 100644 --- a/test/common/http/conn_manager_impl_test_2.cc +++ b/test/common/http/conn_manager_impl_test_2.cc @@ -3762,7 +3762,7 @@ TEST_F(HttpConnectionManagerImplTest, NoProxyProtocolAdded) { startRequest(false); - EXPECT_FALSE(decoder_->streamInfo().filterState()->hasDataWithName( + EXPECT_FALSE(decoder_->streamInfo().filterState()->hasDataGeneric( Network::ProxyProtocolFilterState::key())); // Clean up. filter_callbacks_.connection_.raiseEvent(Network::ConnectionEvent::RemoteClose); From 2b305997c65d928165cf5b026312ce245bae4b68 Mon Sep 17 00:00:00 2001 From: wbpcode Date: Thu, 22 Jun 2023 01:07:53 +0000 Subject: [PATCH 19/36] fix test and update method to generate registry id Signed-off-by: wbpcode --- source/common/common/inline_map_registry.cc | 5 ++++- source/common/common/inline_map_registry.h | 3 +-- test/common/http/conn_manager_impl_test.cc | 2 +- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/source/common/common/inline_map_registry.cc b/source/common/common/inline_map_registry.cc index 59dbe19a51ac2..2dd220e9d508c 100644 --- a/source/common/common/inline_map_registry.cc +++ b/source/common/common/inline_map_registry.cc @@ -2,7 +2,10 @@ namespace Envoy { -std::atomic InlineMapRegistry::global_registry_id_{0}; +uint64_t InlineMapRegistry::generateRegistryId() { + static std::atomic next_id{0}; + return next_id++; +} InlineMapRegistry::InlineKey InlineMapRegistry::registerInlineKey(absl::string_view key) { RELEASE_ASSERT(!finalized_, "Cannot register inline key after finalized()"); diff --git a/source/common/common/inline_map_registry.h b/source/common/common/inline_map_registry.h index de90a3a748d7d..3ffa10b4afa4b 100644 --- a/source/common/common/inline_map_registry.h +++ b/source/common/common/inline_map_registry.h @@ -313,8 +313,7 @@ class InlineMapRegistry { } private: - static std::atomic global_registry_id_; - static uint64_t generateRegistryId() { return global_registry_id_++; } + static uint64_t generateRegistryId(); const uint64_t registry_id_{}; diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index befb0aa44d750..97754f3188a46 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -408,7 +408,7 @@ TEST_F(HttpConnectionManagerImplTest, PopulateStreamInfo) { EXPECT_EQ(filter_callbacks_.connection_.id_, decoder_->streamInfo().downstreamAddressProvider().connectionID().value()); EXPECT_EQ(server_name_, decoder_->streamInfo().downstreamAddressProvider().requestedServerName()); - EXPECT_TRUE(decoder_->streamInfo().filterState()->hasDataWithName( + EXPECT_TRUE(decoder_->streamInfo().filterState()->hasDataGeneric( Network::ProxyProtocolFilterState::key())); // Clean up. From 517cc932ad3151b86ab4ae5219053d6b1854f415 Mon Sep 17 00:00:00 2001 From: wbpcode Date: Mon, 26 Jun 2023 07:24:41 +0000 Subject: [PATCH 20/36] address all comments Signed-off-by: wbpcode --- envoy/stream_info/filter_state.h | 2 +- source/common/common/inline_map_registry.cc | 56 +---- source/common/common/inline_map_registry.h | 221 ++++++++++-------- source/common/stream_info/filter_state_impl.h | 2 +- .../common/inline_map_registry_speed_test.cc | 6 +- .../common/common/inline_map_registry_test.cc | 7 +- 6 files changed, 135 insertions(+), 159 deletions(-) diff --git a/envoy/stream_info/filter_state.h b/envoy/stream_info/filter_state.h index 65d1dda29b4fe..89d1e20502597 100644 --- a/envoy/stream_info/filter_state.h +++ b/envoy/stream_info/filter_state.h @@ -21,7 +21,7 @@ class FilterStateInlineMapScope { static absl::string_view name() { return "inline_map_scope_filter_state"; } }; -using InlineKey = InlineMapRegistry::InlineKey; +using InlineKey = InlineMapRegistryHelper::InlineKey; class FilterState; diff --git a/source/common/common/inline_map_registry.cc b/source/common/common/inline_map_registry.cc index 2dd220e9d508c..5293619059ff9 100644 --- a/source/common/common/inline_map_registry.cc +++ b/source/common/common/inline_map_registry.cc @@ -1,57 +1,3 @@ #include "source/common/common/inline_map_registry.h" -namespace Envoy { - -uint64_t InlineMapRegistry::generateRegistryId() { - static std::atomic next_id{0}; - return next_id++; -} - -InlineMapRegistry::InlineKey InlineMapRegistry::registerInlineKey(absl::string_view key) { - RELEASE_ASSERT(!finalized_, "Cannot register inline key after finalized()"); - - // If the key is already registered, return the existing handle. - if (auto it = registration_map_.find(key); it != registration_map_.end()) { - // It is safe to reference the key here because the key is stored in the node hash set. - return {registry_id_, it->second, it->first}; - } - - // If the key is not registered, create a new handle for it. - - const uint64_t inline_id = registration_map_.size(); - - // Insert the key to the list and keep the reference of the key in the inline handle and - // registration map. - auto result = registration_set_.emplace(key); - - RELEASE_ASSERT(result.second, - "The key should not be registered repeatedly and this should not happen."); - - // It is safe to reference the key here because the key is stored in the node hash set. - registration_map_.emplace(absl::string_view(*result.first), inline_id); - return {registry_id_, inline_id, absl::string_view(*result.first)}; -} - -absl::optional -InlineMapRegistry::getInlineKey(absl::string_view key) const { - ASSERT(finalized_, "Cannot get inline handle before finalized()"); - - if (auto it = registration_map_.find(key); it != registration_map_.end()) { - // It is safe to reference the key here because the key is stored in the node hash set. - return InlineKey(registry_id_, it->second, it->first); - } - - return absl::nullopt; -} - -const InlineMapRegistry::RegistrationMap& InlineMapRegistry::registrationMap() const { - ASSERT(finalized_, "Cannot fetch registration map before finalized()"); - return registration_map_; -} - -uint64_t InlineMapRegistry::registrationNum() const { - ASSERT(finalized_, "Cannot fetch registration map before finalized()"); - return registration_map_.size(); -} - -} // namespace Envoy +namespace Envoy {} // namespace Envoy diff --git a/source/common/common/inline_map_registry.h b/source/common/common/inline_map_registry.h index 3ffa10b4afa4b..36896c843825c 100644 --- a/source/common/common/inline_map_registry.h +++ b/source/common/common/inline_map_registry.h @@ -36,19 +36,20 @@ namespace Envoy { * * // Register the inline key. We should never do this after bootstrapping. Typically, we can * // do this by define a global variable. - * auto inline_handle = registry.registerInlineKey("inline_key"); + * InlineMapRegistry::InlineHandle inline_handle = + * registry.registerInlineKey("inline_key"); * * // Finalize the registry. No further changes are allowed to the registry after this point. * registry.finalize(); * * // Create the inline map. - * auto inline_map = registry.createInlineMap(); + * InlineMapPtr inline_map = registry.createInlineMap(); * * // Get value by inline handle. * inline_map->insert(inline_handle, std::make_unique("value")); * EXPECT_EQ(*inline_map->lookup(inline_handle), "value"); */ -class InlineMapRegistry { +template class InlineMapRegistry { public: /** * This is the handle used to access the key/value pair in the inline map. The handle is @@ -56,101 +57,85 @@ class InlineMapRegistry { */ class InlineKey { public: - /** - * Get the id of the inline map registry that the handle created by. This will be used - * to determine if the handle could be accepted by a inline map. - */ - uint64_t registryId() const { return registry_id_; } - /** * Get the id of the inline entry in the inline map. This could be used to access the - * key/value pair in the inline map. + * key/value pair in the inline vector. */ uint64_t inlineId() const { return inline_id_; } - /** - * Get the key view of the inline key. - */ - absl::string_view inlineKey() const { return inline_key_; } - private: friend class InlineMapRegistry; // This constructor should only be called by InlineMapRegistry. - InlineKey(uint64_t registry_id, uint64_t inline_id, absl::string_view inline_key) - : registry_id_(registry_id), inline_id_(inline_id), inline_key_(inline_key) {} + InlineKey(uint64_t inline_id) : inline_id_(inline_id) {} - uint64_t registry_id_{}; uint64_t inline_id_{}; - absl::string_view inline_key_; - }; - // This is the Hash used for registration map/set and underlay hash map. - using Hash = absl::container_internal::hash_default_hash; +#ifndef NDEBUG + public: + // This is the registry id that the inline key created by. This is used to validate the + // inline key validity. + uint64_t registryId() const { return registry_id_; } - using RegistrationMap = absl::flat_hash_map; + private: + InlineKey(uint64_t inline_id, uint64_t registry_id) + : inline_id_(inline_id), registry_id_(registry_id) {} + uint64_t registry_id_{}; +#endif + }; - // Set to store the inline keys. Because the node of the set have stable address, we can reference - // the key in the handle or registration map safely. - using RegistrationSet = absl::node_hash_set; + using RegistrationMap = absl::flat_hash_map; + using RegistrationSet = std::vector; template class InlineMap : public InlineStorage { public: using TPtr = std::unique_ptr; - using UnderlayHashMap = absl::flat_hash_map; + using UnderlayHashMap = absl::flat_hash_map; // Get the entry for the given key. If the key is not found, return nullptr. - const T* lookup(absl::string_view key) const { return lookupImpl(key); } - T* lookup(absl::string_view key) { return lookupImpl(key); } + template const T* lookup(const ArgK& key) const { return lookupImpl(key); } + template T* lookup(const ArgK& key) { return lookupImpl(key); } - // Get the entry for the given handle. If the handle is not valid, return nullptr. - const T* lookup(InlineKey handle) const { return lookupImpl(handle); } - T* lookup(InlineKey handle) { return lookupImpl(handle); } + // Get the entry for the given inline key. If the handle is not valid, return nullptr. + const T* lookup(InlineKey key) const { return lookupImpl(key); } + T* lookup(InlineKey key) { return lookupImpl(key); } // Set the entry for the given key. If the key is already present, overwrite it. - void insert(absl::string_view key, TPtr value) { - // Compute the hash value for the key and avoid computing it again in the lookup. - const size_t hash_value = absl::Hash()(key); - - if (auto entry_id = inlineLookup(key, hash_value); entry_id.has_value()) { + template void insert(const ArgK& key, TPtr value) { + if (auto entry_id = inlineLookup(key); entry_id.has_value()) { resetInlineMapEntry(*entry_id, std::move(value)); } else { - normal_entries_[key] = std::move(value); + dynamic_entries_[key] = std::move(value); } } // Set the entry for the given handle. If the handle is not valid, do nothing. - void insert(InlineKey handle, TPtr value) { - if (handle.registryId() != registry_.registryId()) { - return; - } - - ASSERT(handle.inlineId() < registry_.registrationNum()); - resetInlineMapEntry(handle.inlineId(), std::move(value)); + void insert(InlineKey key, TPtr value) { +#ifndef NDEBUG + ASSERT(key.registryId() == registry_.registryId(), "Invalid inline key"); +#endif + resetInlineMapEntry(key.inlineId(), std::move(value)); } // Remove the entry for the given key. If the key is not found, do nothing. - void remove(absl::string_view key) { - // Compute the hash value for the key and avoid computing it again in the lookup. - const size_t hash_value = absl::Hash()(key); - - if (auto entry_id = inlineLookup(key, hash_value); entry_id.has_value()) { + template void remove(const ArgK& key) { + if (auto entry_id = inlineLookup(key); entry_id.has_value()) { resetInlineMapEntry(*entry_id); } else { - normal_entries_.erase(key); + dynamic_entries_.erase(key); } } // Remove the entry for the given handle. If the handle is not valid, do nothing. - void remove(InlineKey handle) { - if (handle.registryId() != registry_.registryId()) { - return; - } - resetInlineMapEntry(handle.inlineId()); + void remove(InlineKey key) { +#ifndef NDEBUG + ASSERT(key.registryId() == registry_.registryId(), "Invalid inline key"); +#endif + resetInlineMapEntry(key.inlineId()); } - void iterate(std::function callback) const { - for (const auto& entry : normal_entries_) { + void iterate(std::function callback) const { + for (const auto& entry : dynamic_entries_) { if (!callback(entry.first, entry.second.get())) { return; } @@ -170,13 +155,10 @@ class InlineMapRegistry { } } - uint64_t size() const { return normal_entries_.size() + inline_entries_size_; } + uint64_t size() const { return dynamic_entries_.size() + inline_entries_size_; } bool empty() const { return size() == 0; } - // Get the registry id that the inline map created by. - uint64_t registryId() const { return registry_.registryId(); } - ~InlineMap() { for (uint64_t i = 0; i < registry_.registrationNum(); ++i) { auto entry = inline_entries_[i]; @@ -200,29 +182,24 @@ class InlineMapRegistry { memset(inline_entries_, 0, registry_.registrationNum() * sizeof(TPtr)); } - T* lookupImpl(absl::string_view key) const { - // Compute the hash value for the key and avoid computing it again in the lookup. - const size_t hash_value = InlineMapRegistry::Hash()(key); - + template T* lookupImpl(const ArgK& key) const { // Because the normal string view key is used here, try the normal map entry first. - if (auto it = normal_entries_.find(key, hash_value); it != normal_entries_.end()) { + if (auto it = dynamic_entries_.find(key); it != dynamic_entries_.end()) { return it->second.get(); } - if (auto entry_id = inlineLookup(key, hash_value); entry_id.has_value()) { + if (auto entry_id = inlineLookup(key); entry_id.has_value()) { return inline_entries_[*entry_id]; } return nullptr; } - T* lookupImpl(InlineKey handle) const { - if (handle.registryId() != registry_.registryId()) { - return nullptr; - } - - ASSERT(handle.inlineId() < registry_.registrationNum()); - return inline_entries_[handle.inlineId()]; + T* lookupImpl(InlineKey key) const { +#ifndef NDEBUG + ASSERT(key.registryId() == registry_.registryId(), "Invalid inline key"); +#endif + return inline_entries_[key.inlineId()]; } void resetInlineMapEntry(uint64_t inline_entry_id, TPtr new_entry = nullptr) { @@ -242,9 +219,9 @@ class InlineMapRegistry { inline_entries_[inline_entry_id] = new_entry.release(); } - absl::optional inlineLookup(absl::string_view key, size_t hash_value) const { + template absl::optional inlineLookup(const ArgK& key) const { const auto& map_ref = registry_.registrationMap(); - if (auto iter = map_ref.find(key, hash_value); iter != map_ref.end()) { + if (auto iter = map_ref.find(key); iter != map_ref.end()) { return iter->second; } return absl::nullopt; @@ -255,7 +232,7 @@ class InlineMapRegistry { const InlineMapRegistry& registry_; // This is the underlay hash map for the normal entries. - UnderlayHashMap normal_entries_; + UnderlayHashMap dynamic_entries_; uint64_t inline_entries_size_{}; // This should be the last member of the class and no member should be added after this. @@ -264,12 +241,32 @@ class InlineMapRegistry { template using InlineMapPtr = std::unique_ptr>; - InlineMapRegistry() : registry_id_(generateRegistryId()) {} + InlineMapRegistry() = default; /** * Register a custom inline key. May only be called before finalized(). */ - InlineKey registerInlineKey(absl::string_view key); + template InlineKey registerInlineKey(const ArgK& key) { + RELEASE_ASSERT(!finalized_, "Cannot register inline key after finalized()"); + + uint64_t inline_id = 0; + + if (auto it = registration_map_.find(key); it != registration_map_.end()) { + // If the key is already registered, return the existing key. + inline_id = it->second; + } else { + // If the key is not registered yet, then create a new inline key. + inline_id = registration_map_.size(); + registration_set_.push_back(Key(key)); + registration_map_[key] = inline_id; + } + +#ifndef NDEBUG + return {inline_id, registry_id_}; +#else + return {inline_id}; +#endif + } /** * Fetch the inline handle for the given key. May only be called after finalized(). This should @@ -277,26 +274,46 @@ class InlineMapRegistry { * could used to determine if the key is registered or not at runtime or xDS config loading time * and decide if the key should be used as inline key or normal key. */ - absl::optional getInlineKey(absl::string_view key) const; + template absl::optional getInlineKey(const ArgK& key) const { + ASSERT(finalized_, "Cannot get inline handle before finalized()"); + + if (auto it = registration_map_.find(key); it != registration_map_.end()) { + // It is safe to reference the key here because the key is stored in the node hash set. +#ifndef NDEBUG + return InlineKey{it->second, registry_id_}; +#else + return InlineKey{it->second}; +#endif + } + + return absl::nullopt; + } /** * Get the registration map that contains all registered inline keys. May only be called after * finalized(). */ - const RegistrationMap& registrationMap() const; + const RegistrationMap& registrationMap() const { + ASSERT(finalized_, "Cannot fetch registration map before finalized()"); + return registration_map_; + } /** * Get the registration set that contains all registered inline keys. May only be called after * finalized(). */ - const RegistrationSet& registrationSet() const { return registration_set_; } + const RegistrationSet& registrationSet() const { + ASSERT(finalized_, "Cannot fetch registration set before finalized()"); + return registration_set_; + } /** * Get the number of the registrations. May only be called after finalized(). */ - uint64_t registrationNum() const; - - uint64_t registryId() const { return registry_id_; } + uint64_t registrationNum() const { + ASSERT(finalized_, "Cannot fetch registration map before finalized()"); + return registration_map_.size(); + } // Finalize this registry. No further changes are allowed after this point. This guaranteed that // all map created by the process have the same variable size and custom registrations. This @@ -312,11 +329,22 @@ class InlineMapRegistry { return InlineMap::create(*this); } +#ifndef NDEBUG +public: + // This is the registry id that the inline map created by. This is used to validate the inline + // key validity. + uint64_t registryId() const { return registry_id_; } + private: - static uint64_t generateRegistryId(); + static uint64_t generateRegistryId() { + static std::atomic next_id{0}; + return next_id++; + } - const uint64_t registry_id_{}; + uint64_t registry_id_{generateRegistryId()}; +#endif +private: bool finalized_{}; RegistrationSet registration_set_; RegistrationMap registration_map_; @@ -328,13 +356,14 @@ class InlineMapRegistry { */ class InlineMapRegistryHelper { public: - using ManagedRegistries = absl::flat_hash_map; + using InlineKey = InlineMapRegistry::InlineKey; + using ManagedRegistries = absl::flat_hash_map*>; /** * Get the inline map registry singleton based on the scope. */ template - static InlineMapRegistry::InlineKey registerInlinKey(absl::string_view key) { + static InlineMapRegistry::InlineKey registerInlinKey(absl::string_view key) { return mutableRegistry().registerInlineKey(key); } @@ -342,14 +371,16 @@ class InlineMapRegistryHelper { * Get registered inline key by the key name. */ template - static absl::optional getInlineKey(absl::string_view key) { + static absl::optional::InlineKey> + getInlineKey(absl::string_view key) { return mutableRegistry().getInlineKey(key); } /** * Create inline map based on the scope. */ - template static InlineMapRegistry::InlineMapPtr createInlineMap() { + template + static InlineMapRegistry::InlineMapPtr createInlineMap() { return mutableRegistry().template createInlineMap(); } @@ -370,9 +401,9 @@ class InlineMapRegistryHelper { } private: - template static InlineMapRegistry& mutableRegistry() { - static InlineMapRegistry* registry = []() { - auto* registry = new InlineMapRegistry(); + template static InlineMapRegistry& mutableRegistry() { + static InlineMapRegistry* registry = []() { + auto* registry = new InlineMapRegistry(); auto result = InlineMapRegistryHelper::mutableRegistries().emplace(Scope::name(), registry); RELEASE_ASSERT(result.second, absl::StrCat("Duplicate inline map registry for scope: ", Scope::name())); diff --git a/source/common/stream_info/filter_state_impl.h b/source/common/stream_info/filter_state_impl.h index 08cf9b216e647..1717fb88ccf26 100644 --- a/source/common/stream_info/filter_state_impl.h +++ b/source/common/stream_info/filter_state_impl.h @@ -167,7 +167,7 @@ class FilterStateImpl : public FilterState { FilterStateSharedPtr parent_; const FilterState::LifeSpan life_span_; - InlineMapRegistry::InlineMapPtr data_storage_; + InlineMapRegistry::InlineMapPtr data_storage_; }; } // namespace StreamInfo diff --git a/test/common/common/inline_map_registry_speed_test.cc b/test/common/common/inline_map_registry_speed_test.cc index b4eb3ee34941c..1631dd676ed60 100644 --- a/test/common/common/inline_map_registry_speed_test.cc +++ b/test/common/common/inline_map_registry_speed_test.cc @@ -14,15 +14,15 @@ static void BM_InlineMapFind(benchmark::State& state) { std::vector normal_keys; - InlineMapRegistry registry_200; + InlineMapRegistry registry_200; // Create 200 inline keys. - std::vector inline_handles_200; + std::vector::InlineKey> inline_handles_200; for (size_t i = 0; i < 200; ++i) { const std::string key = "key_" + std::to_string(i); inline_handles_200.push_back(registry_200.registerInlineKey(key)); normal_keys.push_back(key); } - InlineMapRegistry registry_0; + InlineMapRegistry registry_0; registry_200.finalize(); registry_0.finalize(); diff --git a/test/common/common/inline_map_registry_test.cc b/test/common/common/inline_map_registry_test.cc index 6ab1f26677d7e..1829ca7d92330 100644 --- a/test/common/common/inline_map_registry_test.cc +++ b/test/common/common/inline_map_registry_test.cc @@ -6,7 +6,7 @@ namespace Envoy { namespace { TEST(InlineMapWithZeroInlineKey, InlineMapWithZeroInlineKeyTest) { - InlineMapRegistry registry; + InlineMapRegistry registry; registry.finalize(); auto map = registry.createInlineMap(); @@ -32,9 +32,9 @@ TEST(InlineMapWithZeroInlineKey, InlineMapWithZeroInlineKeyTest) { } TEST(InlineMapWith20InlineKey, InlineMapWith20InlineKeyTest) { - InlineMapRegistry registry; + InlineMapRegistry registry; - std::vector inline_keys; + std::vector::InlineKey> inline_keys; // Create 20 inline keys. for (size_t i = 0; i < 20; ++i) { @@ -60,7 +60,6 @@ TEST(InlineMapWith20InlineKey, InlineMapWith20InlineKeyTest) { for (size_t i = 0; i < inline_keys.size(); ++i) { const std::string key = "key_" + std::to_string(i); auto handle = inline_keys[i]; - EXPECT_EQ(handle.inlineKey(), key); EXPECT_EQ(handle.inlineId(), i); EXPECT_EQ(*map->lookup(handle), "value_" + std::to_string(i)); } From deb32abc03df0a0df485733bb99032fcd5535cd5 Mon Sep 17 00:00:00 2001 From: wbpcode Date: Mon, 26 Jun 2023 07:29:05 +0000 Subject: [PATCH 21/36] update comment Signed-off-by: wbpcode --- source/common/common/inline_map_registry.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/common/inline_map_registry.h b/source/common/common/inline_map_registry.h index 36896c843825c..76a0b3d058272 100644 --- a/source/common/common/inline_map_registry.h +++ b/source/common/common/inline_map_registry.h @@ -58,8 +58,8 @@ template class InlineMapRegistry { class InlineKey { public: /** - * Get the id of the inline entry in the inline map. This could be used to access the - * key/value pair in the inline vector. + * Get the id of the inline entry in the inline array. This could be used to access the + * key/value pair in the inline map. */ uint64_t inlineId() const { return inline_id_; } From 986b310612233fba385f7e5ff01b112f7559f82b Mon Sep 17 00:00:00 2001 From: wbpcode Date: Mon, 26 Jun 2023 07:29:35 +0000 Subject: [PATCH 22/36] update comment Signed-off-by: wbpcode --- source/common/common/inline_map_registry.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/common/inline_map_registry.h b/source/common/common/inline_map_registry.h index 76a0b3d058272..e7753d1c2b5b5 100644 --- a/source/common/common/inline_map_registry.h +++ b/source/common/common/inline_map_registry.h @@ -59,7 +59,7 @@ template class InlineMapRegistry { public: /** * Get the id of the inline entry in the inline array. This could be used to access the - * key/value pair in the inline map. + * key/value pair in the inline map without hash searching. */ uint64_t inlineId() const { return inline_id_; } From 37053b4df2313c8a1cea64c8efb753b989463ab9 Mon Sep 17 00:00:00 2001 From: wbpcode Date: Mon, 26 Jun 2023 08:42:40 +0000 Subject: [PATCH 23/36] minor update Signed-off-by: wbpcode --- source/common/common/inline_map_registry.h | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/source/common/common/inline_map_registry.h b/source/common/common/inline_map_registry.h index e7753d1c2b5b5..3c60e89ac8188 100644 --- a/source/common/common/inline_map_registry.h +++ b/source/common/common/inline_map_registry.h @@ -277,16 +277,19 @@ template class InlineMapRegistry { template absl::optional getInlineKey(const ArgK& key) const { ASSERT(finalized_, "Cannot get inline handle before finalized()"); - if (auto it = registration_map_.find(key); it != registration_map_.end()) { - // It is safe to reference the key here because the key is stored in the node hash set. + uint64_t inline_id = 0; + + if (auto it = registration_map_.find(key); it == registration_map_.end()) { + return absl::nullopt; + } else { + inline_id = it->second; + } + #ifndef NDEBUG - return InlineKey{it->second, registry_id_}; + return InlineKey{inline_id, registry_id_}; #else - return InlineKey{it->second}; + return InlineKey{it->second}; #endif - } - - return absl::nullopt; } /** From 253e44485021b9568ecc13c7439348c07d07e155 Mon Sep 17 00:00:00 2001 From: wbpcode Date: Mon, 26 Jun 2023 09:36:59 +0000 Subject: [PATCH 24/36] minor update Signed-off-by: wbpcode --- source/common/common/inline_map_registry.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/common/inline_map_registry.h b/source/common/common/inline_map_registry.h index 3c60e89ac8188..0329e525f7952 100644 --- a/source/common/common/inline_map_registry.h +++ b/source/common/common/inline_map_registry.h @@ -288,7 +288,7 @@ template class InlineMapRegistry { #ifndef NDEBUG return InlineKey{inline_id, registry_id_}; #else - return InlineKey{it->second}; + return InlineKey{inline_id}; #endif } From 44d4865f925340f581f4b3c25dead013b06bb992 Mon Sep 17 00:00:00 2001 From: wbpcode Date: Tue, 27 Jun 2023 08:20:54 +0000 Subject: [PATCH 25/36] addre latest comment Signed-off-by: wbpcode --- envoy/stream_info/BUILD | 2 +- envoy/stream_info/filter_state.h | 2 +- source/common/common/BUILD | 10 + source/common/common/inline_map_registry.h | 478 +++++++++--------- .../common/inline_map_registry_helper.h | 71 +++ .../common/stream_info/filter_state_impl.cc | 8 +- source/common/stream_info/filter_state_impl.h | 25 +- source/server/server.cc | 2 +- .../common/inline_map_registry_speed_test.cc | 14 +- .../common/common/inline_map_registry_test.cc | 14 +- 10 files changed, 359 insertions(+), 267 deletions(-) create mode 100644 source/common/common/inline_map_registry_helper.h diff --git a/envoy/stream_info/BUILD b/envoy/stream_info/BUILD index 6926c81cf92d1..9120d2c536615 100644 --- a/envoy/stream_info/BUILD +++ b/envoy/stream_info/BUILD @@ -34,7 +34,7 @@ envoy_cc_library( hdrs = ["filter_state.h"], external_deps = ["abseil_optional"], deps = [ - "//source/common/common:inline_map_registry", + "//source/common/common:inline_map_registry_helper", "//source/common/common:utility_lib", "//source/common/protobuf", ], diff --git a/envoy/stream_info/filter_state.h b/envoy/stream_info/filter_state.h index 89d1e20502597..c8d18aae7089b 100644 --- a/envoy/stream_info/filter_state.h +++ b/envoy/stream_info/filter_state.h @@ -6,7 +6,7 @@ #include "envoy/common/pure.h" #include "source/common/common/fmt.h" -#include "source/common/common/inline_map_registry.h" +#include "source/common/common/inline_map_registry_helper.h" #include "source/common/common/utility.h" #include "source/common/protobuf/protobuf.h" diff --git a/source/common/common/BUILD b/source/common/common/BUILD index 0d9528012d634..b4cf86fffa2ac 100644 --- a/source/common/common/BUILD +++ b/source/common/common/BUILD @@ -595,3 +595,13 @@ envoy_cc_library( "//source/common/common:utility_lib", ], ) + +envoy_cc_library( + name = "inline_map_registry_helper", + hdrs = [ + "inline_map_registry_helper.h", + ], + deps = [ + ":inline_map_registry", + ], +) diff --git a/source/common/common/inline_map_registry.h b/source/common/common/inline_map_registry.h index 0329e525f7952..681bc5a7546c1 100644 --- a/source/common/common/inline_map_registry.h +++ b/source/common/common/inline_map_registry.h @@ -2,6 +2,9 @@ #include #include +#include + +#include "envoy/common/optref.h" #include "source/common/common/assert.h" #include "source/common/common/macros.h" @@ -13,10 +16,10 @@ namespace Envoy { /** - * This class could be used as an alternative of normal hash map to store the key/value pairs. - * But by this class, you can register some frequently used keys as inline keys and get the - * handle for the key. Then you can use the handle to access the key/value pair in the map without - * even one time hash searching. + * This library provide inline map templates which could be used as an alternative of normal hash + * map to store the key/value pairs. But by these templates, you can register some frequently used + * keys as inline keys and get the handle for the key. Then you can use the handle to access the + * key/value pair in the map without even one time hash searching. * * This is useful when some keys are frequently used and the keys are known at compile time or * bootstrap time. You can get superior performance by using the inline handle. For example, the @@ -31,7 +34,9 @@ namespace Envoy { * These is usage example: * * // Define registry to store the inline keys. The registry should have lifetime that no shorter - * // than all inline maps created by the registry. + * // than all inline maps created by the registry. You can create static and global registry by + * // ``MUTABLE_CONSTRUCT_ON_FIRST_USE`` for cross-modules usage, but it is not required. A local + * // registry is also fine if the inline map is only used in the local scope. * InlineMapRegistry registry; * * // Register the inline key. We should never do this after bootstrapping. Typically, we can @@ -49,6 +54,11 @@ namespace Envoy { * inline_map->insert(inline_handle, std::make_unique("value")); * EXPECT_EQ(*inline_map->lookup(inline_handle), "value"); */ + +/** + * Inline map registry to store the inline keys and mapping of the inline keys to the inline + * id. + */ template class InlineMapRegistry { public: /** @@ -85,169 +95,15 @@ template class InlineMapRegistry { }; using RegistrationMap = absl::flat_hash_map; - using RegistrationSet = std::vector; - - template class InlineMap : public InlineStorage { - public: - using TPtr = std::unique_ptr; - using UnderlayHashMap = absl::flat_hash_map; - - // Get the entry for the given key. If the key is not found, return nullptr. - template const T* lookup(const ArgK& key) const { return lookupImpl(key); } - template T* lookup(const ArgK& key) { return lookupImpl(key); } - - // Get the entry for the given inline key. If the handle is not valid, return nullptr. - const T* lookup(InlineKey key) const { return lookupImpl(key); } - T* lookup(InlineKey key) { return lookupImpl(key); } - - // Set the entry for the given key. If the key is already present, overwrite it. - template void insert(const ArgK& key, TPtr value) { - if (auto entry_id = inlineLookup(key); entry_id.has_value()) { - resetInlineMapEntry(*entry_id, std::move(value)); - } else { - dynamic_entries_[key] = std::move(value); - } - } - - // Set the entry for the given handle. If the handle is not valid, do nothing. - void insert(InlineKey key, TPtr value) { -#ifndef NDEBUG - ASSERT(key.registryId() == registry_.registryId(), "Invalid inline key"); -#endif - resetInlineMapEntry(key.inlineId(), std::move(value)); - } - - // Remove the entry for the given key. If the key is not found, do nothing. - template void remove(const ArgK& key) { - if (auto entry_id = inlineLookup(key); entry_id.has_value()) { - resetInlineMapEntry(*entry_id); - } else { - dynamic_entries_.erase(key); - } - } - - // Remove the entry for the given handle. If the handle is not valid, do nothing. - void remove(InlineKey key) { -#ifndef NDEBUG - ASSERT(key.registryId() == registry_.registryId(), "Invalid inline key"); -#endif - resetInlineMapEntry(key.inlineId()); - } - - void iterate(std::function callback) const { - for (const auto& entry : dynamic_entries_) { - if (!callback(entry.first, entry.second.get())) { - return; - } - } - - for (const auto& id : registry_.registrationMap()) { - ASSERT(id.second < registry_.registrationNum()); - - auto entry = inline_entries_[id.second]; - if (entry == nullptr) { - continue; - } - - if (!callback(id.first, entry)) { - return; - } - } - } - - uint64_t size() const { return dynamic_entries_.size() + inline_entries_size_; } - - bool empty() const { return size() == 0; } - - ~InlineMap() { - for (uint64_t i = 0; i < registry_.registrationNum(); ++i) { - auto entry = inline_entries_[i]; - if (entry != nullptr) { - delete entry; - } - } - memset(inline_entries_, 0, registry_.registrationNum() * sizeof(TPtr)); - } - - private: - friend class InlineMapRegistry; - - static std::unique_ptr create(InlineMapRegistry& registry) { - return std::unique_ptr>(new ( - (registry.registrationMap().size() * sizeof(InlineMap::TPtr))) InlineMap(registry)); - } - - InlineMap(InlineMapRegistry& registry) : registry_(registry) { - // Initialize the inline entries to nullptr. - memset(inline_entries_, 0, registry_.registrationNum() * sizeof(TPtr)); - } - - template T* lookupImpl(const ArgK& key) const { - // Because the normal string view key is used here, try the normal map entry first. - if (auto it = dynamic_entries_.find(key); it != dynamic_entries_.end()) { - return it->second.get(); - } - - if (auto entry_id = inlineLookup(key); entry_id.has_value()) { - return inline_entries_[*entry_id]; - } - - return nullptr; - } - - T* lookupImpl(InlineKey key) const { -#ifndef NDEBUG - ASSERT(key.registryId() == registry_.registryId(), "Invalid inline key"); -#endif - return inline_entries_[key.inlineId()]; - } - - void resetInlineMapEntry(uint64_t inline_entry_id, TPtr new_entry = nullptr) { - ASSERT(inline_entry_id < registry_.registrationNum()); - if (auto entry = inline_entries_[inline_entry_id]; entry != nullptr) { - // Remove and delete the old valid entry. - ASSERT(inline_entries_size_ > 0); - --inline_entries_size_; - delete entry; - } - - if (new_entry != nullptr) { - // Append the new valid entry. - ++inline_entries_size_; - } - - inline_entries_[inline_entry_id] = new_entry.release(); - } - - template absl::optional inlineLookup(const ArgK& key) const { - const auto& map_ref = registry_.registrationMap(); - if (auto iter = map_ref.find(key); iter != map_ref.end()) { - return iter->second; - } - return absl::nullopt; - } - - // This is the reference of the registry that the inline map created by. This is used to - // validate the inline handle validity and get the inline key set. - const InlineMapRegistry& registry_; - - // This is the underlay hash map for the normal entries. - UnderlayHashMap dynamic_entries_; - - uint64_t inline_entries_size_{}; - // This should be the last member of the class and no member should be added after this. - T* inline_entries_[]; - }; - - template using InlineMapPtr = std::unique_ptr>; + using RegistrationVector = std::vector; InlineMapRegistry() = default; /** - * Register a custom inline key. May only be called before finalized(). + * Register a custom inline key. May only be called before finalize(). */ template InlineKey registerInlineKey(const ArgK& key) { - RELEASE_ASSERT(!finalized_, "Cannot register inline key after finalized()"); + RELEASE_ASSERT(!finalized_, "Cannot register inline key after finalize()"); uint64_t inline_id = 0; @@ -257,7 +113,7 @@ template class InlineMapRegistry { } else { // If the key is not registered yet, then create a new inline key. inline_id = registration_map_.size(); - registration_set_.push_back(Key(key)); + registration_vector_.push_back(Key(key)); registration_map_[key] = inline_id; } @@ -269,13 +125,13 @@ template class InlineMapRegistry { } /** - * Fetch the inline handle for the given key. May only be called after finalized(). This should + * Fetch the inline handle for the given key. May only be called after finalize(). This should * be used to get the inline handle for the key registered by registerInlineKey(). This function * could used to determine if the key is registered or not at runtime or xDS config loading time * and decide if the key should be used as inline key or normal key. */ template absl::optional getInlineKey(const ArgK& key) const { - ASSERT(finalized_, "Cannot get inline handle before finalized()"); + ASSERT(finalized_, "Cannot get inline handle before finalize()"); uint64_t inline_id = 0; @@ -294,27 +150,27 @@ template class InlineMapRegistry { /** * Get the registration map that contains all registered inline keys. May only be called after - * finalized(). + * finalize(). */ const RegistrationMap& registrationMap() const { - ASSERT(finalized_, "Cannot fetch registration map before finalized()"); + ASSERT(finalized_, "Cannot fetch registration map before finalize()"); return registration_map_; } /** - * Get the registration set that contains all registered inline keys. May only be called after - * finalized(). + * Get the registration vector that contains all registered inline keys. May only be called after + * finalize(). */ - const RegistrationSet& registrationSet() const { - ASSERT(finalized_, "Cannot fetch registration set before finalized()"); - return registration_set_; + const RegistrationVector& registrationVector() const { + ASSERT(finalized_, "Cannot fetch registration set before finalize()"); + return registration_vector_; } /** - * Get the number of the registrations. May only be called after finalized(). + * Get the number of the registrations. May only be called after finalize(). */ uint64_t registrationNum() const { - ASSERT(finalized_, "Cannot fetch registration map before finalized()"); + ASSERT(finalized_, "Cannot fetch registration map before finalize()"); return registration_map_.size(); } @@ -323,15 +179,6 @@ template class InlineMapRegistry { // function could be called multiple times safely and only the first call will have effect. void finalize() { finalized_ = true; } - template InlineMapPtr createInlineMap() { - ASSERT(finalized_, "Cannot create inline map before finalized()"); - - // Call finalize() anyway to make sure that the registry is finalized and no any new inline - // keys could be registered. - finalize(); - return InlineMap::create(*this); - } - #ifndef NDEBUG public: // This is the registry id that the inline map created by. This is used to validate the inline @@ -349,75 +196,244 @@ template class InlineMapRegistry { private: bool finalized_{}; - RegistrationSet registration_set_; + RegistrationVector registration_vector_; RegistrationMap registration_map_; }; /** - * Helper class to get inline map registry singleton based on the scope. If cross-modules inline map - * registry is necessary, this class should be used. + * This is the inline map that could be used as an alternative of normal hash map to store the + * key/value pairs. */ -class InlineMapRegistryHelper { +template class InlineMap : public InlineStorage { public: - using InlineKey = InlineMapRegistry::InlineKey; - using ManagedRegistries = absl::flat_hash_map*>; + using TypedInlineMapRegistry = InlineMapRegistry; + using InlineKey = typename TypedInlineMapRegistry::InlineKey; - /** - * Get the inline map registry singleton based on the scope. - */ - template - static InlineMapRegistry::InlineKey registerInlinKey(absl::string_view key) { - return mutableRegistry().registerInlineKey(key); + using UnderlayHashMap = absl::flat_hash_map; + struct InlineEntry { + InlineEntry(Value&& value) : value_(std::move(value)) {} + InlineEntry(const Value& value) : value_(value) {} + + Value value_; + typename std::list::iterator entry_; + }; + using UnderlayInlineEntryStorage = std::list; + + // Get the entry for the given key. If the key is not found, return nullptr. + template OptRef lookup(const ArgK& key) const { + // Because the normal string view key is used here, try the normal map entry first. + if (auto it = dynamic_entries_.find(key); it != dynamic_entries_.end()) { + return it->second; + } + + if (auto entry_id = inlineLookup(key); entry_id.has_value()) { + if (auto entry = inline_entries_[*entry_id]; entry != nullptr) { + return entry->value_; + } + } + + return {}; } - /** - * Get registered inline key by the key name. - */ - template - static absl::optional::InlineKey> - getInlineKey(absl::string_view key) { - return mutableRegistry().getInlineKey(key); + template OptRef lookup(const ArgK& key) { + // Because the normal string view key is used here, try the normal map entry first. + if (auto it = dynamic_entries_.find(key); it != dynamic_entries_.end()) { + return it->second; + } + + if (auto entry_id = inlineLookup(key); entry_id.has_value()) { + if (auto entry = inline_entries_[*entry_id]; entry != nullptr) { + return entry->value_; + } + } + + return {}; } - /** - * Create inline map based on the scope. - */ - template - static InlineMapRegistry::InlineMapPtr createInlineMap() { - return mutableRegistry().template createInlineMap(); + // Get the entry for the given inline key. If the handle is not valid, return nullptr. + OptRef lookup(InlineKey key) const { +#ifndef NDEBUG + ASSERT(key.registryId() == registry_.registryId(), "Invalid inline key"); +#endif + if (auto entry = inline_entries_[key.inlineId()]; entry != nullptr) { + return entry->value_; + } + return {}; } - /** - * Get all registries managed by this helper. This this helpful to finalize all registries and - * print the debug info of inline map registry. - */ - static const ManagedRegistries& registries() { return mutableRegistries(); } + OptRef lookup(InlineKey key) { +#ifndef NDEBUG + ASSERT(key.registryId() == registry_.registryId(), "Invalid inline key"); +#endif + if (auto entry = inline_entries_[key.inlineId()]; entry != nullptr) { + return entry->value_; + } + return {}; + } - /** - * Finalize all registries managed by this helper. This should be called after all inline keys - * are registered. - */ - static void finalize() { - for (auto& registry : mutableRegistries()) { - registry.second->finalize(); + // Set the entry for the given key. If the key is already present, insert will fail and + // return false. Otherwise, insert will succeed and return true. + template bool insert(ArgK&& key, ArgV&& value) { + if (auto entry_id = inlineLookup(key); entry_id.has_value()) { + // This key is registered as inline key and try to insert the value to the inline array. + + // If the entry is already valid, insert will fail and return false. + if (inline_entries_[*entry_id] != nullptr) { + return false; + } + + resetInlineMapEntry(*entry_id, std::move(value)); + return true; + } else { + // This key is not registered as inline key and try to insert the value to the normal map. + return dynamic_entries_.emplace(std::forward(key), std::forward(value)).second; + } + } + + // Set the entry for the given handle. If the handle is not valid, do nothing. + template void insert(InlineKey key, ArgV&& value) { +#ifndef NDEBUG + ASSERT(key.registryId() == registry_.registryId(), "Invalid inline key"); +#endif + resetInlineMapEntry(key.inlineId(), std::forward(value)); + } + + // Remove the entry for the given key. If the key is not found, do nothing. + template void remove(const ArgK& key) { + if (auto entry_id = inlineLookup(key); entry_id.has_value()) { + clearInlineMapEntry(*entry_id); + } else { + dynamic_entries_.erase(key); + } + } + + // Remove the entry for the given handle. If the handle is not valid, do nothing. + void remove(InlineKey key) { +#ifndef NDEBUG + ASSERT(key.registryId() == registry_.registryId(), "Invalid inline key"); +#endif + clearInlineMapEntry(key.inlineId()); + } + + void iterate(std::function callback) const { + for (const auto& entry : dynamic_entries_) { + if (!callback(entry.first, entry.second)) { + return; + } + } + + for (const auto& id : registry_.registrationMap()) { + ASSERT(id.second < registry_.registrationNum()); + + auto entry = inline_entries_[id.second]; + if (entry == nullptr) { + continue; + } + + if (!callback(id.first, entry->value_)) { + return; + } + } + } + + uint64_t size() const { return dynamic_entries_.size() + inline_entries_storage_.size(); } + + bool empty() const { return size() == 0; } + + ~InlineMap() { + for (uint64_t i = 0; i < registry_.registrationNum(); ++i) { + clearInlineMapEntry(i); + } + memset(inline_entries_, 0, registry_.registrationNum() * sizeof(Value)); + } + + // Override operator [] to support the normal key assignment. + template Value& operator[](const ArgK& key) { + if (auto entry_id = inlineLookup(key); entry_id.has_value()) { + // This key is registered as inline key and try to insert the value to the inline array. + if (inline_entries_[*entry_id] != nullptr) { + resetInlineMapEntry(*entry_id, Value()); + } + return inline_entries_[*entry_id]->value_; + } else { + // This key is not registered as inline key and try to insert the value to the normal map. + return dynamic_entries_[key]; + } + } + + // Override operator [] to support the inline key assignment. + Value& operator[](InlineKey key) { +#ifndef NDEBUG + ASSERT(key.registryId() == registry_.registryId(), "Invalid inline key"); +#endif + + if (inline_entries_[key.inlineId()] == nullptr) { + resetInlineMapEntry(key.inlineId(), Value()); } + return inline_entries_[key.inlineId()]->value_; + } + + static std::unique_ptr create(TypedInlineMapRegistry& registry) { + // Call finalize() anyway to make sure that the registry is finalized and no any new inline + // keys could be registered. + registry.finalize(); + + return std::unique_ptr( + new ((registry.registrationMap().size() * sizeof(InlineEntry*))) InlineMap(registry)); } private: - template static InlineMapRegistry& mutableRegistry() { - static InlineMapRegistry* registry = []() { - auto* registry = new InlineMapRegistry(); - auto result = InlineMapRegistryHelper::mutableRegistries().emplace(Scope::name(), registry); - RELEASE_ASSERT(result.second, - absl::StrCat("Duplicate inline map registry for scope: ", Scope::name())); - return registry; - }(); - return *registry; + friend class InlineMapRegistry; + + InlineMap(TypedInlineMapRegistry& registry) : registry_(registry) { + // Initialize the inline entries to nullptr. + memset(inline_entries_, 0, registry_.registrationNum() * sizeof(InlineEntry*)); + } + + template void resetInlineMapEntry(uint64_t inline_entry_id, ArgV&& value) { + ASSERT(inline_entry_id < registry_.registrationNum()); + + clearInlineMapEntry(inline_entry_id); + + auto iter = + inline_entries_storage_.emplace(inline_entries_storage_.end(), std::forward(value)); + + iter->entry_ = iter; + inline_entries_[inline_entry_id] = &(*iter); } - static ManagedRegistries& mutableRegistries() { - MUTABLE_CONSTRUCT_ON_FIRST_USE(ManagedRegistries); + void clearInlineMapEntry(uint64_t inline_entry_id) { + ASSERT(inline_entry_id < registry_.registrationNum()); + + // Destruct the old entry if it is valid. + if (auto entry = inline_entries_[inline_entry_id]; entry != nullptr) { + inline_entries_storage_.erase(entry->entry_); + inline_entries_[inline_entry_id] = nullptr; + } + } + + template absl::optional inlineLookup(const ArgK& key) const { + const auto& map_ref = registry_.registrationMap(); + if (auto iter = map_ref.find(key); iter != map_ref.end()) { + return iter->second; + } + return absl::nullopt; } + + // This is the reference of the registry that the inline map created by. This is used to + // validate the inline handle validity and get the inline key set. + const TypedInlineMapRegistry& registry_; + + // This is the underlay hash map for the normal entries. + UnderlayHashMap dynamic_entries_; + + // This is the inline entries storage. The inline entries are stored in the list. + UnderlayInlineEntryStorage inline_entries_storage_; + + // This should be the last member of the class and no member should be added after this. + InlineEntry* inline_entries_[]; }; +template using InlineMapPtr = std::unique_ptr>; + } // namespace Envoy diff --git a/source/common/common/inline_map_registry_helper.h b/source/common/common/inline_map_registry_helper.h new file mode 100644 index 0000000000000..a662dc8a62e48 --- /dev/null +++ b/source/common/common/inline_map_registry_helper.h @@ -0,0 +1,71 @@ +#include "source/common/common/inline_map_registry.h" + +namespace Envoy { + +/** + * Helper class to get inline map registry singleton based on the scope. If cross-modules inline map + * registry is necessary, this class should be used. + */ +class InlineMapRegistryHelper { +public: + using InlineKey = InlineMapRegistry::InlineKey; + using ManagedRegistries = absl::flat_hash_map*>; + + /** + * Get the inline map registry singleton based on the scope. + */ + template + static InlineMapRegistry::InlineKey registerInlinKey(absl::string_view key) { + return mutableRegistry().registerInlineKey(key); + } + + /** + * Get registered inline key by the key name. + */ + template + static absl::optional::InlineKey> + getInlineKey(absl::string_view key) { + return mutableRegistry().getInlineKey(key); + } + + /** + * Create inline map based on the scope. + */ + template static auto createInlineMap() { + return InlineMap::create(mutableRegistry()); + } + + /** + * Get all registries managed by this helper. This this helpful to finalize all registries and + * print the debug info of inline map registry. + */ + static const ManagedRegistries& registries() { return mutableRegistries(); } + + /** + * Finalize all registries managed by this helper. This should be called after all inline keys + * are registered. + */ + static void finalize() { + for (auto& registry : mutableRegistries()) { + registry.second->finalize(); + } + } + +private: + template static InlineMapRegistry& mutableRegistry() { + static InlineMapRegistry* registry = []() { + auto* registry = new InlineMapRegistry(); + auto result = InlineMapRegistryHelper::mutableRegistries().emplace(Scope::name(), registry); + RELEASE_ASSERT(result.second, + absl::StrCat("Duplicate inline map registry for scope: ", Scope::name())); + return registry; + }(); + return *registry; + } + + static ManagedRegistries& mutableRegistries() { + MUTABLE_CONSTRUCT_ON_FIRST_USE(ManagedRegistries); + } +}; + +} // namespace Envoy diff --git a/source/common/stream_info/filter_state_impl.cc b/source/common/stream_info/filter_state_impl.cc index 3bab483dabef7..8da17680b9b59 100644 --- a/source/common/stream_info/filter_state_impl.cc +++ b/source/common/stream_info/filter_state_impl.cc @@ -62,14 +62,14 @@ FilterState::ObjectsPtr FilterStateImpl::objectsSharedWithUpstreamConnection() c auto objects = parent_ ? parent_->objectsSharedWithUpstreamConnection() : std::make_unique(); - data_storage_->iterate([&objects](absl::string_view name, FilterObject* object) -> bool { - switch (object->stream_sharing_) { + data_storage_->iterate([&objects](const std::string& name, const FilterObject& object) -> bool { + switch (object.stream_sharing_) { case StreamSharingMayImpactPooling::SharedWithUpstreamConnection: objects->push_back( - {object->data_, object->state_type_, object->stream_sharing_, std::string(name)}); + {object.data_, object.state_type_, object.stream_sharing_, std::string(name)}); break; case StreamSharingMayImpactPooling::SharedWithUpstreamConnectionOnce: - objects->push_back({object->data_, object->state_type_, StreamSharingMayImpactPooling::None, + objects->push_back({object.data_, object.state_type_, StreamSharingMayImpactPooling::None, std::string(name)}); break; default: diff --git a/source/common/stream_info/filter_state_impl.h b/source/common/stream_info/filter_state_impl.h index 1717fb88ccf26..2197f63b36c89 100644 --- a/source/common/stream_info/filter_state_impl.h +++ b/source/common/stream_info/filter_state_impl.h @@ -95,8 +95,8 @@ class FilterStateImpl : public FilterState { "conflicting life_span on the same data_name."); return; } - const auto* current = data_storage_->lookup(data_key); - if (current != nullptr) { + const auto current = data_storage_->lookup(data_key); + if (current.has_value()) { // We have another object with same data_name. Check for mutability // violations namely: readonly data cannot be overwritten, mutable data // cannot be overwritten by readonly data. @@ -113,24 +113,23 @@ class FilterStateImpl : public FilterState { } } - std::unique_ptr filter_object( - new FilterStateImpl::FilterObject()); - filter_object->data_ = data; - filter_object->state_type_ = state_type; - filter_object->stream_sharing_ = stream_sharing; + FilterStateImpl::FilterObject filter_object; + filter_object.data_ = data; + filter_object.state_type_ = state_type; + filter_object.stream_sharing_ = stream_sharing; data_storage_->insert(data_key, std::move(filter_object)); } // This only checks the local data_storage_ for data_name existence. template bool hasDataInternal(DataKeyType data_key) const { - return data_storage_->lookup(data_key) != nullptr; + return data_storage_->lookup(data_key).has_value(); } template const FilterState::Object* getDataReadOnlyGenericInternal(DataKeyType data_key) const { - const auto* current = data_storage_->lookup(data_key); + const auto current = data_storage_->lookup(data_key); - if (current == nullptr) { + if (!current.has_value()) { if (parent_) { return parent_->getDataReadOnlyGeneric(data_key); } @@ -142,9 +141,9 @@ class FilterStateImpl : public FilterState { template std::shared_ptr getDataSharedMutableGenericInternal(DataKeyType data_key) { - const auto* current = data_storage_->lookup(data_key); + const auto current = data_storage_->lookup(data_key); - if (current == nullptr) { + if (!current.has_value()) { if (parent_) { return parent_->getDataSharedMutableGeneric(data_key); } @@ -167,7 +166,7 @@ class FilterStateImpl : public FilterState { FilterStateSharedPtr parent_; const FilterState::LifeSpan life_span_; - InlineMapRegistry::InlineMapPtr data_storage_; + InlineMapPtr data_storage_; }; } // namespace StreamInfo diff --git a/source/server/server.cc b/source/server/server.cc index 0c8dfc59feac2..3ddc05116b924 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -479,7 +479,7 @@ void InstanceImpl::initialize(Network::Address::InstanceConstSharedPtr local_add ENVOY_LOG(info, "Inline map registry info:"); for (const auto& registry : InlineMapRegistryHelper::registries()) { ENVOY_LOG(info, " {}: {}", registry.first, - absl::StrJoin(registry.second->registrationSet(), ",")); + absl::StrJoin(registry.second->registrationVector(), ",")); }; // Initialize the regex engine and inject to singleton. diff --git a/test/common/common/inline_map_registry_speed_test.cc b/test/common/common/inline_map_registry_speed_test.cc index 1631dd676ed60..b254a712802bf 100644 --- a/test/common/common/inline_map_registry_speed_test.cc +++ b/test/common/common/inline_map_registry_speed_test.cc @@ -27,16 +27,14 @@ static void BM_InlineMapFind(benchmark::State& state) { registry_200.finalize(); registry_0.finalize(); - absl::flat_hash_map> normal_map; - auto inline_map_200 = registry_200.createInlineMap(); - auto inline_map_0 = registry_0.createInlineMap(); + absl::flat_hash_map normal_map; + auto inline_map_200 = InlineMap::create(registry_200); + auto inline_map_0 = InlineMap::create(registry_0); for (size_t i = 0; i < 200; ++i) { - normal_map[normal_keys[i]] = std::make_unique("value_" + std::to_string(i)); - inline_map_200->insert(normal_keys[i], - std::make_unique("value_" + std::to_string(i))); - inline_map_0->insert(normal_keys[i], - std::make_unique("value_" + std::to_string(i))); + normal_map[normal_keys[i]] = "value_" + std::to_string(i); + inline_map_200->insert(normal_keys[i], "value_" + std::to_string(i)); + inline_map_0->insert(normal_keys[i], "value_" + std::to_string(i)); } if (map_type == 0) { diff --git a/test/common/common/inline_map_registry_test.cc b/test/common/common/inline_map_registry_test.cc index 1829ca7d92330..944e06469efc3 100644 --- a/test/common/common/inline_map_registry_test.cc +++ b/test/common/common/inline_map_registry_test.cc @@ -9,12 +9,11 @@ TEST(InlineMapWithZeroInlineKey, InlineMapWithZeroInlineKeyTest) { InlineMapRegistry registry; registry.finalize(); - auto map = registry.createInlineMap(); + auto map = InlineMap::create(registry); // Insert keys. for (size_t i = 0; i < 200; ++i) { - map->insert("key_" + std::to_string(i), - std::make_unique("value_" + std::to_string(i))); + map->insert("key_" + std::to_string(i), "value_" + std::to_string(i)); } // Lookup keys. @@ -23,7 +22,7 @@ TEST(InlineMapWithZeroInlineKey, InlineMapWithZeroInlineKeyTest) { } // Lookup non-existing keys. - EXPECT_EQ(map->lookup("non_existing_key"), nullptr); + EXPECT_EQ(map->lookup("non_existing_key"), OptRef{}); // Remove keys. for (size_t i = 0; i < 200; ++i) { @@ -43,12 +42,11 @@ TEST(InlineMapWith20InlineKey, InlineMapWith20InlineKeyTest) { registry.finalize(); - auto map = registry.createInlineMap(); + auto map = InlineMap::create(registry); // Insert keys. for (size_t i = 0; i < 200; ++i) { - map->insert("key_" + std::to_string(i), - std::make_unique("value_" + std::to_string(i))); + map->insert("key_" + std::to_string(i), "value_" + std::to_string(i)); } // Lookup keys. @@ -65,7 +63,7 @@ TEST(InlineMapWith20InlineKey, InlineMapWith20InlineKeyTest) { } // Lookup non-existing keys. - EXPECT_EQ(map->lookup("non_existing_key"), nullptr); + EXPECT_EQ(map->lookup("non_existing_key"), OptRef{}); // Remove keys by typed inline handle. for (size_t i = 0; i < 10; ++i) { From acebae101c75397c7e9280fae694cb5029f3ffd6 Mon Sep 17 00:00:00 2001 From: wbpcode Date: Tue, 27 Jun 2023 09:53:22 +0000 Subject: [PATCH 26/36] minor update Signed-off-by: wbpcode --- source/common/common/inline_map_registry.h | 7 +------ test/common/common/inline_map_registry_test.cc | 3 +++ 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/source/common/common/inline_map_registry.h b/source/common/common/inline_map_registry.h index 681bc5a7546c1..f661a42e7eb87 100644 --- a/source/common/common/inline_map_registry.h +++ b/source/common/common/inline_map_registry.h @@ -340,12 +340,7 @@ template class InlineMap : public InlineStorage { bool empty() const { return size() == 0; } - ~InlineMap() { - for (uint64_t i = 0; i < registry_.registrationNum(); ++i) { - clearInlineMapEntry(i); - } - memset(inline_entries_, 0, registry_.registrationNum() * sizeof(Value)); - } + ~InlineMap() { memset(inline_entries_, 0, registry_.registrationNum() * sizeof(InlineEntry*)); } // Override operator [] to support the normal key assignment. template Value& operator[](const ArgK& key) { diff --git a/test/common/common/inline_map_registry_test.cc b/test/common/common/inline_map_registry_test.cc index 944e06469efc3..07949f3c31f7b 100644 --- a/test/common/common/inline_map_registry_test.cc +++ b/test/common/common/inline_map_registry_test.cc @@ -28,6 +28,7 @@ TEST(InlineMapWithZeroInlineKey, InlineMapWithZeroInlineKeyTest) { for (size_t i = 0; i < 200; ++i) { map->remove("key_" + std::to_string(i)); } + map.release(); } TEST(InlineMapWith20InlineKey, InlineMapWith20InlineKeyTest) { @@ -78,6 +79,8 @@ TEST(InlineMapWith20InlineKey, InlineMapWith20InlineKeyTest) { } EXPECT_EQ(map->size(), 0); + + map.release(); } } // namespace From 356135fc6e209dbc85502ba697beffefac86d262 Mon Sep 17 00:00:00 2001 From: wbpcode Date: Wed, 28 Jun 2023 08:00:26 +0000 Subject: [PATCH 27/36] complete renaming and some new tests Signed-off-by: wbpcode --- envoy/stream_info/BUILD | 2 +- envoy/stream_info/filter_state.h | 4 +- source/common/common/BUILD | 13 +- source/common/common/inline_map.h | 459 ++++++++++++++++++ source/common/common/inline_map_registry.cc | 3 - source/common/common/inline_map_registry.h | 440 ++--------------- .../common/inline_map_registry_helper.h | 71 --- source/common/network/application_protocol.cc | 5 +- .../common/network/filter_state_proxy_info.cc | 5 +- .../network/proxy_protocol_filter_state.cc | 5 +- source/common/network/upstream_server_name.cc | 5 +- .../network/upstream_subject_alt_names.cc | 5 +- source/common/router/debug_config.cc | 5 +- source/common/router/router.cc | 5 +- source/common/stream_info/filter_state_impl.h | 6 +- source/server/server.cc | 7 +- test/common/common/BUILD | 16 +- ...speed_test.cc => inline_map_speed_test.cc} | 18 +- ...ap_registry_test.cc => inline_map_test.cc} | 54 ++- 19 files changed, 587 insertions(+), 541 deletions(-) create mode 100644 source/common/common/inline_map.h delete mode 100644 source/common/common/inline_map_registry.cc delete mode 100644 source/common/common/inline_map_registry_helper.h rename test/common/common/{inline_map_registry_speed_test.cc => inline_map_speed_test.cc} (83%) rename test/common/common/{inline_map_registry_test.cc => inline_map_test.cc} (51%) diff --git a/envoy/stream_info/BUILD b/envoy/stream_info/BUILD index 9120d2c536615..6926c81cf92d1 100644 --- a/envoy/stream_info/BUILD +++ b/envoy/stream_info/BUILD @@ -34,7 +34,7 @@ envoy_cc_library( hdrs = ["filter_state.h"], external_deps = ["abseil_optional"], deps = [ - "//source/common/common:inline_map_registry_helper", + "//source/common/common:inline_map_registry", "//source/common/common:utility_lib", "//source/common/protobuf", ], diff --git a/envoy/stream_info/filter_state.h b/envoy/stream_info/filter_state.h index c8d18aae7089b..65d1dda29b4fe 100644 --- a/envoy/stream_info/filter_state.h +++ b/envoy/stream_info/filter_state.h @@ -6,7 +6,7 @@ #include "envoy/common/pure.h" #include "source/common/common/fmt.h" -#include "source/common/common/inline_map_registry_helper.h" +#include "source/common/common/inline_map_registry.h" #include "source/common/common/utility.h" #include "source/common/protobuf/protobuf.h" @@ -21,7 +21,7 @@ class FilterStateInlineMapScope { static absl::string_view name() { return "inline_map_scope_filter_state"; } }; -using InlineKey = InlineMapRegistryHelper::InlineKey; +using InlineKey = InlineMapRegistry::InlineKey; class FilterState; diff --git a/source/common/common/BUILD b/source/common/common/BUILD index b4cf86fffa2ac..d47bf36752075 100644 --- a/source/common/common/BUILD +++ b/source/common/common/BUILD @@ -579,12 +579,9 @@ envoy_pch_library( ) envoy_cc_library( - name = "inline_map_registry", - srcs = [ - "inline_map_registry.cc", - ], + name = "inline_map", hdrs = [ - "inline_map_registry.h", + "inline_map.h", ], external_deps = [ "abseil_node_hash_set", @@ -597,11 +594,11 @@ envoy_cc_library( ) envoy_cc_library( - name = "inline_map_registry_helper", + name = "inline_map_registry", hdrs = [ - "inline_map_registry_helper.h", + "inline_map_registry.h", ], deps = [ - ":inline_map_registry", + ":inline_map", ], ) diff --git a/source/common/common/inline_map.h b/source/common/common/inline_map.h new file mode 100644 index 0000000000000..de9476128c755 --- /dev/null +++ b/source/common/common/inline_map.h @@ -0,0 +1,459 @@ +#pragma once + +#include +#include +#include +#include + +#include "envoy/common/optref.h" + +#include "source/common/common/assert.h" +#include "source/common/common/macros.h" +#include "source/common/common/utility.h" + +#include "absl/container/flat_hash_map.h" +#include "absl/container/node_hash_set.h" + +namespace Envoy { + +/** + * This library provide inline map templates which could be used as an alternative of normal hash + * map to store the key/value pairs. But by these templates, you can add some frequently used + * keys as inline keys and get the handle for the key. Then you can use the handle to access the + * key/value pair in the map without even one time hash searching. + * + * This is useful when some keys are frequently used and the keys are known at compile time or + * bootstrap time. You can get superior performance by using the inline handle. For example, the + * filter state always uses the filter name as the key and the filter name is known at compile time. + * By using the inline handle, the filter state could get the key/value pair without any hash. + * + * This class also provides the interface to access the key/value pair by the normal key and the + * interface has similar searching performance as the normal hash map. But the insertion and removal + * by the normal key is slower than the normal hash map. + * + * + * These is usage example: + * + * // Define descriptor to store the inline keys. The descriptor should have lifetime that no + * // shorter than all inline maps created by the descriptor. You can create static and global + * // descriptor by ``MUTABLE_CONSTRUCT_ON_FIRST_USE`` for cross-modules usage, but it is not + * // required. A local descriptor is also fine if the inline map is only used in the local scope. + * InlineMapDescriptor descriptor; + * + * // Create the inline key. We should never do this after bootstrapping. Typically, we can + * // do this by define a global variable. + * InlineMapDescriptor::InlineKey inline_key = descriptor.addInlineKey("inline_key"); + * + * // Finalize the descriptor. No further changes are allowed to the descriptor after this point. + * descriptor.finalize(); + * + * // Create the inline map. + * InlineMapPtr inline_map = InlineMap::create(descriptor); + * + * // Get value by inline handle. + * inline_map->insert(inline_key, "value"); + * EXPECT_EQ(*inline_map->lookup(inline_key), "value"); + */ + +/** + * Inline map descriptor to store the inline keys and mapping of the inline keys to the inline + * id. + */ +template class InlineMapDescriptor { +public: + /** + * This is the handle used to access the key/value pair in the inline map. The handle is + * lightweight and could be copied and passed by value. + */ + class InlineKey { + public: + /** + * Get the id of the inline entry in the inline array. This could be used to access the + * key/value pair in the inline map without hash searching. + */ + uint64_t inlineId() const { return inline_id_; } + + private: + friend class InlineMapDescriptor; + + // This constructor should only be called by InlineMapDescriptor. + InlineKey(uint64_t inline_id) : inline_id_(inline_id) {} + + uint64_t inline_id_{}; + +#ifndef NDEBUG + public: + // This is the descriptor id that the inline key created by. This is used to validate the + // inline key validity. + uint64_t descriptorId() const { return descriptor_id_; } + + private: + InlineKey(uint64_t inline_id, uint64_t descriptor_id) + : inline_id_(inline_id), descriptor_id_(descriptor_id) {} + uint64_t descriptor_id_{}; +#endif + }; + + using InlineKeysMap = absl::flat_hash_map; + using InlineKeys = std::vector; + + InlineMapDescriptor() = default; + + /** + * Add and create an custom inline key. May only be called before finalize(). + */ + template InlineKey addInlineKey(const ArgK& key) { + RELEASE_ASSERT(!finalized_, "Cannot create new inline key after finalize()"); + + uint64_t inline_id = 0; + + if (auto it = inline_keys_map_.find(key); it != inline_keys_map_.end()) { + // If the key is already added, return the existing key. + inline_id = it->second; + } else { + // If the key is not added yet, then create a new inline key. + inline_id = inline_keys_map_.size(); + inline_keys_.push_back(Key(key)); + inline_keys_map_[key] = inline_id; + } + +#ifndef NDEBUG + return {inline_id, descriptor_id_}; +#else + return {inline_id}; +#endif + } + + /** + * Fetch the inline key for the given key. May only be called after finalize(). This should be + * used to get the inline key for the key added by addInlineKey(). This function could used to + * determine if the key is added as inline key or not at runtime or xDS config loading time and + * decide if the key should be used as inline key or normal key. + */ + template absl::optional getInlineKey(const ArgK& key) const { + ASSERT(finalized_, "Cannot get inline handle before finalize()"); + + uint64_t inline_id = 0; + + if (auto it = inline_keys_map_.find(key); it == inline_keys_map_.end()) { + return absl::nullopt; + } else { + inline_id = it->second; + } + +#ifndef NDEBUG + return InlineKey{inline_id, descriptor_id_}; +#else + return InlineKey{inline_id}; +#endif + } + + /** + * Get the inline keys map that contains all inline keys and their index. May only be called after + * finalize(). + */ + const InlineKeysMap& inlineKeysMap() const { + ASSERT(finalized_, "Cannot fetch registration map before finalize()"); + return inline_keys_map_; + } + + /** + * Get the array that contains all added inline keys. May only be called after finalize(). + */ + const InlineKeys& inlineKeys() const { + ASSERT(finalized_, "Cannot fetch registration set before finalize()"); + return inline_keys_; + } + + /** + * Get the number of the inline keys in this descriptor. May only be called after finalize(). + */ + uint64_t inlineKeysNum() const { + ASSERT(finalized_, "Cannot fetch registration map before finalize()"); + return inline_keys_map_.size(); + } + + // Finalize this descriptor. No further changes are allowed after this point. This guaranteed that + // all map created by the process have the same variable size and custom inline key adding. This + // function could be called multiple times safely and only the first call will have effect. + void finalize() { finalized_ = true; } + +#ifndef NDEBUG +public: + // This is the descriptor id that the inline map created by. This is used to validate the inline + // key validity. + uint64_t descriptorId() const { return descriptor_id_; } + +private: + static uint64_t generateDescriptorId() { + static std::atomic next_id{0}; + return next_id++; + } + + uint64_t descriptor_id_{generateDescriptorId()}; +#endif + +private: + bool finalized_{}; + InlineKeys inline_keys_; + InlineKeysMap inline_keys_map_; +}; + +/** + * This is the inline map that could be used as an alternative of normal hash map to store the + * key/value pairs. + */ +template class InlineMap : public InlineStorage { +public: + using TypedInlineMapDescriptor = InlineMapDescriptor; + using InlineKey = typename TypedInlineMapDescriptor::InlineKey; + + using UnderlayHashMap = absl::flat_hash_map; + + // Get the entry for the given key. If the key is not found, return nullptr. + template OptRef lookup(const ArgK& key) const { + // Because the normal string view key is used here, try the normal map entry first. + if (auto it = dynamic_entries_.find(key); it != dynamic_entries_.end()) { + return it->second; + } + + if (auto entry_id = inlineLookup(key); entry_id.has_value()) { + if (inlineEntryValid(*entry_id)) { + return inline_entries_[*entry_id]; + } + } + + return {}; + } + + template OptRef lookup(const ArgK& key) { + // Because the normal string view key is used here, try the normal map entry first. + if (auto it = dynamic_entries_.find(key); it != dynamic_entries_.end()) { + return it->second; + } + + if (auto entry_id = inlineLookup(key); entry_id.has_value()) { + if (inlineEntryValid(*entry_id)) { + return inline_entries_[*entry_id]; + } + } + + return {}; + } + + // Get the entry for the given inline key. If the handle is not valid, return nullptr. + OptRef lookup(InlineKey key) const { +#ifndef NDEBUG + ASSERT(key.descriptorId() == descriptor_.descriptorId(), "Invalid inline key"); +#endif + if (inlineEntryValid(key.inlineId())) { + return inline_entries_[key.inlineId()]; + } + return {}; + } + + OptRef lookup(InlineKey key) { +#ifndef NDEBUG + ASSERT(key.descriptorId() == descriptor_.descriptorId(), "Invalid inline key"); +#endif + if (inlineEntryValid(key.inlineId())) { + return inline_entries_[key.inlineId()]; + } + return {}; + } + + // Set the entry for the given key. If the key is already present, insert will fail and + // return false. Otherwise, insert will succeed and return true. + template bool insert(ArgK&& key, ArgV&& value) { + if (auto entry_id = inlineLookup(key); entry_id.has_value()) { + // This key is registered as inline key and try to insert the value to the inline array. + + // If the entry is already valid, insert will fail and return false. + if (inlineEntryValid(*entry_id)) { + return false; + } + + resetInlineMapEntry(*entry_id, std::move(value)); + return true; + } else { + // This key is not registered as inline key and try to insert the value to the normal map. + return dynamic_entries_.emplace(std::forward(key), std::forward(value)).second; + } + } + + // Set the entry for the given handle. If the handle is not valid, do nothing. + template bool insert(InlineKey key, ArgV&& value) { +#ifndef NDEBUG + ASSERT(key.descriptorId() == descriptor_.descriptorId(), "Invalid inline key"); +#endif + + // If the entry is already valid, insert will fail and return false. + if (inlineEntryValid(key.inlineId())) { + return false; + } + + resetInlineMapEntry(key.inlineId(), std::forward(value)); + return true; + } + + // Remove the entry for the given key. If the key is not found, do nothing. + template void remove(const ArgK& key) { + if (auto entry_id = inlineLookup(key); entry_id.has_value()) { + clearInlineMapEntry(*entry_id); + } else { + dynamic_entries_.erase(key); + } + } + + // Remove the entry for the given handle. If the handle is not valid, do nothing. + void remove(InlineKey key) { +#ifndef NDEBUG + ASSERT(key.descriptorId() == descriptor_.descriptorId(), "Invalid inline key"); +#endif + clearInlineMapEntry(key.inlineId()); + } + + void iterate(std::function callback) const { + for (const auto& entry : dynamic_entries_) { + if (!callback(entry.first, entry.second)) { + return; + } + } + + for (uint64_t i = 0; i < descriptor_.inlineKeysNum(); ++i) { + if (!inlineEntryValid(i)) { + continue; + } + + if (!callback(descriptor_.inlineKeys()[i], inline_entries_[i])) { + return; + } + } + } + + uint64_t size() const { return dynamic_entries_.size() + inline_entries_size_; } + + bool empty() const { return size() == 0; } + + ~InlineMap() { + // Destruct all inline entries. + for (uint64_t i = 0; i < descriptor_.inlineKeysNum(); ++i) { + clearInlineMapEntry(i); + } + memset(inline_entries_storage_, 0, descriptor_.inlineKeysNum() * sizeof(Value)); + }; + + // Override operator [] to support the normal key assignment. + template Value& operator[](const ArgK& key) { + if (auto entry_id = inlineLookup(key); entry_id.has_value()) { + // This key is registered as inline key and try to insert the value to the inline array. + if (!inlineEntryValid(*entry_id)) { + resetInlineMapEntry(*entry_id, Value()); + } + return inline_entries_[*entry_id]; + } else { + // This key is not registered as inline key and try to insert the value to the normal map. + return dynamic_entries_[key]; + } + } + + // Override operator [] to support the inline key assignment. + Value& operator[](InlineKey key) { +#ifndef NDEBUG + ASSERT(key.descriptorId() == descriptor_.descriptorId(), "Invalid inline key"); +#endif + + if (!inlineEntryValid(key.inlineId())) { + resetInlineMapEntry(key.inlineId(), Value()); + } + return inline_entries_[key.inlineId()]; + } + + static std::unique_ptr create(TypedInlineMapDescriptor& registry) { + // Call finalize() anyway to make sure that the registry is finalized and no any new inline + // keys could be registered. + registry.finalize(); + + return std::unique_ptr(new ((registry.inlineKeysNum() * sizeof(Value))) + InlineMap(registry)); + } + +private: + friend class InlineMapDescriptor; + + InlineMap(TypedInlineMapDescriptor& registry) : descriptor_(registry) { + // Initialize the inline entries to nullptr. + uint64_t inline_keys_num = descriptor_.inlineKeysNum(); + inline_entries_valid_.resize(inline_keys_num, false); + memset(inline_entries_storage_, 0, inline_keys_num * sizeof(Value)); + inline_entries_ = reinterpret_cast(inline_entries_storage_); + } + + template void resetInlineMapEntry(uint64_t inline_entry_id, ArgV&& value) { + ASSERT(inline_entry_id < descriptor_.inlineKeysNum()); + + clearInlineMapEntry(inline_entry_id); + + // Construct the new entry in the inline array. + new (inline_entries_ + inline_entry_id) Value(std::forward(value)); + inlineEntryValid(inline_entry_id, true); + } + + void clearInlineMapEntry(uint64_t inline_entry_id) { + ASSERT(inline_entry_id < descriptor_.inlineKeysNum()); + + // Destruct the old entry if it is valid. + if (inlineEntryValid(inline_entry_id)) { + (inline_entries_ + inline_entry_id)->~Value(); + inlineEntryValid(inline_entry_id, false); + } + } + + template absl::optional inlineLookup(const ArgK& key) const { + const auto& map_ref = descriptor_.inlineKeysMap(); + if (auto iter = map_ref.find(key); iter != map_ref.end()) { + return iter->second; + } + return absl::nullopt; + } + + bool inlineEntryValid(uint64_t inline_entry_id) const { + ASSERT(inline_entry_id < descriptor_.inlineKeysNum()); + return inline_entries_valid_[inline_entry_id]; + } + void inlineEntryValid(uint64_t inline_entry_id, bool flag) { + ASSERT(inline_entry_id < descriptor_.inlineKeysNum()); + + if (flag) { + inline_entries_size_++; + } else { + ASSERT(inline_entries_size_ > 0); + inline_entries_size_--; + } + + inline_entries_valid_[inline_entry_id] = flag; + } + + // This is the reference of the registry that the inline map created by. This is used to + // validate the inline handle validity and get the inline key set. + const TypedInlineMapDescriptor& descriptor_; + + // This is the underlay hash map for the normal entries. + UnderlayHashMap dynamic_entries_; + + // This is flags to indicate if the inline entries are valid. + // TODO(wbpcode): use memory in the storage to store the flags. + std::vector inline_entries_valid_; + + uint64_t inline_entries_size_{}; + + Value* inline_entries_{}; + + // This should be the last member of the class and no member should be added after this. + uint8_t inline_entries_storage_[]; +}; + +template using InlineMapPtr = std::unique_ptr>; + +} // namespace Envoy diff --git a/source/common/common/inline_map_registry.cc b/source/common/common/inline_map_registry.cc deleted file mode 100644 index 5293619059ff9..0000000000000 --- a/source/common/common/inline_map_registry.cc +++ /dev/null @@ -1,3 +0,0 @@ -#include "source/common/common/inline_map_registry.h" - -namespace Envoy {} // namespace Envoy diff --git a/source/common/common/inline_map_registry.h b/source/common/common/inline_map_registry.h index f661a42e7eb87..917678443be7c 100644 --- a/source/common/common/inline_map_registry.h +++ b/source/common/common/inline_map_registry.h @@ -1,434 +1,76 @@ -#pragma once - -#include -#include -#include - -#include "envoy/common/optref.h" - -#include "source/common/common/assert.h" -#include "source/common/common/macros.h" -#include "source/common/common/utility.h" - -#include "absl/container/flat_hash_map.h" -#include "absl/container/node_hash_set.h" +#include "source/common/common/inline_map.h" namespace Envoy { /** - * This library provide inline map templates which could be used as an alternative of normal hash - * map to store the key/value pairs. But by these templates, you can register some frequently used - * keys as inline keys and get the handle for the key. Then you can use the handle to access the - * key/value pair in the map without even one time hash searching. - * - * This is useful when some keys are frequently used and the keys are known at compile time or - * bootstrap time. You can get superior performance by using the inline handle. For example, the - * filter state always uses the filter name as the key and the filter name is known at compile time. - * By using the inline handle, the filter state could get the key/value pair without any hash. - * - * This class also provides the interface to access the key/value pair by the normal key and the - * interface has similar searching performance as the normal hash map. But the insertion and removal - * by the normal key is slower than the normal hash map. - * - * - * These is usage example: - * - * // Define registry to store the inline keys. The registry should have lifetime that no shorter - * // than all inline maps created by the registry. You can create static and global registry by - * // ``MUTABLE_CONSTRUCT_ON_FIRST_USE`` for cross-modules usage, but it is not required. A local - * // registry is also fine if the inline map is only used in the local scope. - * InlineMapRegistry registry; - * - * // Register the inline key. We should never do this after bootstrapping. Typically, we can - * // do this by define a global variable. - * InlineMapRegistry::InlineHandle inline_handle = - * registry.registerInlineKey("inline_key"); - * - * // Finalize the registry. No further changes are allowed to the registry after this point. - * registry.finalize(); - * - * // Create the inline map. - * InlineMapPtr inline_map = registry.createInlineMap(); - * - * // Get value by inline handle. - * inline_map->insert(inline_handle, std::make_unique("value")); - * EXPECT_EQ(*inline_map->lookup(inline_handle), "value"); - */ - -/** - * Inline map registry to store the inline keys and mapping of the inline keys to the inline - * id. + * Registry class to get inline map descriptor singleton based on the scope. If cross-modules inline + * map descriptor is necessary, this class should be used. */ -template class InlineMapRegistry { +class InlineMapRegistry { public: - /** - * This is the handle used to access the key/value pair in the inline map. The handle is - * lightweight and could be copied and passed by value. - */ - class InlineKey { - public: - /** - * Get the id of the inline entry in the inline array. This could be used to access the - * key/value pair in the inline map without hash searching. - */ - uint64_t inlineId() const { return inline_id_; } - - private: - friend class InlineMapRegistry; - - // This constructor should only be called by InlineMapRegistry. - InlineKey(uint64_t inline_id) : inline_id_(inline_id) {} - - uint64_t inline_id_{}; - -#ifndef NDEBUG - public: - // This is the registry id that the inline key created by. This is used to validate the - // inline key validity. - uint64_t registryId() const { return registry_id_; } - - private: - InlineKey(uint64_t inline_id, uint64_t registry_id) - : inline_id_(inline_id), registry_id_(registry_id) {} - uint64_t registry_id_{}; -#endif - }; - - using RegistrationMap = absl::flat_hash_map; - using RegistrationVector = std::vector; - - InlineMapRegistry() = default; + using InlineKey = InlineMapDescriptor::InlineKey; + using ManagedDescriptors = absl::flat_hash_map*>; /** - * Register a custom inline key. May only be called before finalize(). + * Get the inline map registry singleton based on the scope. */ - template InlineKey registerInlineKey(const ArgK& key) { - RELEASE_ASSERT(!finalized_, "Cannot register inline key after finalize()"); - - uint64_t inline_id = 0; - - if (auto it = registration_map_.find(key); it != registration_map_.end()) { - // If the key is already registered, return the existing key. - inline_id = it->second; - } else { - // If the key is not registered yet, then create a new inline key. - inline_id = registration_map_.size(); - registration_vector_.push_back(Key(key)); - registration_map_[key] = inline_id; - } - -#ifndef NDEBUG - return {inline_id, registry_id_}; -#else - return {inline_id}; -#endif + template + static InlineMapDescriptor::InlineKey addInlineKey(absl::string_view key) { + return mutableDescriptor().addInlineKey(key); } /** - * Fetch the inline handle for the given key. May only be called after finalize(). This should - * be used to get the inline handle for the key registered by registerInlineKey(). This function - * could used to determine if the key is registered or not at runtime or xDS config loading time - * and decide if the key should be used as inline key or normal key. + * Get the added inline key by the key name. */ - template absl::optional getInlineKey(const ArgK& key) const { - ASSERT(finalized_, "Cannot get inline handle before finalize()"); - - uint64_t inline_id = 0; - - if (auto it = registration_map_.find(key); it == registration_map_.end()) { - return absl::nullopt; - } else { - inline_id = it->second; - } - -#ifndef NDEBUG - return InlineKey{inline_id, registry_id_}; -#else - return InlineKey{inline_id}; -#endif + template + static absl::optional::InlineKey> + getInlineKey(absl::string_view key) { + return mutableDescriptor().getInlineKey(key); } /** - * Get the registration map that contains all registered inline keys. May only be called after - * finalize(). + * Create inline map based on the scope. */ - const RegistrationMap& registrationMap() const { - ASSERT(finalized_, "Cannot fetch registration map before finalize()"); - return registration_map_; + template static auto createInlineMap() { + return InlineMap::create(mutableDescriptor()); } /** - * Get the registration vector that contains all registered inline keys. May only be called after - * finalize(). + * Get all descriptors managed by this registry. This this helpful to finalize all descriptors and + * print the debug info of inline map registry. */ - const RegistrationVector& registrationVector() const { - ASSERT(finalized_, "Cannot fetch registration set before finalize()"); - return registration_vector_; - } + static const ManagedDescriptors& descriptors() { return mutableDescriptors(); } /** - * Get the number of the registrations. May only be called after finalize(). + * Finalize all descriptors managed by this registry. This should be called after all inline keys + * are added. */ - uint64_t registrationNum() const { - ASSERT(finalized_, "Cannot fetch registration map before finalize()"); - return registration_map_.size(); - } - - // Finalize this registry. No further changes are allowed after this point. This guaranteed that - // all map created by the process have the same variable size and custom registrations. This - // function could be called multiple times safely and only the first call will have effect. - void finalize() { finalized_ = true; } - -#ifndef NDEBUG -public: - // This is the registry id that the inline map created by. This is used to validate the inline - // key validity. - uint64_t registryId() const { return registry_id_; } - -private: - static uint64_t generateRegistryId() { - static std::atomic next_id{0}; - return next_id++; - } - - uint64_t registry_id_{generateRegistryId()}; -#endif - -private: - bool finalized_{}; - RegistrationVector registration_vector_; - RegistrationMap registration_map_; -}; - -/** - * This is the inline map that could be used as an alternative of normal hash map to store the - * key/value pairs. - */ -template class InlineMap : public InlineStorage { -public: - using TypedInlineMapRegistry = InlineMapRegistry; - using InlineKey = typename TypedInlineMapRegistry::InlineKey; - - using UnderlayHashMap = absl::flat_hash_map; - struct InlineEntry { - InlineEntry(Value&& value) : value_(std::move(value)) {} - InlineEntry(const Value& value) : value_(value) {} - - Value value_; - typename std::list::iterator entry_; - }; - using UnderlayInlineEntryStorage = std::list; - - // Get the entry for the given key. If the key is not found, return nullptr. - template OptRef lookup(const ArgK& key) const { - // Because the normal string view key is used here, try the normal map entry first. - if (auto it = dynamic_entries_.find(key); it != dynamic_entries_.end()) { - return it->second; + static void finalize() { + for (auto& registry : mutableDescriptors()) { + registry.second->finalize(); } - - if (auto entry_id = inlineLookup(key); entry_id.has_value()) { - if (auto entry = inline_entries_[*entry_id]; entry != nullptr) { - return entry->value_; - } - } - - return {}; - } - - template OptRef lookup(const ArgK& key) { - // Because the normal string view key is used here, try the normal map entry first. - if (auto it = dynamic_entries_.find(key); it != dynamic_entries_.end()) { - return it->second; - } - - if (auto entry_id = inlineLookup(key); entry_id.has_value()) { - if (auto entry = inline_entries_[*entry_id]; entry != nullptr) { - return entry->value_; - } - } - - return {}; - } - - // Get the entry for the given inline key. If the handle is not valid, return nullptr. - OptRef lookup(InlineKey key) const { -#ifndef NDEBUG - ASSERT(key.registryId() == registry_.registryId(), "Invalid inline key"); -#endif - if (auto entry = inline_entries_[key.inlineId()]; entry != nullptr) { - return entry->value_; - } - return {}; - } - - OptRef lookup(InlineKey key) { -#ifndef NDEBUG - ASSERT(key.registryId() == registry_.registryId(), "Invalid inline key"); -#endif - if (auto entry = inline_entries_[key.inlineId()]; entry != nullptr) { - return entry->value_; - } - return {}; - } - - // Set the entry for the given key. If the key is already present, insert will fail and - // return false. Otherwise, insert will succeed and return true. - template bool insert(ArgK&& key, ArgV&& value) { - if (auto entry_id = inlineLookup(key); entry_id.has_value()) { - // This key is registered as inline key and try to insert the value to the inline array. - - // If the entry is already valid, insert will fail and return false. - if (inline_entries_[*entry_id] != nullptr) { - return false; - } - - resetInlineMapEntry(*entry_id, std::move(value)); - return true; - } else { - // This key is not registered as inline key and try to insert the value to the normal map. - return dynamic_entries_.emplace(std::forward(key), std::forward(value)).second; - } - } - - // Set the entry for the given handle. If the handle is not valid, do nothing. - template void insert(InlineKey key, ArgV&& value) { -#ifndef NDEBUG - ASSERT(key.registryId() == registry_.registryId(), "Invalid inline key"); -#endif - resetInlineMapEntry(key.inlineId(), std::forward(value)); - } - - // Remove the entry for the given key. If the key is not found, do nothing. - template void remove(const ArgK& key) { - if (auto entry_id = inlineLookup(key); entry_id.has_value()) { - clearInlineMapEntry(*entry_id); - } else { - dynamic_entries_.erase(key); - } - } - - // Remove the entry for the given handle. If the handle is not valid, do nothing. - void remove(InlineKey key) { -#ifndef NDEBUG - ASSERT(key.registryId() == registry_.registryId(), "Invalid inline key"); -#endif - clearInlineMapEntry(key.inlineId()); - } - - void iterate(std::function callback) const { - for (const auto& entry : dynamic_entries_) { - if (!callback(entry.first, entry.second)) { - return; - } - } - - for (const auto& id : registry_.registrationMap()) { - ASSERT(id.second < registry_.registrationNum()); - - auto entry = inline_entries_[id.second]; - if (entry == nullptr) { - continue; - } - - if (!callback(id.first, entry->value_)) { - return; - } - } - } - - uint64_t size() const { return dynamic_entries_.size() + inline_entries_storage_.size(); } - - bool empty() const { return size() == 0; } - - ~InlineMap() { memset(inline_entries_, 0, registry_.registrationNum() * sizeof(InlineEntry*)); } - - // Override operator [] to support the normal key assignment. - template Value& operator[](const ArgK& key) { - if (auto entry_id = inlineLookup(key); entry_id.has_value()) { - // This key is registered as inline key and try to insert the value to the inline array. - if (inline_entries_[*entry_id] != nullptr) { - resetInlineMapEntry(*entry_id, Value()); - } - return inline_entries_[*entry_id]->value_; - } else { - // This key is not registered as inline key and try to insert the value to the normal map. - return dynamic_entries_[key]; - } - } - - // Override operator [] to support the inline key assignment. - Value& operator[](InlineKey key) { -#ifndef NDEBUG - ASSERT(key.registryId() == registry_.registryId(), "Invalid inline key"); -#endif - - if (inline_entries_[key.inlineId()] == nullptr) { - resetInlineMapEntry(key.inlineId(), Value()); - } - return inline_entries_[key.inlineId()]->value_; - } - - static std::unique_ptr create(TypedInlineMapRegistry& registry) { - // Call finalize() anyway to make sure that the registry is finalized and no any new inline - // keys could be registered. - registry.finalize(); - - return std::unique_ptr( - new ((registry.registrationMap().size() * sizeof(InlineEntry*))) InlineMap(registry)); } private: - friend class InlineMapRegistry; - - InlineMap(TypedInlineMapRegistry& registry) : registry_(registry) { - // Initialize the inline entries to nullptr. - memset(inline_entries_, 0, registry_.registrationNum() * sizeof(InlineEntry*)); - } - - template void resetInlineMapEntry(uint64_t inline_entry_id, ArgV&& value) { - ASSERT(inline_entry_id < registry_.registrationNum()); - - clearInlineMapEntry(inline_entry_id); - - auto iter = - inline_entries_storage_.emplace(inline_entries_storage_.end(), std::forward(value)); - - iter->entry_ = iter; - inline_entries_[inline_entry_id] = &(*iter); + template static InlineMapDescriptor& mutableDescriptor() { + static InlineMapDescriptor* descriptor = []() { + auto* inner = new InlineMapDescriptor(); + auto result = InlineMapRegistry::mutableDescriptors().emplace(Scope::name(), inner); + RELEASE_ASSERT(result.second, + absl::StrCat("Duplicate inline map registry for scope: ", Scope::name())); + return inner; + }(); + return *descriptor; } - void clearInlineMapEntry(uint64_t inline_entry_id) { - ASSERT(inline_entry_id < registry_.registrationNum()); - - // Destruct the old entry if it is valid. - if (auto entry = inline_entries_[inline_entry_id]; entry != nullptr) { - inline_entries_storage_.erase(entry->entry_); - inline_entries_[inline_entry_id] = nullptr; - } + static ManagedDescriptors& mutableDescriptors() { + MUTABLE_CONSTRUCT_ON_FIRST_USE(ManagedDescriptors); } - - template absl::optional inlineLookup(const ArgK& key) const { - const auto& map_ref = registry_.registrationMap(); - if (auto iter = map_ref.find(key); iter != map_ref.end()) { - return iter->second; - } - return absl::nullopt; - } - - // This is the reference of the registry that the inline map created by. This is used to - // validate the inline handle validity and get the inline key set. - const TypedInlineMapRegistry& registry_; - - // This is the underlay hash map for the normal entries. - UnderlayHashMap dynamic_entries_; - - // This is the inline entries storage. The inline entries are stored in the list. - UnderlayInlineEntryStorage inline_entries_storage_; - - // This should be the last member of the class and no member should be added after this. - InlineEntry* inline_entries_[]; }; -template using InlineMapPtr = std::unique_ptr>; +// Macro to add inline key to the descriptor based on the scope. This will create a static variable +// with the name of the key and the scope. +#define REGISTER_INLINE_KEY(Scope, Name, Key) \ + static const auto Name = Envoy::InlineMapRegistry::addInlineKey(Key); } // namespace Envoy diff --git a/source/common/common/inline_map_registry_helper.h b/source/common/common/inline_map_registry_helper.h deleted file mode 100644 index a662dc8a62e48..0000000000000 --- a/source/common/common/inline_map_registry_helper.h +++ /dev/null @@ -1,71 +0,0 @@ -#include "source/common/common/inline_map_registry.h" - -namespace Envoy { - -/** - * Helper class to get inline map registry singleton based on the scope. If cross-modules inline map - * registry is necessary, this class should be used. - */ -class InlineMapRegistryHelper { -public: - using InlineKey = InlineMapRegistry::InlineKey; - using ManagedRegistries = absl::flat_hash_map*>; - - /** - * Get the inline map registry singleton based on the scope. - */ - template - static InlineMapRegistry::InlineKey registerInlinKey(absl::string_view key) { - return mutableRegistry().registerInlineKey(key); - } - - /** - * Get registered inline key by the key name. - */ - template - static absl::optional::InlineKey> - getInlineKey(absl::string_view key) { - return mutableRegistry().getInlineKey(key); - } - - /** - * Create inline map based on the scope. - */ - template static auto createInlineMap() { - return InlineMap::create(mutableRegistry()); - } - - /** - * Get all registries managed by this helper. This this helpful to finalize all registries and - * print the debug info of inline map registry. - */ - static const ManagedRegistries& registries() { return mutableRegistries(); } - - /** - * Finalize all registries managed by this helper. This should be called after all inline keys - * are registered. - */ - static void finalize() { - for (auto& registry : mutableRegistries()) { - registry.second->finalize(); - } - } - -private: - template static InlineMapRegistry& mutableRegistry() { - static InlineMapRegistry* registry = []() { - auto* registry = new InlineMapRegistry(); - auto result = InlineMapRegistryHelper::mutableRegistries().emplace(Scope::name(), registry); - RELEASE_ASSERT(result.second, - absl::StrCat("Duplicate inline map registry for scope: ", Scope::name())); - return registry; - }(); - return *registry; - } - - static ManagedRegistries& mutableRegistries() { - MUTABLE_CONSTRUCT_ON_FIRST_USE(ManagedRegistries); - } -}; - -} // namespace Envoy diff --git a/source/common/network/application_protocol.cc b/source/common/network/application_protocol.cc index 883413773c63e..6dace49c86a45 100644 --- a/source/common/network/application_protocol.cc +++ b/source/common/network/application_protocol.cc @@ -5,9 +5,8 @@ namespace Envoy { namespace Network { -auto application_protocols_inline_key = - InlineMapRegistryHelper::registerInlinKey( - "envoy.network.application_protocols"); +REGISTER_INLINE_KEY(StreamInfo::FilterStateInlineMapScope, application_protocols_inline_key, + "envoy.network.application_protocols"); const StreamInfo::InlineKey ApplicationProtocols::key() { return application_protocols_inline_key; } diff --git a/source/common/network/filter_state_proxy_info.cc b/source/common/network/filter_state_proxy_info.cc index 4db945e30e565..ed746454d0eb0 100644 --- a/source/common/network/filter_state_proxy_info.cc +++ b/source/common/network/filter_state_proxy_info.cc @@ -3,9 +3,8 @@ namespace Envoy { namespace Network { -auto http11_proxy_info_inline_key = - InlineMapRegistryHelper::registerInlinKey( - "envoy.network.transport_socket.http_11_proxy.info"); +REGISTER_INLINE_KEY(StreamInfo::FilterStateInlineMapScope, http11_proxy_info_inline_key, + "envoy.network.transport_socket.http_11_proxy.info"); const StreamInfo::InlineKey Http11ProxyInfoFilterState::key() { return http11_proxy_info_inline_key; diff --git a/source/common/network/proxy_protocol_filter_state.cc b/source/common/network/proxy_protocol_filter_state.cc index 9fd9af7e9714e..2fee89b0f2a7e 100644 --- a/source/common/network/proxy_protocol_filter_state.cc +++ b/source/common/network/proxy_protocol_filter_state.cc @@ -5,9 +5,8 @@ namespace Envoy { namespace Network { -auto proxy_protocol_options_inline_key = - InlineMapRegistryHelper::registerInlinKey( - "envoy.network.proxy_protocol_options"); +REGISTER_INLINE_KEY(StreamInfo::FilterStateInlineMapScope, proxy_protocol_options_inline_key, + "envoy.network.proxy_protocol_options"); const StreamInfo::InlineKey ProxyProtocolFilterState::key() { return proxy_protocol_options_inline_key; diff --git a/source/common/network/upstream_server_name.cc b/source/common/network/upstream_server_name.cc index 52bd8db4ec023..c34e74fe69d59 100644 --- a/source/common/network/upstream_server_name.cc +++ b/source/common/network/upstream_server_name.cc @@ -5,9 +5,8 @@ namespace Envoy { namespace Network { -auto upstream_server_name_inline_key = - InlineMapRegistryHelper::registerInlinKey( - "envoy.network.upstream_server_name"); +REGISTER_INLINE_KEY(StreamInfo::FilterStateInlineMapScope, upstream_server_name_inline_key, + "envoy.network.upstream_server_name"); const StreamInfo::InlineKey UpstreamServerName::key() { return upstream_server_name_inline_key; } diff --git a/source/common/network/upstream_subject_alt_names.cc b/source/common/network/upstream_subject_alt_names.cc index e32698783d72c..3d1cf73f42398 100644 --- a/source/common/network/upstream_subject_alt_names.cc +++ b/source/common/network/upstream_subject_alt_names.cc @@ -5,9 +5,8 @@ namespace Envoy { namespace Network { -auto upstream_subject_alt_names_inline_key = - InlineMapRegistryHelper::registerInlinKey( - "envoy.network.upstream_subject_alt_names"); +REGISTER_INLINE_KEY(StreamInfo::FilterStateInlineMapScope, upstream_subject_alt_names_inline_key, + "envoy.network.upstream_subject_alt_names"); const StreamInfo::InlineKey UpstreamSubjectAltNames::key() { return upstream_subject_alt_names_inline_key; diff --git a/source/common/router/debug_config.cc b/source/common/router/debug_config.cc index 1cf9f7b74c0a4..6a3015fc548b6 100644 --- a/source/common/router/debug_config.cc +++ b/source/common/router/debug_config.cc @@ -5,9 +5,8 @@ namespace Envoy { namespace Router { -auto debug_config_inline_key = - InlineMapRegistryHelper::registerInlinKey( - "envoy.router.debug_config"); +REGISTER_INLINE_KEY(StreamInfo::FilterStateInlineMapScope, debug_config_inline_key, + "envoy.router.debug_config"); DebugConfig::DebugConfig(bool append_cluster, absl::optional cluster_header, bool append_upstream_host, diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 8e56e3fecc376..26370cb7870bc 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -46,9 +46,8 @@ namespace Envoy { namespace Router { namespace { -static StreamInfo::InlineKey NumInternalRedirectsFilterStateName = - InlineMapRegistryHelper::registerInlinKey( - "num_internal_redirects"); +REGISTER_INLINE_KEY(StreamInfo::FilterStateInlineMapScope, NumInternalRedirectsFilterStateName, + "num_internal_redirects"); uint32_t getLength(const Buffer::Instance* instance) { return instance ? instance->length() : 0; } diff --git a/source/common/stream_info/filter_state_impl.h b/source/common/stream_info/filter_state_impl.h index 2197f63b36c89..4c456026ddd0a 100644 --- a/source/common/stream_info/filter_state_impl.h +++ b/source/common/stream_info/filter_state_impl.h @@ -17,7 +17,7 @@ class FilterStateImpl : public FilterState { FilterStateImpl(FilterState::LifeSpan life_span) : life_span_(life_span), data_storage_( - InlineMapRegistryHelper::createInlineMap()) { + InlineMapRegistry::createInlineMap()) { maybeCreateParent(ParentAccessMode::ReadOnly); } @@ -28,7 +28,7 @@ class FilterStateImpl : public FilterState { FilterStateImpl(FilterStateSharedPtr ancestor, FilterState::LifeSpan life_span) : ancestor_(ancestor), life_span_(life_span), data_storage_( - InlineMapRegistryHelper::createInlineMap()) { + InlineMapRegistry::createInlineMap()) { maybeCreateParent(ParentAccessMode::ReadOnly); } @@ -42,7 +42,7 @@ class FilterStateImpl : public FilterState { FilterStateImpl(LazyCreateAncestor lazy_create_ancestor, FilterState::LifeSpan life_span) : ancestor_(lazy_create_ancestor), life_span_(life_span), data_storage_( - InlineMapRegistryHelper::createInlineMap()) { + InlineMapRegistry::createInlineMap()) { maybeCreateParent(ParentAccessMode::ReadOnly); } diff --git a/source/server/server.cc b/source/server/server.cc index 3ddc05116b924..a5cd7dfdd65a5 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -475,11 +475,10 @@ void InstanceImpl::initialize(Network::Address::InstanceConstSharedPtr local_add } // Finalize all managed inline map registries. - InlineMapRegistryHelper::finalize(); + InlineMapRegistry::finalize(); ENVOY_LOG(info, "Inline map registry info:"); - for (const auto& registry : InlineMapRegistryHelper::registries()) { - ENVOY_LOG(info, " {}: {}", registry.first, - absl::StrJoin(registry.second->registrationVector(), ",")); + for (const auto& registry : InlineMapRegistry::descriptors()) { + ENVOY_LOG(info, " {}: {}", registry.first, absl::StrJoin(registry.second->inlineKeys(), ",")); }; // Initialize the regex engine and inject to singleton. diff --git a/test/common/common/BUILD b/test/common/common/BUILD index cd64a80ac23b7..95cfdbfe8acfd 100644 --- a/test/common/common/BUILD +++ b/test/common/common/BUILD @@ -494,19 +494,19 @@ envoy_cc_test( ) envoy_cc_test( - name = "inline_map_registry_test", - srcs = ["inline_map_registry_test.cc"], - deps = ["//source/common/common:inline_map_registry"], + name = "inline_map_test", + srcs = ["inline_map_test.cc"], + deps = ["//source/common/common:inline_map"], ) envoy_cc_benchmark_binary( - name = "inline_map_registry_speed_test", - srcs = ["inline_map_registry_speed_test.cc"], + name = "inline_map_speed_test", + srcs = ["inline_map_speed_test.cc"], external_deps = ["benchmark"], - deps = ["//source/common/common:inline_map_registry"], + deps = ["//source/common/common:inline_map"], ) envoy_benchmark_test( - name = "inline_map_registry_speed_test_benchmark_test", - benchmark_binary = "inline_map_registry_speed_test", + name = "inline_map_speed_test_benchmark_test", + benchmark_binary = "inline_map_speed_test", ) diff --git a/test/common/common/inline_map_registry_speed_test.cc b/test/common/common/inline_map_speed_test.cc similarity index 83% rename from test/common/common/inline_map_registry_speed_test.cc rename to test/common/common/inline_map_speed_test.cc index b254a712802bf..f0055abcfecd3 100644 --- a/test/common/common/inline_map_registry_speed_test.cc +++ b/test/common/common/inline_map_speed_test.cc @@ -1,6 +1,6 @@ #include -#include "source/common/common/inline_map_registry.h" +#include "source/common/common/inline_map.h" #include "benchmark/benchmark.h" @@ -14,22 +14,22 @@ static void BM_InlineMapFind(benchmark::State& state) { std::vector normal_keys; - InlineMapRegistry registry_200; + InlineMapDescriptor descriptor_200; // Create 200 inline keys. - std::vector::InlineKey> inline_handles_200; + std::vector::InlineKey> inline_handles_200; for (size_t i = 0; i < 200; ++i) { const std::string key = "key_" + std::to_string(i); - inline_handles_200.push_back(registry_200.registerInlineKey(key)); + inline_handles_200.push_back(descriptor_200.addInlineKey(key)); normal_keys.push_back(key); } - InlineMapRegistry registry_0; + InlineMapDescriptor descriptor_0; - registry_200.finalize(); - registry_0.finalize(); + descriptor_200.finalize(); + descriptor_0.finalize(); absl::flat_hash_map normal_map; - auto inline_map_200 = InlineMap::create(registry_200); - auto inline_map_0 = InlineMap::create(registry_0); + auto inline_map_200 = InlineMap::create(descriptor_200); + auto inline_map_0 = InlineMap::create(descriptor_0); for (size_t i = 0; i < 200; ++i) { normal_map[normal_keys[i]] = "value_" + std::to_string(i); diff --git a/test/common/common/inline_map_registry_test.cc b/test/common/common/inline_map_test.cc similarity index 51% rename from test/common/common/inline_map_registry_test.cc rename to test/common/common/inline_map_test.cc index 07949f3c31f7b..dcc56a23d0427 100644 --- a/test/common/common/inline_map_registry_test.cc +++ b/test/common/common/inline_map_test.cc @@ -1,4 +1,4 @@ -#include "source/common/common/inline_map_registry.h" +#include "source/common/common/inline_map.h" #include "gtest/gtest.h" @@ -6,19 +6,29 @@ namespace Envoy { namespace { TEST(InlineMapWithZeroInlineKey, InlineMapWithZeroInlineKeyTest) { - InlineMapRegistry registry; - registry.finalize(); + InlineMapDescriptor descriptor; + descriptor.finalize(); - auto map = InlineMap::create(registry); + auto map = InlineMap::create(descriptor); // Insert keys. for (size_t i = 0; i < 200; ++i) { map->insert("key_" + std::to_string(i), "value_" + std::to_string(i)); } + // Insert duplicate keys will fail. + for (size_t i = 0; i < 200; ++i) { + EXPECT_FALSE(map->insert("key_" + std::to_string(i), "value_" + std::to_string(i))); + } + + // Use operator[] to insert keys could overwrite existing keys. + for (size_t i = 0; i < 200; ++i) { + (*map)["key_" + std::to_string(i)] = "value_" + std::to_string(i) + "_new"; + } + // Lookup keys. for (size_t i = 0; i < 200; ++i) { - EXPECT_EQ(*map->lookup("key_" + std::to_string(i)), "value_" + std::to_string(i)); + EXPECT_EQ(*map->lookup("key_" + std::to_string(i)), "value_" + std::to_string(i) + "_new"); } // Lookup non-existing keys. @@ -32,27 +42,47 @@ TEST(InlineMapWithZeroInlineKey, InlineMapWithZeroInlineKeyTest) { } TEST(InlineMapWith20InlineKey, InlineMapWith20InlineKeyTest) { - InlineMapRegistry registry; + InlineMapDescriptor descriptor; - std::vector::InlineKey> inline_keys; + std::vector::InlineKey> inline_keys; // Create 20 inline keys. for (size_t i = 0; i < 20; ++i) { - inline_keys.push_back(registry.registerInlineKey("key_" + std::to_string(i))); + inline_keys.push_back(descriptor.addInlineKey("key_" + std::to_string(i))); } - registry.finalize(); + descriptor.finalize(); - auto map = InlineMap::create(registry); + auto map = InlineMap::create(descriptor); // Insert keys. for (size_t i = 0; i < 200; ++i) { map->insert("key_" + std::to_string(i), "value_" + std::to_string(i)); } + // Insert duplicate keys will fail. + for (size_t i = 0; i < 200; ++i) { + EXPECT_FALSE(map->insert("key_" + std::to_string(i), "value_" + std::to_string(i))); + } + + // Insert duplicate keys with typed inline handle will fail. + for (size_t i = 0; i < 20; ++i) { + EXPECT_FALSE(map->insert(inline_keys[i], "value_" + std::to_string(i))); + } + + // Use operator[] to insert keys could overwrite existing keys. + for (size_t i = 20; i < 200; ++i) { + (*map)["key_" + std::to_string(i)] = "value_" + std::to_string(i) + "_new"; + } + + // Use operator[] to insert keys with typed inline handle could overwrite existing keys. + for (size_t i = 0; i < 20; ++i) { + (*map)[inline_keys[i]] = "value_" + std::to_string(i) + "_new"; + } + // Lookup keys. for (size_t i = 0; i < 200; ++i) { - EXPECT_EQ(*map->lookup("key_" + std::to_string(i)), "value_" + std::to_string(i)); + EXPECT_EQ(*map->lookup("key_" + std::to_string(i)), "value_" + std::to_string(i) + "_new"); } // Lookup by typed inline handle. @@ -60,7 +90,7 @@ TEST(InlineMapWith20InlineKey, InlineMapWith20InlineKeyTest) { const std::string key = "key_" + std::to_string(i); auto handle = inline_keys[i]; EXPECT_EQ(handle.inlineId(), i); - EXPECT_EQ(*map->lookup(handle), "value_" + std::to_string(i)); + EXPECT_EQ(*map->lookup(handle), "value_" + std::to_string(i) + "_new"); } // Lookup non-existing keys. From cae8989808cb055828ba4db94b857cfeda293cc2 Mon Sep 17 00:00:00 2001 From: wbpcode Date: Fri, 1 Sep 2023 04:58:35 +0000 Subject: [PATCH 28/36] refactor an refactor Signed-off-by: wbpcode --- envoy/stream_info/filter_state.h | 2 +- source/common/common/BUILD | 3 + source/common/common/inline_map_registry.cc | 55 ++++++ source/common/common/inline_map_registry.h | 172 +++++++++++++----- source/common/network/application_protocol.cc | 11 +- .../common/network/filter_state_proxy_info.cc | 9 +- .../network/proxy_protocol_filter_state.cc | 8 +- source/common/network/upstream_server_name.cc | 9 +- .../network/upstream_subject_alt_names.cc | 8 +- source/common/router/debug_config.cc | 9 +- source/common/router/router.cc | 16 +- 11 files changed, 236 insertions(+), 66 deletions(-) create mode 100644 source/common/common/inline_map_registry.cc diff --git a/envoy/stream_info/filter_state.h b/envoy/stream_info/filter_state.h index 6e383da9f814f..dd3485ea0e46a 100644 --- a/envoy/stream_info/filter_state.h +++ b/envoy/stream_info/filter_state.h @@ -22,7 +22,7 @@ class FilterStateInlineMapScope { static absl::string_view name() { return "inline_map_scope_filter_state"; } }; -using InlineKey = InlineMapRegistry::InlineKey; +using InlineKey = InlineMapRegistry::Handle; class FilterState; diff --git a/source/common/common/BUILD b/source/common/common/BUILD index 1602832d05cd2..43da9b10fca95 100644 --- a/source/common/common/BUILD +++ b/source/common/common/BUILD @@ -600,6 +600,9 @@ envoy_cc_library( envoy_cc_library( name = "inline_map_registry", + srcs = [ + "inline_map_registry.cc", + ], hdrs = [ "inline_map_registry.h", ], diff --git a/source/common/common/inline_map_registry.cc b/source/common/common/inline_map_registry.cc new file mode 100644 index 0000000000000..7b33a046cd5e1 --- /dev/null +++ b/source/common/common/inline_map_registry.cc @@ -0,0 +1,55 @@ +#include "source/common/common/inline_map_registry.h" + +namespace Envoy { + +InlineMapRegistry::InlineMapDescriptor* +InlineMapRegistry::createDescriptor(absl::string_view scope_name) { + // This should never be triggered because the developer should ensure all used/managed + // descriptors are registered before main() function by static variable initialization. + RELEASE_ASSERT(!finalized(), "Registry is finalized, cannot create new descriptor"); + + // Create the descriptor and insert it into the registry. + auto descriptor = new InlineMapDescriptor(); + auto [_, inserted] = registry().emplace(scope_name, descriptor); + + // Check whether the descriptor is created repeatedly. + RELEASE_ASSERT(inserted, fmt::format("Create descriptor {} repeatedly", scope_name)); + + return descriptor; +} + +InlineMapRegistry::RegistryInfo InlineMapRegistry::registryInfo() { + std::vector> registry_info; + for (const auto& [name, descriptor] : registry()) { + registry_info.emplace_back(name, descriptor->inlineKeysAsString()); + } + return registry_info; +} + +void InlineMapRegistry::finalize(const ScopeInlineKeysVector& scope_inline_keys_vector) { + // Call once to finalize the registry and all managed descriptors. This is used to ensure + // the registry is finalized only once on multi-thread environment. + std::call_once(onceFlag(), [&scope_inline_keys_vector]() { + finalized() = true; + + // Add inline keys to the descriptor based on the scope name. This is last chance to add + // inline keys to the descriptor. + for (const auto& inline_keys : scope_inline_keys_vector) { + auto iter = registry().find(inline_keys.name); + if (iter == registry().end()) { + // Skip the scope that is not registered. + ENVOY_LOG_MISC(warn, "Skip the scope {} that is not registered", inline_keys.name); + continue; + } + + for (const auto& key : inline_keys.keys) { + iter->second->addInlineKey(key); + } + } + + // Finalize all managed descriptors. + std::for_each(registry().begin(), registry().end(), [](auto& p) { p.second->finalize(); }); + }); +} + +} // namespace Envoy diff --git a/source/common/common/inline_map_registry.h b/source/common/common/inline_map_registry.h index 917678443be7c..c6e7969ef2d58 100644 --- a/source/common/common/inline_map_registry.h +++ b/source/common/common/inline_map_registry.h @@ -1,3 +1,5 @@ +#pragma once + #include "source/common/common/inline_map.h" namespace Envoy { @@ -5,72 +7,158 @@ namespace Envoy { /** * Registry class to get inline map descriptor singleton based on the scope. If cross-modules inline * map descriptor is necessary, this class should be used. + * + * There are only two allowed ways to update or modify the registry: + * 1. Add inline map descriptors or inline keys to the registry before the main() function is called + * by the REGISTER_INLINE_MAP_DESCRIPTOR or REGISTER_INLINE_MAP_KEY macro. + * 2. Add inline map descriptors or inline keys in the finalize() function. + * + * The finalize() function will be called once when the first server is initialized. This is safe + * even there are multiple main threads or servers. */ class InlineMapRegistry { public: - using InlineKey = InlineMapDescriptor::InlineKey; - using ManagedDescriptors = absl::flat_hash_map*>; + /** + * Helper template class to initialize descriptor based on the scope explicitly. + */ + template class InlineMapDescriptorRegister { + public: + InlineMapDescriptorRegister(); + }; /** - * Get the inline map registry singleton based on the scope. + * Helper template class to add inline key to the descriptor based on the scope. */ - template - static InlineMapDescriptor::InlineKey addInlineKey(absl::string_view key) { - return mutableDescriptor().addInlineKey(key); - } + template class InlineMapKeyRegister { + public: + InlineMapKeyRegister(absl::string_view); + }; + + using InlineMapDescriptor = InlineMapDescriptor; + using Handle = InlineMapDescriptor::Handle; + + struct ScopeInlineKeys { + std::string name; + absl::flat_hash_set keys; + }; /** - * Get the added inline key by the key name. + * Scope inline keys vector type. This is used to finalize the registry and add inline keys to + * the descriptor based on the scope name. */ - template - static absl::optional::InlineKey> - getInlineKey(absl::string_view key) { - return mutableDescriptor().getInlineKey(key); - } + using ScopeInlineKeysVector = std::vector; /** - * Create inline map based on the scope. + * Registry info type. This is used to get scope name and all managed descriptors' inline keys as + * string. */ - template static auto createInlineMap() { - return InlineMap::create(mutableDescriptor()); - } + using RegistryInfo = std::vector>; + +private: + using Registry = absl::flat_hash_map; + + template friend class InlineMapDescriptorRegister; + template friend class InlineMapKeyRegister; /** - * Get all descriptors managed by this registry. This this helpful to finalize all descriptors and - * print the debug info of inline map registry. + * Call once flag to ensure the registry is finalized only once on multi-thread environment. */ - static const ManagedDescriptors& descriptors() { return mutableDescriptors(); } + static std::once_flag& onceFlag() { MUTABLE_CONSTRUCT_ON_FIRST_USE(std::once_flag); } /** - * Finalize all descriptors managed by this registry. This should be called after all inline keys - * are added. + * Finalized flag to check whether the registry is finalized. */ - static void finalize() { - for (auto& registry : mutableDescriptors()) { - registry.second->finalize(); - } + static bool& finalized() { MUTABLE_CONSTRUCT_ON_FIRST_USE(bool, false); } + + /** + * A set of inline map registry that indexed by the scope name. + */ + static Registry& registry() { MUTABLE_CONSTRUCT_ON_FIRST_USE(Registry); } + + /** + * Create the inline map descriptor based on the scope name and insert it into the registry. + */ + static InlineMapDescriptor* createDescriptor(absl::string_view scope_name); + + /** + * Get or create the inline map registry singleton based on the scope type. + */ + template static InlineMapDescriptor& getOrCreateDescriptor() { + // The function level static variable will be initialized only once and is thread safe. + // This is same with MUTABLE_CONSTRUCT_ON_FIRST_USE but has complex initialization. + static InlineMapDescriptor* initialize_once_descriptor = createDescriptor(Scope::name()); + return *initialize_once_descriptor; } -private: - template static InlineMapDescriptor& mutableDescriptor() { - static InlineMapDescriptor* descriptor = []() { - auto* inner = new InlineMapDescriptor(); - auto result = InlineMapRegistry::mutableDescriptors().emplace(Scope::name(), inner); - RELEASE_ASSERT(result.second, - absl::StrCat("Duplicate inline map registry for scope: ", Scope::name())); - return inner; - }(); - return *descriptor; + /** + * Add inline key to the descriptor based on the scope. + */ + template static void addInlineKey(absl::string_view key) { + getOrCreateDescriptor().addInlineKey(key); } - static ManagedDescriptors& mutableDescriptors() { - MUTABLE_CONSTRUCT_ON_FIRST_USE(ManagedDescriptors); +public: + /** + * Get the inline handle by the key name. + */ + template static absl::optional getHandleByKey(absl::string_view key) { + return getOrCreateDescriptor().getHandleByKey(key); + } + + /** + * Create inline map based on the scope and value type. + */ + template static auto createInlineMap() { + return InlineMap::create(getOrCreateDescriptor()); } + + /** + * Get scope name and all managed descriptors' inline keys as string. + */ + static RegistryInfo registryInfo(); + + /** + * Finalize the registry and all managed descriptors by this registry. This only be called once + * when the server is initialized. This is safe even there are multiple main threads or servers. + */ + static void finalize(const ScopeInlineKeysVector& scope_inline_keys_vector = {}); }; -// Macro to add inline key to the descriptor based on the scope. This will create a static variable -// with the name of the key and the scope. -#define REGISTER_INLINE_KEY(Scope, Name, Key) \ - static const auto Name = Envoy::InlineMapRegistry::addInlineKey(Key); +template +InlineMapRegistry::InlineMapDescriptorRegister::InlineMapDescriptorRegister() { + InlineMapRegistry::getOrCreateDescriptor(); +} + +template +InlineMapRegistry::InlineMapKeyRegister::InlineMapKeyRegister(absl::string_view key) { + InlineMapRegistry::addInlineKey(key); +} + +/** + * Helper macro to register inline map descriptor based on the scope explicitly. If cross-modules + * inline map descriptor is necessary and InlineMapRegistry is used, then this macro should be used. + */ +#define REGISTER_INLINE_MAP_DESCRIPTOR(Scope) \ + static const Envoy::InlineMapRegistry::InlineMapDescriptorRegister< \ + Scope>##InlineMapDescriptorRegister; + +/** + * Helper macro to add inline key to the descriptor based on the scope. If cross-modules inline map + * descriptor is necessary and InlineMapRegistry is used, then this macro should be used. + */ +#define REGISTER_INLINE_MAP_KEY(Scope, Key) \ + static const Envoy::InlineMapRegistry::InlineMapKeyRegister Key##InlineMapKeyRegister(Key); + +/** + * Helper macro to get inline handle by the key name. Note this macro should be used in the function + * that is called to get the inline handle. And please ensure the key is registered before the + * function is called. + */ +#define INLINE_HANDLE_BY_KEY_ON_FIRST_USE(Scope, Key) \ + do { \ + static auto handle = Envoy::InlineMapRegistry::getHandleByKey(Key); \ + ASSERT(handle.has_value(), fmt::format("Cannot get inline handle by key {}", Key)); \ + return handle.value(); \ + } while (0) } // namespace Envoy diff --git a/source/common/network/application_protocol.cc b/source/common/network/application_protocol.cc index 6dace49c86a45..ebb40ccd9043b 100644 --- a/source/common/network/application_protocol.cc +++ b/source/common/network/application_protocol.cc @@ -5,10 +5,15 @@ namespace Envoy { namespace Network { -REGISTER_INLINE_KEY(StreamInfo::FilterStateInlineMapScope, application_protocols_inline_key, - "envoy.network.application_protocols"); +constexpr absl::string_view ApplicationProtocolsKey = "envoy.network.application_protocols"; -const StreamInfo::InlineKey ApplicationProtocols::key() { return application_protocols_inline_key; } +REGISTER_INLINE_MAP_KEY(StreamInfo::FilterStateInlineMapScope, ApplicationProtocolsKey); + +using Scope = StreamInfo::FilterStateInlineMapScope; + +const StreamInfo::InlineKey ApplicationProtocols::key() { + INLINE_HANDLE_BY_KEY_ON_FIRST_USE(StreamInfo::FilterStateInlineMapScope, ApplicationProtocolsKey); +} } // namespace Network } // namespace Envoy diff --git a/source/common/network/filter_state_proxy_info.cc b/source/common/network/filter_state_proxy_info.cc index ed746454d0eb0..f3ac8f0b22705 100644 --- a/source/common/network/filter_state_proxy_info.cc +++ b/source/common/network/filter_state_proxy_info.cc @@ -3,11 +3,14 @@ namespace Envoy { namespace Network { -REGISTER_INLINE_KEY(StreamInfo::FilterStateInlineMapScope, http11_proxy_info_inline_key, - "envoy.network.transport_socket.http_11_proxy.info"); +constexpr absl::string_view Http11ProxyInfoFilterStateKey = + "envoy.network.http_connection_manager.http1_proxy"; + +REGISTER_INLINE_MAP_KEY(StreamInfo::FilterStateInlineMapScope, Http11ProxyInfoFilterStateKey); const StreamInfo::InlineKey Http11ProxyInfoFilterState::key() { - return http11_proxy_info_inline_key; + INLINE_HANDLE_BY_KEY_ON_FIRST_USE(StreamInfo::FilterStateInlineMapScope, + Http11ProxyInfoFilterStateKey); } } // namespace Network diff --git a/source/common/network/proxy_protocol_filter_state.cc b/source/common/network/proxy_protocol_filter_state.cc index 2fee89b0f2a7e..12946d24dcb81 100644 --- a/source/common/network/proxy_protocol_filter_state.cc +++ b/source/common/network/proxy_protocol_filter_state.cc @@ -5,11 +5,13 @@ namespace Envoy { namespace Network { -REGISTER_INLINE_KEY(StreamInfo::FilterStateInlineMapScope, proxy_protocol_options_inline_key, - "envoy.network.proxy_protocol_options"); +constexpr absl::string_view ProxyProtocolFilterStateKey = "envoy.network.proxy_protocol_options"; + +REGISTER_INLINE_MAP_KEY(StreamInfo::FilterStateInlineMapScope, ProxyProtocolFilterStateKey); const StreamInfo::InlineKey ProxyProtocolFilterState::key() { - return proxy_protocol_options_inline_key; + INLINE_HANDLE_BY_KEY_ON_FIRST_USE(StreamInfo::FilterStateInlineMapScope, + ProxyProtocolFilterStateKey); } } // namespace Network diff --git a/source/common/network/upstream_server_name.cc b/source/common/network/upstream_server_name.cc index c34e74fe69d59..b216a720526ca 100644 --- a/source/common/network/upstream_server_name.cc +++ b/source/common/network/upstream_server_name.cc @@ -5,10 +5,13 @@ namespace Envoy { namespace Network { -REGISTER_INLINE_KEY(StreamInfo::FilterStateInlineMapScope, upstream_server_name_inline_key, - "envoy.network.upstream_server_name"); +constexpr absl::string_view UpstreamServerNameKey = "envoy.network.upstream_server_name"; -const StreamInfo::InlineKey UpstreamServerName::key() { return upstream_server_name_inline_key; } +REGISTER_INLINE_MAP_KEY(StreamInfo::FilterStateInlineMapScope, UpstreamServerNameKey); + +const StreamInfo::InlineKey UpstreamServerName::key() { + INLINE_HANDLE_BY_KEY_ON_FIRST_USE(StreamInfo::FilterStateInlineMapScope, UpstreamServerNameKey); +} } // namespace Network } // namespace Envoy diff --git a/source/common/network/upstream_subject_alt_names.cc b/source/common/network/upstream_subject_alt_names.cc index 3d1cf73f42398..b08369c2cd41e 100644 --- a/source/common/network/upstream_subject_alt_names.cc +++ b/source/common/network/upstream_subject_alt_names.cc @@ -5,11 +5,13 @@ namespace Envoy { namespace Network { -REGISTER_INLINE_KEY(StreamInfo::FilterStateInlineMapScope, upstream_subject_alt_names_inline_key, - "envoy.network.upstream_subject_alt_names"); +constexpr absl::string_view UpstreamSubjectAltNamesKey = "envoy.network.upstream_subject_alt_names"; + +REGISTER_INLINE_MAP_KEY(StreamInfo::FilterStateInlineMapScope, UpstreamSubjectAltNamesKey); const StreamInfo::InlineKey UpstreamSubjectAltNames::key() { - return upstream_subject_alt_names_inline_key; + INLINE_HANDLE_BY_KEY_ON_FIRST_USE(StreamInfo::FilterStateInlineMapScope, + UpstreamSubjectAltNamesKey); } } // namespace Network diff --git a/source/common/router/debug_config.cc b/source/common/router/debug_config.cc index 6a3015fc548b6..eeb239a9a6903 100644 --- a/source/common/router/debug_config.cc +++ b/source/common/router/debug_config.cc @@ -5,8 +5,9 @@ namespace Envoy { namespace Router { -REGISTER_INLINE_KEY(StreamInfo::FilterStateInlineMapScope, debug_config_inline_key, - "envoy.router.debug_config"); +constexpr absl::string_view DebugConfigKey = "envoy.router.debug_config"; + +REGISTER_INLINE_MAP_KEY(StreamInfo::FilterStateInlineMapScope, DebugConfigKey); DebugConfig::DebugConfig(bool append_cluster, absl::optional cluster_header, bool append_upstream_host, @@ -19,7 +20,9 @@ DebugConfig::DebugConfig(bool append_cluster, absl::optionallength() : 0; } @@ -1759,13 +1765,13 @@ bool Filter::convertRequestHeadersForInternalRedirect(Http::RequestHeaderMap& do // redirects allowed for this route. StreamInfo::UInt32Accessor* num_internal_redirect{}; - if (num_internal_redirect = filter_state->getDataMutable( - NumInternalRedirectsFilterStateName); + if (num_internal_redirect = + filter_state->getDataMutable(internalRedirectCountKey()); num_internal_redirect == nullptr) { auto state = std::make_shared(0); num_internal_redirect = state.get(); - filter_state->setData(NumInternalRedirectsFilterStateName, std::move(state), + filter_state->setData(internalRedirectCountKey(), std::move(state), StreamInfo::FilterState::StateType::Mutable, StreamInfo::FilterState::LifeSpan::Request); } From 44108978013172c998461631758968236368bd3e Mon Sep 17 00:00:00 2001 From: wbpcode Date: Fri, 1 Sep 2023 06:26:17 +0000 Subject: [PATCH 29/36] fix build Signed-off-by: wbpcode --- envoy/common/optref.h | 2 +- source/common/common/BUILD | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/envoy/common/optref.h b/envoy/common/optref.h index 869720b6ae4c4..411007543d481 100644 --- a/envoy/common/optref.h +++ b/envoy/common/optref.h @@ -58,7 +58,7 @@ template struct OptRef { /** * @return true if the object has a value. */ - bool has_value() const { return ptr_ != nullptr; } + bool has_value() const { return ptr_ != nullptr; } // // NOLINT(readability-identifier-naming) /** * @return true if the object has a value. diff --git a/source/common/common/BUILD b/source/common/common/BUILD index 43da9b10fca95..329500fc5b577 100644 --- a/source/common/common/BUILD +++ b/source/common/common/BUILD @@ -592,6 +592,7 @@ envoy_cc_library( "abseil_node_hash_set", ], deps = [ + "//envoy/common:optref_lib", "//source/common/common:assert_lib", "//source/common/common:macros", "//source/common/common:utility_lib", From fce7ccdf64f159599e9fb8d36226e9d417298d67 Mon Sep 17 00:00:00 2001 From: wbpcode Date: Fri, 1 Sep 2023 09:32:54 +0000 Subject: [PATCH 30/36] fix method name after merge main Signed-off-by: wbpcode --- source/common/stream_info/filter_state_impl.h | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/source/common/stream_info/filter_state_impl.h b/source/common/stream_info/filter_state_impl.h index 4c456026ddd0a..d382e9c0d0be5 100644 --- a/source/common/stream_info/filter_state_impl.h +++ b/source/common/stream_info/filter_state_impl.h @@ -95,7 +95,7 @@ class FilterStateImpl : public FilterState { "conflicting life_span on the same data_name."); return; } - const auto current = data_storage_->lookup(data_key); + const auto current = data_storage_->get(data_key); if (current.has_value()) { // We have another object with same data_name. Check for mutability // violations namely: readonly data cannot be overwritten, mutable data @@ -117,17 +117,17 @@ class FilterStateImpl : public FilterState { filter_object.data_ = data; filter_object.state_type_ = state_type; filter_object.stream_sharing_ = stream_sharing; - data_storage_->insert(data_key, std::move(filter_object)); + data_storage_->set(data_key, std::move(filter_object)); } // This only checks the local data_storage_ for data_name existence. template bool hasDataInternal(DataKeyType data_key) const { - return data_storage_->lookup(data_key).has_value(); + return data_storage_->get(data_key).has_value(); } template const FilterState::Object* getDataReadOnlyGenericInternal(DataKeyType data_key) const { - const auto current = data_storage_->lookup(data_key); + const auto current = data_storage_->get(data_key); if (!current.has_value()) { if (parent_) { @@ -141,7 +141,7 @@ class FilterStateImpl : public FilterState { template std::shared_ptr getDataSharedMutableGenericInternal(DataKeyType data_key) { - const auto current = data_storage_->lookup(data_key); + const auto current = data_storage_->get(data_key); if (!current.has_value()) { if (parent_) { From 27f31950a63e5e31713533fa5d7bc30fa210c003 Mon Sep 17 00:00:00 2001 From: wbpcode Date: Mon, 4 Sep 2023 04:36:12 +0000 Subject: [PATCH 31/36] fix build Signed-off-by: wbpcode --- source/server/server.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/source/server/server.cc b/source/server/server.cc index e3e872bc60aa0..bcfb4e324e05a 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -474,11 +474,11 @@ void InstanceImpl::initialize(Network::Address::InstanceConstSharedPtr local_add absl::StrJoin(info.registered_headers_, ",")); } - // Finalize all managed inline map registries. + // Finalize all inline map descriptors that managed by the inline map registry. InlineMapRegistry::finalize(); ENVOY_LOG(info, "Inline map registry info:"); - for (const auto& registry : InlineMapRegistry::descriptors()) { - ENVOY_LOG(info, " {}: {}", registry.first, absl::StrJoin(registry.second->inlineKeys(), ",")); + for (const auto& info : InlineMapRegistry::registryInfo()) { + ENVOY_LOG(info, " {}: {}", info.first, info.second); }; // Initialize the regex engine and inject to singleton. From fff5b1b2d888cb7ab6bebedb245f129b28e7ffa7 Mon Sep 17 00:00:00 2001 From: wbpcode Date: Mon, 4 Sep 2023 05:58:19 +0000 Subject: [PATCH 32/36] fix test Signed-off-by: wbpcode --- source/common/stream_info/filter_state_impl.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/stream_info/filter_state_impl.h b/source/common/stream_info/filter_state_impl.h index d382e9c0d0be5..8195a29bd2c48 100644 --- a/source/common/stream_info/filter_state_impl.h +++ b/source/common/stream_info/filter_state_impl.h @@ -117,7 +117,7 @@ class FilterStateImpl : public FilterState { filter_object.data_ = data; filter_object.state_type_ = state_type; filter_object.stream_sharing_ = stream_sharing; - data_storage_->set(data_key, std::move(filter_object)); + data_storage_[data_key] = std::move(filter_object); } // This only checks the local data_storage_ for data_name existence. From f895088a89132e0106f36d7989fa8dc134fffc8b Mon Sep 17 00:00:00 2001 From: wbpcode Date: Mon, 4 Sep 2023 06:09:46 +0000 Subject: [PATCH 33/36] fix build Signed-off-by: wbpcode --- source/common/stream_info/filter_state_impl.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/stream_info/filter_state_impl.h b/source/common/stream_info/filter_state_impl.h index 8195a29bd2c48..b06e79180cd83 100644 --- a/source/common/stream_info/filter_state_impl.h +++ b/source/common/stream_info/filter_state_impl.h @@ -117,7 +117,7 @@ class FilterStateImpl : public FilterState { filter_object.data_ = data; filter_object.state_type_ = state_type; filter_object.stream_sharing_ = stream_sharing; - data_storage_[data_key] = std::move(filter_object); + (*data_storage_)[data_key] = std::move(filter_object); } // This only checks the local data_storage_ for data_name existence. From 1a476d2a635e58260c954d58fbf8e888adf9a3bc Mon Sep 17 00:00:00 2001 From: wbpcode Date: Tue, 5 Sep 2023 05:31:48 +0000 Subject: [PATCH 34/36] address most comments Signed-off-by: wbpcode --- envoy/common/optref.h | 2 +- source/common/common/inline_map.h | 16 ++--- source/common/common/inline_map_registry.cc | 28 ++++----- source/common/common/inline_map_registry.h | 65 +++++++++------------ source/server/server.cc | 6 +- 5 files changed, 54 insertions(+), 63 deletions(-) diff --git a/envoy/common/optref.h b/envoy/common/optref.h index 411007543d481..869720b6ae4c4 100644 --- a/envoy/common/optref.h +++ b/envoy/common/optref.h @@ -58,7 +58,7 @@ template struct OptRef { /** * @return true if the object has a value. */ - bool has_value() const { return ptr_ != nullptr; } // // NOLINT(readability-identifier-naming) + bool has_value() const { return ptr_ != nullptr; } /** * @return true if the object has a value. diff --git a/source/common/common/inline_map.h b/source/common/common/inline_map.h index 4992a5971bf6d..359cc3c24352e 100644 --- a/source/common/common/inline_map.h +++ b/source/common/common/inline_map.h @@ -1,5 +1,6 @@ #pragma once +#include #include #include #include @@ -125,13 +126,9 @@ template class InlineMapDescriptor { /** * Finalize this descriptor. No further changes are allowed after this point. This guarantees that - * all map created by the process have the same variable size and custom inline key adding. This - * method should only be called once. + * all map created by the process have the same variable size and custom inline key adding. */ - void finalize() { - ASSERT(!finalized_, "Cannot finalize() an already finalized descriptor"); - finalized_ = true; - } + void finalize() { finalized_ = true; } /** * @return true if the descriptor is finalized. @@ -178,7 +175,12 @@ template class InlineMapDescriptor { return inline_keys_map_.size(); } - bool finalized_{}; + // The finalize()/finalized() methods of cross-modules descriptor may be called at same time + // from different threads. So we need to use atomic to protect the finalized_ flag. + // This is only happens in the multiple threads tests because all cross-modules descriptors + // should be finalized when the first server instance is initialized. + std::atomic finalized_{}; + InlineKeys inline_keys_; InlineKeysMap inline_keys_map_; }; diff --git a/source/common/common/inline_map_registry.cc b/source/common/common/inline_map_registry.cc index 7b33a046cd5e1..7bc62a4e0a03e 100644 --- a/source/common/common/inline_map_registry.cc +++ b/source/common/common/inline_map_registry.cc @@ -2,15 +2,14 @@ namespace Envoy { -InlineMapRegistry::InlineMapDescriptor* -InlineMapRegistry::createDescriptor(absl::string_view scope_name) { +InlineMapRegistry::Descriptor* InlineMapRegistry::createDescriptor(absl::string_view scope_name) { // This should never be triggered because the developer should ensure all used/managed // descriptors are registered before main() function by static variable initialization. - RELEASE_ASSERT(!finalized(), "Registry is finalized, cannot create new descriptor"); + RELEASE_ASSERT(!finalized_, "Registry is finalized, cannot create new descriptor"); // Create the descriptor and insert it into the registry. - auto descriptor = new InlineMapDescriptor(); - auto [_, inserted] = registry().emplace(scope_name, descriptor); + auto descriptor = new Descriptor(); + auto [_, inserted] = descriptors_.emplace(scope_name, descriptor); // Check whether the descriptor is created repeatedly. RELEASE_ASSERT(inserted, fmt::format("Create descriptor {} repeatedly", scope_name)); @@ -18,25 +17,22 @@ InlineMapRegistry::createDescriptor(absl::string_view scope_name) { return descriptor; } -InlineMapRegistry::RegistryInfo InlineMapRegistry::registryInfo() { - std::vector> registry_info; - for (const auto& [name, descriptor] : registry()) { - registry_info.emplace_back(name, descriptor->inlineKeysAsString()); - } - return registry_info; +const InlineMapRegistry::Descriptors& InlineMapRegistry::descriptors() const { + RELEASE_ASSERT(finalized_, "Registry is not finalized, cannot get registry"); + return descriptors_; } void InlineMapRegistry::finalize(const ScopeInlineKeysVector& scope_inline_keys_vector) { // Call once to finalize the registry and all managed descriptors. This is used to ensure // the registry is finalized only once on multi-thread environment. - std::call_once(onceFlag(), [&scope_inline_keys_vector]() { - finalized() = true; + std::call_once(once_flag_, [this, &scope_inline_keys_vector]() { + finalized_ = true; // Add inline keys to the descriptor based on the scope name. This is last chance to add // inline keys to the descriptor. for (const auto& inline_keys : scope_inline_keys_vector) { - auto iter = registry().find(inline_keys.name); - if (iter == registry().end()) { + auto iter = descriptors_.find(inline_keys.name); + if (iter == descriptors_.end()) { // Skip the scope that is not registered. ENVOY_LOG_MISC(warn, "Skip the scope {} that is not registered", inline_keys.name); continue; @@ -48,7 +44,7 @@ void InlineMapRegistry::finalize(const ScopeInlineKeysVector& scope_inline_keys_ } // Finalize all managed descriptors. - std::for_each(registry().begin(), registry().end(), [](auto& p) { p.second->finalize(); }); + std::for_each(descriptors_.begin(), descriptors_.end(), [](auto& p) { p.second->finalize(); }); }); } diff --git a/source/common/common/inline_map_registry.h b/source/common/common/inline_map_registry.h index c6e7969ef2d58..60569d470a901 100644 --- a/source/common/common/inline_map_registry.h +++ b/source/common/common/inline_map_registry.h @@ -34,8 +34,13 @@ class InlineMapRegistry { InlineMapKeyRegister(absl::string_view); }; - using InlineMapDescriptor = InlineMapDescriptor; - using Handle = InlineMapDescriptor::Handle; + using Descriptor = InlineMapDescriptor; + using Handle = Descriptor::Handle; + + /** + * All registered descriptors. This is a map from scope name to the descriptor pointer. + */ + using Descriptors = absl::flat_hash_map; struct ScopeInlineKeys { std::string name; @@ -48,60 +53,42 @@ class InlineMapRegistry { */ using ScopeInlineKeysVector = std::vector; - /** - * Registry info type. This is used to get scope name and all managed descriptors' inline keys as - * string. - */ - using RegistryInfo = std::vector>; - private: - using Registry = absl::flat_hash_map; - template friend class InlineMapDescriptorRegister; template friend class InlineMapKeyRegister; /** - * Call once flag to ensure the registry is finalized only once on multi-thread environment. - */ - static std::once_flag& onceFlag() { MUTABLE_CONSTRUCT_ON_FIRST_USE(std::once_flag); } - - /** - * Finalized flag to check whether the registry is finalized. + * Default constructor. This is private because the registry should be a singleton that is only + * created once and get by the InlineMapRegistry::get() function. */ - static bool& finalized() { MUTABLE_CONSTRUCT_ON_FIRST_USE(bool, false); } - - /** - * A set of inline map registry that indexed by the scope name. - */ - static Registry& registry() { MUTABLE_CONSTRUCT_ON_FIRST_USE(Registry); } + InlineMapRegistry() = default; /** * Create the inline map descriptor based on the scope name and insert it into the registry. */ - static InlineMapDescriptor* createDescriptor(absl::string_view scope_name); + Descriptor* createDescriptor(absl::string_view scope_name); /** - * Get or create the inline map registry singleton based on the scope type. + * Get or create the inline map descriptor singleton based on the scope type and insert + * it into the registry. */ - template static InlineMapDescriptor& getOrCreateDescriptor() { + template static Descriptor& getOrCreateDescriptor() { // The function level static variable will be initialized only once and is thread safe. // This is same with MUTABLE_CONSTRUCT_ON_FIRST_USE but has complex initialization. - static InlineMapDescriptor* initialize_once_descriptor = createDescriptor(Scope::name()); + static Descriptor* initialize_once_descriptor = get().createDescriptor(Scope::name()); return *initialize_once_descriptor; } +public: /** - * Add inline key to the descriptor based on the scope. + * Get the inline map registry singleton. */ - template static void addInlineKey(absl::string_view key) { - getOrCreateDescriptor().addInlineKey(key); - } + static InlineMapRegistry& get() { MUTABLE_CONSTRUCT_ON_FIRST_USE(InlineMapRegistry); } -public: /** * Get the inline handle by the key name. */ - template static absl::optional getHandleByKey(absl::string_view key) { + template absl::optional static getHandleByKey(absl::string_view key) { return getOrCreateDescriptor().getHandleByKey(key); } @@ -113,15 +100,21 @@ class InlineMapRegistry { } /** - * Get scope name and all managed descriptors' inline keys as string. + * Get the registered descriptors. This is used to get all registered descriptors that are + * managed by this registry. */ - static RegistryInfo registryInfo(); + const Descriptors& descriptors() const; /** * Finalize the registry and all managed descriptors by this registry. This only be called once * when the server is initialized. This is safe even there are multiple main threads or servers. */ - static void finalize(const ScopeInlineKeysVector& scope_inline_keys_vector = {}); + void finalize(const ScopeInlineKeysVector& scope_inline_keys_vector = {}); + +private: + Descriptors descriptors_; + std::once_flag once_flag_; + bool finalized_{}; }; template @@ -131,7 +124,7 @@ InlineMapRegistry::InlineMapDescriptorRegister::InlineMapDescriptorRegist template InlineMapRegistry::InlineMapKeyRegister::InlineMapKeyRegister(absl::string_view key) { - InlineMapRegistry::addInlineKey(key); + InlineMapRegistry::getOrCreateDescriptor().addInlineKey(key); } /** diff --git a/source/server/server.cc b/source/server/server.cc index bcfb4e324e05a..072a23396492f 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -475,10 +475,10 @@ void InstanceImpl::initialize(Network::Address::InstanceConstSharedPtr local_add } // Finalize all inline map descriptors that managed by the inline map registry. - InlineMapRegistry::finalize(); + InlineMapRegistry::get().finalize(); ENVOY_LOG(info, "Inline map registry info:"); - for (const auto& info : InlineMapRegistry::registryInfo()) { - ENVOY_LOG(info, " {}: {}", info.first, info.second); + for (const auto& info : InlineMapRegistry::get().descriptors()) { + ENVOY_LOG(info, " {}: {}", info.first, info.second->inlineKeysAsString()); }; // Initialize the regex engine and inject to singleton. From 9c95b23b1313dfd699372154d8764341fbe94353 Mon Sep 17 00:00:00 2001 From: wbpcode Date: Tue, 5 Sep 2023 05:39:29 +0000 Subject: [PATCH 35/36] update comments Signed-off-by: wbpcode --- source/common/common/inline_map.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/common/inline_map.h b/source/common/common/inline_map.h index 359cc3c24352e..3c6efe87be7d1 100644 --- a/source/common/common/inline_map.h +++ b/source/common/common/inline_map.h @@ -178,7 +178,7 @@ template class InlineMapDescriptor { // The finalize()/finalized() methods of cross-modules descriptor may be called at same time // from different threads. So we need to use atomic to protect the finalized_ flag. // This is only happens in the multiple threads tests because all cross-modules descriptors - // should be finalized when the first server instance is initialized. + // should be finalized when the first server instance is initialized in formal running. std::atomic finalized_{}; InlineKeys inline_keys_; From e8b04b1f60ea80b95a34a0a323a460632e57dcf8 Mon Sep 17 00:00:00 2001 From: wbpcode Date: Wed, 6 Sep 2023 12:00:29 +0000 Subject: [PATCH 36/36] a dirty commit that shouldn't merge Signed-off-by: wbpcode --- source/common/common/inline_map.h | 63 ++++++++++--------- source/common/common/inline_map_registry.h | 9 ++- .../common/stream_info/filter_state_impl.cc | 4 +- source/common/stream_info/filter_state_impl.h | 21 +++---- 4 files changed, 49 insertions(+), 48 deletions(-) diff --git a/source/common/common/inline_map.h b/source/common/common/inline_map.h index 3c6efe87be7d1..f11a8c572e599 100644 --- a/source/common/common/inline_map.h +++ b/source/common/common/inline_map.h @@ -3,6 +3,7 @@ #include #include #include +#include #include #include @@ -416,41 +417,42 @@ template class InlineMap : public InlineStorage { return inline_entries_[handle.inlineId()]; } - /** - * Create an inline map with the given descriptor. If the descriptor is not finalized, it will be - * finalized before creating the inline map. - * @param descriptor the descriptor that contains the inline keys. - * @return the created inline map. - */ - static std::unique_ptr create(TypedInlineMapDescriptor& descriptor) { - if (!descriptor.finalized()) { - // Call finalize() to make sure that the descriptor is finalized and no any new inline - // keys could be registered. - descriptor.finalize(); - } - - return std::unique_ptr(new ((descriptor.inlineKeysNum() * sizeof(Value))) - InlineMap(descriptor)); + InlineMap(const TypedInlineMapDescriptor& descriptor) : descriptor_(descriptor) { + ASSERT(descriptor_.finalized(), "Cannot create inline map before finalize()"); } private: + using StoragePtr = std::unique_ptr; friend class InlineMapDescriptor; - InlineMap(TypedInlineMapDescriptor& descriptor) : descriptor_(descriptor) { - // Initialize the inline entries to nullptr. - uint64_t inline_keys_num = descriptor_.inlineKeysNum(); - inline_entries_valid_.resize(inline_keys_num, false); - memset(inline_entries_storage_, 0, inline_keys_num * sizeof(Value)); - inline_entries_ = reinterpret_cast(inline_entries_storage_); + void lazyAllocateMemoryInlineEntries() { + if (inline_entries_storage_ != nullptr) { + return; + } + + const uint64_t inline_keys_num = descriptor_.inlineKeysNum(); + const uint64_t value_memory_size = inline_keys_num * sizeof(Value); + const uint64_t flag_memory_size = inline_keys_num * sizeof(bool); + const uint64_t memory_size = value_memory_size + flag_memory_size; + + inline_entries_storage_.reset(new uint8_t[memory_size]); + memset(inline_entries_storage_.get(), 0, memory_size); + + inline_entries_ = reinterpret_cast(inline_entries_storage_.get()); + inline_entries_valid_ = + reinterpret_cast(inline_entries_storage_.get() + value_memory_size); } template void resetInlineMapEntry(uint64_t inline_entry_id, SetValue&& value) { ASSERT(inline_entry_id < descriptor_.inlineKeysNum()); + // Only allocate the memory for inline entries when the first inline entry is added. + lazyAllocateMemoryInlineEntries(); clearInlineMapEntry(inline_entry_id); // Construct the new entry in the inline array. new (inline_entries_ + inline_entry_id) Value(std::forward(value)); + setInlineEntryValid(inline_entry_id, true); } @@ -486,10 +488,16 @@ template class InlineMap : public InlineStorage { bool inlineEntryValid(uint64_t inline_entry_id) const { ASSERT(inline_entry_id < descriptor_.inlineKeysNum()); + + if (inline_entries_storage_ == nullptr) { + // If the inline entries storage is not allocated, then the inline entry is not valid. + return false; + } return inline_entries_valid_[inline_entry_id]; } void setInlineEntryValid(uint64_t inline_entry_id, bool flag) { ASSERT(inline_entry_id < descriptor_.inlineKeysNum()); + ASSERT(inline_entries_storage_ != nullptr); if (flag) { inline_entries_size_++; @@ -508,20 +516,13 @@ template class InlineMap : public InlineStorage { // This is the underlay hash map for the dynamic map entries. DynamicHashMap dynamic_entries_; - // These are flags to indicate if the inline entries are valid. - // TODO(wbpcode): this will add additional one time memory allocation when constructing inline - // map. It is possible to use memory in the inline_entries_storage_ to store the flags in the - // future. - std::vector inline_entries_valid_; - uint64_t inline_entries_size_{}; Value* inline_entries_{}; + // These are flags to indicate if the inline entries are valid. + bool* inline_entries_valid_{}; - // This should be the last member of the class and no member should be added after this. - uint8_t inline_entries_storage_[]; + StoragePtr inline_entries_storage_; }; -template using InlineMapPtr = std::unique_ptr>; - } // namespace Envoy diff --git a/source/common/common/inline_map_registry.h b/source/common/common/inline_map_registry.h index 60569d470a901..9b3fbd4684806 100644 --- a/source/common/common/inline_map_registry.h +++ b/source/common/common/inline_map_registry.h @@ -93,10 +93,13 @@ class InlineMapRegistry { } /** - * Create inline map based on the scope and value type. + * Get the finalized inline map descriptor singleton based on the scope type. This is used to get + * the finalized descriptor to construct the inline map. */ - template static auto createInlineMap() { - return InlineMap::create(getOrCreateDescriptor()); + template static const Descriptor& finalizedDescriptor() { + auto& descriptor = getOrCreateDescriptor(); + descriptor.finalize(); + return descriptor; } /** diff --git a/source/common/stream_info/filter_state_impl.cc b/source/common/stream_info/filter_state_impl.cc index 8da17680b9b59..12a5d8fbcdced 100644 --- a/source/common/stream_info/filter_state_impl.cc +++ b/source/common/stream_info/filter_state_impl.cc @@ -55,14 +55,14 @@ bool FilterStateImpl::hasDataAtOrAboveLifeSpan(FilterState::LifeSpan life_span) if (life_span > life_span_) { return parent_ && parent_->hasDataAtOrAboveLifeSpan(life_span); } - return !data_storage_->empty() || (parent_ && parent_->hasDataAtOrAboveLifeSpan(life_span)); + return !data_storage_.empty() || (parent_ && parent_->hasDataAtOrAboveLifeSpan(life_span)); } FilterState::ObjectsPtr FilterStateImpl::objectsSharedWithUpstreamConnection() const { auto objects = parent_ ? parent_->objectsSharedWithUpstreamConnection() : std::make_unique(); - data_storage_->iterate([&objects](const std::string& name, const FilterObject& object) -> bool { + data_storage_.iterate([&objects](const std::string& name, const FilterObject& object) -> bool { switch (object.stream_sharing_) { case StreamSharingMayImpactPooling::SharedWithUpstreamConnection: objects->push_back( diff --git a/source/common/stream_info/filter_state_impl.h b/source/common/stream_info/filter_state_impl.h index b06e79180cd83..cc148a309ed5e 100644 --- a/source/common/stream_info/filter_state_impl.h +++ b/source/common/stream_info/filter_state_impl.h @@ -16,8 +16,7 @@ class FilterStateImpl : public FilterState { public: FilterStateImpl(FilterState::LifeSpan life_span) : life_span_(life_span), - data_storage_( - InlineMapRegistry::createInlineMap()) { + data_storage_(InlineMapRegistry::finalizedDescriptor()) { maybeCreateParent(ParentAccessMode::ReadOnly); } @@ -27,8 +26,7 @@ class FilterStateImpl : public FilterState { */ FilterStateImpl(FilterStateSharedPtr ancestor, FilterState::LifeSpan life_span) : ancestor_(ancestor), life_span_(life_span), - data_storage_( - InlineMapRegistry::createInlineMap()) { + data_storage_(InlineMapRegistry::finalizedDescriptor()) { maybeCreateParent(ParentAccessMode::ReadOnly); } @@ -41,8 +39,7 @@ class FilterStateImpl : public FilterState { */ FilterStateImpl(LazyCreateAncestor lazy_create_ancestor, FilterState::LifeSpan life_span) : ancestor_(lazy_create_ancestor), life_span_(life_span), - data_storage_( - InlineMapRegistry::createInlineMap()) { + data_storage_(InlineMapRegistry::finalizedDescriptor()) { maybeCreateParent(ParentAccessMode::ReadOnly); } @@ -95,7 +92,7 @@ class FilterStateImpl : public FilterState { "conflicting life_span on the same data_name."); return; } - const auto current = data_storage_->get(data_key); + const auto current = data_storage_.get(data_key); if (current.has_value()) { // We have another object with same data_name. Check for mutability // violations namely: readonly data cannot be overwritten, mutable data @@ -117,17 +114,17 @@ class FilterStateImpl : public FilterState { filter_object.data_ = data; filter_object.state_type_ = state_type; filter_object.stream_sharing_ = stream_sharing; - (*data_storage_)[data_key] = std::move(filter_object); + data_storage_[data_key] = std::move(filter_object); } // This only checks the local data_storage_ for data_name existence. template bool hasDataInternal(DataKeyType data_key) const { - return data_storage_->get(data_key).has_value(); + return data_storage_.get(data_key).has_value(); } template const FilterState::Object* getDataReadOnlyGenericInternal(DataKeyType data_key) const { - const auto current = data_storage_->get(data_key); + const auto current = data_storage_.get(data_key); if (!current.has_value()) { if (parent_) { @@ -141,7 +138,7 @@ class FilterStateImpl : public FilterState { template std::shared_ptr getDataSharedMutableGenericInternal(DataKeyType data_key) { - const auto current = data_storage_->get(data_key); + const auto current = data_storage_.get(data_key); if (!current.has_value()) { if (parent_) { @@ -166,7 +163,7 @@ class FilterStateImpl : public FilterState { FilterStateSharedPtr parent_; const FilterState::LifeSpan life_span_; - InlineMapPtr data_storage_; + InlineMap data_storage_; }; } // namespace StreamInfo