Implement FlatStorageState #7663
Conversation
mzhangmzz
requested review from
matklad,
Longarithm,
jakmeier and
akhi3030
and removed request for
matklad
…as_between_blocks
chain/chain/src/chain.rs
Outdated
| shard_id, | ||
| store.head().unwrap().height, |
There was a problem hiding this comment.
Is there a reason this changed from ? to .unwrap()?
There was a problem hiding this comment.
Ah good call, changed back.
chain/chain/src/chain.rs
Outdated
| // Right now, we don't implement flat storage for catchup, so we only store | ||
| // the delta if we are not catching up | ||
| if !is_catching_up { | ||
| if let Some(chain_flat_storage) = | ||
| self.runtime_adapter.get_flat_storage_state_for_shard(shard_id) | ||
| { | ||
| let delta = FlatStateDelta::from_state_changes( | ||
| &apply_result.trie_changes.state_changes(), | ||
| ); | ||
| let store_update = chain_flat_storage.add_delta(&block_hash, delta)?; | ||
| self.chain_store_update.merge(store_update); | ||
| } | ||
| } | ||
| self.save_flat_state_changes( |
There was a problem hiding this comment.
Is the comment about catchup still relevant here?
There was a problem hiding this comment.
Yes, I'll move it to inside save_flat_state_changes.
| let mut chain = MockChain::linear_chain(10); | ||
| let store = create_test_store(); | ||
| let mut store_update = store.store_update(); | ||
| store_helper::set_flat_head(&mut store_update, 0, &chain.get_block_hash(0)); |
There was a problem hiding this comment.
Can we check in set_flat_head that flat state head is not set previously, or add a comment that it must be called only once? Though I am not sure how it will work with catchup logic.
Adding comment here because set_flat_head is unchanged in the PR.
There was a problem hiding this comment.
I'm not sure I understand what you are referring to. set_flat_head can be called many times, it is called every time when flat_head is updated. Are you worried that the function set_flat_head can be arbitrarily called in the code, not through update_flat_head?
There was a problem hiding this comment.
Are you worried that the function set_flat_head can be arbitrarily called in the code, not through update_flat_head?
Yeah, that's right. I'm thinking about making set_flat_head private, or explicitly saying that it must be called only inside update_flat_head or on initialization.
There was a problem hiding this comment.
I made it private and added another function set_flat_storage_state_for_genesis
| // TODO (#7327): implement garbage collection of old deltas. | ||
| // TODO (#7327): cache deltas to speed up multiple DB reads. | ||
| /// Get deltas between blocks `target_block_hash`(inclusive) to flat head(inclusive), | ||
| /// in backwards chain order. Returns an error if there is no path between these two them. |
There was a problem hiding this comment.
I think it would be helpful to say what "backwards chain order" means, it's not clear what "chain order" exactly refers to.
| @@ -392,38 +413,41 @@ struct FlatStorageStateInner { | |||
| /// State deltas for all blocks supported by this flat storage. | |||
| /// All these deltas here are stored on disk too. | |||
| #[allow(unused)] | |||
| deltas: HashMap<CryptoHash, FlatStateDelta>, | |||
| deltas: HashMap<CryptoHash, Arc<FlatStateDelta>>, | |||
There was a problem hiding this comment.
Should we combine blocks and deltas into a single hash map, so we don't need to look up twice?
| pub fn merge(&mut self, other: Self) { | ||
| self.0.extend(other.0) | ||
| pub fn merge(&mut self, other: &Self) { | ||
| self.0.extend(other.0.iter().map(|(k, v)| (k.clone(), v.clone()))) |
There was a problem hiding this comment.
nit: other.0.iter().cloned() should work?
| @@ -81,7 +100,7 @@ mod imp { | |||
| /// could charge users for the value length before loading the value. | |||
| // TODO (#7327): support different roots (or block hashes). | |||
| // TODO (#7327): consider inlining small values, so we could use only one db access. | |||
| pub fn get_ref(&self, key: &[u8]) -> Result<Option<ValueRef>, StorageError> { | |||
| pub fn get_ref(&self, key: &[u8]) -> Result<Option<ValueRef>, crate::StorageError> { | |||
| // Take deltas ordered from `self.block_hash` to flat state head. | |||
| // In other words, order of deltas is the opposite of the order of blocks in chain. | |||
| let deltas = self.flat_storage_state.get_deltas_between_blocks(&self.block_hash)?; | |||
There was a problem hiding this comment.
Will we move this to the (maybe lazy) initialization of FlatState? I don't imagine computing this for every single key would be very efficient.
There was a problem hiding this comment.
Yes, but flat_head could change during the lifetime of FlatState, so we can't remove the need for calling get_deltas_between_blocks all together. One optimization we could do is to store the value of flat_head and the deltas path last time when get_deltas_between_blocks is called and only recompute it if flat_head is changed. I'll create a JIRA issue to track this.
There was a problem hiding this comment.
I see, thanks. As a general feedback here I find the names a bit overloaded. Maybe some documentation text on how each of these structures are different / related could be helpful. For example, it's almost impossible to tell what the difference is between FlatStorageState and FlatState by just looking at the names (it's still fuzzy to me even after reading the code).
This PR implements FlatStorageState, the struct that manages deltas in flat storage. After this PR, the main implementation of flat storage is mostly ready. The next step is to add more tests.
This PR implements FlatStorageState, the struct that manages deltas in flat storage. After this PR, the main implementation of flat storage is mostly ready. The next step is to add more tests.
This PR implements FlatStorageState, the struct that manages deltas in flat storage. After this PR, the main implementation of flat storage is mostly ready. The next step is to add more tests.