compaction: Reduce record copying in compaction read paths#31018
Open
WillemKauf wants to merge 4 commits into
Open
compaction: Reduce record copying in compaction read paths#31018WillemKauf wants to merge 4 commits into
compaction: Reduce record copying in compaction read paths#31018WillemKauf wants to merge 4 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR reduces record copying in compaction paths (local storage + cloud topics) by introducing key-only record iteration (record_batch::for_each_record_key{,_async}) backed by a new lightweight model::record_key_metadata and parser.
Changes:
- Add
record_key_metadata+parse_record_key_from_buffer()and expose key-only record iteration APIs onmodel::record_batch. - Switch compaction/deduplication reducers and filters to use key-only iteration to avoid full record materialization.
- Add focused unit tests verifying correct key-only parsing/iteration behavior and update build deps accordingly.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/v/storage/tests/segment_deduplication_test.cc | Updates test predicates to accept record_key_metadata instead of full records. |
| src/v/storage/segment_utils.h | Switches compaction decision helpers to use key-only metadata + adjusts logging. |
| src/v/storage/segment_utils.cc | Updates record callbacks to use record_key_metadata fields. |
| src/v/storage/segment_deduplication_utils.cc | Uses key-only metadata for deduplication key/offset checks. |
| src/v/storage/compaction_reducers.h | Changes reducer callback types to consume record_key_metadata. |
| src/v/storage/compaction_reducers.cc | Migrates reducers to for_each_record_key_async and moves keys into index/map. |
| src/v/model/tests/record_batch_test.cc | Adds deterministic tests for key-only parsing and async stop behavior. |
| src/v/model/tests/BUILD | Adds deps needed by new key-parse tests. |
| src/v/model/record.h | Adds for_each_record_key and for_each_record_key_async APIs. |
| src/v/model/record_utils.h | Introduces record_key_metadata and key-only parse function declaration. |
| src/v/model/record_utils.cc | Implements parse_record_key_from_buffer() with value/header skipping. |
| src/v/compaction/utils.h | Adds new overload of is_latest_record_for_key for pre-extracted key/offset. |
| src/v/compaction/utils.cc | Implements overload and refactors existing overload to call it. |
| src/v/cloud_topics/level_one/maintenance/compaction/compaction_source.cc | Migrates map-building reducer to key-only iteration. |
| src/v/cloud_topics/level_one/maintenance/compaction/compaction_filter.h | Refactors filter API to operate on key/offset/tombstone metadata. |
| src/v/cloud_topics/level_one/maintenance/compaction/compaction_filter.cc | Implements key-only filtering and offset-delta selection. |
| src/v/cloud_topics/level_one/maintenance/compaction/BUILD | Adds compaction key library dependency. |
Add `parse_record_key_from_buffer()` and `record_key_metadata`, which read a record's offset delta, tombstone flag and key while skipping the value and headers. Also add `record_batch::for_each_record_key()` and `for_each_record_key_async()` to iterate over a batch that way. From a benchmark (not checked in) of full record parsing (performed with `batch.for_each_record()`) versus key-only (performed with `batch.for_each_record_key()`): ``` ┌────────────┬──────────┬──────────┬─────────┬──────────────────────────┐ │ value size │ full │ key-only │ speedup │ allocs (full → key-only) │ ├────────────┼──────────┼──────────┼─────────┼──────────────────────────┤ │ 64 B │ 44.1 ns │ 20.2 ns │ 2.2× │ 2.1 → 0 │ ├────────────┼──────────┼──────────┼─────────┼──────────────────────────┤ │ 512 B │ 45.2 ns │ 19.9 ns │ 2.3× │ 2.1 → 0 │ ├────────────┼──────────┼──────────┼─────────┼──────────────────────────┤ │ 4 KiB │ 63.1 ns │ 19.7 ns │ 3.2× │ 2.1 → 0 │ ├────────────┼──────────┼──────────┼─────────┼──────────────────────────┤ │ 16 KiB │ 140.7 ns │ 20.6 ns │ 6.8× │ 2.1 → 0 │ └────────────┴──────────┴──────────┴─────────┴──────────────────────────┘ ```
0087cea to
33d3d06
Compare
Collaborator
CI test resultstest results on build#86733
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reduces the amount of record copying we perform in cloud topics compaction and local storage by adding
record_batch::for_each_record_key(), which only materializes a record's key, offset delta and indication that the record is a tombstone via a new typerecord_key_metadata.This is useful for a number of passes in local storage compaction and cloud topics compaction.
In an ad-hoc, end to end cloud topics L1 compaction reducer benchmark (not checked in):
And from a benchmark (not checked in) of full record parsing (performed with
batch.for_each_record()) versus key-only (performed withbatch.for_each_record_key()):Backports Required
Release Notes
Improvements