fix: proper flat storage deltas removal #8718

Longarithm · 2023-03-13T10:28:56Z

Proper deltas removal during flat storage creation and normal operation. Resolves #8655.

The original issue was that we removed deltas exactly twice. It was already resolved around #8683. Reason: we cached blocks and deltas separately, and flat head was cached whereas flat head delta was not - as we don't need it. But we still iterated over all blocks during GC and executed store_helper::remove_delta for each block hash. This means that remove_delta was called twice: 1) when we move TO flat head; 2) when we move FROM flat head.

Now we call get_all_deltas_metadata on flat storage creation, guarantee that all cached deltas exist, and iterate over existing metadatas, not blocks.

However, now we need to GC deltas created during background creation. We do it in two steps:

once we caught up single block - its delta is not needed
when we create FS object - there can be some fork blocks remaining. We don't even want to load deltas for them anytime, so it's better do to cleanup right now. Theoretical estimation on their metadatas size is fairly small, and in practice we have 2-10 such blocks - so this step is actually needed. After cleanup, on my experiments there were only 2 deltas remaining on DB, as expected.

Side fix: tests revealed that when flat storage is removed, we don't remove deltas and it's better to do it.

At the same time I introduce CACHED_CHANGES_LIMIT and CACHED_CHANGES_SIZE_LIMIT to warn users that there is too much data cached in flat storage.

Testing

Adjusting test_flat_storage_creation_sanity to check that behaviour: create fork block in the meantime; check in logs that it is actually GC-d; check that previous behaviour fails the test.

pugachAG · 2023-03-16T16:32:10Z

core/store/src/flat/store_helper.rs

