string-matcher: reduce per-matcher memory#37782
Conversation
Signed-off-by: Adi Suissa-Peleg <adip@google.com>
Signed-off-by: Adi Suissa-Peleg <adip@google.com>
…rnative Signed-off-by: Adi Suissa-Peleg <adip@google.com>
Signed-off-by: Adi Suissa-Peleg <adip@google.com>
|
I've executed the router_config_impl_speed_test, using the following: and got the following results - showing only the mean of each 10 runs, and the second column is "diff%" (negative number implies that the new version is better - less time, positive number implies that the original version is better):
combined_route_config_impl_speed_test_10_mean.csv IMHO, and from running a few more tests, I've observed that these tests are somewhat noisy, and there's no clear winner. |
Signed-off-by: Adi Suissa-Peleg <adip@google.com>
…rnative Signed-off-by: Adi Suissa-Peleg <adip@google.com>
Signed-off-by: Adi Suissa-Peleg <adip@google.com>
Signed-off-by: Adi Suissa-Peleg <adip@google.com>
|
Do you have stats on the memory usage improvement? |
It depends :) |
|
Are you able to confirm that size reduction using a memory test a la |
|
/wait-any |
Seems that the stats test validates 2 things:
In the current PR we are replacing a class that has multiple data members with an std::variant (union). and And show that the memory is halved, but I'm not sure if that gains much. |
|
Apologies, @jmarantz I've replied to this comment above:
If you think that these are "strict" memory expectations I can add the tests. |
|
I missed that. I don't think it's "strict" but it is useful to get notified a change might increase memory usage. If that's justified then then author can change the expectation, but they'll be constant. And it will document more formally in our codebase what the memory use is. |
Signed-off-by: Adi Suissa-Peleg <adip@google.com>
yanavlasov
left a comment
There was a problem hiding this comment.
LGTM. Memory tests were added
|
@jmarantz I've added the memory tests in a3694e3 I found it interesting that when running locally I've received a non-canonical platform test skipped message: TBH, I'm not sure this has been actually executed by any test environment that uses the Just highlighting that this may be broken, and we should probably fix this... but not as part of this PR. |
|
Can you switch to using EXPECT_MEMORY_LE ? I think that should be better supported -- test it by making an impossibly low target and verifying it fails. |
Signed-off-by: Adi Suissa-Peleg <adip@google.com>
…rnative Signed-off-by: Adi Suissa-Peleg <adip@google.com>
I've updated the tests. Due to non-determinism of tcmalloc memory consumption estimation, I had to add a buffer to the tests thresholds estimations. |
This is similar to #37324 but doesn't use virtual functions (uses std::variant instead). These are the goals for this new StringMatcherImpl class: 1. improve encapsulation and use polymorphism that will make the code a bit more readable. 2. reduce the amount of memory used by the StringMatcher (e.g., an object of type ExactStringMatcher doesn't need to also hold a pointer to a regex object). 3. reduce the amount of memory that is essentially repeated (removing the kept protobuf matcher object that holds redundant information). 4. replace the uses of the [matcher()](https://github.com/envoyproxy/envoy/blob/9aad3320cf58a19cb4bbbbbaa357dbedf8715900/source/common/common/matchers.h#L157) that are needed to find the contents of the string-matcher by introducing the stringRepresentation() method. A [simple test](envoyproxy/envoy#37782 (comment)) showed that for 1000 perfix (non-regex) matchers, the new approach reduced the amount of memory from 570368 to 530176 bytes (~40KiB). For a case of 1000 regex-matchers with a varying length of prefix (1-1001), the new approach reduced the amount of memory from 15671616 to 15021632 bytes (~635KiB). Compared to #37324, this version enjoys the inlining compiler optimizations that can be made when virtual tables aren't used. The downside is the use of std::variant, which may require a bit more memory, and results in a more coupled code for the string matchers, but IMHO it improves upon the current implementation. Risk Level: low - not being used at the moment (but future PRs will). Testing: duplicated the tests needed Docs Changes: N/A Release Notes: N/A Platform Specific Features: N/A --------- Signed-off-by: Adi Suissa-Peleg <adip@google.com>
…asserts Remove the tcmalloc-consumedBytes-based TEST_F(StringMatcher, Memory) which was producing ~5/1000 flakes on gcc CI (page-level allocation jitter, hard-coded constants needing manual updates on every dep bump). Replace with: - Compile-time static_asserts on sizeof(ExactStringMatcher), PrefixStringMatcher, SuffixStringMatcher, ContainsStringMatcher, RegexStringMatcher, CustomStringMatcher, and StringMatcherImpl, expressed in terms of sizeof(std::string) + sizeof(void*) so they are portable across libc++/libstdc++ and 32-/64-bit builds. These fire deterministically at compile time if someone adds an unused field to any variant alternative. - A runtime TEST_F(StringMatcher, SizeIsBounded) that mirrors the static_asserts so the bounds are visible in test-runner output. - A comment block explaining why the old test was removed and what structural property the new assertions enforce, pointing back to PR #37782. Agent-Logs-Url: https://github.com/envoyproxy/envoy/sessions/6c75cfb3-7651-4bd2-9682-cb5b74325ac0 Co-authored-by: phlax <454682+phlax@users.noreply.github.com>
…unds (#44701) 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. --------- Signed-off-by: Ryan Northey <ryan@synca.io>
…unds (envoyproxy#44701) 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 (envoyproxy#37782, envoyproxy#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 --------- Signed-off-by: Ryan Northey <ryan@synca.io>
…unds (envoyproxy#44701) 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 (envoyproxy#37782, envoyproxy#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 --------- Signed-off-by: Ryan Northey <ryan@synca.io>
…unds (envoyproxy#44701) 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 (envoyproxy#37782, envoyproxy#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 --------- Signed-off-by: Ryan Northey <ryan@synca.io>
…unds (envoyproxy#44701) 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 (envoyproxy#37782, envoyproxy#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 --------- Signed-off-by: Ryan Northey <ryan@synca.io>
…unds (envoyproxy#44701) 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 (envoyproxy#37782, envoyproxy#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 envoyproxy#37782, deterministic. --------- Signed-off-by: Ryan Northey <ryan@synca.io>
…unds (envoyproxy#44701) 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 (envoyproxy#37782, envoyproxy#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 --------- Signed-off-by: Ryan Northey <ryan@synca.io>
…unds (envoyproxy#44701) 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 (envoyproxy#37782, envoyproxy#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 --------- Signed-off-by: Ryan Northey <ryan@synca.io>
…unds (envoyproxy#44701) 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 (envoyproxy#37782, envoyproxy#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 --------- Signed-off-by: Ryan Northey <ryan@synca.io>
…unds (#44701) 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 --------- Signed-off-by: Ryan Northey <ryan@synca.io>
…unds (#44701) 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. --------- Signed-off-by: Ryan Northey <ryan@synca.io>
Commit Message: string-matcher: reduce per-matcher memory
Additional Description:
This is similar to #37324 but doesn't use virtual functions (uses std::variant instead).
These are the goals for this new StringMatcherImpl class:
A simple test showed that for 1000 perfix (non-regex) matchers, the new approach reduced the amount of memory from 570368 to 530176 bytes (~40KiB).
For a case of 1000 regex-matchers with a varying length of prefix (1-1001), the new approach reduced the amount of memory from 15671616 to 15021632 bytes (~635KiB).
Compared to #37324, this version enjoys the inlining compiler optimizations that can be made when virtual tables aren't used. The downside is the use of std::variant, which may require a bit more memory, and results in a more coupled code for the string matchers, but IMHO it improves upon the current implementation.
Risk Level: low - not being used at the moment (but future PRs will).
Testing: duplicated the tests needed
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A