seastar icon indicating copy to clipboard operation
seastar copied to clipboard

metrics: change value_vector type to std::deque

Open andrwng opened this issue 1 year ago • 3 comments

We've run into an oversized allocation (over 1MB) coming from the metrics subsystem. A snippet of the backtrace is seen below:

 (inlined by) std::__1::__split_buffer<seastar::metrics::impl::metric_value, std::__1::allocator<seastar::metrics::impl::metric_value>&>::__split_buffer(unsigned long, unsigned long, std::__1::allocator<seastar::metrics::impl::metric_value>&) at /vectorized/llvm/bin/../include/c++/v1/__split_buffer:323
 (inlined by) std::__1::vector<seastar::metrics::impl::metric_value, std::__1::allocator<seastar::metrics::impl::metric_value>>::reserve(unsigned long) at /vectorized/llvm/bin/../include/c++/v1/vector:1503
 (inlined by) seastar::metrics::impl::get_values(int) at /v/build/v_deps_build/seastar-prefix/src/seastar/src/core/metrics.cc:422
auto seastar::prometheus::get_map_value(seastar::prometheus::metrics_families_per_shard&, int)::$_0::operator()<unsigned int>(unsigned int) const::'lambda'()::operator()() const at /v/build/v_deps_build/seastar-prefix/src/seastar/src/core/prometheus.cc:224

The vector being reserved in this case is the value_vector[1].

In a previous change[2] _current_metrics (returned by get_local_impl()->functions()) had its inner container swapped out for std::deque because we'd previously seen the container grow large enough to see oversized allocations there. However, get_values() reserves a value_vector of the same number of elements as each std::deque, so a similar problem remains.

Since these containers are going to have the same cardinality, this patch updates the value_vector to also be a std::deque.

[1] https://github.com/scylladb/seastar/blob/b73f5ffa9e1eb13e9f2c5afde58f70c0df6efc73/src/core/metrics.cc#L370-L371 [2] https://github.com/scylladb/seastar/commit/c4b1f70162652075ef43871824350aa4fc4893fe

andrwng avatar Jan 25 '24 17:01 andrwng

@amnonh Hello! Gentle nudge about this when you get the chance.

I'm happy to answer questions and provide additional context if needed. Alternatively, I could tag the reviewers from https://github.com/scylladb/seastar/pull/1741 as they may also have some context -- whatever is more convenient for you all.

andrwng avatar Feb 16 '24 01:02 andrwng

LGTM

amnonh avatar Feb 16 '24 11:02 amnonh

Thank you @amnonh! What is the protocol for getting this merged?

andrwng avatar Feb 16 '24 22:02 andrwng