From edfb0387d687ae3cdbb460b535f57d56a11bc005 Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Thu, 2 Nov 2023 16:42:57 -0700 Subject: [PATCH] [metadata] Don't ref entire read for unknown metadata added to hpack table (#34863) --- src/core/lib/experiments/experiments.cc | 18 ++++++++++++++++++ src/core/lib/experiments/experiments.h | 11 +++++++++++ src/core/lib/experiments/experiments.yaml | 4 ++++ src/core/lib/experiments/rollouts.yaml | 2 ++ src/core/lib/transport/metadata_batch.h | 7 ++++++- 5 files changed, 41 insertions(+), 1 deletion(-) diff --git a/src/core/lib/experiments/experiments.cc b/src/core/lib/experiments/experiments.cc index e25d090fd50f5..a0fe5bab251d5 100644 --- a/src/core/lib/experiments/experiments.cc +++ b/src/core/lib/experiments/experiments.cc @@ -180,6 +180,10 @@ const char* const description_unconstrained_max_quota_buffer_size = "Discard the cap on the max free pool size for one memory allocator"; const char* const additional_constraints_unconstrained_max_quota_buffer_size = "{}"; +const char* const description_uniquely_unowned = + "Ensure HPACK table takes a unique copy of data when parsing unknown " + "metadata"; +const char* const additional_constraints_uniquely_unowned = "{}"; const char* const description_work_serializer_clears_time_cache = "Have the work serializer clear the time cache when it dispatches work."; const char* const additional_constraints_work_serializer_clears_time_cache = @@ -301,6 +305,8 @@ const ExperimentMetadata g_experiment_metadata[] = { {"unconstrained_max_quota_buffer_size", description_unconstrained_max_quota_buffer_size, additional_constraints_unconstrained_max_quota_buffer_size, false, true}, + {"uniquely_unowned", description_uniquely_unowned, + additional_constraints_uniquely_unowned, true, true}, {"work_serializer_clears_time_cache", description_work_serializer_clears_time_cache, additional_constraints_work_serializer_clears_time_cache, true, true}, @@ -476,6 +482,10 @@ const char* const description_unconstrained_max_quota_buffer_size = "Discard the cap on the max free pool size for one memory allocator"; const char* const additional_constraints_unconstrained_max_quota_buffer_size = "{}"; +const char* const description_uniquely_unowned = + "Ensure HPACK table takes a unique copy of data when parsing unknown " + "metadata"; +const char* const additional_constraints_uniquely_unowned = "{}"; const char* const description_work_serializer_clears_time_cache = "Have the work serializer clear the time cache when it dispatches work."; const char* const additional_constraints_work_serializer_clears_time_cache = @@ -597,6 +607,8 @@ const ExperimentMetadata g_experiment_metadata[] = { {"unconstrained_max_quota_buffer_size", description_unconstrained_max_quota_buffer_size, additional_constraints_unconstrained_max_quota_buffer_size, false, true}, + {"uniquely_unowned", description_uniquely_unowned, + additional_constraints_uniquely_unowned, true, true}, {"work_serializer_clears_time_cache", description_work_serializer_clears_time_cache, additional_constraints_work_serializer_clears_time_cache, true, true}, @@ -772,6 +784,10 @@ const char* const description_unconstrained_max_quota_buffer_size = "Discard the cap on the max free pool size for one memory allocator"; const char* const additional_constraints_unconstrained_max_quota_buffer_size = "{}"; +const char* const description_uniquely_unowned = + "Ensure HPACK table takes a unique copy of data when parsing unknown " + "metadata"; +const char* const additional_constraints_uniquely_unowned = "{}"; const char* const description_work_serializer_clears_time_cache = "Have the work serializer clear the time cache when it dispatches work."; const char* const additional_constraints_work_serializer_clears_time_cache = @@ -893,6 +909,8 @@ const ExperimentMetadata g_experiment_metadata[] = { {"unconstrained_max_quota_buffer_size", description_unconstrained_max_quota_buffer_size, additional_constraints_unconstrained_max_quota_buffer_size, false, true}, + {"uniquely_unowned", description_uniquely_unowned, + additional_constraints_uniquely_unowned, true, true}, {"work_serializer_clears_time_cache", description_work_serializer_clears_time_cache, additional_constraints_work_serializer_clears_time_cache, true, true}, diff --git a/src/core/lib/experiments/experiments.h b/src/core/lib/experiments/experiments.h index c317312278b85..5304e7a549ad6 100644 --- a/src/core/lib/experiments/experiments.h +++ b/src/core/lib/experiments/experiments.h @@ -125,6 +125,8 @@ inline bool IsTcpFrameSizeTuningEnabled() { return false; } inline bool IsTcpRcvLowatEnabled() { return false; } inline bool IsTraceRecordCallopsEnabled() { return false; } inline bool IsUnconstrainedMaxQuotaBufferSizeEnabled() { return false; } +#define GRPC_EXPERIMENT_IS_INCLUDED_UNIQUELY_UNOWNED +inline bool IsUniquelyUnownedEnabled() { return true; } #define GRPC_EXPERIMENT_IS_INCLUDED_WORK_SERIALIZER_CLEARS_TIME_CACHE inline bool IsWorkSerializerClearsTimeCacheEnabled() { return true; } inline bool IsWorkSerializerDispatchEnabled() { return false; } @@ -205,6 +207,8 @@ inline bool IsTcpFrameSizeTuningEnabled() { return false; } inline bool IsTcpRcvLowatEnabled() { return false; } inline bool IsTraceRecordCallopsEnabled() { return false; } inline bool IsUnconstrainedMaxQuotaBufferSizeEnabled() { return false; } +#define GRPC_EXPERIMENT_IS_INCLUDED_UNIQUELY_UNOWNED +inline bool IsUniquelyUnownedEnabled() { return true; } #define GRPC_EXPERIMENT_IS_INCLUDED_WORK_SERIALIZER_CLEARS_TIME_CACHE inline bool IsWorkSerializerClearsTimeCacheEnabled() { return true; } inline bool IsWorkSerializerDispatchEnabled() { return false; } @@ -285,6 +289,8 @@ inline bool IsTcpFrameSizeTuningEnabled() { return false; } inline bool IsTcpRcvLowatEnabled() { return false; } inline bool IsTraceRecordCallopsEnabled() { return false; } inline bool IsUnconstrainedMaxQuotaBufferSizeEnabled() { return false; } +#define GRPC_EXPERIMENT_IS_INCLUDED_UNIQUELY_UNOWNED +inline bool IsUniquelyUnownedEnabled() { return true; } #define GRPC_EXPERIMENT_IS_INCLUDED_WORK_SERIALIZER_CLEARS_TIME_CACHE inline bool IsWorkSerializerClearsTimeCacheEnabled() { return true; } inline bool IsWorkSerializerDispatchEnabled() { return false; } @@ -340,6 +346,7 @@ enum ExperimentIds { kExperimentIdTcpRcvLowat, kExperimentIdTraceRecordCallops, kExperimentIdUnconstrainedMaxQuotaBufferSize, + kExperimentIdUniquelyUnowned, kExperimentIdWorkSerializerClearsTimeCache, kExperimentIdWorkSerializerDispatch, kExperimentIdWriteSizeCap, @@ -516,6 +523,10 @@ inline bool IsTraceRecordCallopsEnabled() { inline bool IsUnconstrainedMaxQuotaBufferSizeEnabled() { return IsExperimentEnabled(kExperimentIdUnconstrainedMaxQuotaBufferSize); } +#define GRPC_EXPERIMENT_IS_INCLUDED_UNIQUELY_UNOWNED +inline bool IsUniquelyUnownedEnabled() { + return IsExperimentEnabled(kExperimentIdUniquelyUnowned); +} #define GRPC_EXPERIMENT_IS_INCLUDED_WORK_SERIALIZER_CLEARS_TIME_CACHE inline bool IsWorkSerializerClearsTimeCacheEnabled() { return IsExperimentEnabled(kExperimentIdWorkSerializerClearsTimeCache); diff --git a/src/core/lib/experiments/experiments.yaml b/src/core/lib/experiments/experiments.yaml index aacf254bafdce..9a4112b771d8d 100644 --- a/src/core/lib/experiments/experiments.yaml +++ b/src/core/lib/experiments/experiments.yaml @@ -307,6 +307,10 @@ expiry: 2024/02/01 owner: ctiller@google.com test_tags: [resource_quota_test] +- name: uniquely_unowned + description: Ensure HPACK table takes a unique copy of data when parsing unknown metadata + expiry: 2024/03/03 + owner: ctiller@google.com - name: work_serializer_clears_time_cache description: Have the work serializer clear the time cache when it dispatches work. diff --git a/src/core/lib/experiments/rollouts.yaml b/src/core/lib/experiments/rollouts.yaml index a22c3e07a2519..c35ce9f1cdb23 100644 --- a/src/core/lib/experiments/rollouts.yaml +++ b/src/core/lib/experiments/rollouts.yaml @@ -134,6 +134,8 @@ default: false - name: unconstrained_max_quota_buffer_size default: false +- name: uniquely_unowned + default: true - name: work_serializer_clears_time_cache default: true - name: work_serializer_dispatch diff --git a/src/core/lib/transport/metadata_batch.h b/src/core/lib/transport/metadata_batch.h index 4b5e3d250b0d6..0a81bd22a0ab2 100644 --- a/src/core/lib/transport/metadata_batch.h +++ b/src/core/lib/transport/metadata_batch.h @@ -40,6 +40,7 @@ #include #include "src/core/lib/compression/compression_internal.h" +#include "src/core/lib/experiments/experiments.h" #include "src/core/lib/gprpp/chunked_vector.h" #include "src/core/lib/gprpp/if_list.h" #include "src/core/lib/gprpp/packed_table.h" @@ -647,7 +648,11 @@ class ParseHelper { absl::string_view key) { return ParsedMetadata( typename ParsedMetadata::FromSlicePair{}, - Slice::FromCopiedString(key), std::move(value_), transport_size_); + Slice::FromCopiedString(key), + IsUniquelyUnownedEnabled() && will_keep_past_request_lifetime_ + ? value_.TakeUniquelyOwned() + : std::move(value_), + transport_size_); } private: