From 0b2900a78bdc0112abe25ac5e74b5e331adf346d Mon Sep 17 00:00:00 2001 From: Matt Klein Date: Thu, 4 Jun 2020 21:14:59 +0000 Subject: [PATCH 1/3] http: O(1) custom header map Signed-off-by: Matt Klein --- include/envoy/http/header_map.h | 164 ++++++-- source/common/grpc/async_client_impl.cc | 4 +- .../common/grpc/google_async_client_impl.cc | 16 +- source/common/http/conn_manager_impl.cc | 4 +- source/common/http/header_map_impl.cc | 97 +++-- source/common/http/header_map_impl.h | 396 ++++++++++++------ source/common/http/http1/codec_impl.h | 9 +- source/common/http/http2/codec_impl.cc | 2 +- source/common/http/http2/codec_impl.h | 20 +- source/common/http/message_impl.h | 2 +- source/common/local_reply/local_reply.cc | 20 +- source/common/router/header_formatter.cc | 11 +- source/common/router/rds_impl.cc | 6 +- source/exe/main_common.cc | 6 +- .../access_loggers/common/access_log_base.cc | 9 +- .../extensions/filters/common/expr/context.cc | 4 +- .../common/ratelimit/ratelimit_impl.cc | 4 +- .../filters/common/rbac/engine_impl.cc | 4 +- .../filters/http/cors/cors_filter.cc | 39 +- .../json_transcoder_filter.cc | 5 +- .../filters/http/jwt_authn/filter.cc | 6 +- .../extensions/filters/http/lua/lua_filter.cc | 4 +- .../filters/http/ratelimit/ratelimit.cc | 2 +- .../network/dubbo_proxy/serializer_impl.h | 6 +- .../filters/network/rocketmq_proxy/metadata.h | 6 +- .../filters/network/thrift_proxy/metadata.h | 6 +- .../quic_listeners/quiche/envoy_quic_utils.h | 4 +- .../extensions/stat_sinks/hystrix/hystrix.cc | 13 +- source/server/admin/admin.cc | 6 +- source/server/admin/admin_filter.cc | 2 +- source/server/server.cc | 2 + test/common/http/header_map_impl_fuzz_test.cc | 4 +- .../common/http/header_map_impl_speed_test.cc | 94 +++-- test/common/http/header_map_impl_test.cc | 110 ++--- test/common/http/utility_fuzz_test.cc | 18 +- .../http/on_demand/on_demand_filter_test.cc | 2 +- test/integration/protocol_integration_test.cc | 6 +- test/test_common/utility.h | 148 ++++--- 38 files changed, 798 insertions(+), 463 deletions(-) diff --git a/include/envoy/http/header_map.h b/include/envoy/http/header_map.h index a518395a3320a..40969a78255b8 100644 --- a/include/envoy/http/header_map.h +++ b/include/envoy/http/header_map.h @@ -258,14 +258,13 @@ class HeaderEntry { }; /** - * The following defines all request headers that Envoy allows direct access to inside of the - * header map. In practice, these are all headers used during normal Envoy request flow + * The following defines all default request headers that Envoy allows direct access to inside of + * the header map. In practice, these are all headers used during normal Envoy request flow * processing. This allows O(1) access to these headers without even a hash lookup. */ #define INLINE_REQ_HEADERS(HEADER_FUNC) \ HEADER_FUNC(Accept) \ HEADER_FUNC(AcceptEncoding) \ - HEADER_FUNC(AccessControlRequestMethod) \ HEADER_FUNC(Authorization) \ HEADER_FUNC(ClientTraceId) \ HEADER_FUNC(EnvoyDownstreamServiceCluster) \ @@ -305,15 +304,9 @@ class HeaderEntry { HEADER_FUNC(UserAgent) /** - * O(1) response headers. + * Default O(1) response headers. */ #define INLINE_RESP_HEADERS(HEADER_FUNC) \ - HEADER_FUNC(AccessControlAllowCredentials) \ - HEADER_FUNC(AccessControlAllowHeaders) \ - HEADER_FUNC(AccessControlAllowMethods) \ - HEADER_FUNC(AccessControlAllowOrigin) \ - HEADER_FUNC(AccessControlExposeHeaders) \ - HEADER_FUNC(AccessControlMaxAge) \ HEADER_FUNC(Date) \ HEADER_FUNC(Etag) \ HEADER_FUNC(EnvoyDegraded) \ @@ -328,7 +321,7 @@ class HeaderEntry { HEADER_FUNC(Vary) /** - * O(1) request and response headers. + * Default O(1) request and response headers. */ #define INLINE_REQ_RESP_HEADERS(HEADER_FUNC) \ HEADER_FUNC(CacheControl) \ @@ -346,7 +339,7 @@ class HeaderEntry { HEADER_FUNC(Via) /** - * O(1) response headers and trailers. + * Default O(1) response headers and trailers. */ #define INLINE_RESP_HEADERS_TRAILERS(HEADER_FUNC) \ HEADER_FUNC(GrpcMessage) \ @@ -374,12 +367,7 @@ class HeaderEntry { virtual void set##name(absl::string_view value) PURE; \ virtual void set##name(uint64_t value) PURE; \ virtual size_t remove##name() PURE; \ - absl::string_view get##name##Value() const { \ - if (name() != nullptr) { \ - return name()->value().getStringView(); \ - } \ - return ""; \ - } + virtual absl::string_view get##name##Value() const PURE; /** * Wraps a set of HTTP headers. @@ -625,42 +613,170 @@ class HeaderMap { using HeaderMapPtr = std::unique_ptr; +/** + * Registry for custom headers. Headers can be registered multiple times in independent + * compilation units and will still point to the same slot. Headers are registered independently + * for each concrete header map type and do not overlap. Handles are strongly typed and do not + * allow mixing. + */ +class CustomInlineHeaderRegistry { +public: + enum class Type { RequestHeaders, RequestTrailers, ResponseHeaders, ResponseTrailers }; + using RegistrationMap = std::map; + + // A "phantom" type is used here to force the compiler to verify that handles are not mixed + // between concrete header map types. + template struct Handle { + Handle(RegistrationMap::const_iterator it) : it_(it) {} + + RegistrationMap::const_iterator it_; + }; + + /** + * Register an inline header and return a handle for use in inline header calls. Must be called + * prior to finalize(). + */ + template + static Handle registerInlineHeader(const LowerCaseString& header_name) { + static size_t inline_header_index = 0; + + ASSERT(!mutableFinalized()); + auto& map = mutableRegistrationMap(); + auto entry = map.find(header_name); + if (entry == map.end()) { + map[header_name] = inline_header_index++; + } + return Handle(map.find(header_name)); + } + + /** + * Fetch the handle for a registered inline header. May only be called after finalized(). + */ + template + static absl::optional> getInlineHeader(const LowerCaseString& header_name) { + ASSERT(mutableFinalized()); + auto& map = mutableRegistrationMap(); + auto entry = map.find(header_name); + if (entry != map.end()) { + return Handle(entry); + } + return absl::nullopt; + } + + /** + * Fetch all registered headers. May only be called after finalized(). + */ + template static const RegistrationMap& headers() { + ASSERT(mutableFinalized()); + return mutableRegistrationMap(); + } + + /** + * Finalize the custom header registrations. No further changes are allowed after this point. + * This guaranteed that all header maps created by the process have the same variable size and + * custom registrations. + */ + template static void finalize() { + ASSERT(!mutableFinalized()); + mutableFinalized() = true; + } + +private: + template static RegistrationMap& mutableRegistrationMap() { + MUTABLE_CONSTRUCT_ON_FIRST_USE(RegistrationMap); + } + template static bool& mutableFinalized() { MUTABLE_CONSTRUCT_ON_FIRST_USE(bool); } +}; + +/** + * Static initializer to register a custom header in a compilation unit. This can be used by + * extensions to register custom headers. + */ +template class RegisterCustomInlineHeader { +public: + RegisterCustomInlineHeader(const LowerCaseString& header) + : handle_(CustomInlineHeaderRegistry::registerInlineHeader(header)) {} + + typename CustomInlineHeaderRegistry::Handle handle() { return handle_; } + +private: + const typename CustomInlineHeaderRegistry::Handle handle_; +}; + +/** + * The following functions allow O(1) access for custom inline headers. + */ +template class CustomInlineHeaderBase { +public: + virtual ~CustomInlineHeaderBase() = default; + + static constexpr CustomInlineHeaderRegistry::Type header_map_type = type; + using Handle = CustomInlineHeaderRegistry::Handle; + + virtual const HeaderEntry* getInline(Handle handle) const PURE; + virtual void appendInline(Handle handle, absl::string_view data, + absl::string_view delimiter) PURE; + virtual void setReferenceInline(Handle, absl::string_view value) PURE; + virtual void setInline(Handle, absl::string_view value) PURE; + virtual void setInline(Handle, uint64_t value) PURE; + virtual size_t removeInline(Handle handle) PURE; + absl::string_view getInlineValue(Handle handle) const { + const auto header = getInline(handle); + if (header != nullptr) { + return header->value().getStringView(); + } + return {}; + } +}; + /** * Typed derived classes for all header map types. */ // Base class for both request and response headers. -class RequestOrResponseHeaderMap : public virtual HeaderMap { +class RequestOrResponseHeaderMap : public HeaderMap { public: INLINE_REQ_RESP_HEADERS(DEFINE_INLINE_HEADER) }; // Request headers. -class RequestHeaderMap : public RequestOrResponseHeaderMap { +class RequestHeaderMap + : public RequestOrResponseHeaderMap, + public CustomInlineHeaderBase { public: INLINE_REQ_HEADERS(DEFINE_INLINE_HEADER) }; using RequestHeaderMapPtr = std::unique_ptr; // Request trailers. -class RequestTrailerMap : public virtual HeaderMap {}; +class RequestTrailerMap + : public HeaderMap, + public CustomInlineHeaderBase {}; using RequestTrailerMapPtr = std::unique_ptr; // Base class for both response headers and trailers. -class ResponseHeaderOrTrailerMap : public virtual HeaderMap { +class ResponseHeaderOrTrailerMap { public: + virtual ~ResponseHeaderOrTrailerMap() = default; + INLINE_RESP_HEADERS_TRAILERS(DEFINE_INLINE_HEADER) }; // Response headers. -class ResponseHeaderMap : public RequestOrResponseHeaderMap, public ResponseHeaderOrTrailerMap { +class ResponseHeaderMap + : public RequestOrResponseHeaderMap, + public ResponseHeaderOrTrailerMap, + public CustomInlineHeaderBase { public: INLINE_RESP_HEADERS(DEFINE_INLINE_HEADER) }; using ResponseHeaderMapPtr = std::unique_ptr; // Response trailers. -class ResponseTrailerMap : public virtual HeaderMap, public ResponseHeaderOrTrailerMap {}; +class ResponseTrailerMap + : public ResponseHeaderOrTrailerMap, + public HeaderMap, + public CustomInlineHeaderBase {}; using ResponseTrailerMapPtr = std::unique_ptr; /** diff --git a/source/common/grpc/async_client_impl.cc b/source/common/grpc/async_client_impl.cc index 19aa96ed460f0..d67f1ded4771c 100644 --- a/source/common/grpc/async_client_impl.cc +++ b/source/common/grpc/async_client_impl.cc @@ -100,7 +100,7 @@ void AsyncStreamImpl::initialize(bool buffer_body_for_retry) { void AsyncStreamImpl::onHeaders(Http::ResponseHeaderMapPtr&& headers, bool end_stream) { const auto http_response_status = Http::Utility::getResponseStatus(*headers); const auto grpc_status = Common::getGrpcStatus(*headers); - callbacks_.onReceiveInitialMetadata(end_stream ? std::make_unique() + callbacks_.onReceiveInitialMetadata(end_stream ? Http::ResponseHeaderMapImpl::create() : std::move(headers)); if (http_response_status != enumToInt(Http::Code::OK)) { // https://github.com/grpc/grpc/blob/master/doc/http-grpc-status-mapping.md requires that @@ -163,7 +163,7 @@ void AsyncStreamImpl::onTrailers(Http::ResponseTrailerMapPtr&& trailers) { } void AsyncStreamImpl::streamError(Status::GrpcStatus grpc_status, const std::string& message) { - callbacks_.onReceiveTrailingMetadata(std::make_unique()); + callbacks_.onReceiveTrailingMetadata(Http::ResponseTrailerMapImpl::create()); callbacks_.onRemoteClose(grpc_status, message); resetStream(); } diff --git a/source/common/grpc/google_async_client_impl.cc b/source/common/grpc/google_async_client_impl.cc index ff7774034ac97..d22903da723e1 100644 --- a/source/common/grpc/google_async_client_impl.cc +++ b/source/common/grpc/google_async_client_impl.cc @@ -171,9 +171,9 @@ void GoogleAsyncStreamImpl::initialize(bool /*buffer_body_for_retry*/) { } // Due to the different HTTP header implementations, we effectively double // copy headers here. - Http::RequestHeaderMapImpl initial_metadata; - callbacks_.onCreateInitialMetadata(initial_metadata); - initial_metadata.iterate( + auto initial_metadata = Http::RequestHeaderMapImpl::create(); + callbacks_.onCreateInitialMetadata(*initial_metadata); + initial_metadata->iterate( [](const Http::HeaderEntry& header, void* ctxt) { auto* client_context = static_cast(ctxt); client_context->AddMetadata(std::string(header.key().getStringView()), @@ -207,9 +207,8 @@ void GoogleAsyncStreamImpl::notifyRemoteClose(Status::GrpcStatus grpc_status, parent_.stats_.streams_closed_[grpc_status]->inc(); } ENVOY_LOG(debug, "notifyRemoteClose {} {}", grpc_status, message); - callbacks_.onReceiveTrailingMetadata(trailing_metadata - ? std::move(trailing_metadata) - : std::make_unique()); + callbacks_.onReceiveTrailingMetadata(trailing_metadata ? std::move(trailing_metadata) + : Http::ResponseTrailerMapImpl::create()); callbacks_.onRemoteClose(grpc_status, message); } @@ -312,7 +311,7 @@ void GoogleAsyncStreamImpl::handleOpCompletion(GoogleAsyncTag::Operation op, boo ASSERT(call_initialized_); rw_->Read(&read_buf_, &read_tag_); ++inflight_tags_; - Http::ResponseHeaderMapPtr initial_metadata = std::make_unique(); + Http::ResponseHeaderMapPtr initial_metadata = Http::ResponseHeaderMapImpl::create(); metadataTranslate(ctxt_.GetServerInitialMetadata(), *initial_metadata); callbacks_.onReceiveInitialMetadata(std::move(initial_metadata)); break; @@ -346,8 +345,7 @@ void GoogleAsyncStreamImpl::handleOpCompletion(GoogleAsyncTag::Operation op, boo case GoogleAsyncTag::Operation::Finish: { ASSERT(finish_pending_); ENVOY_LOG(debug, "Finish with grpc-status code {}", status_.error_code()); - Http::ResponseTrailerMapPtr trailing_metadata = - std::make_unique(); + Http::ResponseTrailerMapPtr trailing_metadata = Http::ResponseTrailerMapImpl::create(); metadataTranslate(ctxt_.GetServerTrailingMetadata(), *trailing_metadata); notifyRemoteClose(static_cast(status_.error_code()), std::move(trailing_metadata), status_.error_message()); diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 22e40d6b5f8de..10c682ae5714b 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -1254,7 +1254,7 @@ RequestTrailerMap& ConnectionManagerImpl::ActiveStream::addDecodedTrailers() { // Trailers can only be added once. ASSERT(!request_trailers_); - request_trailers_ = std::make_unique(); + request_trailers_ = RequestTrailerMapImpl::create(); return *request_trailers_; } @@ -1817,7 +1817,7 @@ ResponseTrailerMap& ConnectionManagerImpl::ActiveStream::addEncodedTrailers() { // Trailers can only be added once. ASSERT(!response_trailers_); - response_trailers_ = std::make_unique(); + response_trailers_ = ResponseTrailerMapImpl::create(); return *response_trailers_; } diff --git a/source/common/http/header_map_impl.cc b/source/common/http/header_map_impl.cc index 73de67f6f0e0f..3bb6d1379c5c9 100644 --- a/source/common/http/header_map_impl.cc +++ b/source/common/http/header_map_impl.cc @@ -26,22 +26,22 @@ void validateCapacity(uint64_t new_capacity) { "Trying to allocate overly large headers."); } -absl::string_view get_str_view(const VariantHeader& buffer) { +absl::string_view getStrView(const VariantHeader& buffer) { return absl::get(buffer); } -InlineHeaderVector& get_in_vec(VariantHeader& buffer) { +InlineHeaderVector& getInVec(VariantHeader& buffer) { return absl::get(buffer); } -const InlineHeaderVector& get_in_vec(const VariantHeader& buffer) { +const InlineHeaderVector& getInVec(const VariantHeader& buffer) { return absl::get(buffer); } } // namespace // Initialize as a Type::Inline HeaderString::HeaderString() : buffer_(InlineHeaderVector()) { - ASSERT((get_in_vec(buffer_).capacity()) >= MaxIntegerLength); + ASSERT((getInVec(buffer_).capacity()) >= MaxIntegerLength); ASSERT(valid()); } @@ -72,19 +72,19 @@ void HeaderString::append(const char* data, uint32_t data_size) { case Type::Reference: { // Rather than be too clever and optimize this uncommon case, we switch to // Inline mode and copy. - const absl::string_view prev = get_str_view(buffer_); + const absl::string_view prev = getStrView(buffer_); buffer_ = InlineHeaderVector(); // Assigning new_capacity to avoid resizing when appending the new data - get_in_vec(buffer_).reserve(new_capacity); - get_in_vec(buffer_).assign(prev.begin(), prev.end()); + getInVec(buffer_).reserve(new_capacity); + getInVec(buffer_).assign(prev.begin(), prev.end()); break; } case Type::Inline: { - get_in_vec(buffer_).reserve(new_capacity); + getInVec(buffer_).reserve(new_capacity); break; } } - get_in_vec(buffer_).insert(get_in_vec(buffer_).end(), data, data + data_size); + getInVec(buffer_).insert(getInVec(buffer_).end(), data, data + data_size); } void HeaderString::rtrim() { @@ -92,21 +92,21 @@ void HeaderString::rtrim() { absl::string_view original = getStringView(); absl::string_view rtrimmed = StringUtil::rtrim(original); if (original.size() != rtrimmed.size()) { - get_in_vec(buffer_).resize(rtrimmed.size()); + getInVec(buffer_).resize(rtrimmed.size()); } } absl::string_view HeaderString::getStringView() const { if (type() == Type::Reference) { - return get_str_view(buffer_); + return getStrView(buffer_); } ASSERT(type() == Type::Inline); - return {get_in_vec(buffer_).data(), get_in_vec(buffer_).size()}; + return {getInVec(buffer_).data(), getInVec(buffer_).size()}; } void HeaderString::clear() { if (type() == Type::Inline) { - get_in_vec(buffer_).clear(); + getInVec(buffer_).clear(); } } @@ -118,8 +118,8 @@ void HeaderString::setCopy(const char* data, uint32_t size) { buffer_ = InlineHeaderVector(); } - get_in_vec(buffer_).reserve(size); - get_in_vec(buffer_).assign(data, data + size); + getInVec(buffer_).reserve(size); + getInVec(buffer_).assign(data, data + size); ASSERT(valid()); } @@ -141,8 +141,8 @@ void HeaderString::setInteger(uint64_t value) { // Switching from Type::Reference to Type::Inline buffer_ = InlineHeaderVector(); } - ASSERT((get_in_vec(buffer_).capacity()) > MaxIntegerLength); - get_in_vec(buffer_).assign(inner_buffer, inner_buffer + int_length); + ASSERT((getInVec(buffer_).capacity()) > MaxIntegerLength); + getInVec(buffer_).assign(inner_buffer, inner_buffer + int_length); } void HeaderString::setReference(absl::string_view ref_value) { @@ -152,10 +152,10 @@ void HeaderString::setReference(absl::string_view ref_value) { uint32_t HeaderString::size() const { if (type() == Type::Reference) { - return get_str_view(buffer_).size(); + return getStrView(buffer_).size(); } ASSERT(type() == Type::Inline); - return get_in_vec(buffer_).size(); + return getInVec(buffer_).size(); } HeaderString::Type HeaderString::type() const { @@ -192,30 +192,47 @@ void HeaderMapImpl::HeaderEntryImpl::value(const HeaderEntry& header) { value(header.value().getStringView()); } -#define INLINE_HEADER_STATIC_MAP_ENTRY(name) \ - add(Headers::get().name.get().c_str(), [](HeaderMapType& h) -> StaticLookupResponse { \ - return {&h.inline_headers_.name##_, &Headers::get().name}; \ - }); +template <> HeaderMapImpl::StaticLookupTable::StaticLookupTable() { +#define REGISTER_DEFAULT_REQUEST_HEADER(name) \ + CustomInlineHeaderRegistry::registerInlineHeader( \ + Headers::get().name); + INLINE_REQ_HEADERS(REGISTER_DEFAULT_REQUEST_HEADER) + INLINE_REQ_RESP_HEADERS(REGISTER_DEFAULT_REQUEST_HEADER) -template <> HeaderMapImpl::StaticLookupTable::StaticLookupTable() { - INLINE_REQ_HEADERS(INLINE_HEADER_STATIC_MAP_ENTRY) - INLINE_REQ_RESP_HEADERS(INLINE_HEADER_STATIC_MAP_ENTRY) + finalizeTable(); // Special case where we map a legacy host header to :authority. - add(Headers::get().HostLegacy.get().c_str(), [](HeaderMapType& h) -> StaticLookupResponse { - return {&h.inline_headers_.Host_, &Headers::get().Host}; + const auto handle = + CustomInlineHeaderRegistry::getInlineHeader( + Headers::get().Host); + add(Headers::get().HostLegacy.get().c_str(), [handle](HeaderMapImpl& h) -> StaticLookupResponse { + return {&h.inlineHeaders()[handle.value().it_->second], &handle.value().it_->first}; }); } -template <> HeaderMapImpl::StaticLookupTable::StaticLookupTable() { - INLINE_RESP_HEADERS(INLINE_HEADER_STATIC_MAP_ENTRY) - INLINE_REQ_RESP_HEADERS(INLINE_HEADER_STATIC_MAP_ENTRY) - INLINE_RESP_HEADERS_TRAILERS(INLINE_HEADER_STATIC_MAP_ENTRY) +template <> HeaderMapImpl::StaticLookupTable::StaticLookupTable() { + finalizeTable(); +} + +template <> HeaderMapImpl::StaticLookupTable::StaticLookupTable() { +#define REGISTER_RESPONSE_HEADER(name) \ + CustomInlineHeaderRegistry::registerInlineHeader( \ + Headers::get().name); + INLINE_RESP_HEADERS(REGISTER_RESPONSE_HEADER) + INLINE_REQ_RESP_HEADERS(REGISTER_RESPONSE_HEADER) + INLINE_RESP_HEADERS_TRAILERS(REGISTER_RESPONSE_HEADER) + + finalizeTable(); } -template <> -HeaderMapImpl::StaticLookupTable::StaticLookupTable(){ - INLINE_RESP_HEADERS_TRAILERS(INLINE_HEADER_STATIC_MAP_ENTRY)} +template <> HeaderMapImpl::StaticLookupTable::StaticLookupTable() { +#define REGISTER_RESPONSE_TRAILER(name) \ + CustomInlineHeaderRegistry::registerInlineHeader( \ + Headers::get().name); + INLINE_RESP_HEADERS_TRAILERS(REGISTER_RESPONSE_TRAILER) + + finalizeTable(); +} uint64_t HeaderMapImpl::appendToHeader(HeaderString& header, absl::string_view data, absl::string_view delimiter) { @@ -454,7 +471,7 @@ HeaderEntry* HeaderMapImpl::getExisting(const LowerCaseString& key) { return nullptr; } -void HeaderMapImpl::iterate(ConstIterateCb cb, void* context) const { +void HeaderMapImpl::iterate(HeaderMap::ConstIterateCb cb, void* context) const { for (const HeaderEntryImpl& header : headers_) { if (cb(header, context) == HeaderMap::Iterate::Break) { break; @@ -462,7 +479,7 @@ void HeaderMapImpl::iterate(ConstIterateCb cb, void* context) const { } } -void HeaderMapImpl::iterateReverse(ConstIterateCb cb, void* context) const { +void HeaderMapImpl::iterateReverse(HeaderMap::ConstIterateCb cb, void* context) const { for (auto it = headers_.rbegin(); it != headers_.rend(); it++) { if (cb(*it, context) == HeaderMap::Iterate::Break) { break; @@ -482,13 +499,13 @@ HeaderMap::Lookup HeaderMapImpl::lookup(const LowerCaseString& key, if (lookup.has_value()) { *entry = *lookup.value().entry_; if (*entry) { - return Lookup::Found; + return HeaderMap::Lookup::Found; } else { - return Lookup::NotFound; + return HeaderMap::Lookup::NotFound; } } else { *entry = nullptr; - return Lookup::NotSupported; + return HeaderMap::Lookup::NotSupported; } } diff --git a/source/common/http/header_map_impl.h b/source/common/http/header_map_impl.h index 57fa82593e26f..d279419ece2c3 100644 --- a/source/common/http/header_map_impl.h +++ b/source/common/http/header_map_impl.h @@ -18,46 +18,36 @@ namespace Http { /** * These are definitions of all of the inline header access functions described inside header_map.h - * TODO(asraa): Simplify code here so macros expand into single virtual calls. */ #define DEFINE_INLINE_HEADER_FUNCS(name) \ public: \ - const HeaderEntry* name() const override { return inline_headers_.name##_; } \ + const HeaderEntry* name() const override { return getInline(HeaderHandles::get().name); } \ void append##name(absl::string_view data, absl::string_view delimiter) override { \ - HeaderEntry& entry = maybeCreateInline(&inline_headers_.name##_, Headers::get().name); \ - addSize(HeaderMapImpl::appendToHeader(entry.value(), data, delimiter)); \ + appendInline(HeaderHandles::get().name, data, delimiter); \ } \ void setReference##name(absl::string_view value) override { \ - HeaderEntry& entry = maybeCreateInline(&inline_headers_.name##_, Headers::get().name); \ - updateSize(entry.value().size(), value.size()); \ - entry.value().setReference(value); \ + setReferenceInline(HeaderHandles::get().name, value); \ } \ void set##name(absl::string_view value) override { \ - HeaderEntry& entry = maybeCreateInline(&inline_headers_.name##_, Headers::get().name); \ - updateSize(entry.value().size(), value.size()); \ - entry.value().setCopy(value); \ + setInline(HeaderHandles::get().name, value); \ } \ - void set##name(uint64_t value) override { \ - HeaderEntry& entry = maybeCreateInline(&inline_headers_.name##_, Headers::get().name); \ - subtractSize(inline_headers_.name##_->value().size()); \ - entry.value().setInteger(value); \ - addSize(inline_headers_.name##_->value().size()); \ - } \ - size_t remove##name() override { return removeInline(&inline_headers_.name##_); } - -#define DEFINE_INLINE_HEADER_STRUCT(name) HeaderEntryImpl* name##_; + void set##name(uint64_t value) override { setInline(HeaderHandles::get().name, value); } \ + size_t remove##name() override { return removeInline(HeaderHandles::get().name); } \ + absl::string_view get##name##Value() const override { \ + return getInlineValue(HeaderHandles::get().name); \ + } /** * Implementation of Http::HeaderMap. This is heavily optimized for performance. Roughly, when - * headers are added to the map, we do a hash lookup to see if it's one of the O(1) headers. + * headers are added to the map, we do a trie lookup to see if it's one of the O(1) headers. * If it is, we store a reference to it that can be accessed later directly. Most high performance * paths use O(1) direct access. In general, we try to copy as little as possible and allocate as * little as possible in any of the paths. - * TODO(mattklein123): The end result of the header refactor should be to make this a fully - * protected base class or a mix-in for the concrete header types below. */ -class HeaderMapImpl : public virtual HeaderMap, NonCopyable { +class HeaderMapImpl : NonCopyable { public: + virtual ~HeaderMapImpl() = default; + // The following "constructors" call virtual functions during construction and must use the // static factory pattern. static void copyFrom(HeaderMap& lhs, const HeaderMap& rhs); @@ -78,30 +68,33 @@ class HeaderMapImpl : public virtual HeaderMap, NonCopyable { // Performs a manual byte size count for test verification. void verifyByteSizeInternalForTest() const; - // Http::HeaderMap - bool operator==(const HeaderMap& rhs) const override; - bool operator!=(const HeaderMap& rhs) const override; - void addViaMove(HeaderString&& key, HeaderString&& value) override; - void addReference(const LowerCaseString& key, absl::string_view value) override; - void addReferenceKey(const LowerCaseString& key, uint64_t value) override; - void addReferenceKey(const LowerCaseString& key, absl::string_view value) override; - void addCopy(const LowerCaseString& key, uint64_t value) override; - void addCopy(const LowerCaseString& key, absl::string_view value) override; - void appendCopy(const LowerCaseString& key, absl::string_view value) override; - void setReference(const LowerCaseString& key, absl::string_view value) override; - void setReferenceKey(const LowerCaseString& key, absl::string_view value) override; - void setCopy(const LowerCaseString& key, absl::string_view value) override; - uint64_t byteSize() const override; - const HeaderEntry* get(const LowerCaseString& key) const override; - void iterate(ConstIterateCb cb, void* context) const override; - void iterateReverse(ConstIterateCb cb, void* context) const override; - Lookup lookup(const LowerCaseString& key, const HeaderEntry** entry) const override; - void clear() override; - size_t remove(const LowerCaseString& key) override; - size_t removePrefix(const LowerCaseString& key) override; - size_t size() const override { return headers_.size(); } - bool empty() const override { return headers_.empty(); } - void dumpState(std::ostream& os, int indent_level = 0) const override; + // Note: This class does not actually implement Http::HeaderMap to avoid virtual inheritance in + // the derived classes. Instead, it is used as a mix-in class for TypedHeaderMapImpl below. This + // both avoid virtual inheritance and allows the concrete final header maps to use a variable + // length member at the end. + bool operator==(const HeaderMap& rhs) const; + bool operator!=(const HeaderMap& rhs) const; + void addViaMove(HeaderString&& key, HeaderString&& value); + void addReference(const LowerCaseString& key, absl::string_view value); + void addReferenceKey(const LowerCaseString& key, uint64_t value); + void addReferenceKey(const LowerCaseString& key, absl::string_view value); + void addCopy(const LowerCaseString& key, uint64_t value); + void addCopy(const LowerCaseString& key, absl::string_view value); + void appendCopy(const LowerCaseString& key, absl::string_view value); + void setReference(const LowerCaseString& key, absl::string_view value); + void setReferenceKey(const LowerCaseString& key, absl::string_view value); + void setCopy(const LowerCaseString& key, absl::string_view value); + uint64_t byteSize() const; + const HeaderEntry* get(const LowerCaseString& key) const; + void iterate(HeaderMap::ConstIterateCb cb, void* context) const; + void iterateReverse(HeaderMap::ConstIterateCb cb, void* context) const; + HeaderMap::Lookup lookup(const LowerCaseString& key, const HeaderEntry** entry) const; + void clear(); + size_t remove(const LowerCaseString& key); + size_t removePrefix(const LowerCaseString& key); + size_t size() const { return headers_.size(); } + bool empty() const { return headers_.empty(); } + void dumpState(std::ostream& os, int indent_level = 0) const; protected: struct HeaderEntryImpl : public HeaderEntry, NonCopyable { @@ -134,20 +127,39 @@ class HeaderMapImpl : public virtual HeaderMap, NonCopyable { /** * Base class for a static lookup table that converts a string key into an O(1) header. */ - template - struct StaticLookupTable : public TrieLookupTable { - using HeaderMapType = T; - + template + struct StaticLookupTable + : public TrieLookupTable> { StaticLookupTable(); - static absl::optional lookup(T& header_map, absl::string_view key) { - auto entry = ConstSingleton::get().find(key); + void finalizeTable() { + CustomInlineHeaderRegistry::finalize(); + auto& headers = CustomInlineHeaderRegistry::headers(); + size_ = headers.size(); + for (const auto& header : headers) { + this->add(header.first.get().c_str(), [&header](HeaderMapImpl& h) -> StaticLookupResponse { + return {&h.inlineHeaders()[header.second], &header.first}; + }); + } + } + + static size_t size() { + // The size of the lookup table is finalized when the singleton lookup table is created. This + // allows for late binding of custom headers as well as envoy header prefix changes. + return ConstSingleton::get().size_; + } + + static absl::optional lookup(HeaderMapImpl& header_map, + absl::string_view key) { + const auto& entry = ConstSingleton::get().find(key); if (entry != nullptr) { return entry(header_map); } else { return absl::nullopt; } } + + size_t size_; }; /** @@ -230,13 +242,9 @@ class HeaderMapImpl : public virtual HeaderMap, NonCopyable { void updateSize(uint64_t from_size, uint64_t to_size); void addSize(uint64_t size); void subtractSize(uint64_t size); - virtual absl::optional staticLookup(absl::string_view) { - // TODO(mattklein123): Make this pure once HeaderMapImpl is a base class only. - return absl::nullopt; - } - virtual void clearInline() { - // TODO(mattklein123): Make this pure once HeaderMapImpl is a base class only. - } + virtual absl::optional staticLookup(absl::string_view) PURE; + virtual void clearInline() PURE; + virtual HeaderEntryImpl** inlineHeaders() PURE; HeaderList headers_; // This holds the internal byte size of the HeaderMap. @@ -244,107 +252,243 @@ class HeaderMapImpl : public virtual HeaderMap, NonCopyable { }; /** - * Typed derived classes for all header map types. + * Typed derived classes for all header map types. This class implements the actual typed + * interface and for the majority of methods just passes through to the HeaderMapImpl mix-in. Per + * above, this avoids virtual inheritance. + */ +template class TypedHeaderMapImpl : public HeaderMapImpl, public Interface { +public: + // Implementation of Http::HeaderMap that passes through to HeaderMapImpl. + bool operator==(const HeaderMap& rhs) const override { return HeaderMapImpl::operator==(rhs); } + bool operator!=(const HeaderMap& rhs) const override { return HeaderMapImpl::operator!=(rhs); } + void addViaMove(HeaderString&& key, HeaderString&& value) override { + HeaderMapImpl::addViaMove(std::move(key), std::move(value)); + } + void addReference(const LowerCaseString& key, absl::string_view value) override { + HeaderMapImpl::addReference(key, value); + } + void addReferenceKey(const LowerCaseString& key, uint64_t value) override { + HeaderMapImpl::addReferenceKey(key, value); + } + void addReferenceKey(const LowerCaseString& key, absl::string_view value) override { + HeaderMapImpl::addReferenceKey(key, value); + } + void addCopy(const LowerCaseString& key, uint64_t value) override { + HeaderMapImpl::addCopy(key, value); + } + void addCopy(const LowerCaseString& key, absl::string_view value) override { + HeaderMapImpl::addCopy(key, value); + } + void appendCopy(const LowerCaseString& key, absl::string_view value) override { + HeaderMapImpl::appendCopy(key, value); + } + void setReference(const LowerCaseString& key, absl::string_view value) override { + HeaderMapImpl::setReference(key, value); + } + void setReferenceKey(const LowerCaseString& key, absl::string_view value) override { + HeaderMapImpl::setReferenceKey(key, value); + } + void setCopy(const LowerCaseString& key, absl::string_view value) override { + HeaderMapImpl::setCopy(key, value); + } + uint64_t byteSize() const override { return HeaderMapImpl::byteSize(); } + const HeaderEntry* get(const LowerCaseString& key) const override { + return HeaderMapImpl::get(key); + } + void iterate(HeaderMap::ConstIterateCb cb, void* context) const override { + HeaderMapImpl::iterate(cb, context); + } + void iterateReverse(HeaderMap::ConstIterateCb cb, void* context) const override { + HeaderMapImpl::iterateReverse(cb, context); + } + HeaderMap::Lookup lookup(const LowerCaseString& key, const HeaderEntry** entry) const override { + return HeaderMapImpl::lookup(key, entry); + } + void clear() override { HeaderMapImpl::clear(); } + size_t remove(const LowerCaseString& key) override { return HeaderMapImpl::remove(key); } + size_t removePrefix(const LowerCaseString& key) override { + return HeaderMapImpl::removePrefix(key); + } + size_t size() const override { return HeaderMapImpl::size(); } + bool empty() const override { return HeaderMapImpl::empty(); } + void dumpState(std::ostream& os, int indent_level = 0) const override { + HeaderMapImpl::dumpState(os, indent_level); + } + + // Generic custom header functions for each fully typed interface. To avoid accidental issues, + // the Handle type is different for each interface, which is why these functions live here vs. + // inside HeaderMapImpl. + using Handle = CustomInlineHeaderRegistry::Handle; + const HeaderEntry* getInline(Handle handle) const override { + ASSERT(handle.it_->second < inlineHeadersSize()); + return constInlineHeaders()[handle.it_->second]; + } + void appendInline(Handle handle, absl::string_view data, absl::string_view delimiter) override { + ASSERT(handle.it_->second < inlineHeadersSize()); + HeaderEntry& entry = maybeCreateInline(&inlineHeaders()[handle.it_->second], handle.it_->first); + addSize(HeaderMapImpl::appendToHeader(entry.value(), data, delimiter)); + } + void setReferenceInline(Handle handle, absl::string_view value) override { + ASSERT(handle.it_->second < inlineHeadersSize()); + HeaderEntry& entry = maybeCreateInline(&inlineHeaders()[handle.it_->second], handle.it_->first); + updateSize(entry.value().size(), value.size()); + entry.value().setReference(value); + } + void setInline(Handle handle, absl::string_view value) override { + ASSERT(handle.it_->second < inlineHeadersSize()); + HeaderEntry& entry = maybeCreateInline(&inlineHeaders()[handle.it_->second], handle.it_->first); + updateSize(entry.value().size(), value.size()); + entry.value().setCopy(value); + } + void setInline(Handle handle, uint64_t value) override { + ASSERT(handle.it_->second < inlineHeadersSize()); + HeaderEntry& entry = maybeCreateInline(&inlineHeaders()[handle.it_->second], handle.it_->first); + subtractSize(entry.value().size()); + entry.value().setInteger(value); + addSize(entry.value().size()); + } + size_t removeInline(Handle handle) override { + ASSERT(handle.it_->second < inlineHeadersSize()); + return HeaderMapImpl::removeInline(&inlineHeaders()[handle.it_->second]); + } + +protected: + static size_t inlineHeadersSize() { + return StaticLookupTable::size() * sizeof(HeaderEntryImpl*); + } + absl::optional staticLookup(absl::string_view key) override { + return StaticLookupTable::lookup(*this, key); + } + virtual const HeaderEntryImpl* const* constInlineHeaders() const PURE; +}; + +#define DEFINE_HEADER_HANDLE(name) \ + Handle name = \ + CustomInlineHeaderRegistry::getInlineHeader(Headers::get().name).value(); + +/** + * Concrete implementation of RequestHeaderMap which allows for variable custom registered inline + * headers. */ -class RequestHeaderMapImpl : public HeaderMapImpl, public RequestHeaderMap { +class RequestHeaderMapImpl final : public TypedHeaderMapImpl, + public InlineStorage { public: + static std::unique_ptr create() { + return std::unique_ptr(new (inlineHeadersSize()) RequestHeaderMapImpl()); + } + INLINE_REQ_HEADERS(DEFINE_INLINE_HEADER_FUNCS) INLINE_REQ_RESP_HEADERS(DEFINE_INLINE_HEADER_FUNCS) -protected: - // Explicit inline headers for the request header map. - // TODO(mattklein123): This is mostly copied between all of the concrete header map types. - // In a future change we can either get rid of O(1) headers completely, or it should be possible - // to statically register all O(1) headers and move to a single dynamically sized class where we - // we reference the O(1) headers in the table by an offset. - struct AllInlineHeaders { - AllInlineHeaders() { clear(); } - void clear() { memset(this, 0, sizeof(*this)); } - - INLINE_REQ_HEADERS(DEFINE_INLINE_HEADER_STRUCT) - INLINE_REQ_RESP_HEADERS(DEFINE_INLINE_HEADER_STRUCT) +private: + struct HeaderHandleValues { + INLINE_REQ_HEADERS(DEFINE_HEADER_HANDLE) + INLINE_REQ_RESP_HEADERS(DEFINE_HEADER_HANDLE) }; - absl::optional staticLookup(absl::string_view key) override { - return StaticLookupTable::lookup(*this, key); - } - void clearInline() override { inline_headers_.clear(); } + using HeaderHandles = ConstSingleton; - AllInlineHeaders inline_headers_; + RequestHeaderMapImpl() { clearInline(); } + void clearInline() override { memset(inline_headers_, 0, inlineHeadersSize()); } + const HeaderEntryImpl* const* constInlineHeaders() const override { return inline_headers_; } + HeaderEntryImpl** inlineHeaders() override { return inline_headers_; } - friend class HeaderMapImpl; + HeaderEntryImpl* inline_headers_[]; }; -class RequestTrailerMapImpl : public HeaderMapImpl, public RequestTrailerMap {}; +/** + * Concrete implementation of RequestTrailerMap which allows for variable custom registered inline + * headers. + */ +class RequestTrailerMapImpl final : public TypedHeaderMapImpl, + public InlineStorage { +public: + static std::unique_ptr create() { + return std::unique_ptr(new (inlineHeadersSize()) + RequestTrailerMapImpl()); + } + +private: + RequestTrailerMapImpl() { clearInline(); } + void clearInline() override { memset(inline_headers_, 0, inlineHeadersSize()); } + const HeaderEntryImpl* const* constInlineHeaders() const override { return inline_headers_; } + HeaderEntryImpl** inlineHeaders() override { return inline_headers_; } -class ResponseHeaderMapImpl : public HeaderMapImpl, public ResponseHeaderMap { + HeaderEntryImpl* inline_headers_[]; +}; + +/** + * Concrete implementation of ResponseHeaderMap which allows for variable custom registered inline + * headers. + */ +class ResponseHeaderMapImpl final : public TypedHeaderMapImpl, + public InlineStorage { public: + static std::unique_ptr create() { + return std::unique_ptr(new (inlineHeadersSize()) + ResponseHeaderMapImpl()); + } + INLINE_RESP_HEADERS(DEFINE_INLINE_HEADER_FUNCS) INLINE_REQ_RESP_HEADERS(DEFINE_INLINE_HEADER_FUNCS) INLINE_RESP_HEADERS_TRAILERS(DEFINE_INLINE_HEADER_FUNCS) -protected: - // Explicit inline headers for the response header map. - // TODO(mattklein123): This is mostly copied between all of the concrete header map types. - // In a future change we can either get rid of O(1) headers completely, or it should be possible - // to statically register all O(1) headers and move to a single dynamically sized class where we - // we reference the O(1) headers in the table by an offset. - struct AllInlineHeaders { - AllInlineHeaders() { clear(); } - void clear() { memset(this, 0, sizeof(*this)); } - - INLINE_RESP_HEADERS(DEFINE_INLINE_HEADER_STRUCT) - INLINE_REQ_RESP_HEADERS(DEFINE_INLINE_HEADER_STRUCT) - INLINE_RESP_HEADERS_TRAILERS(DEFINE_INLINE_HEADER_STRUCT) +private: + struct HeaderHandleValues { + INLINE_RESP_HEADERS(DEFINE_HEADER_HANDLE) + INLINE_REQ_RESP_HEADERS(DEFINE_HEADER_HANDLE) + INLINE_RESP_HEADERS_TRAILERS(DEFINE_HEADER_HANDLE) }; - absl::optional staticLookup(absl::string_view key) override { - return StaticLookupTable::lookup(*this, key); - } - void clearInline() override { inline_headers_.clear(); } + using HeaderHandles = ConstSingleton; - AllInlineHeaders inline_headers_; + ResponseHeaderMapImpl() { clearInline(); } + void clearInline() override { memset(inline_headers_, 0, inlineHeadersSize()); } + const HeaderEntryImpl* const* constInlineHeaders() const override { return inline_headers_; } + HeaderEntryImpl** inlineHeaders() override { return inline_headers_; } - friend class HeaderMapImpl; + HeaderEntryImpl* inline_headers_[]; }; -class ResponseTrailerMapImpl : public HeaderMapImpl, public ResponseTrailerMap { +/** + * Concrete implementation of ResponseTrailerMap which allows for variable custom registered + * inline headers. + */ +class ResponseTrailerMapImpl final : public TypedHeaderMapImpl, + public InlineStorage { public: + static std::unique_ptr create() { + return std::unique_ptr(new (inlineHeadersSize()) + ResponseTrailerMapImpl()); + } + INLINE_RESP_HEADERS_TRAILERS(DEFINE_INLINE_HEADER_FUNCS) -protected: - // Explicit inline headers for the response trailer map. - // TODO(mattklein123): This is mostly copied between all of the concrete header map types. - // In a future change we can either get rid of O(1) headers completely, or it should be possible - // to statically register all O(1) headers and move to a single dynamically sized class where we - // reference the O(1) headers in the table by an offset. - struct AllInlineHeaders { - AllInlineHeaders() { clear(); } - void clear() { memset(this, 0, sizeof(*this)); } - - INLINE_RESP_HEADERS_TRAILERS(DEFINE_INLINE_HEADER_STRUCT) +private: + struct HeaderHandleValues { + INLINE_RESP_HEADERS_TRAILERS(DEFINE_HEADER_HANDLE) }; - absl::optional staticLookup(absl::string_view key) override { - return StaticLookupTable::lookup(*this, key); - } - void clearInline() override { inline_headers_.clear(); } + using HeaderHandles = ConstSingleton; - AllInlineHeaders inline_headers_; + ResponseTrailerMapImpl() { clearInline(); } + void clearInline() override { memset(inline_headers_, 0, inlineHeadersSize()); } + const HeaderEntryImpl* const* constInlineHeaders() const override { return inline_headers_; } + HeaderEntryImpl** inlineHeaders() override { return inline_headers_; } - friend class HeaderMapImpl; + HeaderEntryImpl* inline_headers_[]; }; template std::unique_ptr createHeaderMap(const std::initializer_list>& values) { - auto new_header_map = std::make_unique(); + auto new_header_map = T::create(); HeaderMapImpl::initFromInitList(*new_header_map, values.begin(), values.end()); return new_header_map; } template std::unique_ptr createHeaderMap(It begin, It end) { - auto new_header_map = std::make_unique(); + auto new_header_map = T::create(); HeaderMapImpl::initFromInitList(*new_header_map, begin, end); return new_header_map; } @@ -355,10 +499,18 @@ template std::unique_ptr createHeaderMap(const HeaderMap& rhs) { // a few places when dealing with gRPC headers/trailers conversions so it's not trivial to remove. // We should revisit this to figure how to make this a bit safer as a non-intentional conversion // may have surprising results with different O(1) headers, implementations, etc. - auto new_header_map = std::make_unique(); + auto new_header_map = T::create(); HeaderMapImpl::copyFrom(*new_header_map, rhs); return new_header_map; } +struct EmptyHeaders { + RequestHeaderMapPtr request_headers = RequestHeaderMapImpl::create(); + ResponseHeaderMapPtr response_headers = ResponseHeaderMapImpl::create(); + ResponseTrailerMapPtr response_trailers = ResponseTrailerMapImpl::create(); +}; + +using StaticEmptyHeaders = ConstSingleton; + } // namespace Http } // namespace Envoy diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h index 5ebc73b8363d1..d96a175722a82 100644 --- a/source/common/http/http1/codec_impl.h +++ b/source/common/http/http1/codec_impl.h @@ -490,12 +490,12 @@ class ServerConnectionImpl : public ServerConnection, public ConnectionImpl { } void allocHeaders() override { ASSERT(nullptr == absl::get(headers_or_trailers_)); - headers_or_trailers_.emplace(std::make_unique()); + headers_or_trailers_.emplace(RequestHeaderMapImpl::create()); } void maybeAllocTrailers() override { ASSERT(processing_trailers_); if (!absl::holds_alternative(headers_or_trailers_)) { - headers_or_trailers_.emplace(std::make_unique()); + headers_or_trailers_.emplace(RequestTrailerMapImpl::create()); } } @@ -573,13 +573,12 @@ class ClientConnectionImpl : public ClientConnection, public ConnectionImpl { } void allocHeaders() override { ASSERT(nullptr == absl::get(headers_or_trailers_)); - headers_or_trailers_.emplace(std::make_unique()); + headers_or_trailers_.emplace(ResponseHeaderMapImpl::create()); } void maybeAllocTrailers() override { ASSERT(processing_trailers_); if (!absl::holds_alternative(headers_or_trailers_)) { - headers_or_trailers_.emplace( - std::make_unique()); + headers_or_trailers_.emplace(ResponseTrailerMapImpl::create()); } } diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index b2c2e6e8ded69..24e8d8ebbebf8 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -205,7 +205,7 @@ void ConnectionImpl::StreamImpl::encodeTrailersBase(const HeaderMap& trailers) { // In this case we want trailers to come after we release all pending body data that is // waiting on window updates. We need to save the trailers so that we can emit them later. ASSERT(!pending_trailers_to_encode_); - pending_trailers_to_encode_ = createHeaderMap(trailers); + pending_trailers_to_encode_ = cloneTrailers(trailers); } else { submitTrailers(trailers); parent_.sendPendingFrames(); diff --git a/source/common/http/http2/codec_impl.h b/source/common/http/http2/codec_impl.h index 371252374f232..c6c1176b3040b 100644 --- a/source/common/http/http2/codec_impl.h +++ b/source/common/http/http2/codec_impl.h @@ -219,6 +219,7 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable()) {} + headers_or_trailers_(ResponseHeaderMapImpl::create()) {} // StreamImpl void submitHeaders(const std::vector& final_headers, @@ -330,13 +331,14 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable( - std::make_unique()); + headers_or_trailers_.emplace(ResponseHeaderMapImpl::create()); } else { - headers_or_trailers_.emplace( - std::make_unique()); + headers_or_trailers_.emplace(ResponseTrailerMapImpl::create()); } } + HeaderMapPtr cloneTrailers(const HeaderMap& trailers) override { + return createHeaderMap(trailers); + } // RequestEncoder void encodeHeaders(const RequestHeaderMap& headers, bool end_stream) override; @@ -356,8 +358,7 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable()) {} + : StreamImpl(parent, buffer_limit), headers_or_trailers_(RequestHeaderMapImpl::create()) {} // StreamImpl void submitHeaders(const std::vector& final_headers, @@ -373,7 +374,10 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable(std::make_unique()); + headers_or_trailers_.emplace(RequestTrailerMapImpl::create()); + } + HeaderMapPtr cloneTrailers(const HeaderMap& trailers) override { + return createHeaderMap(trailers); } // ResponseEncoder diff --git a/source/common/http/message_impl.h b/source/common/http/message_impl.h index a42a0a2afd5d6..698c51824a069 100644 --- a/source/common/http/message_impl.h +++ b/source/common/http/message_impl.h @@ -19,7 +19,7 @@ template class MessageImpl : public Message { public: - MessageImpl() : headers_(std::make_unique()) {} + MessageImpl() : headers_(HeadersImplType::create()) {} MessageImpl(std::unique_ptr&& headers) : headers_(std::move(headers)) {} // Http::Message diff --git a/source/common/local_reply/local_reply.cc b/source/common/local_reply/local_reply.cc index 2c18db1ff4b5c..9574de79c6fde 100644 --- a/source/common/local_reply/local_reply.cc +++ b/source/common/local_reply/local_reply.cc @@ -12,16 +12,6 @@ namespace Envoy { namespace LocalReply { -namespace { - -struct EmptyHeaders { - Http::RequestHeaderMapImpl request_headers; - Http::ResponseTrailerMapImpl response_trailers; -}; - -using StaticEmptyHeaders = ConstSingleton; - -} // namespace class BodyFormatter { public: @@ -137,14 +127,14 @@ class LocalReplyImpl : public LocalReply { stream_info.response_code_ = static_cast(code); if (request_headers == nullptr) { - request_headers = &StaticEmptyHeaders::get().request_headers; + request_headers = Http::StaticEmptyHeaders::get().request_headers.get(); } BodyFormatter* final_formatter{}; for (const auto& mapper : mappers_) { if (mapper->matchAndRewrite(*request_headers, response_headers, - StaticEmptyHeaders::get().response_trailers, stream_info, code, - body, final_formatter)) { + *Http::StaticEmptyHeaders::get().response_trailers, stream_info, + code, body, final_formatter)) { break; } } @@ -153,8 +143,8 @@ class LocalReplyImpl : public LocalReply { final_formatter = body_formatter_.get(); } return final_formatter->format(*request_headers, response_headers, - StaticEmptyHeaders::get().response_trailers, stream_info, body, - content_type); + *Http::StaticEmptyHeaders::get().response_trailers, stream_info, + body, content_type); } private: diff --git a/source/common/router/header_formatter.cc b/source/common/router/header_formatter.cc index 5793dcfec99e9..4e12d6fd8712d 100644 --- a/source/common/router/header_formatter.cc +++ b/source/common/router/header_formatter.cc @@ -323,14 +323,13 @@ StreamInfoHeaderFormatter::StreamInfoHeaderFormatter(absl::string_view field_nam } field_extractor_ = [this, pattern](const Envoy::StreamInfo::StreamInfo& stream_info) { const auto& formatters = start_time_formatters_.at(pattern); - static const Http::RequestHeaderMapImpl empty_request_headers; - static const Http::ResponseHeaderMapImpl empty_response_headers; - static const Http::ResponseTrailerMapImpl empty_response_trailers; std::string formatted; for (const auto& formatter : formatters) { - absl::StrAppend(&formatted, formatter->format(empty_request_headers, empty_response_headers, - empty_response_trailers, stream_info, - absl::string_view())); + absl::StrAppend(&formatted, + formatter->format(*Http::StaticEmptyHeaders::get().request_headers, + *Http::StaticEmptyHeaders::get().response_headers, + *Http::StaticEmptyHeaders::get().response_trailers, + stream_info, absl::string_view())); } return formatted; }; diff --git a/source/common/router/rds_impl.cc b/source/common/router/rds_impl.cc index 3c4baddf6cee9..8a041ce0f12c9 100644 --- a/source/common/router/rds_impl.cc +++ b/source/common/router/rds_impl.cc @@ -280,9 +280,9 @@ void RdsRouteConfigProviderImpl::onConfigUpdate() { auto found = aliases.find(it->alias_); if (found != aliases.end()) { // TODO(dmitri-d) HeaderMapImpl is expensive, need to profile this - Http::RequestHeaderMapImpl host_header; - host_header.setHost(VhdsSubscription::aliasToDomainName(it->alias_)); - const bool host_exists = config->virtualHostExists(host_header); + auto host_header = Http::RequestHeaderMapImpl::create(); + host_header->setHost(VhdsSubscription::aliasToDomainName(it->alias_)); + const bool host_exists = config->virtualHostExists(*host_header); std::weak_ptr current_cb(it->cb_); it->thread_local_dispatcher_.post([current_cb, host_exists] { if (auto cb = current_cb.lock()) { diff --git a/source/exe/main_common.cc b/source/exe/main_common.cc index d0a1708bd2f44..01b88fcc6cd15 100644 --- a/source/exe/main_common.cc +++ b/source/exe/main_common.cc @@ -177,10 +177,10 @@ void MainCommonBase::adminRequest(absl::string_view path_and_query, absl::string std::string path_and_query_buf = std::string(path_and_query); std::string method_buf = std::string(method); server_->dispatcher().post([this, path_and_query_buf, method_buf, handler]() { - Http::ResponseHeaderMapImpl response_headers; + auto response_headers = Http::ResponseHeaderMapImpl::create(); std::string body; - server_->admin().request(path_and_query_buf, method_buf, response_headers, body); - handler(response_headers, body); + server_->admin().request(path_and_query_buf, method_buf, *response_headers, body); + handler(*response_headers, body); }); } diff --git a/source/extensions/access_loggers/common/access_log_base.cc b/source/extensions/access_loggers/common/access_log_base.cc index 99c3fa9e12f51..0323013d63504 100644 --- a/source/extensions/access_loggers/common/access_log_base.cc +++ b/source/extensions/access_loggers/common/access_log_base.cc @@ -12,17 +12,14 @@ void ImplBase::log(const Http::RequestHeaderMap* request_headers, const Http::ResponseHeaderMap* response_headers, const Http::ResponseTrailerMap* response_trailers, const StreamInfo::StreamInfo& stream_info) { - ConstSingleton empty_request_headers; - ConstSingleton empty_response_headers; - ConstSingleton empty_response_trailers; if (!request_headers) { - request_headers = &empty_request_headers.get(); + request_headers = Http::StaticEmptyHeaders::get().request_headers.get(); } if (!response_headers) { - response_headers = &empty_response_headers.get(); + response_headers = Http::StaticEmptyHeaders::get().response_headers.get(); } if (!response_trailers) { - response_trailers = &empty_response_trailers.get(); + response_trailers = Http::StaticEmptyHeaders::get().response_trailers.get(); } if (filter_ && !filter_->evaluate(stream_info, *request_headers, *response_headers, *response_trailers)) { diff --git a/source/extensions/filters/common/expr/context.cc b/source/extensions/filters/common/expr/context.cc index 44929440b70de..aa132bea068a2 100644 --- a/source/extensions/filters/common/expr/context.cc +++ b/source/extensions/filters/common/expr/context.cc @@ -136,8 +136,8 @@ absl::optional ResponseWrapper::operator[](CelValue key) const { return CelValue::CreateInt64(info_.responseFlags()); } else if (value == GrpcStatus) { auto const& optional_status = Grpc::Common::getGrpcStatus( - trailers_.value_ ? *trailers_.value_ : ConstSingleton::get(), - headers_.value_ ? *headers_.value_ : ConstSingleton::get(), + trailers_.value_ ? *trailers_.value_ : *Http::StaticEmptyHeaders::get().response_trailers, + headers_.value_ ? *headers_.value_ : *Http::StaticEmptyHeaders::get().response_headers, info_); if (optional_status.has_value()) { return CelValue::CreateInt64(optional_status.value()); diff --git a/source/extensions/filters/common/ratelimit/ratelimit_impl.cc b/source/extensions/filters/common/ratelimit/ratelimit_impl.cc index 2df6f8445debb..100e75338b4f8 100644 --- a/source/extensions/filters/common/ratelimit/ratelimit_impl.cc +++ b/source/extensions/filters/common/ratelimit/ratelimit_impl.cc @@ -78,14 +78,14 @@ void GrpcClientImpl::onSuccess( Http::ResponseHeaderMapPtr response_headers_to_add; Http::RequestHeaderMapPtr request_headers_to_add; if (!response->response_headers_to_add().empty()) { - response_headers_to_add = std::make_unique(); + response_headers_to_add = Http::ResponseHeaderMapImpl::create(); for (const auto& h : response->response_headers_to_add()) { response_headers_to_add->addCopy(Http::LowerCaseString(h.key()), h.value()); } } if (!response->request_headers_to_add().empty()) { - request_headers_to_add = std::make_unique(); + request_headers_to_add = Http::RequestHeaderMapImpl::create(); for (const auto& h : response->request_headers_to_add()) { request_headers_to_add->addCopy(Http::LowerCaseString(h.key()), h.value()); } diff --git a/source/extensions/filters/common/rbac/engine_impl.cc b/source/extensions/filters/common/rbac/engine_impl.cc index bd8a0a9cd0cac..d9717ef509c02 100644 --- a/source/extensions/filters/common/rbac/engine_impl.cc +++ b/source/extensions/filters/common/rbac/engine_impl.cc @@ -51,8 +51,8 @@ bool RoleBasedAccessControlEngineImpl::allowed(const Network::Connection& connec bool RoleBasedAccessControlEngineImpl::allowed(const Network::Connection& connection, const StreamInfo::StreamInfo& info, std::string* effective_policy_id) const { - static const Http::RequestHeaderMapImpl* empty_header = new Http::RequestHeaderMapImpl(); - return allowed(connection, *empty_header, info, effective_policy_id); + return allowed(connection, *Http::StaticEmptyHeaders::get().request_headers, info, + effective_policy_id); } } // namespace RBAC diff --git a/source/extensions/filters/http/cors/cors_filter.cc b/source/extensions/filters/http/cors/cors_filter.cc index e482ca565b33f..103ee87277e11 100644 --- a/source/extensions/filters/http/cors/cors_filter.cc +++ b/source/extensions/filters/http/cors/cors_filter.cc @@ -1,6 +1,7 @@ #include "extensions/filters/http/cors/cors_filter.h" #include "envoy/http/codes.h" +#include "envoy/http/header_map.h" #include "envoy/stats/scope.h" #include "common/common/empty_string.h" @@ -13,6 +14,21 @@ namespace Extensions { namespace HttpFilters { namespace Cors { +Http::RegisterCustomInlineHeader + access_control_request_method(Http::Headers::get().AccessControlRequestMethod); +Http::RegisterCustomInlineHeader + access_control_allow_origin(Http::Headers::get().AccessControlAllowOrigin); +Http::RegisterCustomInlineHeader + access_control_allow_credentials(Http::Headers::get().AccessControlAllowCredentials); +Http::RegisterCustomInlineHeader + access_control_allow_methods(Http::Headers::get().AccessControlAllowMethods); +Http::RegisterCustomInlineHeader + access_control_allow_headers(Http::Headers::get().AccessControlAllowHeaders); +Http::RegisterCustomInlineHeader + access_control_max_age(Http::Headers::get().AccessControlMaxAge); +Http::RegisterCustomInlineHeader + access_control_expose_headers(Http::Headers::get().AccessControlExposeHeaders); + CorsFilterConfig::CorsFilterConfig(const std::string& stats_prefix, Stats::Scope& scope) : stats_(generateStats(stats_prefix + "cors.", scope)) {} @@ -58,31 +74,31 @@ Http::FilterHeadersStatus CorsFilter::decodeHeaders(Http::RequestHeaderMap& head return Http::FilterHeadersStatus::Continue; } - const auto requestMethod = headers.AccessControlRequestMethod(); - if (requestMethod == nullptr || requestMethod->value().empty()) { + if (headers.getInlineValue(access_control_request_method.handle()).empty()) { return Http::FilterHeadersStatus::Continue; } auto response_headers{Http::createHeaderMap( {{Http::Headers::get().Status, std::to_string(enumToInt(Http::Code::OK))}})}; - response_headers->setAccessControlAllowOrigin(origin_->value().getStringView()); + response_headers->setInline(access_control_allow_origin.handle(), + origin_->value().getStringView()); if (allowCredentials()) { - response_headers->setReferenceAccessControlAllowCredentials( - Http::Headers::get().CORSValues.True); + response_headers->setReferenceInline(access_control_allow_credentials.handle(), + Http::Headers::get().CORSValues.True); } if (!allowMethods().empty()) { - response_headers->setAccessControlAllowMethods(allowMethods()); + response_headers->setInline(access_control_allow_methods.handle(), allowMethods()); } if (!allowHeaders().empty()) { - response_headers->setAccessControlAllowHeaders(allowHeaders()); + response_headers->setInline(access_control_allow_headers.handle(), allowHeaders()); } if (!maxAge().empty()) { - response_headers->setAccessControlMaxAge(maxAge()); + response_headers->setInline(access_control_max_age.handle(), maxAge()); } decoder_callbacks_->encodeHeaders(std::move(response_headers), true); @@ -97,13 +113,14 @@ Http::FilterHeadersStatus CorsFilter::encodeHeaders(Http::ResponseHeaderMap& hea return Http::FilterHeadersStatus::Continue; } - headers.setAccessControlAllowOrigin(origin_->value().getStringView()); + headers.setInline(access_control_allow_origin.handle(), origin_->value().getStringView()); if (allowCredentials()) { - headers.setReferenceAccessControlAllowCredentials(Http::Headers::get().CORSValues.True); + headers.setReferenceInline(access_control_allow_credentials.handle(), + Http::Headers::get().CORSValues.True); } if (!exposeHeaders().empty()) { - headers.setAccessControlExposeHeaders(exposeHeaders()); + headers.setInline(access_control_expose_headers.handle(), exposeHeaders()); } return Http::FilterHeadersStatus::Continue; diff --git a/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc b/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc index 8de5069993d41..219ac96354870 100644 --- a/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc +++ b/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc @@ -720,7 +720,10 @@ bool JsonTranscoderFilter::maybeConvertGrpcStatus(Grpc::Status::GrpcStatus grpc_ return false; } - auto status_details = Grpc::Common::getGrpcStatusDetailsBin(trailers); + // TODO(mattklein123): The dynamic cast here is needed because ResponseHeaderOrTrailerMap is not + // a header map. This can likely be cleaned up. + auto status_details = + Grpc::Common::getGrpcStatusDetailsBin(dynamic_cast(trailers)); if (!status_details) { // If no rpc.Status object was sent in the grpc-status-details-bin header, // construct it from the grpc-status and grpc-message headers. diff --git a/source/extensions/filters/http/jwt_authn/filter.cc b/source/extensions/filters/http/jwt_authn/filter.cc index 8cc7d85e56ac2..36b49855a068e 100644 --- a/source/extensions/filters/http/jwt_authn/filter.cc +++ b/source/extensions/filters/http/jwt_authn/filter.cc @@ -16,11 +16,13 @@ namespace JwtAuthn { namespace { +Http::RegisterCustomInlineHeader + access_control_request_method(Http::Headers::get().AccessControlRequestMethod); + bool isCorsPreflightRequest(const Http::RequestHeaderMap& headers) { return headers.getMethodValue() == Http::Headers::get().MethodValues.Options && headers.Origin() && !headers.Origin()->value().empty() && - headers.AccessControlRequestMethod() && - !headers.AccessControlRequestMethod()->value().empty(); + !headers.getInlineValue(access_control_request_method.handle()).empty(); } } // namespace diff --git a/source/extensions/filters/http/lua/lua_filter.cc b/source/extensions/filters/http/lua/lua_filter.cc index 3429627624d41..d63c109fd647a 100644 --- a/source/extensions/filters/http/lua/lua_filter.cc +++ b/source/extensions/filters/http/lua/lua_filter.cc @@ -115,7 +115,7 @@ Http::AsyncClient::Request* makeHttpCall(lua_State* state, Filter& filter, luaL_error(state, "http call cluster invalid. Must be configured"); } - auto headers = std::make_unique(); + auto headers = Http::RequestHeaderMapImpl::create(); buildHeadersFromTable(*headers, state, 3); Http::RequestMessagePtr message(new Http::RequestMessageImpl(std::move(headers))); @@ -240,7 +240,7 @@ int StreamHandleWrapper::luaRespond(lua_State* state) { luaL_checktype(state, 2, LUA_TTABLE); size_t body_size; const char* raw_body = luaL_optlstring(state, 3, nullptr, &body_size); - auto headers = std::make_unique(); + auto headers = Http::ResponseHeaderMapImpl::create(); buildHeadersFromTable(*headers, state, 2); uint64_t status; diff --git a/source/extensions/filters/http/ratelimit/ratelimit.cc b/source/extensions/filters/http/ratelimit/ratelimit.cc index cc4b8dd5bb770..69075249162dd 100644 --- a/source/extensions/filters/http/ratelimit/ratelimit.cc +++ b/source/extensions/filters/http/ratelimit/ratelimit.cc @@ -154,7 +154,7 @@ void Filter::complete(Filters::Common::RateLimit::LimitStatus status, false}; httpContext().codeStats().chargeResponseStat(info); if (response_headers_to_add_ == nullptr) { - response_headers_to_add_ = std::make_unique(); + response_headers_to_add_ = Http::ResponseHeaderMapImpl::create(); } response_headers_to_add_->setReferenceEnvoyRateLimited( Http::Headers::get().EnvoyRateLimitedValues.True); diff --git a/source/extensions/filters/network/dubbo_proxy/serializer_impl.h b/source/extensions/filters/network/dubbo_proxy/serializer_impl.h index 983843c6f7fe2..cec6ac1a0252b 100644 --- a/source/extensions/filters/network/dubbo_proxy/serializer_impl.h +++ b/source/extensions/filters/network/dubbo_proxy/serializer_impl.h @@ -14,8 +14,6 @@ class RpcInvocationImpl : public RpcInvocationBase { using ParameterValueMap = std::unordered_map; using ParameterValueMapPtr = std::unique_ptr; - using HeaderMapPtr = std::unique_ptr; - RpcInvocationImpl() = default; ~RpcInvocationImpl() override = default; @@ -32,7 +30,7 @@ class RpcInvocationImpl : public RpcInvocationBase { private: inline void assignHeaderIfNeed() { if (!headers_) { - headers_ = std::make_unique(); + headers_ = Http::RequestHeaderMapImpl::create(); } } @@ -43,7 +41,7 @@ class RpcInvocationImpl : public RpcInvocationBase { } ParameterValueMapPtr parameter_map_; - HeaderMapPtr headers_; // attachment + Http::HeaderMapPtr headers_; // attachment }; class RpcResultImpl : public RpcResult { diff --git a/source/extensions/filters/network/rocketmq_proxy/metadata.h b/source/extensions/filters/network/rocketmq_proxy/metadata.h index 8fca6ab7811a4..ed913a1f92e0c 100644 --- a/source/extensions/filters/network/rocketmq_proxy/metadata.h +++ b/source/extensions/filters/network/rocketmq_proxy/metadata.h @@ -25,14 +25,14 @@ class MessageMetadata { /** * @return HeaderMap of current headers */ - const Http::HeaderMap& headers() const { return headers_; } - Http::HeaderMap& headers() { return headers_; } + const Http::HeaderMap& headers() const { return *headers_; } + Http::HeaderMap& headers() { return *headers_; } private: bool is_oneway_{false}; absl::optional topic_name_{}; - Http::HeaderMapImpl headers_; + Http::HeaderMapPtr headers_{Http::RequestHeaderMapImpl::create()}; }; using MessageMetadataSharedPtr = std::shared_ptr; diff --git a/source/extensions/filters/network/thrift_proxy/metadata.h b/source/extensions/filters/network/thrift_proxy/metadata.h index 7ee3e68f297fd..525c9fb4ae2a3 100644 --- a/source/extensions/filters/network/thrift_proxy/metadata.h +++ b/source/extensions/filters/network/thrift_proxy/metadata.h @@ -54,8 +54,8 @@ class MessageMetadata { /** * @return HeaderMap of current headers (never throws) */ - const Http::HeaderMap& headers() const { return headers_; } - Http::HeaderMap& headers() { return headers_; } + const Http::HeaderMap& headers() const { return *headers_; } + Http::HeaderMap& headers() { return *headers_; } /** * @return SpanList an immutable list of Spans @@ -104,7 +104,7 @@ class MessageMetadata { absl::optional method_name_{}; absl::optional seq_id_{}; absl::optional msg_type_{}; - Http::HeaderMapImpl headers_; + Http::HeaderMapPtr headers_{Http::RequestHeaderMapImpl::create()}; absl::optional app_ex_type_; absl::optional app_ex_msg_; bool protocol_upgrade_message_{false}; diff --git a/source/extensions/quic_listeners/quiche/envoy_quic_utils.h b/source/extensions/quic_listeners/quiche/envoy_quic_utils.h index 3348a1096b5fb..eecaa9045d41c 100644 --- a/source/extensions/quic_listeners/quiche/envoy_quic_utils.h +++ b/source/extensions/quic_listeners/quiche/envoy_quic_utils.h @@ -38,7 +38,7 @@ quic::QuicSocketAddress envoyAddressInstanceToQuicSocketAddress( // The returned header map has all keys in lower case. template std::unique_ptr quicHeadersToEnvoyHeaders(const quic::QuicHeaderList& header_list) { - auto headers = std::make_unique(); + auto headers = T::create(); for (const auto& entry : header_list) { // TODO(danzh): Avoid copy by referencing entry as header_list is already validated by QUIC. headers->addCopy(Http::LowerCaseString(entry.first), entry.second); @@ -48,7 +48,7 @@ std::unique_ptr quicHeadersToEnvoyHeaders(const quic::QuicHeaderList& header_ template std::unique_ptr spdyHeaderBlockToEnvoyHeaders(const spdy::SpdyHeaderBlock& header_block) { - auto headers = std::make_unique(); + auto headers = T::create(); for (auto entry : header_block) { // TODO(danzh): Avoid temporary strings and addCopy() with std::string_view. std::string key(entry.first); diff --git a/source/extensions/stat_sinks/hystrix/hystrix.cc b/source/extensions/stat_sinks/hystrix/hystrix.cc index 0596dd4cda41f..c2317658a03bd 100644 --- a/source/extensions/stat_sinks/hystrix/hystrix.cc +++ b/source/extensions/stat_sinks/hystrix/hystrix.cc @@ -22,6 +22,11 @@ namespace Extensions { namespace StatSinks { namespace Hystrix { +Http::RegisterCustomInlineHeader + access_control_allow_origin(Http::Headers::get().AccessControlAllowOrigin); +Http::RegisterCustomInlineHeader + access_control_allow_headers(Http::Headers::get().AccessControlAllowHeaders); + const uint64_t HystrixSink::DEFAULT_NUM_BUCKETS; ClusterStatsCache::ClusterStatsCache(const std::string& cluster_name) : cluster_name_(cluster_name) {} @@ -290,10 +295,10 @@ Http::Code HystrixSink::handlerHystrixEventStream(absl::string_view, response_headers.setReferenceContentType(Http::Headers::get().ContentTypeValues.TextEventStream); response_headers.setReferenceCacheControl(Http::Headers::get().CacheControlValues.NoCache); response_headers.setReferenceConnection(Http::Headers::get().ConnectionValues.Close); - response_headers.setReferenceAccessControlAllowHeaders( - AccessControlAllowHeadersValue.AllowHeadersHystrix); - response_headers.setReferenceAccessControlAllowOrigin( - Http::Headers::get().AccessControlAllowOriginValue.All); + response_headers.setReferenceInline(access_control_allow_headers.handle(), + AccessControlAllowHeadersValue.AllowHeadersHystrix); + response_headers.setReferenceInline(access_control_allow_origin.handle(), + Http::Headers::get().AccessControlAllowOriginValue.All); Http::StreamDecoderFilterCallbacks& stream_decoder_filter_callbacks = admin_stream.getDecoderFilterCallbacks(); diff --git a/source/server/admin/admin.cc b/source/server/admin/admin.cc index 5be7a96653590..208d1caf7595b 100644 --- a/source/server/admin/admin.cc +++ b/source/server/admin/admin.cc @@ -811,9 +811,9 @@ Http::Code AdminImpl::request(absl::string_view path_and_query, absl::string_vie Http::ResponseHeaderMap& response_headers, std::string& body) { AdminFilter filter(createCallbackFunction()); - Http::RequestHeaderMapImpl request_headers; - request_headers.setMethod(method); - filter.decodeHeaders(request_headers, false); + auto request_headers = Http::RequestHeaderMapImpl::create(); + request_headers->setMethod(method); + filter.decodeHeaders(*request_headers, false); Buffer::OwnedImpl response; Http::Code code = runCallback(path_and_query, response_headers, response, filter); diff --git a/source/server/admin/admin_filter.cc b/source/server/admin/admin_filter.cc index 92ac92ad289b1..0cfb768393256 100644 --- a/source/server/admin/admin_filter.cc +++ b/source/server/admin/admin_filter.cc @@ -66,7 +66,7 @@ void AdminFilter::onComplete() { ENVOY_STREAM_LOG(debug, "request complete: path: {}", *decoder_callbacks_, path); Buffer::OwnedImpl response; - Http::ResponseHeaderMapPtr header_map{new Http::ResponseHeaderMapImpl}; + auto header_map = Http::ResponseHeaderMapImpl::create(); RELEASE_ASSERT(request_headers_, ""); Http::Code code = admin_server_callback_func_(path, *header_map, response, *this); Utility::populateFallbackResponseHeaders(code, *header_map); diff --git a/source/server/server.cc b/source/server/server.cc index 246aac405ffe7..255dd94783a87 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -309,6 +309,8 @@ void InstanceImpl::initialize(const Options& options, // setPrefix has a release assert verifying that setPrefix() is not called after prefix() ThreadSafeSingleton::get().setPrefix(bootstrap_.header_prefix().c_str()); } + // TODO(mattklein123): Custom O(1) headers can be registered at this point for creating/finalizing + // any header maps. // Needs to happen as early as possible in the instantiation to preempt the objects that require // stats. diff --git a/test/common/http/header_map_impl_fuzz_test.cc b/test/common/http/header_map_impl_fuzz_test.cc index bfd7507e05587..5ab9e79ca2ed0 100644 --- a/test/common/http/header_map_impl_fuzz_test.cc +++ b/test/common/http/header_map_impl_fuzz_test.cc @@ -16,7 +16,7 @@ namespace Envoy { // Fuzz the header map implementation. DEFINE_PROTO_FUZZER(const test::common::http::HeaderMapImplFuzzTestCase& input) { - auto header_map = std::make_unique(); + auto header_map = Http::RequestHeaderMapImpl::create(); std::vector> lower_case_strings; std::vector> strings; uint64_t set_integer; @@ -149,7 +149,7 @@ DEFINE_PROTO_FUZZER(const test::common::http::HeaderMapImplFuzzTestCase& input) break; } case test::common::http::Action::kCopy: { - header_map = Http::createHeaderMap(*header_map); + header_map = Http::createHeaderMap(*header_map); break; } case test::common::http::Action::kLookup: { diff --git a/test/common/http/header_map_impl_speed_test.cc b/test/common/http/header_map_impl_speed_test.cc index eb0f5e980a65c..b77336a37f65b 100644 --- a/test/common/http/header_map_impl_speed_test.cc +++ b/test/common/http/header_map_impl_speed_test.cc @@ -16,11 +16,13 @@ static void addDummyHeaders(HeaderMap& headers, size_t num_headers) { } } -/** Measure the construction/destruction speed of HeaderMapImpl.*/ +/** Measure the construction/destruction speed of RequestHeaderMapImpl.*/ static void HeaderMapImplCreate(benchmark::State& state) { + // Make sure first time construction is not counted. + Http::ResponseHeaderMapImpl::create(); for (auto _ : state) { - HeaderMapImpl headers; - benchmark::DoNotOptimize(headers.size()); + auto headers = Http::ResponseHeaderMapImpl::create(); + benchmark::DoNotOptimize(headers->size()); } } BENCHMARK(HeaderMapImplCreate); @@ -35,12 +37,12 @@ BENCHMARK(HeaderMapImplCreate); static void HeaderMapImplSetReference(benchmark::State& state) { const LowerCaseString key("example-key"); const std::string value("01234567890123456789"); - HeaderMapImpl headers; - addDummyHeaders(headers, state.range(0)); + auto headers = Http::ResponseHeaderMapImpl::create(); + addDummyHeaders(*headers, state.range(0)); for (auto _ : state) { - headers.setReference(key, value); + headers->setReference(key, value); } - benchmark::DoNotOptimize(headers.size()); + benchmark::DoNotOptimize(headers->size()); } BENCHMARK(HeaderMapImplSetReference)->Arg(0)->Arg(1)->Arg(10)->Arg(50); @@ -55,12 +57,12 @@ BENCHMARK(HeaderMapImplSetReference)->Arg(0)->Arg(1)->Arg(10)->Arg(50); static void HeaderMapImplGet(benchmark::State& state) { const LowerCaseString key("example-key"); const std::string value("01234567890123456789"); - HeaderMapImpl headers; - addDummyHeaders(headers, state.range(0)); - headers.setReference(key, value); + auto headers = Http::ResponseHeaderMapImpl::create(); + addDummyHeaders(*headers, state.range(0)); + headers->setReference(key, value); size_t successes = 0; for (auto _ : state) { - successes += (headers.get(key) != nullptr); + successes += (headers->get(key) != nullptr); } benchmark::DoNotOptimize(successes); } @@ -72,12 +74,12 @@ BENCHMARK(HeaderMapImplGet)->Arg(0)->Arg(1)->Arg(10)->Arg(50); */ static void HeaderMapImplGetInline(benchmark::State& state) { const std::string value("01234567890123456789"); - RequestHeaderMapImpl headers; - addDummyHeaders(headers, state.range(0)); - headers.setReferenceConnection(value); + auto headers = Http::ResponseHeaderMapImpl::create(); + addDummyHeaders(*headers, state.range(0)); + headers->setReferenceConnection(value); size_t size = 0; for (auto _ : state) { - size += headers.Connection()->value().size(); + size += headers->Connection()->value().size(); } benchmark::DoNotOptimize(size); } @@ -89,12 +91,12 @@ BENCHMARK(HeaderMapImplGetInline)->Arg(0)->Arg(1)->Arg(10)->Arg(50); */ static void HeaderMapImplSetInlineMacro(benchmark::State& state) { const std::string value("01234567890123456789"); - RequestHeaderMapImpl headers; - addDummyHeaders(headers, state.range(0)); + auto headers = Http::ResponseHeaderMapImpl::create(); + addDummyHeaders(*headers, state.range(0)); for (auto _ : state) { - headers.setReferenceConnection(value); + headers->setReferenceConnection(value); } - benchmark::DoNotOptimize(headers.size()); + benchmark::DoNotOptimize(headers->size()); } BENCHMARK(HeaderMapImplSetInlineMacro)->Arg(0)->Arg(1)->Arg(10)->Arg(50); @@ -104,22 +106,22 @@ BENCHMARK(HeaderMapImplSetInlineMacro)->Arg(0)->Arg(1)->Arg(10)->Arg(50); */ static void HeaderMapImplSetInlineInteger(benchmark::State& state) { uint64_t value = 12345; - RequestHeaderMapImpl headers; - addDummyHeaders(headers, state.range(0)); + auto headers = Http::ResponseHeaderMapImpl::create(); + addDummyHeaders(*headers, state.range(0)); for (auto _ : state) { - headers.setConnection(value); + headers->setConnection(value); } - benchmark::DoNotOptimize(headers.size()); + benchmark::DoNotOptimize(headers->size()); } BENCHMARK(HeaderMapImplSetInlineInteger)->Arg(0)->Arg(1)->Arg(10)->Arg(50); /** Measure the speed of the byteSize() estimation method. */ static void HeaderMapImplGetByteSize(benchmark::State& state) { - HeaderMapImpl headers; - addDummyHeaders(headers, state.range(0)); + auto headers = Http::ResponseHeaderMapImpl::create(); + addDummyHeaders(*headers, state.range(0)); uint64_t size = 0; for (auto _ : state) { - size += headers.byteSize(); + size += headers->byteSize(); } benchmark::DoNotOptimize(size); } @@ -127,15 +129,15 @@ BENCHMARK(HeaderMapImplGetByteSize)->Arg(0)->Arg(1)->Arg(10)->Arg(50); /** Measure the speed of iteration with a lightweight callback. */ static void HeaderMapImplIterate(benchmark::State& state) { - HeaderMapImpl headers; + auto headers = Http::ResponseHeaderMapImpl::create(); size_t num_callbacks = 0; - addDummyHeaders(headers, state.range(0)); + addDummyHeaders(*headers, state.range(0)); auto counting_callback = [](const HeaderEntry&, void* context) -> HeaderMap::Iterate { (*static_cast(context))++; return HeaderMap::Iterate::Continue; }; for (auto _ : state) { - headers.iterate(counting_callback, &num_callbacks); + headers->iterate(counting_callback, &num_callbacks); } benchmark::DoNotOptimize(num_callbacks); } @@ -145,12 +147,12 @@ BENCHMARK(HeaderMapImplIterate)->Arg(0)->Arg(1)->Arg(10)->Arg(50); static void HeaderMapImplLookup(benchmark::State& state) { const LowerCaseString key("connection"); const std::string value("01234567890123456789"); - HeaderMapImpl headers; - addDummyHeaders(headers, state.range(0)); - headers.addReference(key, value); + auto headers = Http::ResponseHeaderMapImpl::create(); + addDummyHeaders(*headers, state.range(0)); + headers->addReference(key, value); for (auto _ : state) { const HeaderEntry* entry = nullptr; - auto result = headers.lookup(key, &entry); + auto result = headers->lookup(key, &entry); benchmark::DoNotOptimize(result); } } @@ -164,13 +166,13 @@ BENCHMARK(HeaderMapImplLookup)->Arg(0)->Arg(1)->Arg(10)->Arg(50); static void HeaderMapImplRemove(benchmark::State& state) { const LowerCaseString key("example-key"); const std::string value("01234567890123456789"); - HeaderMapImpl headers; - addDummyHeaders(headers, state.range(0)); + auto headers = Http::ResponseHeaderMapImpl::create(); + addDummyHeaders(*headers, state.range(0)); for (auto _ : state) { - headers.addReference(key, value); - headers.remove(key); + headers->addReference(key, value); + headers->remove(key); } - benchmark::DoNotOptimize(headers.size()); + benchmark::DoNotOptimize(headers->size()); } BENCHMARK(HeaderMapImplRemove)->Arg(0)->Arg(1)->Arg(10)->Arg(50); @@ -183,13 +185,13 @@ BENCHMARK(HeaderMapImplRemove)->Arg(0)->Arg(1)->Arg(10)->Arg(50); static void HeaderMapImplRemoveInline(benchmark::State& state) { const LowerCaseString key("connection"); const std::string value("01234567890123456789"); - HeaderMapImpl headers; - addDummyHeaders(headers, state.range(0)); + auto headers = Http::ResponseHeaderMapImpl::create(); + addDummyHeaders(*headers, state.range(0)); for (auto _ : state) { - headers.addReference(key, value); - headers.remove(key); + headers->addReference(key, value); + headers->remove(key); } - benchmark::DoNotOptimize(headers.size()); + benchmark::DoNotOptimize(headers->size()); } BENCHMARK(HeaderMapImplRemoveInline)->Arg(0)->Arg(1)->Arg(10)->Arg(50); @@ -211,11 +213,11 @@ static void HeaderMapImplPopulate(benchmark::State& state) { {LowerCaseString("set-cookie"), "_cookie2=12345678; path = /; secure"}, }; for (auto _ : state) { - HeaderMapImpl headers; + auto headers = Http::ResponseHeaderMapImpl::create(); for (const auto& key_value : headers_to_add) { - headers.addReference(key_value.first, key_value.second); + headers->addReference(key_value.first, key_value.second); } - benchmark::DoNotOptimize(headers.size()); + benchmark::DoNotOptimize(headers->size()); } } BENCHMARK(HeaderMapImplPopulate); diff --git a/test/common/http/header_map_impl_test.cc b/test/common/http/header_map_impl_test.cc index 7095daf5ec420..b283e214a0ab8 100644 --- a/test/common/http/header_map_impl_test.cc +++ b/test/common/http/header_map_impl_test.cc @@ -355,30 +355,30 @@ TEST(HeaderStringTest, All) { } #define TEST_INLINE_HEADER_FUNCS(name) \ - header_map.addCopy(Headers::get().name, #name); \ - EXPECT_EQ(header_map.name()->value().getStringView(), #name); \ - header_map.remove##name(); \ - EXPECT_EQ(nullptr, header_map.name()); \ - header_map.set##name(#name); \ - EXPECT_EQ(header_map.get(Headers::get().name)->value().getStringView(), #name); + header_map->addCopy(Headers::get().name, #name); \ + EXPECT_EQ(header_map->name()->value().getStringView(), #name); \ + header_map->remove##name(); \ + EXPECT_EQ(nullptr, header_map->name()); \ + header_map->set##name(#name); \ + EXPECT_EQ(header_map->get(Headers::get().name)->value().getStringView(), #name); // Make sure that the O(1) headers are wired up properly. TEST(HeaderMapImplTest, AllInlineHeaders) { { - RequestHeaderMapImpl header_map; + auto header_map = RequestHeaderMapImpl::create(); INLINE_REQ_HEADERS(TEST_INLINE_HEADER_FUNCS) INLINE_REQ_RESP_HEADERS(TEST_INLINE_HEADER_FUNCS) } { // No request trailer O(1) headers. } { - ResponseHeaderMapImpl header_map; + auto header_map = ResponseHeaderMapImpl::create(); INLINE_RESP_HEADERS(TEST_INLINE_HEADER_FUNCS) INLINE_REQ_RESP_HEADERS(TEST_INLINE_HEADER_FUNCS) INLINE_RESP_HEADERS_TRAILERS(TEST_INLINE_HEADER_FUNCS) } { - ResponseTrailerMapImpl header_map; + auto header_map = ResponseTrailerMapImpl::create(); INLINE_RESP_HEADERS_TRAILERS(TEST_INLINE_HEADER_FUNCS) } } @@ -545,7 +545,7 @@ TEST(HeaderMapImplTest, RemoveRegex) { } TEST(HeaderMapImplTest, SetRemovesAllValues) { - TestHeaderMapImpl headers; + TestRequestHeaderMapImpl headers; LowerCaseString key1("hello"); LowerCaseString key2("olleh"); @@ -636,7 +636,7 @@ TEST(HeaderMapImplTest, DoubleInlineAdd) { // Per https://github.com/envoyproxy/envoy/issues/7488 make sure we don't // combine set-cookie headers TEST(HeaderMapImplTest, DoubleCookieAdd) { - TestHeaderMapImpl headers; + TestRequestHeaderMapImpl headers; const std::string foo("foo"); const std::string bar("bar"); const LowerCaseString& set_cookie = Http::Headers::get().SetCookie; @@ -660,7 +660,7 @@ TEST(HeaderMapImplTest, DoubleInlineSet) { } TEST(HeaderMapImplTest, AddReferenceKey) { - TestHeaderMapImpl headers; + TestRequestHeaderMapImpl headers; LowerCaseString foo("hello"); headers.addReferenceKey(foo, "world"); EXPECT_NE("world", headers.get(foo)->value().getStringView().data()); @@ -668,7 +668,7 @@ TEST(HeaderMapImplTest, AddReferenceKey) { } TEST(HeaderMapImplTest, SetReferenceKey) { - TestHeaderMapImpl headers; + TestRequestHeaderMapImpl headers; LowerCaseString foo("hello"); headers.setReferenceKey(foo, "world"); EXPECT_NE("world", headers.get(foo)->value().getStringView().data()); @@ -803,8 +803,8 @@ TEST(HeaderMapImplTest, AddCopy) { } TEST(HeaderMapImplTest, Equality) { - TestHeaderMapImpl headers1; - TestHeaderMapImpl headers2; + TestRequestHeaderMapImpl headers1; + TestRequestHeaderMapImpl headers2; EXPECT_EQ(headers1, headers2); headers1.addCopy(LowerCaseString("hello"), "world"); @@ -815,7 +815,7 @@ TEST(HeaderMapImplTest, Equality) { } TEST(HeaderMapImplTest, LargeCharInHeader) { - TestHeaderMapImpl headers; + TestRequestHeaderMapImpl headers; LowerCaseString static_key("\x90hello"); std::string ref_value("value"); headers.addReference(static_key, ref_value); @@ -823,7 +823,7 @@ TEST(HeaderMapImplTest, LargeCharInHeader) { } TEST(HeaderMapImplTest, Iterate) { - TestHeaderMapImpl headers; + TestRequestHeaderMapImpl headers; headers.addCopy(LowerCaseString("hello"), "world"); headers.addCopy(LowerCaseString("foo"), "xxx"); headers.addCopy(LowerCaseString("world"), "hello"); @@ -847,7 +847,7 @@ TEST(HeaderMapImplTest, Iterate) { } TEST(HeaderMapImplTest, IterateReverse) { - TestHeaderMapImpl headers; + TestRequestHeaderMapImpl headers; headers.addCopy(LowerCaseString("hello"), "world"); headers.addCopy(LowerCaseString("foo"), "bar"); LowerCaseString world_key("world"); @@ -902,31 +902,31 @@ TEST(HeaderMapImplTest, Lookup) { TEST(HeaderMapImplTest, Get) { { - auto headers = createHeaderMap( - {{Headers::get().Path, "/"}, {LowerCaseString("hello"), "world"}}); - EXPECT_EQ("/", headers->get(LowerCaseString(":path"))->value().getStringView()); - EXPECT_EQ("world", headers->get(LowerCaseString("hello"))->value().getStringView()); - EXPECT_EQ(nullptr, headers->get(LowerCaseString("foo"))); + auto headers = + TestRequestHeaderMapImpl({{Headers::get().Path, "/"}, {LowerCaseString("hello"), "world"}}); + EXPECT_EQ("/", headers.get(LowerCaseString(":path"))->value().getStringView()); + EXPECT_EQ("world", headers.get(LowerCaseString("hello"))->value().getStringView()); + EXPECT_EQ(nullptr, headers.get(LowerCaseString("foo"))); } { - auto headers = createHeaderMap( - {{Headers::get().Path, "/"}, {LowerCaseString("hello"), "world"}}); + auto headers = + TestRequestHeaderMapImpl({{Headers::get().Path, "/"}, {LowerCaseString("hello"), "world"}}); // There is not HeaderMap method to set a header and copy both the key and value. const LowerCaseString path(":path"); - headers->setReferenceKey(path, "/new_path"); - EXPECT_EQ("/new_path", headers->get(LowerCaseString(":path"))->value().getStringView()); + headers.setReferenceKey(path, "/new_path"); + EXPECT_EQ("/new_path", headers.get(LowerCaseString(":path"))->value().getStringView()); const LowerCaseString foo("hello"); - headers->setReferenceKey(foo, "world2"); - EXPECT_EQ("world2", headers->get(foo)->value().getStringView()); - EXPECT_EQ(nullptr, headers->get(LowerCaseString("foo"))); + headers.setReferenceKey(foo, "world2"); + EXPECT_EQ("world2", headers.get(foo)->value().getStringView()); + EXPECT_EQ(nullptr, headers.get(LowerCaseString("foo"))); } } TEST(HeaderMapImplTest, CreateHeaderMapFromIterator) { std::vector> iter_headers{ {LowerCaseString(Headers::get().Path), "/"}, {LowerCaseString("hello"), "world"}}; - auto headers = createHeaderMap(iter_headers.cbegin(), iter_headers.cend()); + auto headers = createHeaderMap(iter_headers.cbegin(), iter_headers.cend()); EXPECT_EQ("/", headers->get(LowerCaseString(":path"))->value().getStringView()); EXPECT_EQ("world", headers->get(LowerCaseString("hello"))->value().getStringView()); EXPECT_EQ(nullptr, headers->get(LowerCaseString("foo"))); @@ -936,8 +936,8 @@ TEST(HeaderMapImplTest, TestHeaderList) { std::array keys{Headers::get().Path, LowerCaseString("hello")}; std::array values{"/", "world"}; - auto headers = createHeaderMap({{keys[0], values[0]}, {keys[1], values[1]}}); - HeaderListView header_list(headers->header_map_); + auto headers = TestRequestHeaderMapImpl({{keys[0], values[0]}, {keys[1], values[1]}}); + HeaderListView header_list(headers); auto to_string_views = [](const HeaderListView::HeaderStringRefs& strs) -> std::vector { std::vector str_views(strs.size()); @@ -953,7 +953,7 @@ TEST(HeaderMapImplTest, TestHeaderList) { TEST(HeaderMapImplTest, TestAppendHeader) { // Test appending to a string with a value. { - TestHeaderMapImpl headers; + TestRequestHeaderMapImpl headers; LowerCaseString foo("key1"); headers.addCopy(foo, "some;"); headers.appendCopy(foo, "test"); @@ -962,7 +962,7 @@ TEST(HeaderMapImplTest, TestAppendHeader) { // Test appending to an empty string. { - TestHeaderMapImpl headers; + TestRequestHeaderMapImpl headers; LowerCaseString key2("key2"); headers.appendCopy(key2, "my tag data"); EXPECT_EQ(headers.get(key2)->value().getStringView(), "my tag data"); @@ -970,7 +970,7 @@ TEST(HeaderMapImplTest, TestAppendHeader) { // Test empty data case. { - TestHeaderMapImpl headers; + TestRequestHeaderMapImpl headers; LowerCaseString key3("key3"); headers.addCopy(key3, "empty"); headers.appendCopy(key3, ""); @@ -1019,7 +1019,7 @@ TEST(HeaderMapImplTest, PseudoHeaderOrder) { { LowerCaseString foo("hello"); - Http::TestHeaderMapImpl headers{}; + Http::TestRequestHeaderMapImpl headers{}; EXPECT_EQ(0UL, headers.size()); EXPECT_TRUE(headers.empty()); @@ -1173,11 +1173,11 @@ TEST(HeaderMapImplTest, PseudoHeaderOrder) { // Starting with a normal header { - auto headers = createHeaderMap({{Headers::get().ContentType, "text/plain"}, - {Headers::get().Method, "GET"}, - {Headers::get().Path, "/"}, - {LowerCaseString("hello"), "world"}, - {Headers::get().Host, "host"}}); + auto headers = TestRequestHeaderMapImpl({{Headers::get().ContentType, "text/plain"}, + {Headers::get().Method, "GET"}, + {Headers::get().Path, "/"}, + {LowerCaseString("hello"), "world"}, + {Headers::get().Host, "host"}}); InSequence seq; EXPECT_CALL(cb, Call(":method", "GET")); @@ -1186,7 +1186,7 @@ TEST(HeaderMapImplTest, PseudoHeaderOrder) { EXPECT_CALL(cb, Call("content-type", "text/plain")); EXPECT_CALL(cb, Call("hello", "world")); - headers->iterate( + headers.iterate( [](const Http::HeaderEntry& header, void* cb_v) -> HeaderMap::Iterate { static_cast(cb_v)->Call(std::string(header.key().getStringView()), std::string(header.value().getStringView())); @@ -1197,11 +1197,11 @@ TEST(HeaderMapImplTest, PseudoHeaderOrder) { // Starting with a pseudo-header { - auto headers = createHeaderMap({{Headers::get().Path, "/"}, - {Headers::get().ContentType, "text/plain"}, - {Headers::get().Method, "GET"}, - {LowerCaseString("hello"), "world"}, - {Headers::get().Host, "host"}}); + auto headers = TestRequestHeaderMapImpl({{Headers::get().Path, "/"}, + {Headers::get().ContentType, "text/plain"}, + {Headers::get().Method, "GET"}, + {LowerCaseString("hello"), "world"}, + {Headers::get().Host, "host"}}); InSequence seq; EXPECT_CALL(cb, Call(":path", "/")); @@ -1210,7 +1210,7 @@ TEST(HeaderMapImplTest, PseudoHeaderOrder) { EXPECT_CALL(cb, Call("content-type", "text/plain")); EXPECT_CALL(cb, Call("hello", "world")); - headers->iterate( + headers.iterate( [](const Http::HeaderEntry& header, void* cb_v) -> HeaderMap::Iterate { static_cast(cb_v)->Call(std::string(header.key().getStringView()), std::string(header.value().getStringView())); @@ -1220,18 +1220,18 @@ TEST(HeaderMapImplTest, PseudoHeaderOrder) { } } -// Validate that TestHeaderMapImpl copy construction and assignment works. This is a +// Validate that TestRequestHeaderMapImpl copy construction and assignment works. This is a // regression for where we were missing a valid copy constructor and had the // default (dangerous) move semantics takeover. -TEST(HeaderMapImplTest, TestHeaderMapImplyCopy) { - TestHeaderMapImpl foo; +TEST(HeaderMapImplTest, TestRequestHeaderMapImplyCopy) { + TestRequestHeaderMapImpl foo; foo.addCopy(LowerCaseString("foo"), "bar"); - auto headers = std::make_unique(foo); + auto headers = std::make_unique(foo); EXPECT_EQ("bar", headers->get(LowerCaseString("foo"))->value().getStringView()); - TestHeaderMapImpl baz{{"foo", "baz"}}; + TestRequestHeaderMapImpl baz{{"foo", "baz"}}; baz = *headers; EXPECT_EQ("bar", baz.get(LowerCaseString("foo"))->value().getStringView()); - const TestHeaderMapImpl& baz2 = baz; + const TestRequestHeaderMapImpl& baz2 = baz; baz = baz2; EXPECT_EQ("bar", baz.get(LowerCaseString("foo"))->value().getStringView()); } diff --git a/test/common/http/utility_fuzz_test.cc b/test/common/http/utility_fuzz_test.cc index e81c10e4ae97d..e3524fde27e46 100644 --- a/test/common/http/utility_fuzz_test.cc +++ b/test/common/http/utility_fuzz_test.cc @@ -23,22 +23,22 @@ DEFINE_PROTO_FUZZER(const test::common::http::UtilityTestCase& input) { } case test::common::http::UtilityTestCase::kParseCookieValue: { const auto& parse_cookie_value = input.parse_cookie_value(); - // Use the production HeaderMapImpl to avoid timeouts from TestHeaderMapImpl asserts. - Http::HeaderMapImpl headers; + // Use the production RequestHeaderMapImpl to avoid timeouts from TestHeaderMapImpl asserts. + auto headers = Http::RequestHeaderMapImpl::create(); for (const std::string& cookie : parse_cookie_value.cookies()) { - headers.addCopy(Http::LowerCaseString("cookie"), replaceInvalidCharacters(cookie)); + headers->addCopy(Http::LowerCaseString("cookie"), replaceInvalidCharacters(cookie)); } - Http::Utility::parseCookieValue(headers, parse_cookie_value.key()); + Http::Utility::parseCookieValue(*headers, parse_cookie_value.key()); break; } case test::common::http::UtilityTestCase::kGetLastAddressFromXff: { const auto& get_last_address_from_xff = input.get_last_address_from_xff(); - // Use the production HeaderMapImpl to avoid timeouts from TestHeaderMapImpl asserts. - Http::RequestHeaderMapImpl headers; - headers.addCopy(Http::LowerCaseString("x-forwarded-for"), - replaceInvalidCharacters(get_last_address_from_xff.xff())); + // Use the production RequestHeaderMapImpl to avoid timeouts from TestHeaderMapImpl asserts. + auto headers = Http::RequestHeaderMapImpl::create(); + headers->addCopy(Http::LowerCaseString("x-forwarded-for"), + replaceInvalidCharacters(get_last_address_from_xff.xff())); // Take num_to_skip modulo 32 to avoid wasting time in lala land. - Http::Utility::getLastAddressFromXFF(headers, get_last_address_from_xff.num_to_skip() % 32); + Http::Utility::getLastAddressFromXFF(*headers, get_last_address_from_xff.num_to_skip() % 32); break; } case test::common::http::UtilityTestCase::kExtractHostPathFromUri: { diff --git a/test/extensions/filters/http/on_demand/on_demand_filter_test.cc b/test/extensions/filters/http/on_demand/on_demand_filter_test.cc index 1724898c05b89..d226ee4dcec60 100644 --- a/test/extensions/filters/http/on_demand/on_demand_filter_test.cc +++ b/test/extensions/filters/http/on_demand/on_demand_filter_test.cc @@ -56,7 +56,7 @@ TEST_F(OnDemandFilterTest, TestDecodeHeadersWhenRouteConfigIsNotAvailable) { } TEST_F(OnDemandFilterTest, TestDecodeTrailers) { - Http::RequestTrailerMapImpl headers; + Http::TestRequestTrailerMapImpl headers; EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_->decodeTrailers(headers)); } diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index 3bbcf5c9ce8a9..28e07f427443f 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -1491,9 +1491,9 @@ TEST_P(DownstreamProtocolIntegrationTest, ManyTrailerHeaders) { max_request_headers_count_); }); - Http::RequestTrailerMapImpl request_trailers; + auto request_trailers = Http::RequestTrailerMapImpl::create(); for (int i = 0; i < 20000; i++) { - request_trailers.addCopy(Http::LowerCaseString(std::to_string(i)), ""); + request_trailers->addCopy(Http::LowerCaseString(std::to_string(i)), ""); } initialize(); @@ -1505,7 +1505,7 @@ TEST_P(DownstreamProtocolIntegrationTest, ManyTrailerHeaders) { {":authority", "host"}}); request_encoder_ = &encoder_decoder.first; auto response = std::move(encoder_decoder.second); - codec_client_->sendTrailers(*request_encoder_, request_trailers); + codec_client_->sendTrailers(*request_encoder_, *request_trailers); waitForNextUpstreamRequest(); upstream_request_->encodeHeaders(default_response_headers_, true); response->waitForEndStream(); diff --git a/test/test_common/utility.h b/test/test_common/utility.h index e66100184743d..60b71b0cc5c43 100644 --- a/test/test_common/utility.h +++ b/test/test_common/utility.h @@ -633,28 +633,29 @@ namespace Http { */ #define DEFINE_TEST_INLINE_HEADER_FUNCS(name) \ public: \ - const HeaderEntry* name() const override { return header_map_.name(); } \ + const HeaderEntry* name() const override { return header_map_->name(); } \ void append##name(absl::string_view data, absl::string_view delimiter) override { \ - header_map_.append##name(data, delimiter); \ - header_map_.verifyByteSizeInternalForTest(); \ + header_map_->append##name(data, delimiter); \ + header_map_->verifyByteSizeInternalForTest(); \ } \ void setReference##name(absl::string_view value) override { \ - header_map_.setReference##name(value); \ - header_map_.verifyByteSizeInternalForTest(); \ + header_map_->setReference##name(value); \ + header_map_->verifyByteSizeInternalForTest(); \ } \ void set##name(absl::string_view value) override { \ - header_map_.set##name(value); \ - header_map_.verifyByteSizeInternalForTest(); \ + header_map_->set##name(value); \ + header_map_->verifyByteSizeInternalForTest(); \ } \ void set##name(uint64_t value) override { \ - header_map_.set##name(value); \ - header_map_.verifyByteSizeInternalForTest(); \ + header_map_->set##name(value); \ + header_map_->verifyByteSizeInternalForTest(); \ } \ size_t remove##name() override { \ - size_t headers_removed = header_map_.remove##name(); \ - header_map_.verifyByteSizeInternalForTest(); \ + const size_t headers_removed = header_map_->remove##name(); \ + header_map_->verifyByteSizeInternalForTest(); \ return headers_removed; \ - } + } \ + absl::string_view get##name##Value() const override { return header_map_->get##name##Value(); } /** * Base class for all test header map types. This class wraps an underlying real header map @@ -668,23 +669,30 @@ template class TestHeaderMapImplBase : public Inte TestHeaderMapImplBase() = default; TestHeaderMapImplBase(const std::initializer_list>& values) { for (auto& value : values) { - header_map_.addCopy(LowerCaseString(value.first), value.second); + header_map_->addCopy(LowerCaseString(value.first), value.second); } - header_map_.verifyByteSizeInternalForTest(); + header_map_->verifyByteSizeInternalForTest(); + } + TestHeaderMapImplBase( + const std::initializer_list>& values) { + for (auto& value : values) { + header_map_->addCopy(value.first, value.second); + } + header_map_->verifyByteSizeInternalForTest(); } TestHeaderMapImplBase(const TestHeaderMapImplBase& rhs) - : TestHeaderMapImplBase(rhs.header_map_) {} + : TestHeaderMapImplBase(*rhs.header_map_) {} TestHeaderMapImplBase(const HeaderMap& rhs) { - HeaderMapImpl::copyFrom(header_map_, rhs); - header_map_.verifyByteSizeInternalForTest(); + HeaderMapImpl::copyFrom(*header_map_, rhs); + header_map_->verifyByteSizeInternalForTest(); } TestHeaderMapImplBase& operator=(const TestHeaderMapImplBase& rhs) { if (this == &rhs) { return *this; } clear(); - HeaderMapImpl::copyFrom(header_map_, rhs); - header_map_.verifyByteSizeInternalForTest(); + HeaderMapImpl::copyFrom(*header_map_, rhs); + header_map_->verifyByteSizeInternalForTest(); return *this; } @@ -706,86 +714,112 @@ template class TestHeaderMapImplBase : public Inte size_t remove(const std::string& key) { return remove(LowerCaseString(key)); } // HeaderMap - bool operator==(const HeaderMap& rhs) const override { return header_map_.operator==(rhs); } - bool operator!=(const HeaderMap& rhs) const override { return header_map_.operator!=(rhs); } + bool operator==(const HeaderMap& rhs) const override { return header_map_->operator==(rhs); } + bool operator!=(const HeaderMap& rhs) const override { return header_map_->operator!=(rhs); } void addViaMove(HeaderString&& key, HeaderString&& value) override { - header_map_.addViaMove(std::move(key), std::move(value)); - header_map_.verifyByteSizeInternalForTest(); + header_map_->addViaMove(std::move(key), std::move(value)); + header_map_->verifyByteSizeInternalForTest(); } void addReference(const LowerCaseString& key, absl::string_view value) override { - header_map_.addReference(key, value); - header_map_.verifyByteSizeInternalForTest(); + header_map_->addReference(key, value); + header_map_->verifyByteSizeInternalForTest(); } void addReferenceKey(const LowerCaseString& key, uint64_t value) override { - header_map_.addReferenceKey(key, value); - header_map_.verifyByteSizeInternalForTest(); + header_map_->addReferenceKey(key, value); + header_map_->verifyByteSizeInternalForTest(); } void addReferenceKey(const LowerCaseString& key, absl::string_view value) override { - header_map_.addReferenceKey(key, value); - header_map_.verifyByteSizeInternalForTest(); + header_map_->addReferenceKey(key, value); + header_map_->verifyByteSizeInternalForTest(); } void addCopy(const LowerCaseString& key, uint64_t value) override { - header_map_.addCopy(key, value); - header_map_.verifyByteSizeInternalForTest(); + header_map_->addCopy(key, value); + header_map_->verifyByteSizeInternalForTest(); } void addCopy(const LowerCaseString& key, absl::string_view value) override { - header_map_.addCopy(key, value); - header_map_.verifyByteSizeInternalForTest(); + header_map_->addCopy(key, value); + header_map_->verifyByteSizeInternalForTest(); } void appendCopy(const LowerCaseString& key, absl::string_view value) override { - header_map_.appendCopy(key, value); - header_map_.verifyByteSizeInternalForTest(); + header_map_->appendCopy(key, value); + header_map_->verifyByteSizeInternalForTest(); } void setReference(const LowerCaseString& key, absl::string_view value) override { - header_map_.setReference(key, value); - header_map_.verifyByteSizeInternalForTest(); + header_map_->setReference(key, value); + header_map_->verifyByteSizeInternalForTest(); } void setReferenceKey(const LowerCaseString& key, absl::string_view value) override { - header_map_.setReferenceKey(key, value); + header_map_->setReferenceKey(key, value); } void setCopy(const LowerCaseString& key, absl::string_view value) override { - header_map_.setCopy(key, value); - header_map_.verifyByteSizeInternalForTest(); + header_map_->setCopy(key, value); + header_map_->verifyByteSizeInternalForTest(); + } + uint64_t byteSize() const override { return header_map_->byteSize(); } + const HeaderEntry* get(const LowerCaseString& key) const override { + return header_map_->get(key); } - uint64_t byteSize() const override { return header_map_.byteSize(); } - const HeaderEntry* get(const LowerCaseString& key) const override { return header_map_.get(key); } void iterate(HeaderMap::ConstIterateCb cb, void* context) const override { - header_map_.iterate(cb, context); + header_map_->iterate(cb, context); } void iterateReverse(HeaderMap::ConstIterateCb cb, void* context) const override { - header_map_.iterateReverse(cb, context); + header_map_->iterateReverse(cb, context); } HeaderMap::Lookup lookup(const LowerCaseString& key, const HeaderEntry** entry) const override { - return header_map_.lookup(key, entry); + return header_map_->lookup(key, entry); } void clear() override { - header_map_.clear(); - header_map_.verifyByteSizeInternalForTest(); + header_map_->clear(); + header_map_->verifyByteSizeInternalForTest(); } size_t remove(const LowerCaseString& key) override { - size_t headers_removed = header_map_.remove(key); - header_map_.verifyByteSizeInternalForTest(); + size_t headers_removed = header_map_->remove(key); + header_map_->verifyByteSizeInternalForTest(); return headers_removed; } size_t removePrefix(const LowerCaseString& key) override { - size_t headers_removed = header_map_.removePrefix(key); - header_map_.verifyByteSizeInternalForTest(); + size_t headers_removed = header_map_->removePrefix(key); + header_map_->verifyByteSizeInternalForTest(); return headers_removed; } - size_t size() const override { return header_map_.size(); } - bool empty() const override { return header_map_.empty(); } + size_t size() const override { return header_map_->size(); } + bool empty() const override { return header_map_->empty(); } void dumpState(std::ostream& os, int indent_level = 0) const override { - header_map_.dumpState(os, indent_level); + header_map_->dumpState(os, indent_level); + } + + using Handle = typename CustomInlineHeaderRegistry::Handle; + const HeaderEntry* getInline(Handle handle) const override { + return header_map_->getInline(handle); + } + void appendInline(Handle handle, absl::string_view data, absl::string_view delimiter) override { + header_map_->appendInline(handle, data, delimiter); + header_map_->verifyByteSizeInternalForTest(); + } + void setReferenceInline(Handle handle, absl::string_view value) override { + header_map_->setReferenceInline(handle, value); + header_map_->verifyByteSizeInternalForTest(); + } + void setInline(Handle handle, absl::string_view value) override { + header_map_->setInline(handle, value); + header_map_->verifyByteSizeInternalForTest(); + } + void setInline(Handle handle, uint64_t value) override { + header_map_->setInline(handle, value); + header_map_->verifyByteSizeInternalForTest(); + } + size_t removeInline(Handle handle) override { + const size_t rc = header_map_->removeInline(handle); + header_map_->verifyByteSizeInternalForTest(); + return rc; } - Impl header_map_; + std::unique_ptr header_map_{Impl::create()}; }; /** * Typed test implementations for all of the concrete header types. */ -using TestHeaderMapImpl = TestHeaderMapImplBase; - class TestRequestHeaderMapImpl : public TestHeaderMapImplBase { public: From f25b4f497a3033a467b1fec47ac84f2999affd0f Mon Sep 17 00:00:00 2001 From: Matt Klein Date: Mon, 15 Jun 2020 19:39:19 +0000 Subject: [PATCH 2/3] comments Signed-off-by: Matt Klein --- source/common/grpc/async_client_impl.cc | 4 ++ source/common/http/header_map_impl.cc | 29 +++++++++++ source/common/http/header_map_impl.h | 26 +++++++--- source/server/server.cc | 5 ++ .../common/http/header_map_impl_speed_test.cc | 48 +++++++++---------- 5 files changed, 82 insertions(+), 30 deletions(-) diff --git a/source/common/grpc/async_client_impl.cc b/source/common/grpc/async_client_impl.cc index d67f1ded4771c..ecb5709288f71 100644 --- a/source/common/grpc/async_client_impl.cc +++ b/source/common/grpc/async_client_impl.cc @@ -108,6 +108,10 @@ void AsyncStreamImpl::onHeaders(Http::ResponseHeaderMapPtr&& headers, bool end_s if (end_stream && grpc_status) { // Due to headers/trailers type differences we need to copy here. This is an uncommon case but // we can potentially optimize in the future. + + // TODO(mattklein123): clang-tidy is showing a use after move when passing to + // onReceiveInitialMetadata() above. This looks like an actual bug that I will fix in a + // follow up. onTrailers(Http::createHeaderMap(*headers)); return; } diff --git a/source/common/http/header_map_impl.cc b/source/common/http/header_map_impl.cc index 3bb6d1379c5c9..3935818f9bf54 100644 --- a/source/common/http/header_map_impl.cc +++ b/source/common/http/header_map_impl.cc @@ -5,6 +5,8 @@ #include #include +#include "envoy/http/header_map.h" + #include "common/common/assert.h" #include "common/common/dump_state_utils.h" #include "common/common/empty_string.h" @@ -620,5 +622,32 @@ size_t HeaderMapImpl::removeInline(HeaderEntryImpl** ptr_to_entry) { return 1; } +namespace { +template +HeaderMapImplUtility::HeaderMapImplInfo makeHeaderMapImplInfo(const std::string& name) { + // Constructing a header map implementation will force the custom headers and sizing to be + // finalized, so do that first. + auto header_map = T::create(); + + HeaderMapImplUtility::HeaderMapImplInfo info; + info.name_ = name; + info.size_ = T::inlineHeadersSize() + sizeof(T); + for (const auto& header : CustomInlineHeaderRegistry::headers()) { + info.registered_headers_.push_back(header.first.get()); + } + return info; +} +} // namespace + +std::vector +HeaderMapImplUtility::getAllHeaderMapImplInfo() { + std::vector ret; + ret.push_back(makeHeaderMapImplInfo("request header map")); + ret.push_back(makeHeaderMapImplInfo("request trailer map")); + ret.push_back(makeHeaderMapImplInfo("response header map")); + ret.push_back(makeHeaderMapImplInfo("response trailer map")); + return ret; +} + } // namespace Http } // namespace Envoy diff --git a/source/common/http/header_map_impl.h b/source/common/http/header_map_impl.h index d279419ece2c3..dc6627268a719 100644 --- a/source/common/http/header_map_impl.h +++ b/source/common/http/header_map_impl.h @@ -39,10 +39,10 @@ public: /** * Implementation of Http::HeaderMap. This is heavily optimized for performance. Roughly, when - * headers are added to the map, we do a trie lookup to see if it's one of the O(1) headers. - * If it is, we store a reference to it that can be accessed later directly. Most high performance - * paths use O(1) direct access. In general, we try to copy as little as possible and allocate as - * little as possible in any of the paths. + * headers are added to the map by string, we do a trie lookup to see if it's one of the O(1) + * headers. If it is, we store a reference to it that can be accessed later directly via direct + * method access. Most high performance paths use O(1) direct method access. In general, we try to + * copy as little as possible and allocate as little as possible in any of the paths. */ class HeaderMapImpl : NonCopyable { public: @@ -351,11 +351,11 @@ template class TypedHeaderMapImpl : public HeaderMapImpl, publ ASSERT(handle.it_->second < inlineHeadersSize()); return HeaderMapImpl::removeInline(&inlineHeaders()[handle.it_->second]); } - -protected: static size_t inlineHeadersSize() { return StaticLookupTable::size() * sizeof(HeaderEntryImpl*); } + +protected: absl::optional staticLookup(absl::string_view key) override { return StaticLookupTable::lookup(*this, key); } @@ -512,5 +512,19 @@ struct EmptyHeaders { using StaticEmptyHeaders = ConstSingleton; +class HeaderMapImplUtility { +public: + struct HeaderMapImplInfo { + std::string name_; + size_t size_; + std::vector registered_headers_; + }; + + /** + * Fetch detailed information about each header map implementation for use in logging. + */ + static std::vector getAllHeaderMapImplInfo(); +}; + } // namespace Http } // namespace Envoy diff --git a/source/server/server.cc b/source/server/server.cc index 255dd94783a87..678aaa4ec067a 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -311,6 +311,11 @@ void InstanceImpl::initialize(const Options& options, } // TODO(mattklein123): Custom O(1) headers can be registered at this point for creating/finalizing // any header maps. + ENVOY_LOG(info, "HTTP header map info:"); + for (const auto& info : Http::HeaderMapImplUtility::getAllHeaderMapImplInfo()) { + ENVOY_LOG(info, " {}: {} bytes: {}", info.name_, info.size_, + absl::StrJoin(info.registered_headers_, ",")); + } // Needs to happen as early as possible in the instantiation to preempt the objects that require // stats. diff --git a/test/common/http/header_map_impl_speed_test.cc b/test/common/http/header_map_impl_speed_test.cc index b77336a37f65b..1c65c3a19a13d 100644 --- a/test/common/http/header_map_impl_speed_test.cc +++ b/test/common/http/header_map_impl_speed_test.cc @@ -17,7 +17,7 @@ static void addDummyHeaders(HeaderMap& headers, size_t num_headers) { } /** Measure the construction/destruction speed of RequestHeaderMapImpl.*/ -static void HeaderMapImplCreate(benchmark::State& state) { +static void headerMapImplCreate(benchmark::State& state) { // Make sure first time construction is not counted. Http::ResponseHeaderMapImpl::create(); for (auto _ : state) { @@ -25,7 +25,7 @@ static void HeaderMapImplCreate(benchmark::State& state) { benchmark::DoNotOptimize(headers->size()); } } -BENCHMARK(HeaderMapImplCreate); +BENCHMARK(headerMapImplCreate); /** * Measure the speed of setting/overwriting a header value. The numeric Arg passed @@ -34,7 +34,7 @@ BENCHMARK(HeaderMapImplCreate); * identify whether the speed of setReference() is dependent on the number of other * headers in the HeaderMapImpl. */ -static void HeaderMapImplSetReference(benchmark::State& state) { +static void headerMapImplSetReference(benchmark::State& state) { const LowerCaseString key("example-key"); const std::string value("01234567890123456789"); auto headers = Http::ResponseHeaderMapImpl::create(); @@ -44,7 +44,7 @@ static void HeaderMapImplSetReference(benchmark::State& state) { } benchmark::DoNotOptimize(headers->size()); } -BENCHMARK(HeaderMapImplSetReference)->Arg(0)->Arg(1)->Arg(10)->Arg(50); +BENCHMARK(headerMapImplSetReference)->Arg(0)->Arg(1)->Arg(10)->Arg(50); /** * Measure the speed of retrieving a header value. The numeric Arg passed by the @@ -54,7 +54,7 @@ BENCHMARK(HeaderMapImplSetReference)->Arg(0)->Arg(1)->Arg(10)->Arg(50); * method depends (or doesn't depend) on the number of other headers in the * HeaderMapImpl. */ -static void HeaderMapImplGet(benchmark::State& state) { +static void headerMapImplGet(benchmark::State& state) { const LowerCaseString key("example-key"); const std::string value("01234567890123456789"); auto headers = Http::ResponseHeaderMapImpl::create(); @@ -66,13 +66,13 @@ static void HeaderMapImplGet(benchmark::State& state) { } benchmark::DoNotOptimize(successes); } -BENCHMARK(HeaderMapImplGet)->Arg(0)->Arg(1)->Arg(10)->Arg(50); +BENCHMARK(headerMapImplGet)->Arg(0)->Arg(1)->Arg(10)->Arg(50); /** * Measure the retrieval speed of a header for which HeaderMapImpl is expected to * provide special optimizations. */ -static void HeaderMapImplGetInline(benchmark::State& state) { +static void headerMapImplGetInline(benchmark::State& state) { const std::string value("01234567890123456789"); auto headers = Http::ResponseHeaderMapImpl::create(); addDummyHeaders(*headers, state.range(0)); @@ -83,13 +83,13 @@ static void HeaderMapImplGetInline(benchmark::State& state) { } benchmark::DoNotOptimize(size); } -BENCHMARK(HeaderMapImplGetInline)->Arg(0)->Arg(1)->Arg(10)->Arg(50); +BENCHMARK(headerMapImplGetInline)->Arg(0)->Arg(1)->Arg(10)->Arg(50); /** * Measure the speed of writing to a header for which HeaderMapImpl is expected to * provide special optimizations. */ -static void HeaderMapImplSetInlineMacro(benchmark::State& state) { +static void headerMapImplSetInlineMacro(benchmark::State& state) { const std::string value("01234567890123456789"); auto headers = Http::ResponseHeaderMapImpl::create(); addDummyHeaders(*headers, state.range(0)); @@ -98,13 +98,13 @@ static void HeaderMapImplSetInlineMacro(benchmark::State& state) { } benchmark::DoNotOptimize(headers->size()); } -BENCHMARK(HeaderMapImplSetInlineMacro)->Arg(0)->Arg(1)->Arg(10)->Arg(50); +BENCHMARK(headerMapImplSetInlineMacro)->Arg(0)->Arg(1)->Arg(10)->Arg(50); /** * Measure the speed of writing to a header for which HeaderMapImpl is expected to * provide special optimizations. */ -static void HeaderMapImplSetInlineInteger(benchmark::State& state) { +static void headerMapImplSetInlineInteger(benchmark::State& state) { uint64_t value = 12345; auto headers = Http::ResponseHeaderMapImpl::create(); addDummyHeaders(*headers, state.range(0)); @@ -113,10 +113,10 @@ static void HeaderMapImplSetInlineInteger(benchmark::State& state) { } benchmark::DoNotOptimize(headers->size()); } -BENCHMARK(HeaderMapImplSetInlineInteger)->Arg(0)->Arg(1)->Arg(10)->Arg(50); +BENCHMARK(headerMapImplSetInlineInteger)->Arg(0)->Arg(1)->Arg(10)->Arg(50); /** Measure the speed of the byteSize() estimation method. */ -static void HeaderMapImplGetByteSize(benchmark::State& state) { +static void headerMapImplGetByteSize(benchmark::State& state) { auto headers = Http::ResponseHeaderMapImpl::create(); addDummyHeaders(*headers, state.range(0)); uint64_t size = 0; @@ -125,10 +125,10 @@ static void HeaderMapImplGetByteSize(benchmark::State& state) { } benchmark::DoNotOptimize(size); } -BENCHMARK(HeaderMapImplGetByteSize)->Arg(0)->Arg(1)->Arg(10)->Arg(50); +BENCHMARK(headerMapImplGetByteSize)->Arg(0)->Arg(1)->Arg(10)->Arg(50); /** Measure the speed of iteration with a lightweight callback. */ -static void HeaderMapImplIterate(benchmark::State& state) { +static void headerMapImplIterate(benchmark::State& state) { auto headers = Http::ResponseHeaderMapImpl::create(); size_t num_callbacks = 0; addDummyHeaders(*headers, state.range(0)); @@ -141,10 +141,10 @@ static void HeaderMapImplIterate(benchmark::State& state) { } benchmark::DoNotOptimize(num_callbacks); } -BENCHMARK(HeaderMapImplIterate)->Arg(0)->Arg(1)->Arg(10)->Arg(50); +BENCHMARK(headerMapImplIterate)->Arg(0)->Arg(1)->Arg(10)->Arg(50); /** Measure the speed of the HeaderMapImpl lookup() method. */ -static void HeaderMapImplLookup(benchmark::State& state) { +static void headerMapImplLookup(benchmark::State& state) { const LowerCaseString key("connection"); const std::string value("01234567890123456789"); auto headers = Http::ResponseHeaderMapImpl::create(); @@ -156,14 +156,14 @@ static void HeaderMapImplLookup(benchmark::State& state) { benchmark::DoNotOptimize(result); } } -BENCHMARK(HeaderMapImplLookup)->Arg(0)->Arg(1)->Arg(10)->Arg(50); +BENCHMARK(headerMapImplLookup)->Arg(0)->Arg(1)->Arg(10)->Arg(50); /** * Measure the speed of removing a header by key name. * @note The measured time for each iteration includes the time needed to add * one copy of the header. */ -static void HeaderMapImplRemove(benchmark::State& state) { +static void headerMapImplRemove(benchmark::State& state) { const LowerCaseString key("example-key"); const std::string value("01234567890123456789"); auto headers = Http::ResponseHeaderMapImpl::create(); @@ -174,7 +174,7 @@ static void HeaderMapImplRemove(benchmark::State& state) { } benchmark::DoNotOptimize(headers->size()); } -BENCHMARK(HeaderMapImplRemove)->Arg(0)->Arg(1)->Arg(10)->Arg(50); +BENCHMARK(headerMapImplRemove)->Arg(0)->Arg(1)->Arg(10)->Arg(50); /** * Measure the speed of removing a header by key name, for the special case of @@ -182,7 +182,7 @@ BENCHMARK(HeaderMapImplRemove)->Arg(0)->Arg(1)->Arg(10)->Arg(50); * @note The measured time for each iteration includes the time needed to add * one copy of the header. */ -static void HeaderMapImplRemoveInline(benchmark::State& state) { +static void headerMapImplRemoveInline(benchmark::State& state) { const LowerCaseString key("connection"); const std::string value("01234567890123456789"); auto headers = Http::ResponseHeaderMapImpl::create(); @@ -193,13 +193,13 @@ static void HeaderMapImplRemoveInline(benchmark::State& state) { } benchmark::DoNotOptimize(headers->size()); } -BENCHMARK(HeaderMapImplRemoveInline)->Arg(0)->Arg(1)->Arg(10)->Arg(50); +BENCHMARK(headerMapImplRemoveInline)->Arg(0)->Arg(1)->Arg(10)->Arg(50); /** * Measure the speed of creating a HeaderMapImpl and populating it with a realistic * set of response headers. */ -static void HeaderMapImplPopulate(benchmark::State& state) { +static void headerMapImplPopulate(benchmark::State& state) { const std::pair headers_to_add[] = { {LowerCaseString("cache-control"), "max-age=0, private, must-revalidate"}, {LowerCaseString("content-encoding"), "gzip"}, @@ -220,7 +220,7 @@ static void HeaderMapImplPopulate(benchmark::State& state) { benchmark::DoNotOptimize(headers->size()); } } -BENCHMARK(HeaderMapImplPopulate); +BENCHMARK(headerMapImplPopulate); } // namespace Http } // namespace Envoy From 17a0f39f96f7ecbab0f604d3c0762505a66806ed Mon Sep 17 00:00:00 2001 From: Matt Klein Date: Wed, 17 Jun 2020 16:34:42 +0000 Subject: [PATCH 3/3] comments Signed-off-by: Matt Klein --- source/common/http/header_map_impl.cc | 4 +-- source/common/http/header_map_impl.h | 48 ++++++++++++++++++++------- source/docs/header_map.md | 30 +++++++++++++++++ 3 files changed, 68 insertions(+), 14 deletions(-) create mode 100644 source/docs/header_map.md diff --git a/source/common/http/header_map_impl.cc b/source/common/http/header_map_impl.cc index 3935818f9bf54..809fa2402e6e6 100644 --- a/source/common/http/header_map_impl.cc +++ b/source/common/http/header_map_impl.cc @@ -624,13 +624,13 @@ size_t HeaderMapImpl::removeInline(HeaderEntryImpl** ptr_to_entry) { namespace { template -HeaderMapImplUtility::HeaderMapImplInfo makeHeaderMapImplInfo(const std::string& name) { +HeaderMapImplUtility::HeaderMapImplInfo makeHeaderMapImplInfo(absl::string_view name) { // Constructing a header map implementation will force the custom headers and sizing to be // finalized, so do that first. auto header_map = T::create(); HeaderMapImplUtility::HeaderMapImplInfo info; - info.name_ = name; + info.name_ = std::string(name); info.size_ = T::inlineHeadersSize() + sizeof(T); for (const auto& header : CustomInlineHeaderRegistry::headers()) { info.registered_headers_.push_back(header.first.get()); diff --git a/source/common/http/header_map_impl.h b/source/common/http/header_map_impl.h index dc6627268a719..30e2eab892b92 100644 --- a/source/common/http/header_map_impl.h +++ b/source/common/http/header_map_impl.h @@ -145,7 +145,12 @@ class HeaderMapImpl : NonCopyable { static size_t size() { // The size of the lookup table is finalized when the singleton lookup table is created. This - // allows for late binding of custom headers as well as envoy header prefix changes. + // allows for late binding of custom headers as well as envoy header prefix changes. This + // does mean that once the first header map is created of this type, no further changes are + // possible. + // TODO(mattklein123): If we decide to keep this implementation, it is conceivable that header + // maps could be created by an API factory that is owned by the listener/HCM, thus making + // O(1) header delivery over xDS possible. return ConstSingleton::get().size_; } @@ -380,6 +385,15 @@ class RequestHeaderMapImpl final : public TypedHeaderMapImpl, INLINE_REQ_HEADERS(DEFINE_INLINE_HEADER_FUNCS) INLINE_REQ_RESP_HEADERS(DEFINE_INLINE_HEADER_FUNCS) +protected: + // NOTE: Because inline_headers_ is a variable size member, it must be the last member in the + // most derived class. This forces the definition of the following three functions to also be + // in the most derived class and thus duplicated. There may be a way to consolidate thus but it's + // not clear and can be deferred for now. + void clearInline() override { memset(inline_headers_, 0, inlineHeadersSize()); } + const HeaderEntryImpl* const* constInlineHeaders() const override { return inline_headers_; } + HeaderEntryImpl** inlineHeaders() override { return inline_headers_; } + private: struct HeaderHandleValues { INLINE_REQ_HEADERS(DEFINE_HEADER_HANDLE) @@ -389,9 +403,6 @@ class RequestHeaderMapImpl final : public TypedHeaderMapImpl, using HeaderHandles = ConstSingleton; RequestHeaderMapImpl() { clearInline(); } - void clearInline() override { memset(inline_headers_, 0, inlineHeadersSize()); } - const HeaderEntryImpl* const* constInlineHeaders() const override { return inline_headers_; } - HeaderEntryImpl** inlineHeaders() override { return inline_headers_; } HeaderEntryImpl* inline_headers_[]; }; @@ -408,12 +419,15 @@ class RequestTrailerMapImpl final : public TypedHeaderMapImpl RequestTrailerMapImpl()); } -private: - RequestTrailerMapImpl() { clearInline(); } +protected: + // See comment in RequestHeaderMapImpl. void clearInline() override { memset(inline_headers_, 0, inlineHeadersSize()); } const HeaderEntryImpl* const* constInlineHeaders() const override { return inline_headers_; } HeaderEntryImpl** inlineHeaders() override { return inline_headers_; } +private: + RequestTrailerMapImpl() { clearInline(); } + HeaderEntryImpl* inline_headers_[]; }; @@ -433,6 +447,12 @@ class ResponseHeaderMapImpl final : public TypedHeaderMapImpl INLINE_REQ_RESP_HEADERS(DEFINE_INLINE_HEADER_FUNCS) INLINE_RESP_HEADERS_TRAILERS(DEFINE_INLINE_HEADER_FUNCS) +protected: + // See comment in RequestHeaderMapImpl. + void clearInline() override { memset(inline_headers_, 0, inlineHeadersSize()); } + const HeaderEntryImpl* const* constInlineHeaders() const override { return inline_headers_; } + HeaderEntryImpl** inlineHeaders() override { return inline_headers_; } + private: struct HeaderHandleValues { INLINE_RESP_HEADERS(DEFINE_HEADER_HANDLE) @@ -443,9 +463,6 @@ class ResponseHeaderMapImpl final : public TypedHeaderMapImpl using HeaderHandles = ConstSingleton; ResponseHeaderMapImpl() { clearInline(); } - void clearInline() override { memset(inline_headers_, 0, inlineHeadersSize()); } - const HeaderEntryImpl* const* constInlineHeaders() const override { return inline_headers_; } - HeaderEntryImpl** inlineHeaders() override { return inline_headers_; } HeaderEntryImpl* inline_headers_[]; }; @@ -464,6 +481,12 @@ class ResponseTrailerMapImpl final : public TypedHeaderMapImpl; ResponseTrailerMapImpl() { clearInline(); } - void clearInline() override { memset(inline_headers_, 0, inlineHeadersSize()); } - const HeaderEntryImpl* const* constInlineHeaders() const override { return inline_headers_; } - HeaderEntryImpl** inlineHeaders() override { return inline_headers_; } HeaderEntryImpl* inline_headers_[]; }; @@ -515,8 +535,12 @@ using StaticEmptyHeaders = ConstSingleton; class HeaderMapImplUtility { public: struct HeaderMapImplInfo { + // Human readable name for the header map used in info logging. std::string name_; + // The byte size of the header map including both fixed space as well as variable space used + // by the registered custom headers. size_t size_; + // All registered custom headers for the header map. std::vector registered_headers_; }; diff --git a/source/docs/header_map.md b/source/docs/header_map.md new file mode 100644 index 0000000000000..59ae9db78621a --- /dev/null +++ b/source/docs/header_map.md @@ -0,0 +1,30 @@ +# Header map implementation overview + +The Envoy header map implementation (`HeaderMapImpl`) has the following properties: +* Headers are stored in a linked list (`HeaderList`) in the order they are added, with pseudo + headers kept at the front of the list. +* O(1) direct access is possible for common headers needed during data plane processing. This is + provided by a table of pointers that reach directly into a linked list that is populated when + headers are added or removed from the map. When O(1) headers are accessed by direct method + (`DEFINE_INLINE_HEADER` and `CustomInlineHeaderBase`) they use direct pointer access to see + whether a header is present, add it, modify it, etc. When headers are added by name a trie is used to lookup the pointer in the table (`StaticLookupTable`). +* Custom headers can be registered statically against a specific implementation (request headers, + request trailers, response headers, and response trailers) via core code and extensions + (`CustomInlineHeaderRegistry`). Each registered header increases the size of the table by the size of a single pointer. +* Operations that search, replace, etc. for a header by name that is not one of the O(1) headers + will incur an O(N) search through the linked list. This is an implementation deficiency for + certain usage patterns that will be improved in future changes. + +## Implementation details + +* O(1) registered headers are tracked during static initialization via the `CustomInlineHeaderBase` + class. +* The first time a header map is constructed (in practice this is after bootstrap load and the + Envoy header prefix is finalized when `getAllHeaderMapImplInfo` is called), the + `StaticLookupTable` is finalized for each header map type. No further changes are possible after + this point. The `StaticLookupTable` defines the amount of variable pointer table space that is + require for each header map type. +* Each concrete header map type derives from `InlineStorage` with a variable length member at the + end of the definition. +* Each concrete header map type uses a factory function and a provide constructor. The required + size is determined via the `inlineHeadersSize` function. \ No newline at end of file