From b638ccba581733a7e6528233e3db4bcb026cd611 Mon Sep 17 00:00:00 2001 From: Ryan Northey Date: Tue, 28 Apr 2026 13:55:20 +0100 Subject: [PATCH 1/4] test: replace flaky StringMatcher.Memory with deterministic sizeof static_asserts Signed-off-by: Ryan Northey --- test/common/common/matchers_test.cc | 113 +++++++++++++++---------- tools/spelling/spelling_dictionary.txt | 2 + 2 files changed, 72 insertions(+), 43 deletions(-) diff --git a/test/common/common/matchers_test.cc b/test/common/common/matchers_test.cc index 59f227523d354..329d51ffa3b46 100644 --- a/test/common/common/matchers_test.cc +++ b/test/common/common/matchers_test.cc @@ -453,49 +453,76 @@ TEST_F(StringMatcher, NoMatcherRejected) { fmt::format("Configuration must define a matcher: {}", matcher.DebugString())); } -// Validates the amount of memory that is being used by the different string -// matchers. Requested as part of https://github.com/envoyproxy/envoy/pull/37782. -TEST_F(StringMatcher, Memory) { - const uint32_t matchers_num = 1000; - // Prefix matcher. - { - // Add 1000 Prefix-String Matchers of varying string lengths (1 to 1000). - std::vector all_matchers; - all_matchers.reserve(matchers_num); - Memory::TestUtil::MemoryTest memory_test; - for (uint32_t i = 0; i < matchers_num; ++i) { - envoy::type::matcher::v3::StringMatcher matcher; - matcher.set_prefix(std::string(i + 1, 'a')); - all_matchers.emplace_back(Matchers::StringMatcherImpl(matcher, context_)); - } - const size_t prefix_consumed_bytes = memory_test.consumedBytes(); - // The memory constraints were added to ensure that the amount of memory - // used by matchers is carefully analyzed. These constraints can be relaxed - // when additional features are added, but it should be done in a thoughtful manner. - // Adding 5*8192 bytes because tcmalloc consumption estimation may return - // different values depending on memory alignment. - EXPECT_MEMORY_LE(prefix_consumed_bytes, 530176 + 5 * 8192); - } - // Regex matcher. - { - // Add 1000 Regex-String Matchers of varying string lengths (1 to 1000). - std::vector all_matchers; - all_matchers.reserve(matchers_num); - Memory::TestUtil::MemoryTest memory_test; - for (uint32_t i = 0; i < matchers_num; ++i) { - envoy::type::matcher::v3::StringMatcher matcher; - matcher.mutable_safe_regex()->mutable_google_re2(); - matcher.mutable_safe_regex()->set_regex(std::string(i + 1, 'a')); - all_matchers.emplace_back(Matchers::StringMatcherImpl(matcher, context_)); - } - const size_t regex_consumed_bytes = memory_test.consumedBytes(); - // The memory constraints were added to ensure that the amount of memory - // used by matchers is carefully analyzed. These constraints can be relaxed - // when additional features are added, but it should be done in a thoughtful manner. - // Adding 10*8192 bytes because tcmalloc consumption estimation may return - // different values depending on memory alignment. - EXPECT_MEMORY_LE(regex_consumed_bytes, 15603776 + 10 * 8192); - } +// Compile-time size bounds for the variant alternatives inside StringMatcherImpl. +// These enforce the structural property from PR #37782: each alternative carries only +// the data it needs; no alternative holds redundant fields from another; and +// StringMatcherImpl does not retain a copy of the proto used to construct it. +// +// Bounds are expressed in terms of sizeof(std::string) and sizeof(void*) so they +// are portable across libc++ (sizeof(std::string)==24) and libstdc++ +// (sizeof(std::string)==32), and across 32- and 64-bit builds. +// +// Why the old EXPECT_MEMORY_LE test was removed (PR #37782 / #43467): +// It measured tcmalloc *page-level* heap consumption, which is sensitive to +// size-class bucketing, span layout, page-heap fragmentation, and prior heap +// state — none of which are properties of StringMatcherImpl. The ±N*8192 slack +// was an acknowledgement of page-sized noise, yet it still produced ~5/1000 +// flakes on gcc CI (most recently: actual 15710496 vs ceiling 15685696, only +// ~24 KiB over a ~16 MiB bound). The magic constants also required manual +// hand-tuning on every tcmalloc/abseil/protobuf bump. The structural invariant +// that PR #37782 actually wanted to protect is captured deterministically by +// the sizeof assertions below. + +// String-holding alternatives: one std::string + one bool rounded up to pointer +// alignment. sizeof(ExactStringMatcher) must not grow beyond this. +static_assert(sizeof(Matchers::ExactStringMatcher) <= sizeof(std::string) + sizeof(void*), + "ExactStringMatcher unexpectedly grew; verify any new field is necessary " + "and does not duplicate state already held in StringMatcherImpl."); +static_assert(sizeof(Matchers::PrefixStringMatcher) <= sizeof(std::string) + sizeof(void*), + "PrefixStringMatcher unexpectedly grew; verify any new field is necessary " + "and does not duplicate state already held in StringMatcherImpl."); +static_assert(sizeof(Matchers::SuffixStringMatcher) <= sizeof(std::string) + sizeof(void*), + "SuffixStringMatcher unexpectedly grew; verify any new field is necessary " + "and does not duplicate state already held in StringMatcherImpl."); +static_assert(sizeof(Matchers::ContainsStringMatcher) <= sizeof(std::string) + sizeof(void*), + "ContainsStringMatcher unexpectedly grew; verify any new field is necessary " + "and does not duplicate state already held in StringMatcherImpl."); + +// Pointer-holding alternatives: a single unique_ptr (one pointer-sized member). +static_assert(sizeof(Matchers::RegexStringMatcher) <= 2 * sizeof(void*), + "RegexStringMatcher unexpectedly grew; verify the new field is necessary."); +static_assert(sizeof(Matchers::CustomStringMatcher) <= 2 * sizeof(void*), + "CustomStringMatcher unexpectedly grew; verify the new field is necessary."); + +// StringMatcherImpl layout — accounting for all four pointer-sized contributions: +// [1] vtable pointer for ValueMatcher base (+1 * sizeof(void*)) +// [2] vtable pointer for StringMatcher base (+1 * sizeof(void*)) +// [3] absl::variant payload = max(sizeof(alternatives)) +// = sizeof(std::string) [the string itself] +// + sizeof(void*) [bool member rounded up to pointer alignment] +// [4] absl::variant discriminant (+1 * sizeof(void*)) +// Total upper bound: sizeof(std::string) + 4 * sizeof(void*). +static_assert(sizeof(Matchers::StringMatcherImpl) <= sizeof(std::string) + 4 * sizeof(void*), + "StringMatcherImpl unexpectedly grew; if you added a data member or a new variant " + "alternative larger than the existing string-holding ones, ensure the per-matcher " + "footprint regression is intentional. See PR #37782 for the design context."); + +// Verifies the size bounds above at runtime so failures appear in the test-runner +// output. The primary check is the compile-time static_assert above; this test +// provides discoverability via the gtest output and documents the expected sizes. +TEST_F(StringMatcher, SizeIsBounded) { + // String-holding alternatives: std::string + bool padded to pointer alignment. + EXPECT_LE(sizeof(Matchers::ExactStringMatcher), sizeof(std::string) + sizeof(void*)); + EXPECT_LE(sizeof(Matchers::PrefixStringMatcher), sizeof(std::string) + sizeof(void*)); + EXPECT_LE(sizeof(Matchers::SuffixStringMatcher), sizeof(std::string) + sizeof(void*)); + EXPECT_LE(sizeof(Matchers::ContainsStringMatcher), sizeof(std::string) + sizeof(void*)); + + // Pointer-holding alternatives: one unique_ptr. + EXPECT_LE(sizeof(Matchers::RegexStringMatcher), 2 * sizeof(void*)); + EXPECT_LE(sizeof(Matchers::CustomStringMatcher), 2 * sizeof(void*)); + + // StringMatcherImpl: two vtable ptrs + variant (largest alternative + discriminant). + EXPECT_LE(sizeof(Matchers::StringMatcherImpl), sizeof(std::string) + 4 * sizeof(void*)); } class PathMatcher : public BaseTest {}; diff --git a/tools/spelling/spelling_dictionary.txt b/tools/spelling/spelling_dictionary.txt index de177c01c1ec8..0fb9d1f35f6a7 100644 --- a/tools/spelling/spelling_dictionary.txt +++ b/tools/spelling/spelling_dictionary.txt @@ -3,6 +3,7 @@ # will accept title case (e.g. lyft matches Lyft). Prefixes (e.g., un-) or suffixes (e.g. -ing) # are allowed for any otherwise correctly spelled word. +acknowledgement Allshard FLUSHALL NUMPAT @@ -70,6 +71,7 @@ consume cstr deadcode DFP +discoverability Dynatrace DOM DONTFRAG From 12921a82718a581f73ca86cd5ccf4ec56b62456a Mon Sep 17 00:00:00 2001 From: Ryan Northey Date: Tue, 28 Apr 2026 16:01:09 +0100 Subject: [PATCH 2/4] raven-feedback Signed-off-by: Ryan Northey --- test/common/common/matchers_test.cc | 112 ++++++++++++---------------- 1 file changed, 49 insertions(+), 63 deletions(-) diff --git a/test/common/common/matchers_test.cc b/test/common/common/matchers_test.cc index 329d51ffa3b46..2cd98b67de72c 100644 --- a/test/common/common/matchers_test.cc +++ b/test/common/common/matchers_test.cc @@ -12,6 +12,7 @@ #include "test/mocks/server/server_factory_context.h" #include "test/test_common/utility.h" +#include "gmock/gmock.h" #include "gtest/gtest.h" namespace Envoy { @@ -453,76 +454,61 @@ TEST_F(StringMatcher, NoMatcherRejected) { fmt::format("Configuration must define a matcher: {}", matcher.DebugString())); } -// Compile-time size bounds for the variant alternatives inside StringMatcherImpl. -// These enforce the structural property from PR #37782: each alternative carries only -// the data it needs; no alternative holds redundant fields from another; and -// StringMatcherImpl does not retain a copy of the proto used to construct it. +// Self-documenting matcher used by SizeIsBounded below. On failure, gtest will +// print a message like: +// "does not use more than 32: think carefully before increasing this, and if +// you're sure, update the corresponding expectation" +// which tells the next person touching StringMatcherImpl exactly what to do. +MATCHER_P(MemNotMoreThan, sz, + "does not use more than " + std::to_string(sz) + + ": think carefully before increasing this, and if you're sure, " + "update the corresponding expectation") { + return arg <= sz; +} + +// Bounds the per-alternative and overall size of StringMatcherImpl. This +// enforces the structural property from PR #37782: each variant alternative +// carries only the data it needs; no alternative holds redundant fields from +// another; and StringMatcherImpl does not retain a copy of the proto used to +// construct it. // -// Bounds are expressed in terms of sizeof(std::string) and sizeof(void*) so they -// are portable across libc++ (sizeof(std::string)==24) and libstdc++ +// Bounds are expressed in terms of sizeof(std::string) and sizeof(void*) so +// they are portable across libc++ (sizeof(std::string)==24) and libstdc++ // (sizeof(std::string)==32), and across 32- and 64-bit builds. // // Why the old EXPECT_MEMORY_LE test was removed (PR #37782 / #43467): // It measured tcmalloc *page-level* heap consumption, which is sensitive to // size-class bucketing, span layout, page-heap fragmentation, and prior heap -// state — none of which are properties of StringMatcherImpl. The ±N*8192 slack -// was an acknowledgement of page-sized noise, yet it still produced ~5/1000 -// flakes on gcc CI (most recently: actual 15710496 vs ceiling 15685696, only -// ~24 KiB over a ~16 MiB bound). The magic constants also required manual -// hand-tuning on every tcmalloc/abseil/protobuf bump. The structural invariant -// that PR #37782 actually wanted to protect is captured deterministically by -// the sizeof assertions below. - -// String-holding alternatives: one std::string + one bool rounded up to pointer -// alignment. sizeof(ExactStringMatcher) must not grow beyond this. -static_assert(sizeof(Matchers::ExactStringMatcher) <= sizeof(std::string) + sizeof(void*), - "ExactStringMatcher unexpectedly grew; verify any new field is necessary " - "and does not duplicate state already held in StringMatcherImpl."); -static_assert(sizeof(Matchers::PrefixStringMatcher) <= sizeof(std::string) + sizeof(void*), - "PrefixStringMatcher unexpectedly grew; verify any new field is necessary " - "and does not duplicate state already held in StringMatcherImpl."); -static_assert(sizeof(Matchers::SuffixStringMatcher) <= sizeof(std::string) + sizeof(void*), - "SuffixStringMatcher unexpectedly grew; verify any new field is necessary " - "and does not duplicate state already held in StringMatcherImpl."); -static_assert(sizeof(Matchers::ContainsStringMatcher) <= sizeof(std::string) + sizeof(void*), - "ContainsStringMatcher unexpectedly grew; verify any new field is necessary " - "and does not duplicate state already held in StringMatcherImpl."); - -// Pointer-holding alternatives: a single unique_ptr (one pointer-sized member). -static_assert(sizeof(Matchers::RegexStringMatcher) <= 2 * sizeof(void*), - "RegexStringMatcher unexpectedly grew; verify the new field is necessary."); -static_assert(sizeof(Matchers::CustomStringMatcher) <= 2 * sizeof(void*), - "CustomStringMatcher unexpectedly grew; verify the new field is necessary."); - -// StringMatcherImpl layout — accounting for all four pointer-sized contributions: -// [1] vtable pointer for ValueMatcher base (+1 * sizeof(void*)) -// [2] vtable pointer for StringMatcher base (+1 * sizeof(void*)) -// [3] absl::variant payload = max(sizeof(alternatives)) -// = sizeof(std::string) [the string itself] -// + sizeof(void*) [bool member rounded up to pointer alignment] -// [4] absl::variant discriminant (+1 * sizeof(void*)) -// Total upper bound: sizeof(std::string) + 4 * sizeof(void*). -static_assert(sizeof(Matchers::StringMatcherImpl) <= sizeof(std::string) + 4 * sizeof(void*), - "StringMatcherImpl unexpectedly grew; if you added a data member or a new variant " - "alternative larger than the existing string-holding ones, ensure the per-matcher " - "footprint regression is intentional. See PR #37782 for the design context."); - -// Verifies the size bounds above at runtime so failures appear in the test-runner -// output. The primary check is the compile-time static_assert above; this test -// provides discoverability via the gtest output and documents the expected sizes. +// state — none of which are properties of StringMatcherImpl. The ±N*8192 +// slack was an acknowledgement of page-sized noise, yet it still produced +// ~5/1000 flakes on gcc CI (most recently: actual 15710496 vs ceiling +// 15685696, only ~24 KiB over a ~16 MiB bound). The magic constants also +// required manual hand-tuning on every tcmalloc/abseil/protobuf bump. The +// structural invariant that PR #37782 actually wanted to protect is +// captured deterministically by the sizeof checks below. TEST_F(StringMatcher, SizeIsBounded) { - // String-holding alternatives: std::string + bool padded to pointer alignment. - EXPECT_LE(sizeof(Matchers::ExactStringMatcher), sizeof(std::string) + sizeof(void*)); - EXPECT_LE(sizeof(Matchers::PrefixStringMatcher), sizeof(std::string) + sizeof(void*)); - EXPECT_LE(sizeof(Matchers::SuffixStringMatcher), sizeof(std::string) + sizeof(void*)); - EXPECT_LE(sizeof(Matchers::ContainsStringMatcher), sizeof(std::string) + sizeof(void*)); - - // Pointer-holding alternatives: one unique_ptr. - EXPECT_LE(sizeof(Matchers::RegexStringMatcher), 2 * sizeof(void*)); - EXPECT_LE(sizeof(Matchers::CustomStringMatcher), 2 * sizeof(void*)); - - // StringMatcherImpl: two vtable ptrs + variant (largest alternative + discriminant). - EXPECT_LE(sizeof(Matchers::StringMatcherImpl), sizeof(std::string) + 4 * sizeof(void*)); + // String-holding alternatives: one std::string + one bool rounded up to + // pointer alignment. + const size_t string_alt_bound = sizeof(std::string) + sizeof(void*); + EXPECT_THAT(sizeof(Matchers::ExactStringMatcher), MemNotMoreThan(string_alt_bound)); + EXPECT_THAT(sizeof(Matchers::PrefixStringMatcher), MemNotMoreThan(string_alt_bound)); + EXPECT_THAT(sizeof(Matchers::SuffixStringMatcher), MemNotMoreThan(string_alt_bound)); + EXPECT_THAT(sizeof(Matchers::ContainsStringMatcher), MemNotMoreThan(string_alt_bound)); + + // Pointer-holding alternatives: a single unique_ptr. + const size_t ptr_alt_bound = 2 * sizeof(void*); + EXPECT_THAT(sizeof(Matchers::RegexStringMatcher), MemNotMoreThan(ptr_alt_bound)); + EXPECT_THAT(sizeof(Matchers::CustomStringMatcher), MemNotMoreThan(ptr_alt_bound)); + + // StringMatcherImpl layout — accounting for all four pointer-sized + // contributions: + // [1] vtable pointer for ValueMatcher base (+1 * sizeof(void*)) + // [2] vtable pointer for StringMatcher base (+1 * sizeof(void*)) + // [3] absl::variant payload = max(sizeof(alternatives)) + // = sizeof(std::string) + sizeof(void*) (string + padded bool) + // [4] absl::variant discriminant (+1 * sizeof(void*)) + EXPECT_THAT(sizeof(Matchers::StringMatcherImpl), + MemNotMoreThan(sizeof(std::string) + 4 * sizeof(void*))); } class PathMatcher : public BaseTest {}; From c35db801996aa5b9f1bbaaa5fa52776f9022b968 Mon Sep 17 00:00:00 2001 From: Ryan Northey Date: Tue, 28 Apr 2026 17:08:50 +0100 Subject: [PATCH 3/4] comments Signed-off-by: Ryan Northey --- test/common/common/matchers_test.cc | 28 +++++----------------------- 1 file changed, 5 insertions(+), 23 deletions(-) diff --git a/test/common/common/matchers_test.cc b/test/common/common/matchers_test.cc index 2cd98b67de72c..ea58a99be03b5 100644 --- a/test/common/common/matchers_test.cc +++ b/test/common/common/matchers_test.cc @@ -454,11 +454,6 @@ TEST_F(StringMatcher, NoMatcherRejected) { fmt::format("Configuration must define a matcher: {}", matcher.DebugString())); } -// Self-documenting matcher used by SizeIsBounded below. On failure, gtest will -// print a message like: -// "does not use more than 32: think carefully before increasing this, and if -// you're sure, update the corresponding expectation" -// which tells the next person touching StringMatcherImpl exactly what to do. MATCHER_P(MemNotMoreThan, sz, "does not use more than " + std::to_string(sz) + ": think carefully before increasing this, and if you're sure, " @@ -466,26 +461,13 @@ MATCHER_P(MemNotMoreThan, sz, return arg <= sz; } -// Bounds the per-alternative and overall size of StringMatcherImpl. This -// enforces the structural property from PR #37782: each variant alternative -// carries only the data it needs; no alternative holds redundant fields from -// another; and StringMatcherImpl does not retain a copy of the proto used to -// construct it. +// Validates the per-matcher memory footprint of the different string matchers. +// Requested as part of https://github.com/envoyproxy/envoy/pull/37782: each +// variant alternative should carry only the data it needs, and +// StringMatcherImpl should not retain the proto used to construct it. // // Bounds are expressed in terms of sizeof(std::string) and sizeof(void*) so -// they are portable across libc++ (sizeof(std::string)==24) and libstdc++ -// (sizeof(std::string)==32), and across 32- and 64-bit builds. -// -// Why the old EXPECT_MEMORY_LE test was removed (PR #37782 / #43467): -// It measured tcmalloc *page-level* heap consumption, which is sensitive to -// size-class bucketing, span layout, page-heap fragmentation, and prior heap -// state — none of which are properties of StringMatcherImpl. The ±N*8192 -// slack was an acknowledgement of page-sized noise, yet it still produced -// ~5/1000 flakes on gcc CI (most recently: actual 15710496 vs ceiling -// 15685696, only ~24 KiB over a ~16 MiB bound). The magic constants also -// required manual hand-tuning on every tcmalloc/abseil/protobuf bump. The -// structural invariant that PR #37782 actually wanted to protect is -// captured deterministically by the sizeof checks below. +// they are portable across libc++, libstdc++, and 32/64-bit builds. TEST_F(StringMatcher, SizeIsBounded) { // String-holding alternatives: one std::string + one bool rounded up to // pointer alignment. From 6564b19a9a078f8f56dbe4120ec9682795661a3f Mon Sep 17 00:00:00 2001 From: Ryan Northey Date: Tue, 28 Apr 2026 17:21:34 +0100 Subject: [PATCH 4/4] revert-dictionary Signed-off-by: Ryan Northey --- tools/spelling/spelling_dictionary.txt | 2 -- 1 file changed, 2 deletions(-) diff --git a/tools/spelling/spelling_dictionary.txt b/tools/spelling/spelling_dictionary.txt index 0fb9d1f35f6a7..de177c01c1ec8 100644 --- a/tools/spelling/spelling_dictionary.txt +++ b/tools/spelling/spelling_dictionary.txt @@ -3,7 +3,6 @@ # will accept title case (e.g. lyft matches Lyft). Prefixes (e.g., un-) or suffixes (e.g. -ing) # are allowed for any otherwise correctly spelled word. -acknowledgement Allshard FLUSHALL NUMPAT @@ -71,7 +70,6 @@ consume cstr deadcode DFP -discoverability Dynatrace DOM DONTFRAG