Add possibility to submit a list of changes to rocksdb #2619
+386
−138
Conversation
This file contains 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
AurelienFT
requested review from
xgreenx,
Dentosal and
MitchTurner
as code owners
5 tasks
|
Works but draft until mature enough to fit requirements of : #2618 |
## Description
<!-- List of detailed changes -->
Was looking into:
```rs
// We have to clone the prefix and start, because we need to pass them to the iterator
// if someone finds a solution without making it a vec, feel free to contribute :)
let column = column.id();
let prefix = prefix.map(|prefix| prefix.to_vec());
let start = start.map(|start| start.to_vec());
```
and did some refactoring to get to the bottom of the problem.
I don't know if there is a solution unless we decide to change the
signature of `iter_store_keys` to something like:
```rs
fn iter_store_keys<'a>(
&'a self,
column: Self::Column,
prefix: Option<&'a [u8]>,
start: Option<&'a [u8]>,
direction: IterDirection,
) -> BoxedIter<KeyItem>;
```
But that's not realistic in this PR.
Might as well keep the refactoring because I feel like it's cleaner.
MitchTurner
reviewed
Jan 30, 2025
Comment on lines
+152
to
+170
| StorageChanges::ChangesList(changes_list) => { | ||
| // We have to clone the prefix and start, because we need to pass them to the iterator | ||
| // if someone finds a solution without making it a vec, feel free to contribute :) | ||
| let column = column.id(); | ||
| let prefix = prefix.map(|prefix| prefix.to_vec()); | ||
| let start = start.map(|start| start.to_vec()); | ||
| changes_list | ||
| .iter() | ||
| .flat_map(move |changes| { | ||
| get_insert_keys_from_changes( | ||
| changes, | ||
| column, | ||
| prefix.as_deref(), | ||
| start.as_deref(), | ||
| direction, | ||
| ) | ||
| }) | ||
| .into_boxed() | ||
| } |
There was a problem hiding this comment.
IDK if this would be better or worse, but it avoids the to_vecs :P
Suggested change
| StorageChanges::ChangesList(changes_list) => { | |
| // We have to clone the prefix and start, because we need to pass them to the iterator | |
| // if someone finds a solution without making it a vec, feel free to contribute :) | |
| let column = column.id(); | |
| let prefix = prefix.map(|prefix| prefix.to_vec()); | |
| let start = start.map(|start| start.to_vec()); | |
| changes_list | |
| .iter() | |
| .flat_map(move |changes| { | |
| get_insert_keys_from_changes( | |
| changes, | |
| column, | |
| prefix.as_deref(), | |
| start.as_deref(), | |
| direction, | |
| ) | |
| }) | |
| .into_boxed() | |
| } | |
| StorageChanges::ChangesList(changes_list) => { | |
| let column = column.id(); | |
| changes_list | |
| .iter() | |
| .flat_map(move |changes| { | |
| get_insert_keys_from_changes( | |
| changes, column, prefix, start, direction, | |
| ) | |
| }) | |
| .collect::<Vec<_>>() | |
| .into_iter() | |
| .into_boxed() | |
| } |
There was a problem hiding this comment.
mehhh, i don't really like that because start and prefix are pretty small in most of the cases where the number of elements iterated on could be pretty big so iterating twice and creating a heap allocation with all elements will be more costly than 2 little allocation
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.
Description
The goal of this PR is to allow to insert a list of changes instead of one
HashMapof changes. This avoid multiple update produced by different threads for example, to be merged in one transaction.By removing this step we remove a whole iteration and insert/deletion in a big
HashMapof changes.It's currently not used anywhere in the code but you can see a usage example here : #2618
In this PR when we have to write history modifications we are still merging the changes in one big transaction before calling
store_modifications_historybecause I don't think it's possible to use this algorithm with changes splitted in different parts. @acerone85 if you have an idea I would be happy to here it but in any case I think this should go to follow-up PR to not overcharge this one. (I made an attempt in branch try_adapt_history_vec branch but it's mehhh)Checklist
Before requesting review