diff --git a/src/core/lib/transport/metadata_batch.h b/src/core/lib/transport/metadata_batch.h index 2216fc7ccc7..4686ef88866 100644 --- a/src/core/lib/transport/metadata_batch.h +++ b/src/core/lib/transport/metadata_batch.h @@ -739,6 +739,30 @@ struct Value> { StorageType value; }; +// Encoder to copy some metadata +template +class CopySink { + public: + explicit CopySink(Output* dst) : dst_(dst) {} + + template + void Encode(T trait, V value) { + dst_->Set(trait, value); + } + + template + void Encode(T trait, const Slice& value) { + dst_->Set(trait, std::move(value.AsOwned())); + } + + void Encode(const Slice& key, const Slice& value) { + dst_->AppendUnknown(key.as_string_view(), value.Ref()); + } + + private: + Output* dst_; +}; + } // namespace metadata_detail // Helper function for encoders @@ -757,18 +781,28 @@ MetadataValueAsSlice(typename Which::ValueType value) { } // MetadataMap encodes the mapping of metadata keys to metadata values. -// Right now the API presented is the minimal one that will allow us to -// substitute this type for grpc_metadata_batch in a relatively easy fashion. At -// that point we'll start iterating this API into something that's ergonomic -// again, whilst minimally holding the performance bar already set (and -// hopefully improving some things). -// In the meantime, we're not going to invest much time in ephemeral API -// documentation, so if you must use one of these APIs and it's not obvious -// how, reach out to ctiller. // -// MetadataMap takes a list of traits. Each of these trait objects defines -// one metadata field that is used by core, and so should have more specialized -// handling than just using the generic APIs. +// MetadataMap takes a derived class and list of traits. Each of these trait +// objects defines one metadata field that is used by core, and so should have +// more specialized handling than just using the generic APIs. +// +// MetadataMap is the backing type for some derived type via the curiously +// recursive template pattern. This is because many types consumed by +// MetadataMap require the container type to operate on, and many of those +// types are instantiated one per trait. A naive implementation without the +// Derived type would, for traits A,B,C, then instantiate for some +// T: +// - T, A>, +// - T, B>, +// - T, C>. +// Since these types ultimately need to be recorded in the .dynstr segment +// for dynamic linkers (if gRPC is linked as a static library) this would +// create O(N^2) bytes of symbols even in stripped libraries. To avoid this +// we use the derived type (e.g. grpc_metadata_batch right now) to capture +// the container type, and we would write T, etc... +// Note that now the container type uses a number of bytes that is independent +// of the number of traits, and so we return to a linear symbol table growth +// function. // // Each trait object has the following signature: // // Traits for the grpc-xyz metadata field: @@ -815,7 +849,7 @@ MetadataValueAsSlice(typename Which::ValueType value) { // for grpc-timeout we make the memento the timeout expressed on the wire, but // we make the value the timestamp of when the timeout will expire (i.e. the // deadline). -template +template class MetadataMap { public: explicit MetadataMap(Arena* arena); @@ -824,7 +858,10 @@ class MetadataMap { MetadataMap(const MetadataMap&) = delete; MetadataMap& operator=(const MetadataMap&) = delete; MetadataMap(MetadataMap&&) noexcept; - MetadataMap& operator=(MetadataMap&&) noexcept; + // We never create MetadataMap directly, instead we create Derived, but we + // want to be able to move it without redeclaring this. + // NOLINTNEXTLINE(misc-unconventional-assign-operator) + Derived& operator=(MetadataMap&&) noexcept; // Encode this metadata map into some encoder. // For each field that is set in the MetadataMap, call @@ -910,7 +947,7 @@ class MetadataMap { // Remove some metadata by name void Remove(absl::string_view key) { - metadata_detail::RemoveHelper helper(this); + metadata_detail::RemoveHelper helper(static_cast(this)); metadata_detail::NameLookup::Lookup(key, &helper); } @@ -919,7 +956,8 @@ class MetadataMap { // Retrieve some metadata by name absl::optional GetStringValue(absl::string_view name, std::string* buffer) const { - metadata_detail::GetStringValueHelper helper(this, buffer); + metadata_detail::GetStringValueHelper helper( + static_cast(this), buffer); return metadata_detail::NameLookup::Lookup(name, &helper); } @@ -958,36 +996,39 @@ class MetadataMap { // that result. // TODO(ctiller): key should probably be an absl::string_view. // Once we don't care about interning anymore, make that change! - static ParsedMetadata Parse(absl::string_view key, Slice value, - uint32_t transport_size, - MetadataParseErrorFn on_error) { - metadata_detail::ParseHelper helper(value.TakeOwned(), - on_error, transport_size); + static ParsedMetadata Parse(absl::string_view key, Slice value, + uint32_t transport_size, + MetadataParseErrorFn on_error) { + metadata_detail::ParseHelper helper(value.TakeOwned(), on_error, + transport_size); return metadata_detail::NameLookup::Lookup(key, &helper); } // Set a value from a parsed metadata object. - void Set(const ParsedMetadata& m) { m.SetOnContainer(this); } + void Set(const ParsedMetadata& m) { + m.SetOnContainer(static_cast(this)); + } // Append a key/value pair - takes ownership of value void Append(absl::string_view key, Slice value, MetadataParseErrorFn on_error) { - metadata_detail::AppendHelper helper(this, value.TakeOwned(), - on_error); + metadata_detail::AppendHelper helper(static_cast(this), + value.TakeOwned(), on_error); metadata_detail::NameLookup::Lookup(key, &helper); } void Clear(); size_t TransportSize() const; - MetadataMap Copy() const; + Derived Copy() const; bool empty() const { return table_.empty() && unknown_.empty(); } size_t count() const { return table_.count() + unknown_.size(); } private: - friend class metadata_detail::AppendHelper; - friend class metadata_detail::GetStringValueHelper; - friend class metadata_detail::RemoveHelper; - friend class ParsedMetadata; + friend class metadata_detail::AppendHelper; + friend class metadata_detail::GetStringValueHelper; + friend class metadata_detail::RemoveHelper; + friend class metadata_detail::CopySink; + friend class ParsedMetadata; template using Value = metadata_detail::Value; @@ -1032,29 +1073,6 @@ class MetadataMap { uint32_t size_ = 0; }; - // Encoder to copy some metadata - class CopySink { - public: - explicit CopySink(MetadataMap* dst) : dst_(dst) {} - - template - void Encode(T trait, V value) { - dst_->Set(trait, value); - } - - template - void Encode(T trait, const Slice& value) { - dst_->Set(trait, std::move(value.AsOwned())); - } - - void Encode(const Slice& key, const Slice& value) { - dst_->AppendUnknown(key.as_string_view(), value.Ref()); - } - - private: - MetadataMap* dst_; - }; - // Encoder to log some metadata class LogEncoder { public: @@ -1109,53 +1127,56 @@ class MetadataMap { // Ok/not-ok check for metadata maps that contain GrpcStatusMetadata, so that // they can be used as result types for TrySeq. -template -inline bool IsStatusOk(const MetadataMap& m) { +template +inline bool IsStatusOk(const MetadataMap& m) { return m.get(GrpcStatusMetadata()).value_or(GRPC_STATUS_UNKNOWN) == GRPC_STATUS_OK; } -template -MetadataMap::MetadataMap(Arena* arena) : unknown_(arena) {} +template +MetadataMap::MetadataMap(Arena* arena) : unknown_(arena) {} -template -MetadataMap::MetadataMap(MetadataMap&& other) noexcept +template +MetadataMap::MetadataMap(MetadataMap&& other) noexcept : table_(std::move(other.table_)), unknown_(std::move(other.unknown_)) {} -template -MetadataMap& MetadataMap::operator=( +// We never create MetadataMap directly, instead we create Derived, but we want +// to be able to move it without redeclaring this. +// NOLINTNEXTLINE(misc-unconventional-assign-operator) +template +Derived& MetadataMap::operator=( MetadataMap&& other) noexcept { table_ = std::move(other.table_); unknown_ = std::move(other.unknown_); - return *this; + return static_cast(*this); } -template -MetadataMap::~MetadataMap() = default; +template +MetadataMap::~MetadataMap() = default; -template -void MetadataMap::Clear() { +template +void MetadataMap::Clear() { table_.ClearAll(); unknown_.Clear(); } -template -size_t MetadataMap::TransportSize() const { +template +size_t MetadataMap::TransportSize() const { TransportSizeEncoder enc; Encode(&enc); return enc.size(); } -template -MetadataMap MetadataMap::Copy() const { - MetadataMap out(unknown_.arena()); - CopySink sink(&out); +template +Derived MetadataMap::Copy() const { + Derived out(unknown_.arena()); + metadata_detail::CopySink sink(&out); Encode(&sink); return out; } -template -void MetadataMap::Log( +template +void MetadataMap::Log( absl::FunctionRef log_fn) const { LogEncoder enc(log_fn); @@ -1164,7 +1185,10 @@ void MetadataMap::Log( } // namespace grpc_core -using grpc_metadata_batch = grpc_core::MetadataMap< +struct grpc_metadata_batch; + +using grpc_metadata_batch_base = grpc_core::MetadataMap< + grpc_metadata_batch, // Colon prefixed headers first grpc_core::HttpPathMetadata, grpc_core::HttpAuthorityMetadata, grpc_core::HttpMethodMetadata, grpc_core::HttpStatusMetadata, @@ -1181,4 +1205,8 @@ using grpc_metadata_batch = grpc_core::MetadataMap< grpc_core::GrpcTagsBinMetadata, grpc_core::GrpcLbClientStatsMetadata, grpc_core::LbCostBinMetadata, grpc_core::LbTokenMetadata>; +struct grpc_metadata_batch : public grpc_metadata_batch_base { + using grpc_metadata_batch_base::grpc_metadata_batch_base; +}; + #endif /* GRPC_CORE_LIB_TRANSPORT_METADATA_BATCH_H */ diff --git a/test/core/promise/try_seq_metadata_test.cc b/test/core/promise/try_seq_metadata_test.cc index beb0ff549a1..da344fd7b22 100644 --- a/test/core/promise/try_seq_metadata_test.cc +++ b/test/core/promise/try_seq_metadata_test.cc @@ -23,7 +23,9 @@ namespace grpc_core { static auto* g_memory_allocator = new MemoryAllocator( ResourceQuota::Default()->memory_quota()->CreateMemoryAllocator("test")); -using TestMap = MetadataMap; +struct TestMap : public MetadataMap { + using MetadataMap::MetadataMap; +}; TEST(PromiseTest, SucceedAndThenFail) { auto arena = MakeScopedArena(1024, g_memory_allocator); diff --git a/test/core/transport/metadata_map_test.cc b/test/core/transport/metadata_map_test.cc index 7ef3e5002e8..9b112e39c72 100644 --- a/test/core/transport/metadata_map_test.cc +++ b/test/core/transport/metadata_map_test.cc @@ -27,19 +27,28 @@ namespace testing { static auto* g_memory_allocator = new MemoryAllocator( ResourceQuota::Default()->memory_quota()->CreateMemoryAllocator("test")); +struct EmptyMetadataMap : public MetadataMap { + using MetadataMap::MetadataMap; +}; + +struct TimeoutOnlyMetadataMap + : public MetadataMap { + using MetadataMap::MetadataMap; +}; + TEST(MetadataMapTest, Noop) { auto arena = MakeScopedArena(1024, g_memory_allocator); - MetadataMap<>(arena.get()); + EmptyMetadataMap(arena.get()); } TEST(MetadataMapTest, NoopWithDeadline) { auto arena = MakeScopedArena(1024, g_memory_allocator); - MetadataMap(arena.get()); + TimeoutOnlyMetadataMap(arena.get()); } TEST(MetadataMapTest, SimpleOps) { auto arena = MakeScopedArena(1024, g_memory_allocator); - MetadataMap map(arena.get()); + TimeoutOnlyMetadataMap map(arena.get()); EXPECT_EQ(map.get_pointer(GrpcTimeoutMetadata()), nullptr); EXPECT_EQ(map.get(GrpcTimeoutMetadata()), absl::nullopt); map.Set(GrpcTimeoutMetadata(), 1234); @@ -74,7 +83,7 @@ class FakeEncoder { TEST(MetadataMapTest, EmptyEncodeTest) { FakeEncoder encoder; auto arena = MakeScopedArena(1024, g_memory_allocator); - MetadataMap map(arena.get()); + TimeoutOnlyMetadataMap map(arena.get()); map.Encode(&encoder); EXPECT_EQ(encoder.output(), ""); } @@ -82,7 +91,7 @@ TEST(MetadataMapTest, EmptyEncodeTest) { TEST(MetadataMapTest, TimeoutEncodeTest) { FakeEncoder encoder; auto arena = MakeScopedArena(1024, g_memory_allocator); - MetadataMap map(arena.get()); + TimeoutOnlyMetadataMap map(arena.get()); map.Set(GrpcTimeoutMetadata(), 1234); map.Encode(&encoder); EXPECT_EQ(encoder.output(), "grpc-timeout: deadline=1234\n");