cloud_storage: serde for segment_meta_cstore #9102

andijcr · 2023-02-24T19:25:39Z

This PR builds upon #7898
Fixes #9232

Adds segment_meta_cstore::from_iobuf/to_iobuf via serde of the sub-objects.
most of the work is done via serde_fields and serde::envelope, but some manual work is done via serde_write/serde_read, to support std::list and absl::btree_map

std::array is added to the list of containers recognized by serde. serialization is done like other sequence containers, as size,[elements...]. Deserialization first checks the serialized size against std::tuple_size<std_array_t> and throws in case of mismatch. this is done to catch bugs where the size of the std::array is changed in code, but the writer didn't add decoding logic for older binary data. it's not meant to be a recoverable error.

a new unit test checks round-trip serialization/deserialization

a benchmark check serialization/deserialization against a memcpy baseline

test	iterations	median	mad	min	max	allocs
cstore_bench.column_store_serialize_baseline	1553	92.628us	64.712ns	92.469us	92.744us	38.000
cstore_bench.column_store_serialize_result	217	299.284us	449.493ns	298.768us	299.870us	7644.059
cstore_bench.column_store_deserialize_baseline	1043	272.181us	11.634ns	271.952us	272.606us	2507.000
cstore_bench.column_store_deserialize_result	215	36.598us	25.693ns	36.521us	36.624us	283.864

Backports Required

UX Changes

Release Notes

none

dotnwat · 2023-03-05T22:45:52Z

src/v/reflection/type_traits.h

@@ -18,6 +18,7 @@
 #include <seastar/core/circular_buffer.hh>


serde/serde.h: added support for std::array as size,elements (+ throw)

what does + throw mean?

It means that it will throw a serde_exception if the serialized size does not match the std::array size

dotnwat · 2023-03-05T22:49:53Z

src/v/serde/serde.h

@@ -287,6 +288,9 @@ void write(iobuf& out, T t);
 template<typename T>
 void write(iobuf& out, std::vector<T> t);

+template<typename T, size_t Size>


std::size_t is used in the concept, but size_t here

dotnwat · 2023-03-05T22:56:45Z

src/v/utils/delta_for.h

