Merge transactionnal state with uncommitted changes #28
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
bilboquet
force-pushed
the
mergeable_transactional_state
branch
from
92bcb49
to
69f6f96
Compare
Co-authored-by: Damir Vodenicarevic <[email protected]> Co-authored-by: AurelienFT <[email protected]> Co-authored-by: Jean-François <[email protected]> Signed-off-by: Jean-François <[email protected]>
bilboquet
force-pushed
the
mergeable_transactional_state
branch
from
69f6f96
to
7c65199
Compare
bilboquet
changed the title
tdelabro
approved these changes
Apr 18, 2024
src/trie/merkle_tree.rs
Outdated
| @@ -296,7 +292,7 @@ pub struct MerkleTree<H: StarkHash> { | |||
| /// The list of nodes that should be removed from the underlying database during the next commit. | |||
| death_row: Vec<TrieKey>, | |||
| /// The list of leaves that have been modified during the current commit. | |||
| cache_leaf_modified: HashMap<Vec<u8>, InsertOrRemove<Felt>>, | |||
| pub cache_leaf_modified: HashMap<Vec<u8>, InsertOrRemove<Felt>>, | |||
There was a problem hiding this comment.
If you only read it for reading, maybe a getter will be better, than pub
src/lib.rs
Outdated
Comment on lines
493
to
506
| //self.tries.db_mut().merge() work for all test but one | ||
| //(merge_with_uncommitted_remove, it fails with RocksDB.Busy error, | ||
| //which seems to be related to use of OptimisticTransactions, but | ||
| //removing them does not solve this particular problem but causes | ||
| //others). | ||
|
|
||
| //Applying changes after call to merge() would imply a big refactor as | ||
| //merge take ownership of its arguments, hence changes are not | ||
| //available anymore. | ||
|
|
||
| //Hence the solution which is a tradeoff between the two solutions: | ||
| //1. memorize changes | ||
| //2. merge tries | ||
| //3. apply changes |
There was a problem hiding this comment.
will remove, the code is self-explanatory
src/lib.rs
Outdated
Comment on lines
499
to
501
| //Applying changes after call to merge() would imply a big refactor as | ||
| //merge take ownership of its arguments, hence changes are not | ||
| //available anymore. |
There was a problem hiding this comment.
Do you want to create an issue for this? Or is it not even worth it?
There was a problem hiding this comment.
I don't think it worth it. And I think this whole comment is not really useful (it was for the internal review). I'm going to remove it.
Signed-off-by: Jean-François <[email protected]>
|
I'm merging this as there is a lot of perfs improvements that will come in the following days and don't want to block the process (given the fact that you already approved @tdelabro also) |
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.
@tdelabro @antiyro
Please find this work done at Massalabs that allows to merge transactional state with uncommitted changes into bonsai storage.
Co-authored-by: Damir Vodenicarevic [email protected]
Co-authored-by: AurelienFT [email protected]
Co-authored-by: Jean-François [email protected]