container: introduce chunked vector

[rockwotj]

Introduce chunked_vector: like a fragmented_vector but with a better allocation profile for small vectors.

Additionally improve the implementation by making indexing faster (removing bounds checks) and adding a debug mode that checks for invalid iterator usage.

Backports Required

• none - not a bug fix
• none - this is a backport
• none - issue does not exist in previous branches
• none - papercut/not impactful enough to backport
• v23.3.x
• v23.2.x
• v23.1.x

Release Notes

• none

[BenPope]

Pretty cool.

I think it's worth adding reserve(), and perhaps capacity().

[travisdowns]

This is a great idea, but I would strongly encourage you to share as much implementation and testing as possible with fragmented_vector.

Proposal: you could still have the newly named chunked_vector class, but internally it's an ~empty class derived from the frag vector template, with an additional parameter indicating the variation in the growth behavior. We can combine all tests to work on both classes.

Any new methods or improvements could be in frag vector, perhaps with a few exceptions (e.g., maybe reserve() only makes sense on this new variation, so it would appear only there).

[rockwotj]

Force push: Fold test rewrite and benchmark into existing fragmented_vector.

Note to reviewers: please hold off, I'm still working on this. Will re-request reviews in a bit

[rockwotj]

I took a stab at what it looks like to not use the inner std::vector and it's complex and I'm pretty sure there is a leak somewhere (despite what lsan says).

6422007

Anyways I've chose not to do that here, but instead have introduced chunked_vector and kept the inner vectors.

[rockwotj]

Force push: rebase against dev because of wierd build failure about unused variables in code that hasn't been touched in months.

[rockwotj]

Force push: remove unused fields the build was complaining about

[rockwotj]

Force push: add GTEST arg to new test

[vbotbuildovich]

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/44319#018d42e7-acac-4772-aa56-92bdfe17a52c

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/44319#018d430e-db3a-451e-afe7-7ede2be65abf

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/44319#018d430e-db2f-45d3-a577-764938e18765

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/44336#018d4419-201a-48fa-9902-2e5e03d0a670

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/44336#018d4419-2017-4646-bdb0-47be76a2856b

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/44434#018d5620-6516-4b52-83a4-7356fbca5c6c

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/44434#018d5631-d0b2-4fef-93ad-2ed5fa437b70

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/44628#018d6856-b93e-43d5-85aa-55fb20ad2798

[rockwotj]

Force push: fix a bug in reserve with empty fragments. Correct class documentation for fragmented_vector

[rockwotj]

When would I prefer fragmented_vector over chunked_vector?

My argument is never. I think we should move all usages over to chunked_vector.

Would you be willing to create a PR that just does that?

I'm game for it being this one, but that might be more contentious, so feel free to create another that either switches everything out or basically ignores the template arg and removes the small and large instances? I wonder if shrink_to_fit is a real concern (I doubt it).

If anybody disagrees, please advise what would be required to convince you to switch all current uses of fragmented_vector to chunked_vector (or preferably, just making std::dynamic_extent the default).

I am happy to do this, but would like to do it async since I may be OOO any day now. So if it's okay with you I would like to merge this as is, then start the thread with the team to see what people think of switching everything over to this.

As for shrink to fit, there is only once usage of that method in the leadership balancer, I don't suspect it to be a big deal at the moment. There are ways to implement shrink to fit.

So another question; should chunked_vector, however it ends up being spelled, be preferred to std::vector as a default?

