stats: Simplify stats structures, part 1#7295
stats: Simplify stats structures, part 1#7295mattklein123 merged 8 commits intoenvoyproxy:masterfrom
Conversation
…f RawStatData. Signed-off-by: Joshua Marantz <jmarantz@google.com>
…e don't every GC unreferenced stats. 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>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
mattklein123
left a comment
There was a problem hiding this comment.
Nice. This is great. A couple of comments.
/wait-any
| @@ -0,0 +1,33 @@ | |||
| #pragma once | |||
There was a problem hiding this comment.
Can you make sure we have coverage for all the null stuff? I think this was the remaining thing in the stats coverage issue? Maybe just close that out as part of this PR?
There was a problem hiding this comment.
I think that null-stats missing coverage line was fixed in some other PR, and anyway is not evident in https://s3.amazonaws.com/lyft-envoy/coverage/report-master/coverage.html, though several other coverage-gaps in stats exist that are not related to this PR.
Can we address those in a separate PR?
|
|
||
| private: | ||
| HeapStatData& data_; | ||
| HeapStatDataAllocator& alloc_; |
There was a problem hiding this comment.
thought for consideration: If we are going all nutso on saving memory, 8 bytes for a reference to an effectively singleton allocator seems not worth it to me. I wonder if we should consider some type of scoped singleton here just for this case, same for gauge below. If you like this idea maybe a TODO/comment for later consideration?
There was a problem hiding this comment.
I think this 8 bytes is recoverable with a lot of work, but alas it is not a singleton. IsolatedStatsStore makes its own HeapStatDataAllocator object, so there can be lots of allocators in a process.
The recovery of this 8 bytes involves passing the allocator to lots of class methods that currently don't require them, notably having a private destructor and an explicit free(), but also Counter::name(), which is probably used a lot in tests. It's possible but quite a bit of work. There's a bunch of other things to do which are easier, with more potential benefit.
Description: Partially addresses #7109 by removing a layer of templating in the stat allocator, enabling better data-hiding. This doesn't change the memory layout; just moves data structures around a little.
Also changes a few places around the codebase where a gauge was being looked up by name as a counter. This doesn't really matter in the current codebase but will in a future revision when we add more separation between the implementations of counters and gauges.
Risk Level: low
Testing: //test/...
Docs Changes: n/a
Release Notes: n/a