stats: Adds common stats memory testing infrastructure and tests for TLS and SymbolTable.#4714
Conversation
Signed-off-by: Joshua Marantz <jmarantz@google.com>
…mbol-table mem. Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
| namespace TestUtil { | ||
|
|
||
| // We can only test absolute memory usage if the malloc library is a known | ||
| // quantity. This decision is centralized here. |
There was a problem hiding this comment.
I'm not sure I follow this comment. It sounds like you're saying this feature only works if we're using TCMALLOC, but why not just say that? I might be missing something.
There was a problem hiding this comment.
Added to the comment:
// We can only test absolute memory usage if the malloc library is a known
// quantity. This decision is centralized here. As the preferred malloc
// library for Envoy is TCMALLOC that's what we test for here. If we switch
// to a different malloc library than we'd have to re-evaluate all the
// thresholds in the tests referencing ENABLE_MEMORY_USAGE_TESTS.
better?
|
|
||
| // This test only works if Memory::Stats::totalCurrentlyAllocated() works, which | ||
| // appears not to be the case in some tests, including asan, tsan, and mac. | ||
| if (Memory::Stats::totalCurrentlyAllocated() == 0) { |
There was a problem hiding this comment.
Does it make sense to run any of the above if this returns 0?
There was a problem hiding this comment.
Maybe not...actually I think I'll clean this up. There are now two tests:
- are we using tcmalloc
- does the malloc-stats library return anything interesting.
I think I'll just make it one functional test so that (a) we always compile the tests and (b) we never run them if they don't do anything.
…tats are robust at runtime. Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
| // thresholds in the tests referencing hasDeterministicMallocStats(). | ||
| #ifdef TCMALLOC | ||
| const std::string str("surely this will have to allocate some memory"); | ||
| return Memory::Stats::totalCurrentlyAllocated() != 0; |
There was a problem hiding this comment.
LGTM, though I suppose since this is a const std::string initialized from a string literal it could conceivably not allocate anything on the heap. It might be worth trying internally too, to make sure you get the expected results.
There was a problem hiding this comment.
Good point; I decided to make an explicit call to new char[] in a unique_ptr instead, and verify this increased the allocated memory by at least the amount allocated.
Signed-off-by: Joshua Marantz <jmarantz@google.com>
|
@mattklein123 this would be the most useful to look at first, though any suggested changes cannot be made until wednesday 10/24 |
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
Cool, just a small comment.
|
|
||
| void forEachStat(int num_clusters, std::function<void(absl::string_view)> fn) { | ||
| // These are stats that are repeated for each cluster as of Oct 2018. | ||
| static const char* cluster_stats[] = {"bind_errors", |
There was a problem hiding this comment.
I strongly suspect this could be auto generated using the same macro magic we use to define all the stats in prod code? Or if that doesn't work creating a cluster stat scope and seeing what stats are in it? Is it possible to look into that so we avoid this getting out of date?
There was a problem hiding this comment.
Seems possible, absolutely. The downside is that then the absolute numbers we compare against in teata will need to be updated with each new stat added
The purpose of this is not to fully enumerate all stats (so it is poorly named, mea culpa) but to provide a realistic data set to measure the relative memory impact of data structure choices.
I think maybe what I would prefer to do is to rename the function so it doesn't make false claims of completeness. Wdyt?
There was a problem hiding this comment.
Sure that's fine. I would probably rename the function and remove the dates then.
There was a problem hiding this comment.
ok ptal; not sure if the new name adds clarity but the doc is specific. suggestions welcome.
Signed-off-by: Joshua Marantz <jmarantz@google.com>
…eflect intent. Would appreciate suggestions on how to improve the name, if you have any. Signed-off-by: Joshua Marantz <jmarantz@google.com>
4 participants
Description: To get a unit-testable baseline for stat memory, adds tests for current state before a sequence of optimizations is added to the codebase. This relies on a new testing helper library function that enumerates the stat-names used by Envoy for a given number of clusters.
This is one step toward resolving #4196
Risk Level: low
Testing: //test/common/stats/...
Docs Changes: n/a
Release Notes: n/a