stats: Include a full log of stats memory changes in the memory test, and employ exact comparisons #6688
Conversation
…ons. Signed-off-by: Joshua Marantz <[email protected]>
| // 2019/04/12 6477 59576 Implementing Endpoint lease... | ||
| // 2019/04/23 6659 59512 Reintroduce dispatcher stats... | ||
|
|
||
| EXPECT_EQ(m_per_cluster, 59512); |
There was a problem hiding this comment.
Do we only run this on one particular build type and compiler? Just wondering how we can be so precise. Looks good otherwise.
There was a problem hiding this comment.
Well the compiler doesn't appear to matter that much, at least between g++ and clang. We do run only in one STL impl -- at one point in @cmluciano 's PR we were trying to work with two different libraries but abandoned that for reasons I forgot.
If any stability issues arise on any particular platform, we should adjust hasDeterministicMallocStats to skip that one.
There was a problem hiding this comment.
Maybe we use that, but then either: hasDeterministicMallocStats() returns false for some other reason (e.g. not using TCMALLOC) or libc++ has exactly the same memory usage for the operations this is measuring, as CI seems to work on Mac.
There was a problem hiding this comment.
I'll merge, but I think this whole situation is very fragile and will probably break new architectures/compilers. I hope we aren't using hasDeterministicMallocStats() for any compiler or STL with a different layout than the one we expect.
There was a problem hiding this comment.
Thanks Harvey -- I think it will be a pretty clear test failure if that happens that could be addressed by changing the definition of hasDeterministicMallocStats() (and maybe its name...maybe 'deterministic' is the wrong name for that....)
In the meantime, I've noticed since starting on this journey of trying to prune memory that it's bloated quite a bit and so I'm hoping this makes us more conscious of costs.
Description: Per @ambuc's comment on #6161 I think it would be better to keep a log of the memory consumed by stats, and also use exact comparisons. That way we can get historical perspective into the relative impact of adding new stats or families of stats.
Risk Level: low, but could cause more changes to this one test.
Testing: just this one test.
Docs Changes: n/a
Release Notes: n/a