feat: trie cache configuration #7578

jakmeier · 2022-09-07T11:26:45Z

Make the shard cache max total bytes configurable.
Also add separate configuration for view caches.

This deprecates the old format for configuring cache capacity.
The old format will still work for now but values in the new format
will overwrite any values set in the old format.

Example of the new format, that sets all normal caches to 50MB, aurora's shard to 100MB, shard 3 to 3GB, and view caches to 30MB:

{
  "trie_cache": {
    "default_max_bytes": 50000000,
    "per_shard_max_bytes": {
      "shard1.v1": 10000000,
      "shard3.v1": 3000000000
    }
  },
  "view_trie_cache": {
    "default_max_bytes": 30000000
  }
}

resolves #7564

Make the shard cache max total bytes configurable. Also add separate configuration for view caches. This deprecates the old format for configuring cache capacity. The old format will still work for now but values in the new format will overwrite any values set in the old format, Example of the new format: ```json { "trie_cache": { "shard3.v1": { "max_entries": 45000000, "max_bytes": 3000000 } }, "view_trie_cache": { "shard0.v1": { "max_entries": 1, "max_bytes": 1 }, "shard1.v1": { "max_entries": 1, "max_bytes": 1 }, "shard2.v1": { "max_entries": 1, "max_bytes": 1 }, "shard3.v1": { "max_entries": 45000000, "max_bytes": 3000000 } } } ``` resolves near#7564

core/store/src/config.rs

matklad · 2022-09-08T11:45:30Z

core/primitives/src/shard_layout.rs

