feat: trie cache configuration #7578
Conversation
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
3fa3ffe to
4556556
Compare
core/primitives/src/shard_layout.rs
Outdated
| 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") { |
There was a problem hiding this comment.
using strip_prefix now but not sure if there is a more elegant way...
There was a problem hiding this comment.
I guess ok_or_else makes it quite a bit cleaner
core/store/src/config.rs
Outdated
| /// 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>, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@Longarithm What do you think? Would it make sense to only have a configuration for total bytes and drop the number of entries config?
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? |
|
If we measure the size of each entry as |
- 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
|
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:
@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 |
core/store/src/trie/trie_storage.rs
Outdated
| @@ -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 | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
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
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