Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cloud_storage: segment_meta_cstore serde optimizations #10805

Merged

Conversation

andijcr
Copy link
Contributor

@andijcr andijcr commented May 16, 2023

previously to_iobuf was achieved by constructing a temporary cstore,
appending all the segments and serializing it. this is done so that
to_iobuf can be const, in turn to be able to use it in
base_manifest::serialize.

To minimize operations and memory, this commits adds the ability to
create a column_store that shares all the iobuf with the *this object,
and then serialize it.

sharing is done by constructing on top of deltafor_encoder::share()
const a series of unsafe_alias() const methods, responsible to construct
the subobjects.

this new schema saves time by not re-encoding the segment_meta, and
saves memory by sharing the underlying iobuf objects, to write them
directly in the final serde iobuf

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.1.x
  • v22.3.x
  • v22.2.x

Release Notes

  • none

@andijcr andijcr changed the title Feat/cstore/serde optimizations cloud_storage: segment_meta_cstore serde optimizations May 17, 2023
@andijcr andijcr added the area/cloud-storage Shadow indexing subsystem label May 17, 2023
@andijcr andijcr force-pushed the feat/cstore/serde_optimizations branch from c6f7eb6 to c1ee47f Compare May 17, 2023 21:51
@andijcr andijcr marked this pull request as ready for review May 17, 2023 21:53
@andijcr andijcr requested a review from Lazin May 17, 2023 21:53
@andijcr
Copy link
Contributor Author

andijcr commented May 18, 2023

@andijcr
Copy link
Contributor Author

andijcr commented May 18, 2023

@andijcr andijcr requested review from jcsp and dotnwat May 18, 2023 10:56
@andijcr andijcr force-pushed the feat/cstore/serde_optimizations branch from c1ee47f to 4685e37 Compare May 18, 2023 12:47
@andijcr
Copy link
Contributor Author

andijcr commented May 18, 2023

https://buildkite.com/redpanda/redpanda/builds/29377#01882f17-cd34-47bf-a778-f19dc0cd14df interesting failure:

rptest.tests.cloud_storage_timing_stress_test.CloudStorageTimingStressTest.test_cloud_storage_with_partition_moves
rptest.services.utils.BadLogLines: <BadLogLines nodes=docker-rp-18(1) example="ERROR 2023-05-18 14:19:07,100 [shard 1] cloud_storage - [fiber424 0cc65d23/kafka/test-topic/0_20/5632-5761-4213209-1-v1.log.1] - segment_chunk_data_source.cc:84 - failed to hydrate chunk starting at 0, error: seastar::abort_requested_exception (abort requested)">

edit: should be solved by #10852

the other is #10240

@andijcr andijcr force-pushed the feat/cstore/serde_optimizations branch from 4685e37 to 9d22464 Compare May 18, 2023 16:18
@andijcr
Copy link
Contributor Author

andijcr commented May 19, 2023

@andijcr andijcr force-pushed the feat/cstore/serde_optimizations branch from 9d22464 to 2c8bf3c Compare May 19, 2023 14:56
@andijcr
Copy link
Contributor Author

andijcr commented May 22, 2023

just a bit of benchmark:
on dev right now:

test                                      iterations      median         mad         min         max      allocs       tasks        inst
cstore_bench.column_store_serialize_result          86     6.290ms    11.559us     6.278ms     6.326ms  317674.205       0.000         0.0

with this pr:

test                                      iterations      median         mad         min         max      allocs       tasks        inst
cstore_bench.column_store_serialize_result         163   717.051us   402.663ns   716.021us   720.228us   41877.167       0.000         0.0

test size is 10000 segment_meta

Lazin
Lazin previously approved these changes May 24, 2023
@@ -733,6 +734,16 @@ class column_store
member_fields());
}

auto unsafe_alias() const -> column_store {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the naming here

@andijcr
Copy link
Contributor Author

andijcr commented May 24, 2023

fixed conflict

@andijcr andijcr requested a review from Lazin May 24, 2023 15:34
@Lazin
Copy link
Contributor

Lazin commented May 24, 2023

fixed conflict

looks like CI fails to rebase it

@andijcr andijcr force-pushed the feat/cstore/serde_optimizations branch from 244c4c0 to ce91f53 Compare May 24, 2023 16:24
@andijcr
Copy link
Contributor Author

andijcr commented May 24, 2023

rebase - second attempt

@andijcr
Copy link
Contributor Author

andijcr commented May 24, 2023

Failure is #10977

@andijcr andijcr force-pushed the feat/cstore/serde_optimizations branch from ce91f53 to f7ad42d Compare May 26, 2023 12:30
@andijcr
Copy link
Contributor Author

andijcr commented May 26, 2023

@andijcr
Copy link
Contributor Author

andijcr commented May 26, 2023

/ci-repeat

@andijcr andijcr force-pushed the feat/cstore/serde_optimizations branch 2 times, most recently from a16be1c to c818704 Compare May 31, 2023 14:41
previously to_iobuf was achieved by constructing a temporary cstore,
appending all the segments and serializing it. this is done so that
to_iobuf can be const, in turn to be able to use it in
base_manifest::serialize.

To minimize operations and memory, this commits adds the ability to
create a column_store that shares all the iobuf with the *this object,
and then serialize it.

sharing is done by constructing on top of deltafor_encoder::share()
const a series of unsafe_alias() const methods, responsible to construct
the subobjects.

this new schema saves time by not re-encoding the segment_meta, and
saves memory by sharing the underlying iobuf objects, to write them
directly in the final serde iobuf
@andijcr andijcr force-pushed the feat/cstore/serde_optimizations branch from c818704 to 549a837 Compare June 5, 2023 13:20
@andijcr
Copy link
Contributor Author

andijcr commented Jun 5, 2023

force push: rebase on dev

@andijcr
Copy link
Contributor Author

andijcr commented Jun 5, 2023

issues are
#11166
#11179
#8217
#10674
#8217
#11044

@piyushredpanda piyushredpanda merged commit edfe35f into redpanda-data:dev Jun 27, 2023
@andijcr andijcr deleted the feat/cstore/serde_optimizations branch June 27, 2023 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cloud-storage Shadow indexing subsystem area/redpanda
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants