kafka: oversized alloc in list_offsets_topic #23792
Conversation
IoannisRP
requested review from
rockwotj,
a team and
oleiman
and removed request for
a team
|
ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/56539#01929128-f2d3-453e-adf2-6e4f47a32ae3 |
| .then([name = std::move(topic.name)]( | ||
| std::vector<list_offset_partition_response> parts) mutable { | ||
| chunked_vector<list_offset_partition_response> parts) mutable { |
There was a problem hiding this comment.
how many entries were in the vector for it to get an oversized alloc? i wonder if instead of switching to a non-vector version, we should also be limiting parallelism? e.g. did we just spawn off like 10,000 background futures?
There was a problem hiding this comment.
I was reproducing it as 4000 partitions, which is an overkill.
Every future<list_offset_partition_response> is 120b. With warnings starting at 128kb, we could be seeing warnings from this area from a bit more than 1000 partitions.
src/v/ssx/when_all.h
Outdated
| template<typename Container> | ||
| concept emplace_backable = requires(Container c) { | ||
| typename Container::value_type; | ||
| c.emplace_back(std::declval<typename Container::value_type>()); | ||
| }; |
There was a problem hiding this comment.
To me, emplace_backable would look more like this (even though it's not required here):
| template<typename Container> | |
| concept emplace_backable = requires(Container c) { | |
| typename Container::value_type; | |
| c.emplace_back(std::declval<typename Container::value_type>()); | |
| }; | |
| template<typename Container, typename... Args> | |
| concept emplace_backable = requires(Container c, Args&&... args) { | |
| typename Container::value_type; | |
| c.emplace_back(std::forward<Args>(args)...); | |
| }; |
But unless you explicitly want to allow conversions, push_back feels like the right tool for the job (you have an ss::future<Container::value_type> that you want to move.
There was a problem hiding this comment.
push_back vs emplace_back: yes, it felt like an easy way to get a little bit more flexibility on what is allowed, conversions included.
on the emplace_backable:
This is also touched on a couple more comments, so i am replying here for everything.
The things that is actually required is to be able to add an element out of whatever comes out of the futures.
Looking at it again, this emplace_backable is asking if you can copy-emplace.
The combination of both of these tests is to ensure the compatibility of the two containers.
Instead of this emplace_backable and convertible it seems like a better idea to explicitly ask if the future::value_type is emplace-able to the output container, using your proposed version of emplace_backable. I will re-work it.
There was a problem hiding this comment.
My understanding about emplace_backable was somewhat flawed. I was expecting it that it would verify "in-depth" if it was valid. It turns out that
emplace_backable<std::vector<std::string>, int>
only checks that the expression
std::vector<std::string> vec;
vec.template emplace_back<int>(...);
is valid. Which is. It's the instantiation of this function that fails.
To protect against the above scenario, i kept the std::constructible_from check.
Let me know what you think about it.
src/v/ssx/when_all.h
Outdated
| && std::ranges:: | ||
| output_range<FutureRange, typename FutureRange::value_type> |
There was a problem hiding this comment.
I would expect that ResolvedContainer needs to be the output range, but you're not using it as an output_iterator, so it seems overconstrained.
|
Not sure if it makes sense to fix upstream seastar first or at all? Fragmentation is real for seastar users in general so they are more or less open to changes that break up large allocations. The main downside is in that project there are fewer chunked containers in the first place, though perhaps chunked_fifo is a fine-drop in here, not sure. |
|
FYI regarding commits, see: https://github.com/redpanda-data/redpanda/blob/dev/CONTRIBUTING.md |
Are you referring to the follow up changes? I am going to rebase and squash these in the end. Or is it something else? |
|
Just about the squashing and merging 👍 |
the idea was to solve it locally first and see at a later stage if it makes sense to upstream. I mainly wanted to make this configurable, instead of just hardcoding a |
There was a problem hiding this comment.
looks like commit history needs to be fixed up a bit.
this commit does not build with bazel.
commit e398316753ff3ae9ef8f248d09925ecce3f83584 (HEAD)
Author: Ioannis Kavvadias <ioannis.kavvadias@redpanda.com>
Date: Wed Oct 16 14:35:02 2024 +0100
Addressing PR comments:
- Updated emplace_backable concept
- Relaxed FutureRange requirements to input_range
- Added test for move-only types
src/v/ssx/when_all.h
Outdated
| requires seastar::Future<typename FutureRange::value_type> | ||
| && std::ranges::input_range<FutureRange> | ||
| && std::ranges:: | ||
| output_range<FutureRange, typename FutureRange::value_type> | ||
| && std::constructible_from< | ||
| typename ResolvedContainer::value_type, | ||
| decltype(std::declval<FutureRange>().begin()->get())> | ||
| && detail::emplace_backable<ResolvedContainer> | ||
| inline seastar::future<ResolvedContainer> | ||
| when_all_succeed(FutureRange futures) { | ||
| for (auto& fut : futures) { | ||
| fut = co_await seastar::coroutine::as_future(std::move(fut)); | ||
| } | ||
|
|
||
| std::remove_reference_t< | ||
| decltype(std::declval<FutureRange>().begin()->get())>> | ||
| && detail::emplace_backable< | ||
| ResolvedContainer, | ||
| std::remove_reference_t< | ||
| decltype(std::declval<FutureRange>().begin()->get())>> |
There was a problem hiding this comment.
This is a bit nitpicky, but the decltype(std::declval<>()) can be avoided, and some more range types can be satisfied by:
| requires seastar::Future<typename FutureRange::value_type> | |
| && std::ranges::input_range<FutureRange> | |
| && std::ranges:: | |
| output_range<FutureRange, typename FutureRange::value_type> | |
| && std::constructible_from< | |
| typename ResolvedContainer::value_type, | |
| decltype(std::declval<FutureRange>().begin()->get())> | |
| && detail::emplace_backable<ResolvedContainer> | |
| inline seastar::future<ResolvedContainer> | |
| when_all_succeed(FutureRange futures) { | |
| for (auto& fut : futures) { | |
| fut = co_await seastar::coroutine::as_future(std::move(fut)); | |
| } | |
| std::remove_reference_t< | |
| decltype(std::declval<FutureRange>().begin()->get())>> | |
| && detail::emplace_backable< | |
| ResolvedContainer, | |
| std::remove_reference_t< | |
| decltype(std::declval<FutureRange>().begin()->get())>> | |
| requires seastar::Future<std::ranges::range_value_t<FutureRange>> | |
| && std::ranges::input_range<FutureRange> | |
| && std::constructible_from< | |
| std::ranges::range_value_t<ResolvedContainer>, | |
| typename std::ranges::range_value_t<FutureRange>::value_type> | |
| && detail::emplace_backable< | |
| ResolvedContainer, | |
| typename std::ranges::range_value_t<FutureRange>::value_type> |
There was a problem hiding this comment.
Thinking about this some more; do we need to check that !std::ranges::borrowed_range<FutureRange> to ensure lifetime across the coroutine frame?
ss::future<> foo() {
std::vector<int> vals;
return ssx::when_all_succeed(std::views::as_const(vals));
}Or perhaps ensure the input range is an owning_view?
There was a problem hiding this comment.
borrowed_range seems like the wrong tool for this job. To (over)simplify, it checks whether an object is lvalue or not.
I think that what we want is to ensure that FutureRange is a Container. Especially the part that says
taking care of the management of the memory used by the objects it contains.
Funny enough, this is not described in technical requirements in the slightest.
To the best of my knowledge there isn't a way to identify if a pointer (or an iterator or range, by extention) is owning or not (apart from inspecting the code around it).
ss::when_all_succeed gets around this problem by having two (relevant) overloads
auto when_all_succeed(std::vector<future<T>>);
auto when_all_succeed(Iterator begin, Iterator end);
the vector version has the lifetime sorted, as vector is owning.
The range version moves the whole range into a vector.
For ssx::when_all_succeed, where the range version is all there is, it feels like a major pessimisation to have to move everything in a new container every time. Also it brings up the question of what container to use. Should it be hardcoded (and which) or should it be customisable.
Given the pros/cons, I think it's best to just add it as a precondition to the function, in the documentation, that the FutureRange needs to own the futures.
There was a problem hiding this comment.
For ssx::when_all_succeed, where the range version is all there is, it feels like a major pessimisation to have to move everything in a new container every time. Also it brings up the question of what container to use. Should it be hardcoded (and which) or should it be customisable.
Given the pros/cons, I think it's best to just add it as a precondition to the function, in the documentation, that the FutureRange needs to own the futures.
Fair enough. I was hoping there was an easy way to have the compiler check CP.53 for us, I also agree the pessimization may not be the right trade-off, but it can be really easy to get the lifetime wrong.
IoannisRP
force-pushed
the
ik-topic-offsets-mem-warning
branch
from
92909a9 to
b1738cd
Compare
The new "ssx::when_all_succeed" utility functions have the same functionality as the seastar ones, but they are not restrained to using std::vector. Any vector-like container can be used for both input and output.
IoannisRP
force-pushed
the
ik-topic-offsets-mem-warning
branch
from
b1738cd to
efde79a
Compare
|
Added lifetime-management precondition comment for |
|
non flaky failures in https://buildkite.com/redpanda/redpanda/builds/56803#01929f60-c156-4208-b244-835b3a9fbe2b: non flaky failures in https://buildkite.com/redpanda/redpanda/builds/56803#01929f60-c15c-4ab6-b7a8-d371ef18565f: non flaky failures in https://buildkite.com/redpanda/redpanda/builds/56803#01929f60-c158-49f9-8178-ac73b78b405e: non flaky failures in https://buildkite.com/redpanda/redpanda/builds/56803#01929f60-c15a-4488-bb32-d283fdceadc9: non flaky failures in https://buildkite.com/redpanda/redpanda/builds/56803#01929f64-44dc-40b6-9af9-08df621958c9: non flaky failures in https://buildkite.com/redpanda/redpanda/builds/56803#01929f64-44de-44bc-987d-89ddf95eafc4: non flaky failures in https://buildkite.com/redpanda/redpanda/builds/56803#01929f64-44dd-42b3-83b3-14710ee0ffe0: non flaky failures in https://buildkite.com/redpanda/redpanda/builds/56803#01929f64-44da-49fd-8619-8601f00bb61c: |
Retry command for Build#56803please wait until all jobs are finished before running the slash command |
|
/backport v24.2.x |
|
/backport v24.1.x |
|
/backport v23.3.x |
|
Failed to create a backport PR to v24.1.x branch. I tried: |
|
Failed to create a backport PR to v23.3.x branch. I tried: |
|
Failed to create a backport PR to v24.2.x branch. I tried: |
Fixes: CORE-7778
std::vectorinstances withchunked_vector.seastar::when_all_succeedis hardcoded to use astd::vectorinternally to receive/store the input range and astd::vectorto return the result. As this was causing further "big allocations", anssx::when_all_succeedutility is introduced that can accept any type of vector-like input range and can output to any type of vector-like container.No tests where added in respect to oversized allocation memory warnings. Results where only confirmed locally.
Backports Required
Release Notes