c/topic_table: replaced partition metadata map with chunked_vector #16919
Conversation
|
/dt |
|
/dt |
|
/dt |
|
ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/45961#018e2cb1-27b3-4c79-b1c0-bb328053a234 |
mmaslankaprv
requested review from
ztlpn,
bharathv,
StephanDollberg and
rockwotj
|
This is cool, instead of using I guess this might not work non-default-constructible elements. |
There was a problem hiding this comment.
Really nice! I like this structure
I guess this might not work non-default-constructible elements.
I think we'd have to drop down into using uninitialized memory, which is honestly a lot of work and tricky to get right. I would be OK with requiring default constructable for values in this map too. I don't know how performance critical this structure's uses are.
| * or decrementing iterators. | ||
| */ | ||
| template<std::integral KeyT, typename ValueT> | ||
| class contiguous_range_map { |
There was a problem hiding this comment.
(very optional) alternative name to consider: sorted_vector_map
There was a problem hiding this comment.
i am happy to change the name, was struggling with the proper one. Anyone else has other ideas ?
There was a problem hiding this comment.
I like contiguous_range_map more, although it has a shortcoming in that nothing points that the range is 0-based.
| static_assert(std::copy_constructible<int_range_map::iterator>); | ||
| static_assert(std::copy_constructible<int_range_map::const_iterator>); | ||
|
|
||
| struct verifier { |
| using mapped_type = ValueT; | ||
|
|
||
| private: | ||
| using underlying_t = chunked_vector<std::optional<value_type>>; |
There was a problem hiding this comment.
what is the use of making it chunked_vector<std::optional<vaue_type>> instead of chunked_vector<std::optional<ValueT>>, for iterator we already have access to the index so we can construct the pair on the fly?
There was a problem hiding this comment.
we would always need a copy then. I was considering that but the API wasn't ergonomic and it was differnt than for the standard map
There was a problem hiding this comment.
Can you expand on this a bit more? I am wondering the same as Bharath. I don't think it being different to the standard map is a big issue but yeah if it's not ergonomic that's a different question.
we would always need a copy then
You mean in the iterator or somewhere different?
mmaslankaprv
force-pushed
the
fix-15610
branch
3 times, most recently
from
e6655ef
to
772f538
Compare
| using mapped_type = ValueT; | ||
|
|
||
| private: | ||
| using underlying_t = chunked_vector<std::optional<value_type>>; |
There was a problem hiding this comment.
Can you expand on this a bit more? I am wondering the same as Bharath. I don't think it being different to the standard map is a big issue but yeah if it's not ergonomic that's a different question.
we would always need a copy then
You mean in the iterator or somewhere different?
|
Could you also share the output of the benchmark please? |
mmaslankaprv
force-pushed
the
fix-15610
branch
2 times, most recently
from
14dbf75
to
fe17294
Compare
Contiguous range map is an associative sorted container backed by chunked_vector designed to efficiently store objects indexed with contiguous and limited range of integers. The container provides a wrapper around basic chunked vector that reassembles API of a standard map. The wrapper allows random inserts of elements to the map event tho this is not supported by standard vector. Underlying container is resized every time its size is not sufficient to account for emplaced key. The contiguous_range_map tolerates gap in the range of keys however number and size of gaps is proportional to performance penalty hit when incrementing or decrementing iterators. Signed-off-by: Michal Maslanka <michal@redpanda.com>
Signed-off-by: Michal Maslanka <michal@redpanda.com>
To avoid large allocations and still provide good performance, replaced the `absl::node_hash_map` containing per partition metadata with `contiguous_range_map` which is backed by chunked vector. Signed-off-by: Michal Maslanka <michal@redpanda.com>
|
/backport v23.3.x |
|
/backport v23.2.x |
|
Failed to create a backport PR to v23.2.x branch. I tried: |
|
Failed to create a backport PR to v23.3.x branch. I tried: |
A set of partition ids in each topic is a monotonically increasing sequence of numbers without gaps. This allows us to use a vector instead of an associative container to keep the partition metadata in topics table. Usage of
chunked_vectorprevents large memory allocations.Fixes: #15610
Benchmark results
std::mapabsl::btree_mapcontiguous_range_mapBackports Required
Release Notes