+        let (shard_str, version_str) = s
+            .split_once(".")
+            .ok_or_else(|| format!("shard version and number must be separated by \".\""))?;
+        if !version_str.starts_with("v") {


strip_prefix

using strip_prefix now but not sure if there is a more elegant way...

I guess ok_or_else makes it quite a bit cleaner

matklad

LGTM, but consider if it is worth to switch to size-only bound (which admitedly inflates the scope of work here quite a bit, but also user-visible configs are annoying to change later, so it makes sense to upfront-design this a bit)?

core/primitives/src/shard_layout.rs

core/store/src/config.rs

matklad · 2022-09-08T13:26:13Z

core/store/src/config.rs

+    /// Limit the number trie nodes that fit in the cache.
+    pub max_entries: Option<u64>,
+    /// Limit the sum of all value sizes that fit in the cache.
+    pub max_bytes: Option<u64>,


TBH, after the investigation into trie sieze which showed that we have about 100 bytes-per-entry overhead, I am leaning stronger that we should configure just the size in bytes, and limit the number of entries by saying that each entry is 100 bytes + val.len() in size.

This also makes sense from the user's pov: it's clear what 2gig of cache means. IN contrast, 2mil entries is opaque.

@Longarithm What do you think? Would it make sense to only have a configuration for total bytes and drop the number of entries config?

core/primitives/src/shard_layout.rs

jakmeier · 2022-09-19T16:25:36Z

TBH, after the investigation into trie sieze which showed that we have about 100 bytes-per-entry overhead, I am leaning stronger that we should configure just the size in bytes, and limit the number of entries by saying that each entry is 100 bytes + val.len() in size.

This also makes sense from the user's pov: it's clear what 2gig of cache means. IN contrast, 2mil entries is opaque.

Hey @Longarithm, what do you think about this? I am undecided here but tend towards @matklad's suggestion. Which is to change user-facing configuration of shard cache sizes to be based on total bytes only. This means we would remove the number of entries configuration. (Or rather, deprecate it for now and remove it later.)

The only argument to keep both config options that I can see here is to have some consistency in how the shard cache behaves. While we obviously need this for the chunk cache, I don't think it's important in the shard cache, right?

Longarithm · 2022-09-30T10:29:26Z

If we measure the size of each entry as 100 bytes + val.len() or so, then number of entries can be safely deprecated.

- do not create a new config option for number of entries - emit deprecation warning - some code style improvements note: we should first change the config behaviour before merging this, otherwise we add a bunch of TODO's and code that we can remove again very soon

jakmeier · 2022-10-21T13:33:07Z

Ok now that #7749 changed the internal behaviour to always limit by memory, as suggested by @matklad, this has become a lot simpler.

Roughly my changes are:

Removed all fields tracking the number of entries.
Logic around "num entries" -> "memory limit" calculation is now only done once upfront.
Removed the extra struct ShardCacheConfig and just use TrieCacheConfig directly.
Rearranged the format in config.json to make it a bit more user friendly

@matklad I think your earlier approval needs a refresh given the substantial changes

@Longarithm probably makes sense for you to look over this, too, to check I didn't mess up the cache limit calculations and to keep you in the loop of how we configure things now

matklad

Yup, new format looks reasonable.

Longarithm · 2022-10-28T14:59:39Z

core/store/src/trie/trie_storage.rs

@@ -233,7 +233,12 @@ pub struct TrieCache(pub(crate) Arc<Mutex<TrieCacheInner>>);

 impl TrieCache {
    pub fn new(config: &TrieConfig, shard_uid: ShardUId, is_view: bool) -> Self {
-        let total_size_limit = config.shard_cache_total_size_limit(shard_uid, is_view);
+        let total_size_limit = config
+            .shard_cache_config


We should take view_shard_cache_config here and below if is_view is true, and possibly have a test checking that we take correct values for view and regular caches.

Good catch! Fixed and added a test.

Make the shard cache max total bytes configurable. Also add separate configuration for view caches. This deprecates the old format for configuring cache capacity. The old format will still work for now but values in the new format will overwrite any values set in the old format. Example of the new format, that sets all normal caches to 50MB, aurora's shard to 100MB, shard 3 to 3GB, and view caches to 30MB: ```json { "trie_cache": { "default_max_bytes": 50000000, "per_shard_max_bytes": { "shard1.v1": 10000000, "shard3.v1": 3000000000 } }, "view_trie_cache": { "default_max_bytes": 30000000 } } ``` resolves #7564

jakmeier added 2 commits September 7, 2022 13:25

update changelog

b383379

jakmeier requested review from Longarithm and matklad September 7, 2022 11:31

jakmeier marked this pull request as ready for review September 7, 2022 11:31

jakmeier requested a review from a team as a code owner September 7, 2022 11:31

fix serialization format to match deserialization

955b25e

matklad reviewed Sep 7, 2022

View reviewed changes

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

jakmeier added 2 commits September 7, 2022 17:27

go through FromStr and Display

5fb3429

remove whitespace

bd758b9

jakmeier requested a review from matklad September 7, 2022 15:49

jakmeier added 2 commits September 7, 2022 18:05

fix serialization format again

9ef49f5

improve error message, remove newline

4556556

jakmeier force-pushed the cache-config branch from 3fa3ffe to 4556556 Compare September 7, 2022 17:13

Deserialize ShardUId backward compatibly

dbbd249

matklad reviewed Sep 8, 2022

View reviewed changes

deserialize_any

5a4bfcc

jakmeier requested a review from matklad September 8, 2022 12:39

use map_err

698c6b0

matklad approved these changes Sep 8, 2022

View reviewed changes

jakmeier mentioned this pull request Sep 19, 2022

Cleanup cache configuration code #7638

Closed

jakmeier mentioned this pull request Oct 3, 2022

feat: limit trie cache by memory consumption #7749

Merged

This was referenced Oct 19, 2022

Tracking issue: Leftover after Sweatcoin launch #7634

Open

Remove deprecated trie_cache_capacities in store config #7894

Closed

jakmeier added 2 commits October 21, 2022 12:58

Merge remote-tracking branch 'upstream/master' into cache-config

f9259c2

remove per entry config internally + simplify

6d15804

matklad approved these changes Oct 21, 2022

View reviewed changes

Longarithm approved these changes Oct 28, 2022

View reviewed changes

jakmeier added 2 commits October 28, 2022 21:02

fix view trie cache config and add test

7624da3

Merge remote-tracking branch 'upstream/master' into cache-config

8faf0be

jakmeier added the S-automerge label Oct 28, 2022

near-bulldozer bot merged commit 3b9012d into near:master Oct 28, 2022

jakmeier deleted the cache-config branch December 25, 2022 10:09

jakmeier mentioned this pull request Jan 5, 2023

Remove two sources of truth for default trie cache size #7066

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: trie cache configuration #7578

feat: trie cache configuration #7578

jakmeier commented Sep 7, 2022 •

edited

matklad Sep 8, 2022

jakmeier Sep 8, 2022

jakmeier Sep 8, 2022 •

edited

matklad left a comment

matklad Sep 8, 2022

jakmeier Sep 8, 2022

jakmeier commented Sep 19, 2022

Longarithm commented Sep 30, 2022

jakmeier commented Oct 21, 2022

matklad left a comment

Longarithm Oct 28, 2022

jakmeier Oct 28, 2022

feat: trie cache configuration #7578

feat: trie cache configuration #7578

Conversation

jakmeier commented Sep 7, 2022 • edited

matklad Sep 8, 2022

Choose a reason for hiding this comment

jakmeier Sep 8, 2022

Choose a reason for hiding this comment

jakmeier Sep 8, 2022 • edited

Choose a reason for hiding this comment

matklad left a comment

Choose a reason for hiding this comment

matklad Sep 8, 2022

Choose a reason for hiding this comment

jakmeier Sep 8, 2022

Choose a reason for hiding this comment

jakmeier commented Sep 19, 2022

Longarithm commented Sep 30, 2022

jakmeier commented Oct 21, 2022

matklad left a comment

Choose a reason for hiding this comment

Longarithm Oct 28, 2022

Choose a reason for hiding this comment

jakmeier Oct 28, 2022

Choose a reason for hiding this comment

jakmeier commented Sep 7, 2022 •

edited

jakmeier Sep 8, 2022 •

edited