Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Implement TrieCache for validate_block #2432

Closed
bkchr opened this issue Apr 4, 2023 · 2 comments · Fixed by #2813
Closed

Implement TrieCache for validate_block #2432

bkchr opened this issue Apr 4, 2023 · 2 comments · Fixed by #2813
Assignees
Labels
I7-refactor Code needs refactoring. I9-optimisation An enhancement to provide better overall performance in terms of time-to-completion for a task. J0-enhancement An additional feature request.

Comments

@bkchr
Copy link
Member

bkchr commented Apr 4, 2023

When validating a block through validate_block the storage proof is loaded in a MemoryDb. This MemoryDb works as backend for the trie when accessing any data in the state. While implementing the TrieCache in Substrate the benchmarks have shown that reading the Trie by going to the backend/database was up to factor 10 slower. In case of validate_block we don't need to go to disc, as we have everything in memory. However, the experiments also showed that the constant decoding of trie nodes is also degrading the performance. So, we should implement support for using the TrieCache as well in validate_block. This will require some internal changes to Substrate to make the TrieCache in sp-state-machine generic. This didn't worked when implementing the TrieCache in Substrate, but now with GATs being available in Rust stable we can change Substrate to have a generic TrieCache. We should also provide our own implementation of the TrieCache trait in Cumulus that can be much simpler than the one we have in Substrate. We don't need any kind of ensuring that the data is not overflowing the memory, because we already have all data in memory and we can probably simplify a lot of other things.

As a perquisite we should implement the following benchmark: #2048
This should theoretically show some performance improvements after this issue is tackled and it would be nice to show this through this benchmark.

This should not only improve the performance of validate_block, but also should lead to an improved memory usage. If we look at the current situation. We have the storage proof stored in the MemoryDb and we need to decode every trie node every time we access it. This decoding requires memory allocations and memory copying and at the end we also copy the actual value to return it. With the TrieCache we will decode the nodes only once and every other access to the trie node should go through the cache. This means that we can implement a OneTimeReadBackend that removes the data from its internal store on the first read, because the cache should afterwards serve these requests. To be fully "memory optimized" this will require certain other changes in the trie crate to not return Vec<u8> and instead return trie::Bytes to prevent that we need to clone values when returning them from the trie, but this is some further optimization.

@bkchr bkchr added I7-refactor Code needs refactoring. I9-optimisation An enhancement to provide better overall performance in terms of time-to-completion for a task. J0-enhancement An additional feature request. labels Apr 4, 2023
@bkchr bkchr added this to SDK Node Apr 4, 2023
@github-project-automation github-project-automation bot moved this to Backlog 🗒 in SDK Node Apr 4, 2023
@bkchr
Copy link
Member Author

bkchr commented Apr 4, 2023

his will require some internal changes to Substrate to make the TrieCache in sp-state-machine generic. This didn't worked when implementing the TrieCache in Substrate, but now with GATs being available in Rust stable we can change Substrate to have a generic TrieCache.

We basically will need to change this here to have LocalCache be some kind of trait that looks like this:

trait LocalCache<H> {
     type TrieCache<'a>: TrieCache<H> + 'a;

     fn as_trie_db_cache<'a>(&'a self, storage_root: H) -> &Self::TrieCache<'a>;
}

That should be enough to support our requirements here.

@bkchr
Copy link
Member Author

bkchr commented Apr 4, 2023

If we would had: paritytech/polkadot-sdk#37

We could also change the signature of as_trie_db_cache to take &mut self and thus, not requiring any kind of internal mutability in TrieCache.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
I7-refactor Code needs refactoring. I9-optimisation An enhancement to provide better overall performance in terms of time-to-completion for a task. J0-enhancement An additional feature request.
Projects
Status: done
Development

Successfully merging a pull request may close this issue.

2 participants