feat: limit trie cache by memory consumption #7749

jakmeier · 2022-10-03T10:05:50Z

Instead of checking the number of values and their sizes, the caches are
now limited by the actual (approximated) memory consumption.

This changes what total_size in TrieCacheInner means, which is also
observable through Prometheus metrics.

Existing configuration works with slightly altered effects.
Number of entries convert to an implicit size limit. Since the explicit
default size limit currently is 3GB and the default max entries is set
50k, the implicit limit = 50k * 1000B = 50MB is stronger. This still
limits the number of largest entries to 50k but allows the cache to
be filled with more entries when the values are smaller.

For shard 3, however, where the number of entries is set to 45M in code,
the memory limit of 3GB is active. Since we change how this limit is
calculated we will see fewer entries cached with this change.
Shard 3 should still be okay since we have a prefetcher in place now
that works even when the cache is empty.

Instead of checking the number of value and their sizes, the caches are now limited by the actual (approximated) cache consumption. This changes what `total_size` in `TrieCacheInner` means, which is also observable through prometheus metrics. Existing configuration continues to work as it did, limits on number of entries are converted to an implicit size limit. Since the explicit default size limit currently is 3GB and the default max entries is set 50k, the implicit limit = 50k * 100B = 50kB is stronger. Therefore this change is not visible in practice for shard with default config. For shard 3, however, where the number of entries is set to 45M in code, the memory limit of 3GB is active. Since we change how this limit is, calculated we will see fewer entries cached with this change. Shard 3 should still be okay since we have a prefetcher in place now that works even when the cache is empty.

jakmeier · 2022-10-03T10:08:20Z

If we merge this change first, I can finalise #7578 without adding temporary code only to deal with capacity configuration that we want to ditch anyway.

matklad

Broadly ok, but left a couple of Qs

core/store/src/trie/config.rs

matklad · 2022-10-03T10:27:16Z

core/store/src/trie/trie_storage.rs

+        assert!(total_size_limit > 0);
+        // upper bound on capacity for allocation purposes only
+        let cache_capacity =
+            (total_size_limit + Self::PER_ENTRY_OVERHEAD - 1) / Self::PER_ENTRY_OVERHEAD;


Interesting! So in practice I think this means we always end up over-allocating the cache?

There's perhaps something smarter here (like dividing by 2 * per entry overhead) I don't immediately see an obviously better appraoch.

seems to make much more sense to just use unbounded lru cache now

matklad · 2022-10-03T10:28:27Z

core/store/src/trie/trie_storage.rs

        deletions_queue_capacity: usize,
        total_size_limit: u64,
        shard_id: ShardId,
        is_view: bool,
    ) -> Self {
-        assert!(cache_capacity > 0 && total_size_limit > 0);
+        assert!(total_size_limit > 0);


Could you sanity-check that the behavior is correct when total_size_limit is less than even a single entry?

I added a test that shows that we can still insert one value. I hope I understood your concern right and that addresses it, otherwise let me know.

core/store/src/trie/trie_storage.rs

unbounded cache means caches might use more memory in non-worst cases

jakmeier · 2022-10-04T15:14:46Z

Thanks for the review @matklad! I think I addressed all your comments.

cc also @Longarithm, to be very clear here, this change is going to have an effect on existing shard caches.

The old config of 50k per slot is now converted to 50k * 1kB = 50MB. If it means we fit in more than 50k (most likely we will) then there will be more entries in the shard cache now.
The old config for shard 3, 3GB / 45M slots, is still limited by 3GB, but now factoring in the 100B overhead. So it will effectively contain less in the cache.

I believe this should all be okay. But I am not going to merge this without your approval on these two specific changes @Longarithm :)

jakmeier · 2022-10-04T15:20:14Z

Note to self: Add a comment in CHANGELOG after rebasing

matklad

I'd delegate final review to @Longarithm (but here's ✔️ to avoid blocking)

Longarithm · 2022-10-14T12:48:53Z

I'll leave a review on Monday/Tuesday
UPD: let it be Wednesday because today I took another deep look on #7445, and there is no rush

Longarithm · 2022-10-20T12:47:52Z

core/store/src/trie/trie_storage.rs

    pub fn len(&self) -> usize {
        self.cache.len()
    }