@@ -31,30 +31,33 @@ static constexpr uint32_t FOR_buffer_depth = 16;
 */
 struct delta_xor {


with a nttp value

what is nttp?

Acronym for Non-Type Template Parameter. It's a really bad name for a template parameter that is a value and not a type

ahh, of course. thanks

graphcareful · 2023-03-07T19:55:39Z

src/v/serde/serde.h

@@ -713,6 +729,20 @@ void read_nested(iobuf_parser& in, T& t, std::size_t const bytes_left_limit) {
        for (auto i = 0U; i < size; ++i) {
            t.push_back(read_nested<value_type>(in, bytes_left_limit));
        }
+    } else if constexpr (reflection::is_std_array<Type>) {


whats the implication of this? Curious i don't know myself but would the compiler generate a new branch of generated code here once per every type of std::array with a different size parameter?

but would the compiler generate a new branch of generated code here once per every type of std::array with a different size parameter?

yeh, that's the way i understand it. i think that alone is a good argument for not supporting std::array. OTOH, we probably have very few instances of it...

Maybe we should isntead support std::span<T, std::dynamic_extent>?

it's probably ok unless we have any good alternative ideas. the most important thing is that we keep an on-disk format that doesn't constrain us much. and i think we have here: its the same as vector, for exmaple.

yes std::span seems a good idea. let me try

https://gcc.godbolt.org/z/sf3GoeoGj did a quick test. gcc will produce reasonable code, but clang needs attribute((noinline)).
If it's ok i can use this code.
another option is to only add support for std::span in serde, and i can use serde_write/serde_read to convert std::array to std::span.
let me know if I should go with the first or the second option

Yeah i'd vote to keep it as it is

Lazin

nit: benchmark test for the serialisation will be very helpful going forward, otherwise LGTM

andijcr · 2023-03-08T22:36:26Z

nit: benchmark test for the serialisation will be very helpful going forward, otherwise LGTM

Since I have to fix conflicts and modify how std::array is serialized, I'll also add a benchmark test

serialization is done as std::vector, deserialization will check that size matches the std::tuple_size<std_array_t>, and will throw if different. This is done as a safety measure in case of a size change without a change in decoding logic of old binary data. It's meant to catch bugs during testing, not as a recoverable error.

this in done in a retro-compatible way, so as to not touch preexisting run-time use. delta_for_encoder can now be used with a runtime version of delta_* algo or with a nttp value. at the same time, delta_for_encoder and delta_stream_pos_t are serde-enabled, and serde is aware if delta_* is nttp or not, reducing slightly the amount of data used

serde is done naively via serde::envelope. std::list and absl::btree_map are not serde enabled, they are processed in their containing class via serde_write/read mirroring what it's done for other sequence/map types (size,[elements...]/[(key,value)...]). the nttp version of delta_* algorithm is used to remove the need to serde it. this has a cascading effect of removing the need for some user defined constructors. column_store::hint_vec_t is transformed from a std::tuple to a std::array since all the elements of the tuple were of type hint_t. memory and performance wise there should be no difference, but it's compatible with serde. a unit test is added that serializes and deserializes a segment_meta_cstore and checks the elements

andijcr · 2023-03-09T14:23:53Z

force push: rebase and benchmark for serialization.

andijcr · 2023-03-10T08:53:22Z

ducktape failure https://buildkite.com/redpanda/redpanda/builds/24707#0186c6dd-483a-457d-b52d-def05a028b21 issue is #9315

Lazin

LGTM

github-actions bot added the area/redpanda label Feb 24, 2023

andijcr force-pushed the feat/manifest-compression/serde branch from a345a36 to b4e8c4d Compare February 28, 2023 18:50

andijcr changed the title ~~Draft: manifest-compression serde addition~~ cloud_storage: serde for segment_meta_cstore Mar 3, 2023

andijcr force-pushed the feat/manifest-compression/serde branch from f75c1d8 to 71ae315 Compare March 3, 2023 11:47

andijcr marked this pull request as ready for review March 3, 2023 11:48

andijcr added the area/cloud-storage Shadow indexing subsystem label Mar 3, 2023

andijcr requested review from Lazin, jcsp and graphcareful March 3, 2023 11:49

dotnwat reviewed Mar 5, 2023

View reviewed changes

graphcareful reviewed Mar 7, 2023

View reviewed changes

Lazin reviewed Mar 8, 2023

View reviewed changes

andijcr added 4 commits March 9, 2023 11:15

tests/segment_meta_cstore_bench: segment_meta_cstore (de)serialization

ee99486

andijcr force-pushed the feat/manifest-compression/serde branch from 71ae315 to ee99486 Compare March 9, 2023 14:21

andijcr requested a review from Lazin March 9, 2023 14:31

Lazin approved these changes Mar 10, 2023

View reviewed changes

Lazin merged commit 27b2dc7 into redpanda-data:dev Mar 10, 2023

andijcr deleted the feat/manifest-compression/serde branch March 10, 2023 11:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cloud_storage: serde for segment_meta_cstore #9102

cloud_storage: serde for segment_meta_cstore #9102

andijcr commented Feb 24, 2023 •

edited

Loading

dotnwat Mar 5, 2023

andijcr Mar 8, 2023

dotnwat Mar 5, 2023

andijcr Mar 8, 2023

dotnwat Mar 5, 2023

andijcr Mar 8, 2023 •

edited

Loading

dotnwat Mar 13, 2023

graphcareful Mar 7, 2023

dotnwat Mar 7, 2023

dotnwat Mar 7, 2023

dotnwat Mar 7, 2023

andijcr Mar 8, 2023

andijcr Mar 8, 2023

graphcareful Mar 9, 2023

Lazin left a comment

andijcr commented Mar 8, 2023

andijcr commented Mar 9, 2023

andijcr commented Mar 10, 2023

Lazin left a comment

		@@ -31,30 +31,33 @@ static constexpr uint32_t FOR_buffer_depth = 16;
		*/
		struct delta_xor {

cloud_storage: serde for segment_meta_cstore #9102

cloud_storage: serde for segment_meta_cstore #9102

Conversation

andijcr commented Feb 24, 2023 • edited Loading

Backports Required

UX Changes

Release Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andijcr Mar 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lazin left a comment

Choose a reason for hiding this comment

andijcr commented Mar 8, 2023

andijcr commented Mar 9, 2023

andijcr commented Mar 10, 2023

Lazin left a comment

Choose a reason for hiding this comment

andijcr commented Feb 24, 2023 •

edited

Loading

andijcr Mar 8, 2023 •

edited

Loading