I think so, except for the situation where the performance is needed (you can't accept the indirection) and you know you will never have an oversized allocation. I think there are probably few if any use cases like this, but I don't know all the dark corners of Redpanda.

[BenPope]

LGTM.

I expect more than one passing review before merge.

[BenPope]

LGTM.

I expect more than one passing review before merge.

[rockwotj]

Thanks, I know @travisdowns is planning on looking too

[travisdowns]

Not a request for change, but does gtest not have handy assertions which can check > and then output a good error message which also includes the values that failed?

[rockwotj]

Not really in "helper" validation, otherwise if you're in a TEST macro you can directly use EXPECT_LT, etc, but for building helpers you're on the hook for all the logic, which is a little annoying, but at least AssertionResult has good support for usage in EXPECT_TRUE(...) .

See https://chromium.googlesource.com/external/github.com/google/googletest/+/HEAD/docs/advanced.md#predicate-assertions-for-better-error-messages

[travisdowns]

Can you document this public method?

On vector reserve() has at least two purposes; (a) performance and (b) iterator and pointer stability: if you reserve space for N elements it is guaranteed that you can push back N elements without breaking pointer stability.

If have the same or different or no guarantee around that we should document it.

We should document what it does on the performance size too.

[rockwotj]

I think to make things easier conceptually (especially for new folks) we should make the exact same guarantee as std::vector. To me this class should be "std::vector" but under the hood doesn't have oversized allocs.

[travisdowns]

Why don't we just make this method available only on the fixed-size vector if it does nothing in the dynamic variant?

[rockwotj]

Do you mean to reverse that? How would propose to do this? Make a subclass for the dynamic variant? I'm honestly hoping that the non-dynamic version goes away eventually.

[travisdowns]

Yes, I meant to reverse that.

You could use SFINAE/enable_if but the easiest is probably just static_assert.

[travisdowns]

(see suggestion above)

[travisdowns]

I'm honestly hoping that the non-dynamic version goes away eventually.

hard agree

[rockwotj]

Added a static assert for now.

[travisdowns]

So for chunked vector this relies on the first fragment going up to exactly the right amount without our explicit control?

Can't we just manage memory like we did before, with _capacity but with an additional condition when there is 1 frag && is_chunked_vector?

This would "solve" the problem mentioned in shrink_to_fit, and the over-allocation in reserve() which both can waste a fair amount of memory: having reserve is a really nice feature of chunked_vector.

This would also solve the problem on relying on the exactly behavior of std::vector to avoid overshoot: I'm not sure how possible that is especially as we add methods: sure it may double in general when growing, but with other types of mutation it seems hard to predict exactly what the vector will do (plus implementation may due some rounding/adjusting as we do ).

I'm thinking something along these lines:

    void add_capacity() {
        if constexpr (is_chunked_vector) {
            if (
              _frags.size() == 1 && _frags.back().capacity() < elems_per_frag) {
                auto new_cap = std::min(
                  elems_per_frag, _frags.back().capacity() * 2);
                _capacity += new_cap - _frags.back().capacity();
                _frags.back().reserve(new_cap);
                return;
            } else if (_frags.empty()) {
                _capacity = 10;
                _frags.emplace_back().reserve(_capacity);
                return;
            }
        }
        _frags.emplace_back().reserve(elems_per_frag);
        _capacity += elems_per_frag;
    }

    [[gnu::always_inline]] void maybe_add_capacity() {
        if (unlikely(_size == _capacity)) {
            add_capacity();
        }
    }

So yeah the downside is extra complexity in this method (but less elsewhere, maybe).

10 should be replaced by a constexpr initial capacity which is like how many elements fit in 32 bytes or something like that since doing many allocations for the first elements, when they are small is quite wasteful since the memory savings is slow as there is already a significant fixed overhead in the structure itself.

This benchmarks about the same as the existing one (using Lifo and Fifo tests only), though some of that may be related to other changes I made (emplace_back() over push_back(std::move(frag)) does save time).

[rockwotj]

So for chunked vector this relies on the first fragment going up to exactly the right amount without our explicit control?

Yupp. There are tests that the "right thing" happens, but agreed anything seems fragile that isn't re-creating the inner container to not be std::vector but something custom. That feels like a risker change that would probably take more care and time than I can devote at the moment.

but with other types of mutation it seems hard to predict exactly what the vector will do (plus implementation may due some rounding/adjusting as we do ).

Yeah in general there is not garentees by the stdlib, but practically we can test (or just not use the stdlib).

I've adapted this method, I think it is less error prone.

[rockwotj]

This would "solve" the problem mentioned in shrink_to_fit

Only when calling shrink_to_fit on _frags.size() == 1, otherwise if you have 5 fragments allocated, call shrink to fit on the last and then push back, you could get an overalloc.

[travisdowns]

@rockwotj wrote:

Yupp. There are tests that the "right thing" happens, but agreed anything seems fragile that isn't re-creating the inner container to not be std::vector but something custom.

Right, I don't want you to re-create the inner vector, but I think you can handle it by managing capacity along the lines I suggested. Current push looks good on this aspect modulo comments.

[travisdowns]

Some comments, the main thing is about the way we maintain the capacity of the first vector.

[rockwotj]

Force push: Respond to Travis' comments.

[travisdowns]

LGTM.

There's the oustanding discussion about whether to "remove" reserve() for !is_chunked_vector but I think it's a nit so add the static assert or not, up to you.

[travisdowns]

Excited about this change!

[rockwotj]

There's the oustanding discussion about whether to "remove" reserve() for !is_chunked_vector but I think it's a nit so add the static assert or not, up to you.

I removed it.

Thanks for the review! I'm excited too, and hopefully we can encourage usage of this everywhere we've been using chunked_fifo or the static fragmented_vector.

[rockwotj]

@travisdowns I had to revert the static_assert due to it breaking serde and some template magic that looks for reserve automatically (example). I'll leave the noop reserve in and hopefully it's a moot point because we will switch over to this from fragmented vector completely.

[travisdowns]

LGTM.