+    /// Account consumed memory for a new entry in the cache.
+    pub(crate) fn add_value_of_size(&mut self, len: usize) {
+        let memory_consumed = len as u64 + Self::PER_ENTRY_OVERHEAD;


nit: it may make sense to make it a function entry_size(len) and to avoid duplication. We can even replace (TrieCacheInner::PER_ENTRY_OVERHEAD + TrieConfig::max_cached_value_size() as u64) with it while we support old config field.

Longarithm

LGTM. Is it right that after this cache for shard 3 will hold less entries, but we don't care much because we have prefetching as well?

core/store/src/trie/config.rs

- link todos to issues - Self::entry_size(len)

jakmeier · 2022-10-21T06:06:25Z

LGTM. Is it right that after this cache for shard 3 will hold less entries, but we don't care much because we have prefetching as well?

yes, that's exactly right

Instead of checking the number of values and their sizes, the caches are now limited by the actual (approximated) memory consumption. This changes what `total_size` in `TrieCacheInner` means, which is also observable through Prometheus metrics. Existing configuration works with slightly altered effects. Number of entries convert to an implicit size limit. Since the explicit default size limit currently is 3GB and the default max entries is set 50k, the implicit limit = 50k * 1000B = 50MB is stronger. This still limits the number of largest entries to 50k but allows the cache to be filled with more entries when the values are smaller. For shard 3, however, where the number of entries is set to 45M in code, the memory limit of 3GB is active. Since we change how this limit is calculated we will see fewer entries cached with this change. Shard 3 should still be okay since we have a prefetcher in place now that works even when the cache is empty.

jakmeier requested review from matklad and Longarithm October 3, 2022 10:05

jakmeier requested a review from a team as a code owner October 3, 2022 10:05

matklad reviewed Oct 3, 2022

View reviewed changes

jakmeier added 2 commits October 4, 2022 15:02

add comments, use unbounded lru cache

ee8434c

unbounded cache means caches might use more memory in non-worst cases

cargo fmt

bde903d

jakmeier requested a review from matklad October 4, 2022 15:14

matklad approved these changes Oct 4, 2022

View reviewed changes

Longarithm reviewed Oct 20, 2022

View reviewed changes

Longarithm approved these changes Oct 20, 2022

View reviewed changes

core/store/src/trie/config.rs Show resolved Hide resolved

jakmeier mentioned this pull request Oct 21, 2022

Remove deprecated trie_cache_capacities in store config #7894

Closed

reviewer findings

9d55cc2

- link todos to issues - Self::entry_size(len)

Merge branch 'master' into memory-limit-trie-cache

1b9f1c1

jakmeier added S-automerge and removed S-automerge labels Oct 21, 2022

update changelog

3da3801

jakmeier added the S-automerge label Oct 21, 2022

near-bulldozer bot merged commit faa3fed into near:master Oct 21, 2022

jakmeier mentioned this pull request Oct 21, 2022

feat: trie cache configuration #7578

Merged

jakmeier deleted the memory-limit-trie-cache branch October 28, 2022 19:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: limit trie cache by memory consumption #7749

feat: limit trie cache by memory consumption #7749

jakmeier commented Oct 3, 2022 •

edited

Loading

jakmeier commented Oct 3, 2022

matklad left a comment

matklad Oct 3, 2022

jakmeier Oct 4, 2022

matklad Oct 3, 2022

jakmeier Oct 4, 2022

jakmeier commented Oct 4, 2022

jakmeier commented Oct 4, 2022

matklad left a comment

Longarithm commented Oct 14, 2022 •

edited

Loading

Longarithm Oct 20, 2022 •

edited

Loading

Longarithm left a comment •

edited

Loading

jakmeier commented Oct 21, 2022

feat: limit trie cache by memory consumption #7749

feat: limit trie cache by memory consumption #7749

Conversation

jakmeier commented Oct 3, 2022 • edited Loading

jakmeier commented Oct 3, 2022

matklad left a comment

Choose a reason for hiding this comment

matklad Oct 3, 2022

Choose a reason for hiding this comment

jakmeier Oct 4, 2022

Choose a reason for hiding this comment

matklad Oct 3, 2022

Choose a reason for hiding this comment

jakmeier Oct 4, 2022

Choose a reason for hiding this comment

jakmeier commented Oct 4, 2022

jakmeier commented Oct 4, 2022

matklad left a comment

Choose a reason for hiding this comment

Longarithm commented Oct 14, 2022 • edited Loading

Longarithm Oct 20, 2022 • edited Loading

Choose a reason for hiding this comment

Longarithm left a comment • edited Loading

Choose a reason for hiding this comment

jakmeier commented Oct 21, 2022

jakmeier commented Oct 3, 2022 •

edited

Loading

Longarithm commented Oct 14, 2022 •

edited

Loading

Longarithm Oct 20, 2022 •

edited

Loading

Longarithm left a comment •

edited

Loading