rm_stm: replace std::vector usage with fragmented_vector #9484
Conversation
bharathv
force-pushed
the
switch_to_frag
branch
2 times, most recently
from
d249ea0
to
690b50c
Compare
|
Awesome. ! Should we do this for the Kafka api layer as well. |
bharathv
force-pushed
the
switch_to_frag
branch
from
690b50c
to
ef9769f
Compare
bharathv
changed the title
+1. We had some recent work in #8469 |
|
/ci-repeat 10 |
| @@ -33,6 +34,14 @@ template<typename T, template<typename...> class C> | |||
| inline constexpr bool is_specialization_of_v | |||
| = is_specialization_of<T, C>::value; | |||
|
|
|||
| template<class T, template<class, size_t> class C> | |||
There was a problem hiding this comment.
serde.h depends on reflection/type_traits.h and defines the same constexprs so you may remove a definition there
bharathv
force-pushed
the
switch_to_frag
branch
from
ef9769f
to
5d6ff2b
Compare
|
Fixed another vector compat issue in fragmented_vector and tests passing locally. Force pushed. |
@dotnwat : can we have a tracking ticket for this? |
- Adds additional typedefs for vector compatibility. - Implement front() - Make push_back() accept a forwarding reference. - Defines a custom move c-tor / assignment that guarantees empty() after move similar to std::vector.
Uses the same underlying serialization as std::vector for compatibility. Adds a bounds check to limit vector size to int32_t_max which was an implicit assumption but a vector can be much bigger.
bharathv
force-pushed
the
switch_to_frag
branch
from
5d6ff2b
to
89e2b17
Compare
|
Most failures are this.. seems to be fixed.. retrying CI. |
|
/ci-repeat 2 |
|
CI Failure: #8496 (unrelated known issue) |
| @@ -907,7 +907,7 @@ ss::future<cloud_storage::upload_result> ntp_archiver::upload_tx( | |||
|
|
|||
| auto path = segment_path_for_candidate(candidate); | |||
|
|
|||
| cloud_storage::tx_range_manifest manifest(path, tx_range); | |||
| cloud_storage::tx_range_manifest manifest(path, std::move(tx_range)); | |||
There was a problem hiding this comment.
excellent. and since we have copies deleted we can also catch a bunch of inefficiencies!
| } | ||
| _ranges.shrink_to_fit(); | ||
| } | ||
| remote_segment_path spath, fragmented_vector<model::tx_range>&& range) |
| @@ -1922,7 +1922,7 @@ rm_stm::do_aborted_transactions(model::offset from, model::offset to) { | |||
| continue; | |||
| } | |||
| if (_log_state.last_abort_snapshot.match(idx)) { | |||
| auto opt = _log_state.last_abort_snapshot; | |||
| auto& opt = _log_state.last_abort_snapshot; | |||
There was a problem hiding this comment.
should probably be const auto&? before it was a value, so without going to look at filter_interesting having the reference be const here would be helpful.
| @@ -2825,11 +2825,13 @@ uint64_t rm_stm::get_snapshot_size() const { | |||
| return persisted_stm::get_snapshot_size() + abort_snapshots_size; | |||
| } | |||
|
|
|||
| ss::future<> rm_stm::save_abort_snapshot(abort_snapshot snapshot) { | |||
| auto filename = abort_idx_name(snapshot.first, snapshot.last); | |||
| ss::future<> rm_stm::save_abort_snapshot(abort_snapshot&& snapshot) { | |||
There was a problem hiding this comment.
please prefer pass-by-value, especially for coroutines.
|
/backport v23.1.x |
|
Failed to run cherry-pick command. I executed the below command: |
|
@bharathv : Let's not forget backport to v22.3.x, please? |
|
Just to close the loop here, as discussed offline, I already backported it manually via #9632 due to conflicts. |
|
v23.1.x backport: #9626 |
A lot of state in
rm_stmis backed bystd::vector. With frequent snapshotting + non trivial pid counts large contiguous allocations cannot be satisfied if the memory is fragmented. This patch switches to usingfragmented_vectorfor this usecase.Backports Required
Release Notes