Minor metrics memory optimizations #2554
Conversation
The list of registered metrics that each `metric_groups_impl` stores is represented by a `metric_id` per registered metric. `metric_id` is quite a large struct as it contains the full name and label set. Hence, this list causes a measurable memory overhead. Instead just store a shared_ptr to the registration itself which we can use to reference the `metric_id` and hence save a lot of memory per registration (16 bytes vs. 136+ bytes).
Each registered metric stores a shared pointer to the metrics impl such that the metrics impl stays alive while there are metrics still registered. This adds 16 bytes of overhead per `registered_metric`. To reduce that we can move the shared_ptr to `metric_groups_impl` which is alive as long as the registered metrics are alive.
| public: | ||
| metric_groups_impl() = default; | ||
| metric_groups_impl(); |
There was a problem hiding this comment.
This looks like a leftover from some mid-code changes; the contractor still uses the default implementation.
There was a problem hiding this comment.
Yes you are right. I had this at first when I tried initing the impl SP in the constructor directly but then had to move it because of the bug mentioned in the comment.
|
Overall, it's a nice optimization. It's not going to have a significant impact on memory consumption, but it's good and worth adding. |
| metric_groups_impl::~metric_groups_impl() { | ||
| for (const auto& i : _registration) { | ||
| unregister_metric(i->info().id); | ||
| } | ||
| } | ||
|
|
||
| metric_groups_impl& metric_groups_impl::add_metric(group_name_type name, const metric_definition& md) { | ||
| // We could just do this in the constructor but some metric groups (like the | ||
| // smp queue ones) are actually constructed on a different shard originally |
There was a problem hiding this comment.
That's a bug. Of course, you're not required to fix it as part of the series.
Yes, I do actually have a related patch for that as well which makes a big difference for us. I will put it up for review shortly. |
The metrics subsystem is quite a heavy memory user at high metric counts.
This PR features two smaller improvements reducing memory usage by removing some redundant storage. I have a higher impact change following that internalizes labels.
Two commits (details in commit):