+pub fn remove_all_deltas(store_update: &mut StoreUpdate, shard_uid: ShardUId) {
+    let key_from = shard_uid.to_bytes();
+    let mut key_to = key_from;
+    key_to[7] += 1;


what if key_to[7] is u8::MAX_VALUE?
I suggest implementing fn next_shard_prefix(cur: &[u8]) -> Vec<u8> function which does +1 with proper carry over handling. Then we can re-use it for flat state removal as well.

I see. There would be no impact because we have < 256 shards, but I don't want it to suddenly fail at some point. Introduced ShardUId::next_shard_prefix(bytes) because it makes more sense as slice operation.

Side note: little-endian is quite annoying here, because I can't just increment shard id. Still we can rely on the fact that all shard uids are unique.

pugachAG · 2023-03-16T16:35:28Z

core/store/src/flat/storage.rs

+    /// Expected limits for in-memory stored changes, under which flat storage must keep working.
+    /// If they are exceeded, warnings are displayed. Flat storage still will work, but its
+    /// performance will slow down, and eventually it can cause OOM error.
+    const CACHED_CHANGES_LIMIT: usize = 50;


Why do we want to track this separately? As far as I understand there is no risk of having more than 50 small deltas cached and producing warn in this case is a false-alarm, at least in the context in flat storage. I suggest removing that and only tracking the total size of cached changes.

As far as I remember, there is a risk of node slowdown if there are too many deltas, even if they are small, so it is also worth a warning.

Please elaborate on what those risks are. In my understanding the bottleneck is hash map access, but then this value should be much larger: I expect main memory access to be ~100ns (which is a pessimistic case with processor cache miss). Even assuming we need 2 memory accesses for each hash map access and acceptable flat storage latency is 100us then we can still support 500 deltas without major performance degradation.
In general I'm against unnecessary tracking/warn because it introduces maintenance overhead for the future. If you insist on keeping that then please make sure to write an elaborate comment describing calculation behind the value as well as what could happen when we exceed it. For example current comment eventually it can cause OOM error. behind CACHED_CHANGES_LIMIT is very misleading, because that is only applicable to CACHED_CHANGES_SIZE_LIMIT

ah, looks like @jakmeier has just proven me wrong: #8006 (comment) and we can reasonable support around only around 100 deltas 😞

Tbh I didn't know exact numbers, but now I can happily add them to comments

Let's keep it at 100 and add a link to Jakob's table in the comment

chain/chain/src/flat_storage_creator.rs

pugachAG · 2023-03-17T09:12:24Z

core/store/src/flat/storage.rs

+    /// Expected limits for in-memory stored changes, under which flat storage must keep working.
+    /// If they are exceeded, warnings are displayed. Flat storage still will work, but its
+    /// performance will slow down, and eventually it can cause OOM error.
+    const CACHED_CHANGES_LIMIT: usize = 50;


Please elaborate on what those risks are. In my understanding the bottleneck is hash map access, but then this value should be much larger: I expect main memory access to be ~100ns (which is a pessimistic case with processor cache miss). Even assuming we need 2 memory accesses for each hash map access and acceptable flat storage latency is 100us then we can still support 500 deltas without major performance degradation.
In general I'm against unnecessary tracking/warn because it introduces maintenance overhead for the future. If you insist on keeping that then please make sure to write an elaborate comment describing calculation behind the value as well as what could happen when we exceed it. For example current comment eventually it can cause OOM error. behind CACHED_CHANGES_LIMIT is very misleading, because that is only applicable to CACHED_CHANGES_SIZE_LIMIT

pugachAG · 2023-03-17T09:13:27Z

core/store/src/flat/storage.rs

@@ -283,6 +291,15 @@ impl FlatStorage {
            block_hash,
            CachedFlatStateDelta { metadata: delta.metadata, changes: Arc::new(cached_changes) },
        );
+        let cached_changes_num_items = guard.metrics.cached_changes_num_items.get() as usize;


please don't use prometheus metrics data source, that is super hacky :) use cached_changes.len() and cached_changes.total_size() instead

Btw this line was even wrong. I wanted to count number of deltas, not number of items in them - fixed.

chain/chain/src/flat_storage_creator.rs

pugachAG

update_delta_metrics is pretty neat!
overall LGTM, thanks for addressing my comments.

pugachAG · 2023-03-17T14:59:33Z

core/store/src/flat/storage.rs

+        let cached_deltas = self.deltas.len();
+        let mut cached_changes_num_items = 0;
+        let mut cached_changes_size = 0;
+        for (_, changes) in self.deltas.iter() {


super nit: for changes in self.deltas.values() should work

Longarithm self-assigned this Mar 13, 2023

Longarithm linked an issue Mar 13, 2023 that may be closed by this pull request

Proper deletion of FlatStateDeltas #8655

Closed

proper deltas removal

8c9af96

Longarithm force-pushed the fs-fix-deletion branch from 9cae858 to 8c9af96 Compare March 16, 2023 11:39

Looogarithm added 6 commits March 16, 2023 15:52

Merge branch 'master' into fs-fix-deletion

be24755

fix merge

0985ca1

Merge branch 'master' into fs-fix-deletion

5572477

minor fix

6bf56e3

minor fix

ed45d54

better comment

04514fc

Longarithm changed the title ~~draft: proper deltas removal during creation~~ feat: proper flat storage deltas removal Mar 16, 2023

Longarithm changed the title ~~feat: proper flat storage deltas removal~~ fix: proper flat storage deltas removal Mar 16, 2023

Longarithm requested review from pugachAG and jakmeier March 16, 2023 12:27

Longarithm marked this pull request as ready for review March 16, 2023 12:27

Longarithm requested a review from a team as a code owner March 16, 2023 12:27

clippy fix

279b865

pugachAG reviewed Mar 16, 2023

View reviewed changes

pugachAG requested changes Mar 16, 2023

View reviewed changes

chain/chain/src/flat_storage_creator.rs Show resolved Hide resolved

shard prefix

4f1900d

Longarithm requested a review from pugachAG March 16, 2023 20:46

clippy

220af94

pugachAG requested changes Mar 17, 2023

View reviewed changes

Looogarithm added 4 commits March 17, 2023 15:52

log from separate variable

a629119

simplify metrics code

5eb6911

remove mut

57138bf

Merge branch 'master' into fs-fix-deletion

73c06b2

Longarithm requested a review from pugachAG March 17, 2023 13:38

pugachAG approved these changes Mar 17, 2023

View reviewed changes

Looogarithm added 2 commits March 17, 2023 20:53

nit

e2eff78

Merge branch 'master' into fs-fix-deletion

095f7c9

Longarithm added the S-automerge label Mar 17, 2023

near-bulldozer bot merged commit fddc418 into near:master Mar 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: proper flat storage deltas removal #8718

fix: proper flat storage deltas removal #8718

Longarithm commented Mar 13, 2023 •

edited

Loading

pugachAG Mar 16, 2023 •

edited

Loading

Longarithm Mar 16, 2023

pugachAG Mar 16, 2023

Longarithm Mar 16, 2023

pugachAG Mar 17, 2023

pugachAG Mar 17, 2023

Longarithm Mar 17, 2023

pugachAG Mar 17, 2023

pugachAG Mar 17, 2023

pugachAG Mar 17, 2023

Longarithm Mar 17, 2023

pugachAG left a comment

pugachAG Mar 17, 2023

fix: proper flat storage deltas removal #8718

fix: proper flat storage deltas removal #8718

Conversation

Longarithm commented Mar 13, 2023 • edited Loading

Testing

pugachAG Mar 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pugachAG left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Longarithm commented Mar 13, 2023 •

edited

Loading

pugachAG Mar 16, 2023 •

edited

Loading