Improve the performance of the access to storage.#5744
Conversation
Instruction Count Benchmark Results
Deterministic metrics — reproducible across runs (34 benchmarks)
Cache-dependent metrics — expect fluctuations between runs (34 benchmarks)
|
| &mut self, | ||
| short_key: &[u8], | ||
| ) -> Result<Arc<RwLock<W>>, ViewError> { | ||
| self.try_load_view_mut(short_key).await |
There was a problem hiding this comment.
If they are identical, why do we need both try_load_view_mut and try_load_view_arc?
| if let Some(arc) = self.key_value_stores.get(application_id) { | ||
| return Ok(arc.clone()); | ||
| } | ||
| let arc = self.state.users.try_load_view_arc(application_id).await?; |
There was a problem hiding this comment.
I'm probably still misunderstanding the underlying problem, but:
Now that we call try_load_view_arc/try_load_view_mut instead of just try_load_view, we are inserting the child view into the reentrant collection view's internal map, aren't we? So is it even necessary to keep the map in the actor, too? Isn't just replacing try_load_view with try_load_view_mut enough to make the collection view effectively cache its children?
There was a problem hiding this comment.
It is very similar, but not exactly the same since we are instead storing the serialized key in the ReentrantCollectionView and the key itself in the KeyValueStoreView.
There was a problem hiding this comment.
And serializing the key (just an app ID, is it?) is taking that much time that it's worth introducing a separate map?
There was a problem hiding this comment.
The problem in my view is that there are a lot of storage calls. So, improving their speed is quite important. But yes, there is another way to address this: use the ByteReentrantCollectionView and pass the serialization.
But to be honest, I was hoping for more improvements: Avoiding the access to the entry in the BTreeMap altogether. But that proved impossible.
|
That PR brings benefit, but they are small. So, I prefer to kill it. |
2 participants
Motivation
Access to storage is done in the
execution_chain_actor. It always goes in twosteps of accessing first to the view and then loading doing the operation.
This is repetitive operation, already with the serialization. This is hitting us
quite hard because a contract would have a lot of read operations.
We used to have a cache of views in
ReentrantCollectionView. But we removed itbecause it is not great to put the cache in the views.
Proposal
Put it in the
ExecutionStateActor. We unfortunately cannot access by an index becauseof long lived services. So, for any query, we have to access via a
BTreeMap<_,_>.The views are accessed via
Arc<RwLock<KeyValueStoreView<C>>>. So, we exposesome of the internals of the
ReentrantCollectionViewto the user. That is a littleunfortunate.
Other approaches:
Arc<RwLock<KeyValueStoreView<C>>>in theSyncRuntimeContract. But it is a major change and for a start we do not even have aContextinSyncRuntimeContract. Soblock_onwould be needed to use.KeyValueStoreViewas a singleBTreeMap<Vec<u8>,Vec<u8>>stored as aRegisterView. This is a somewhat orthogonal approach but still something to consider.Test Plan
CI.
Release Plan
It can be put in
testnet_conway.Links
None