perf/bg_trie_update update #4988
Conversation
Lines of code reportTotal lines added: Detailed view |
| last_id: usize, | ||
| layers: BTreeMap<H256, Arc<TrieLayer>>, | ||
| last_id: Arc<AtomicUsize>, | ||
| layers: Arc<Mutex<BTreeMap<H256, Arc<TrieLayer>>>>, |
There was a problem hiding this comment.
i think the mutex surrounding the btreemap is worth it (as is done now), since its only used to do the get and clone ASAP to unlock again. using an Arc<Mutex<Arc>> would be too complex IMHO
There was a problem hiding this comment.
This one forces the tree traversal to be done with the mutex taken. We want to avoid locking in the read path, that's why we would rather have a single outer lock and an Arc clone there.
| parent, | ||
| id: self.last_id, | ||
| } | ||
| id: last_id + 1, |
There was a problem hiding this comment.
this +1 is to maintain the same ids used before
There was a problem hiding this comment.
These kind of comments belong in the code.
| } | ||
| db.write(batch)?; | ||
| // Phase 3: update diff layers with the removal of bottom layer. | ||
| *trie_cache.lock().map_err(|_| StoreError::LockError)? = Arc::new(trie_mut); |
There was a problem hiding this comment.
This mutation needs to happen at the end. Otherwise you lose the bottom layer BEFORE there is a backing disk layer with the data visible to users..
| @@ -1,3 +1,5 @@ | |||
| #![allow(clippy::type_complexity)] | |||
2 participants
Currently the upstream branch has
trie_cache: Arc<Mutex<Arc<TrieLayerCache>>>,,where TrieLayerCache has the field
layers: BTreeMap<H256, Arc<TrieLayer>>,however from the changes it suggests the author wanted to use Arc<BTreeMap<H256, Arc>> here.And the stores have
Arc<Mutex<Arc<TrieLayerCache>>>,There are too many Arc layers here, it made some arc atomic swaps complex, for example to update the trielayercache one would need lock trie_cache, clone the arc, then clone the trielayrcache, then if the layers btreemap had a surrounding arc, another btreemap clone, get the trielayer, clone it, update the trie layer, put it in the cloned btree, update the layers btreemap, then update the trielayercache
Instead i made TrieLayerCache completely clone safe and the method have all the complexity of managing the arc swaps.
So users of the cache simply store a
trie_cache: Arc<TrieLayerCache>,and call get, put_batch, get_commitable, etc.If this design seems good, there are still things to do: