test: replace flaky StringMatcher.Memory with deterministic sizeof bounds

[phlax]

Replaces TEST_F(StringMatcher, Memory) with StringMatcher.SizeIsBounded.

The old test asserted tcmalloc page-level consumedBytes() against
hand-tuned ceilings, which produced ~5/1000 CI flakes and had to be
re-tuned on every tcmalloc/abseil/protobuf bump (#37782, #43467).

The new test bounds sizeof of each variant alternative and of
StringMatcherImpl itself, expressed in sizeof(std::string) /
sizeof(void*) for libc++ / libstdc++ portability. Same intent as
#37782, deterministic.

[phlax]

botsplanation:

StringMatcher.Memory was asserting tcmalloc page-level heap consumption against hard-coded byte ceilings (530176 + 5*8192, 15603776 + 10*8192). These constants are sensitive to size-class bucketing, span layout, and prior heap state — producing ~5/1000 flakes on gcc CI and requiring manual re-tuning on every tcmalloc/abseil/protobuf bump (PRs #37782, #43467). Simultaneously too tight (flakes on allocator noise) and too loose (can't detect small genuine regressions within the slack).

Changes

• Removed TEST_F(StringMatcher, Memory) and all EXPECT_MEMORY_LE / Memory::TestUtil::MemoryTest usage.

• Added compile-time static_asserts that enforce the structural invariant PR string-matcher: reduce per-matcher memory #37782 actually cared about — that each variant alternative carries only its required data and StringMatcherImpl doesn't retain the proto:

// String-holding alternatives: std::string + bool (padded) only.
static_assert(sizeof(Matchers::ExactStringMatcher) <= sizeof(std::string) + sizeof(void*), ...);
// ... same for Prefix, Suffix, Contains

// Pointer-holding alternatives: one unique_ptr only.
static_assert(sizeof(Matchers::RegexStringMatcher) <= 2 * sizeof(void*), ...);
static_assert(sizeof(Matchers::CustomStringMatcher) <= 2 * sizeof(void*), ...);

// StringMatcherImpl: 2 vtable ptrs + variant (largest alt + discriminant).
static_assert(sizeof(Matchers::StringMatcherImpl) <= sizeof(std::string) + 4 * sizeof(void*), ...);

Bounds use sizeof(std::string) and sizeof(void*) rather than raw byte counts, making them portable across libc++ (sizeof(std::string)==24) and libstdc++ (sizeof(std::string)==32).

• Added TEST_F(StringMatcher, SizeIsBounded) mirroring the static_asserts at runtime for test-runner visibility, with a comment block documenting the removed test's failure mode and the design intent.

• No BUILD changes needed — memory_test_utility_lib was never an explicit dep of matchers_test.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

• dl.google.com
  • Triggering command: /build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/embedded_tools/jdk/bin/java bazel(envoy) --add-opens=java.base/java.lang=ALL-UNNAMED -Xverify:none -Djava.util.logging.config.file=/build/bazel_root/base/javalog.properties -Dcom.google.devtools.build.lib.util.LogHandlerQuerier.class=com.google.devtools.build.lib.util.SimpleLogHandler$HandlerQuerier -XX:-MaxFDLimit -Djava.library.path=/build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/embedded_tools/jdk/lib:/build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/embedded_tools/jdk/lib/server:/build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/ -Dfile.encoding=ISO-8859-1 -Duser.country= -Duser.language= -Duser.variant= -Xmx3g -DBAZEL_TRACK_SOURCE_DIRECTORIES=1 -Djavax.net.ssl.trustStore=/tmp/custom-cacerts -Djavax.net.ssl.trustStorePassword=changeit -jar /build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/A-server.jar --max_idle_secs=10800 --noshutdown_on_low_sys_mem --connect_timeout_secs=30 (dns block)
  • Triggering command: /home/REDACTED/.cache/envoy-bazel/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/embedded_tools/jdk/bin/java bazel(envoy) --add-opens=java.base/java.lang=ALL-UNNAMED -Xverify:none -Djava.util.logging.config.file=/home/REDACTED/.cache/envoy-bazel/bazel_root/base/javalog.properties -Dcom.google.devtools.build.lib.util.LogHandlerQuerier.class=com.google.devtools.build.lib.util.SimpleLogHandler$HandlerQuerier -XX:-MaxFDLimit -Djava.library.path=/home/REDACTED/.cache/envoy-bazel/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/embedded_tools/jdk/lib:/home/REDACTED/.cache/envoy-bazel/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/embedded_tools/jdk/lib/server:/home/REDACTED/. 1/x64/lib/node_modules/npm/node_modules/@npmcli/run-script/lib/node-gyp-bin/node (dns block)
• ftpmirror.gnu.org
  • Triggering command: /build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/embedded_tools/jdk/bin/java bazel(envoy) --add-opens=java.base/java.lang=ALL-UNNAMED -Xverify:none -Djava.util.logging.config.file=/build/bazel_root/base/javalog.properties -Dcom.google.devtools.build.lib.util.LogHandlerQuerier.class=com.google.devtools.build.lib.util.SimpleLogHandler$HandlerQuerier -XX:-MaxFDLimit -Djava.library.path=/build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/embedded_tools/jdk/lib:/build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/embedded_tools/jdk/lib/server:/build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/ -Dfile.encoding=ISO-8859-1 -Duser.country= -Duser.language= -Duser.variant= -Xmx3g -DBAZEL_TRACK_SOURCE_DIRECTORIES=1 -Djavax.net.ssl.trustStore=/tmp/custom-cacerts -Djavax.net.ssl.trustStorePassword=changeit -jar /build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/A-server.jar --max_idle_secs=10800 --noshutdown_on_low_sys_mem --connect_timeout_secs=30 (dns block)
• mirror.bazel.build
  • Triggering command: /build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/embedded_tools/jdk/bin/java bazel(envoy) --add-opens=java.base/java.lang=ALL-UNNAMED -Xverify:none -Djava.util.logging.config.file=/build/bazel_root/base/javalog.properties -Dcom.google.devtools.build.lib.util.LogHandlerQuerier.class=com.google.devtools.build.lib.util.SimpleLogHandler$HandlerQuerier -XX:-MaxFDLimit -Djava.library.path=/build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/embedded_tools/jdk/lib:/build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/embedded_tools/jdk/lib/server:/build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/ -Dfile.encoding=ISO-8859-1 -Duser.country= -Duser.language= -Duser.variant= -Xmx3g -DBAZEL_TRACK_SOURCE_DIRECTORIES=1 -Djavax.net.ssl.trustStore=/tmp/custom-cacerts -Djavax.net.ssl.trustStorePassword=changeit -jar /build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/A-server.jar --max_idle_secs=10800 --noshutdown_on_low_sys_mem --connect_timeout_secs=30 (dns block)
• pkgconfig.freedesktop.org
  • Triggering command: /build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/embedded_tools/jdk/bin/java bazel(envoy) --add-opens=java.base/java.lang=ALL-UNNAMED -Xverify:none -Djava.util.logging.config.file=/build/bazel_root/base/javalog.properties -Dcom.google.devtools.build.lib.util.LogHandlerQuerier.class=com.google.devtools.build.lib.util.SimpleLogHandler$HandlerQuerier -XX:-MaxFDLimit -Djava.library.path=/build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/embedded_tools/jdk/lib:/build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/embedded_tools/jdk/lib/server:/build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/ -Dfile.encoding=ISO-8859-1 -Duser.country= -Duser.language= -Duser.variant= -Xmx3g -DBAZEL_TRACK_SOURCE_DIRECTORIES=1 -Djavax.net.ssl.trustStore=/tmp/custom-cacerts -Djavax.net.ssl.trustStorePassword=changeit -jar /build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/A-server.jar --max_idle_secs=10800 --noshutdown_on_low_sys_mem --connect_timeout_secs=30 (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

• Configure Actions setup steps to set up my environment, which run before the firewall is enabled
• Add the appropriate URLs or hosts to the custom allowlist in this repository's Copilot coding agent settings (admins only)

Original prompt

Background

TEST_F(StringMatcher, Memory) in test/common/common/matchers_test.cc was added in PR #37782 to guard against per-matcher memory regressions in StringMatcherImpl (the std::variant-based redesign).

It asserts that the tcmalloc-reported consumedBytes() after constructing 1000 matchers stays under a hard-coded ceiling (530176 + 5 * 8192 for prefix, 15603776 + 10 * 8192 for regex).

This test is fundamentally broken and is the source of recurring gcc CI flakes (~5/1000 reproduction rate, observed continuously on main since at least the start of the current dev cycle). Specifically:

1. It measures tcmalloc page-level consumption, which is sensitive to size-class bucketing, span layout, page heap fragmentation, allocator release policy, and prior heap state — none of which are properties of StringMatcherImpl.
2. The hard-coded constants are derived from a single point measurement on the original author's machine and have to be hand-tuned every time tcmalloc/abseil/protobuf is bumped (see deps: bump up tcmalloc version #43467 which pushed 15038016 → 15603776).
3. The + N * 8192 slack is an explicit acknowledgement that the signal has page-sized noise — so the test is simultaneously too tight (flakes on allocator noise) and too loose (won't detect a small genuine regression of, say, +16 bytes/matcher × 1000 = 16 KiB, which fits inside the slack).
4. It's not hermetic — both sub-blocks share the same heap, prior tests' state leaks in.
5. It catches the wrong thing: the actual property string-matcher: reduce per-matcher memory #37782 wanted to enforce was structural ("ExactStringMatcher doesn't carry an unused regex pointer; StringMatcherImpl doesn't keep a copy of the proto"), not "the heap is N bytes after 1000 constructions."

The recent failure mode, for reference:

test/common/common/matchers_test.cc:497: Failure
Expected: (regex_consumed_bytes) <= (15603776 + 10 * 8192), actual: 15710496 vs 15685696

— i.e. only ~24 KiB over a ~16 MiB ceiling.

Goal

Remove the flaky EXPECT_MEMORY_LE-based test and replace it with deterministic checks that actually enforce the structural property #37782 was supposed to guard. Do not just delete the test — the per-matcher-size invariant is real and worth testing; the implementation just needs to test it deterministically.

What to do

In source/common/common/matchers.h the relevant types are:

• ExactStringMatcher — holds std::string exact_; bool ignore_case_;
• PrefixStringMatcher — holds std::string prefix_; bool ignore_case_;
• SuffixStringMatcher — holds std::string suffix_; bool ignore_case_;
• ContainsStringMatcher — holds std::string contents_; bool ignore_case_;
• RegexStringMatcher — holds Regex::CompiledMatcherPtr regex_;
• CustomStringMatcher — holds StringMatcherPtr custom_;
• StringMatcherImpl — wraps absl::variant<...> of the above, plus two vtable pointers (it inherits ValueMatcher and StringMatcher).

The point of the redesign was that sizeof(StringMatcherImpl) should be roughly sizeof(largest variant alternative) + variant discriminator + vtable pointers — and crucially, that no alternative carries fields it doesn't need (e.g. ExactStringMatcher must not hold a RegexMatcher pointer; StringMatcherImpl must not hold a copy of the original proto).

Replacement test design

In test/common/common/matchers_test.cc, delete the existing TEST_F(StringMatcher, Memory) and replace it with the following deterministic checks. Put them in a new test (or two) — pick whatever names fit local conventions, e.g. StringMatcher.SizeIsBounded and StringMatcher.AllocationsScaleLinearly.

1. Compile-time static_assert size bounds

Add static_asserts — either inside an anonymous-namespace test in matchers_test.cc, or directly in source/common/common/matchers.cc if that's preferred for visibility — that pin the size of each variant alternative and of StringMatcherImpl itself. Use generous-but-tight bounds expressed in terms of sizeof(std::string), sizeof(void*), etc., not raw byte counts, so they are portable across libc++/libstdc++ and 32/64-bit. For example:

// ExactStringMatcher / PrefixStringMatcher / SuffixStringMatcher / ContainsStringMatcher
// must not carry anything beyond a std::string + a bool (rounded up to alignment).
static_assert(sizeof(ExactStringMatcher) <= sizeof(std::string) + sizeof(void*),
              "ExactStringMatcher unexpectedly grew; if you added a field, "
              "verify it is actually needed for all exact matchers.");
static_assert(sizeof(PrefixStringMatcher) <= sizeof(std::string) + sizeof(void*),
              "PrefixStringMatcher unexpectedly grew; ...");
// ... same for Suffix, Contains.

// RegexStringMatcher / CustomStringMatcher should hold only one pointer-sized member.
...

</details>

[phlax]

/retest flakey github

[ravenblackx]

This looks like an improvement, though it loses the "size with data in it" measuring that the old way was doing. However, the old way was measuring "size with data in it" in a way that, if the size-per-type went up by 4, the 8K of mystery slop space would mean the test wouldn't fail it would just become flaky. And it also was testing with 1000 different lengths of string, which would give unrealistic outcomes for e.g. changes to string types anyway, so while it might have drawn attention to the memory impact of a change, it would only be confusing, often meaningless attention. Plus it was flaky.