TrieDBMutBase added: no commit on drop
#226
Conversation
| [package] | ||
| name = "trie-db" | ||
| version = "0.30.0" | ||
| version = "0.31.0" |
There was a problem hiding this comment.
nit: I generally prefer having a dedicated "chore" release PR with no/minimal changes, we'll also need to add this PR to the trie-db changelog 🤔
There was a problem hiding this comment.
well, just copied the "modus operandi" from some other commit in this repo :), which makes a change and bumps version.
There was a problem hiding this comment.
I think that works too, proly need a few more bumps? 🤔
trie-db/src/triedbmut.rs
Outdated
| /// | ||
| /// Note that no changes are committed to the database until `commit` is called. | ||
| /// | ||
| /// Querying the root or dropping the instance will commit automatically. |
There was a problem hiding this comment.
Is the fn root() ++ fn drop() a noop in case no keys were added in-between?
In other words:
let mut trie = CommitOnDrop..;
trie.insert(key, val);
// T0. At this point we perform a `commit` under the hood?
let root = trie.root();
// T1. Do we redo the `commit` at this point although the trie is already commited and it suffered no changes?
drop(trie);There was a problem hiding this comment.
drop always calls commit, exactly as it was before this PR.
There was a problem hiding this comment.
and, yeah commit should be no-op at least from what I understand.
| /// implicitly. | ||
| /// | ||
| /// Refer to [`TrieDBMutBase`] for `Trie` implementation that does not perform `commit` on `drop`. | ||
| pub type TrieDBMut<'a, L> = CommitOnDrop<'a, L>; |
There was a problem hiding this comment.
Did you point polkadot-sdk to this branch an confirm everything still properly compiles, to make sure no unpleasant surprises appear after merge.
There was a problem hiding this comment.
Yes. But we still need to release and bump version in polkadot-sdk, so we have time to handle surprises.
|
|
||
| impl<'a, L> TrieDBMut<'a, L> | ||
| /// Wrapper around [`TrieDBMutBase`] that commits changes on drop. | ||
| pub struct CommitOnDrop<'a, L: TrieLayout>(TrieDBMutBase<'a, L>); |
There was a problem hiding this comment.
The naming here could be better CommitOnDrop doesn't tell you much this is still a TrieDbMutWithCommitOnDrop.
There was a problem hiding this comment.
Basically this one one-liner type is self-explanatory, but, yeah I can make it longer if you wish so. There is also legacy alias which is supposed to be used.
| { | ||
| fn drop(&mut self) { | ||
| self.commit(); | ||
| self.0.commit(); |
There was a problem hiding this comment.
Personally, I would've just went with a flag here on TrieDbMut, commit_on_drop,
if commit_on_drop self.0.commit(),
There was a problem hiding this comment.
I think both are fine, but I tend to lean towards flag approach. Simply because it should be less code to digest and inspect. And as reader a commit_on_drop flag is kind of easy to understand. Ofc default should keep the old behaviour.
Refactor
TrieDBMutto avoid implicit commit on drop.This PR refactors the
TrieDBMutimplementation to provide better control over when changes are committed to the database.Changes:
TrieDBMutwas split into two components:TrieDBMutBase: core trie functionality without automatic commit on drop,CommitOnDrop: a wrapper that provides the original auto-commit on drop behavior,TrieDBMutis now a type alias forCommitOnDropto maintain backward compatibility,build_base()method toTrieDBMutBuilderthat creates aTrieDBMutBaseinstance for cases where commit on drop is undesired.Part of: paritytech/polkadot-sdk#6020