refactor: trie cache configuration #7566
Conversation
This is a pure refactor, including two main refactors: - replace `TrieCacheFactory` with `TrieConfig` - move config constants into `TrieConfig` The motivation is to facilitate wider configuration and changes to the config format in follow-up PRs.
|
@matklad this should partially address the points you raised in #7022 (comment) I will change the config format and add more config options in a follow-up. |
jakmeier
requested review from
Longarithm and
matklad
and removed request for
mzhangmzz
| pub view_shard_cache_config: ShardCacheConfig, | ||
| } | ||
|
|
||
| pub struct ShardCacheConfig { |
There was a problem hiding this comment.
FWIW, I now understand terminology around shards/chunks, and it still doesn't makes sense to me that we call our caches shard cache and chunk cache :) Unrelated to the PR.
There was a problem hiding this comment.
Well, I think it makes some sense. Both are trie node caches for a single shard. But the lifetime of the chunk cache is a single chunk. The lifetime of the shard cache is for as long as the client runs.
But it is very confusing indeed. The cache naming scheme that I am used to would call them something like L0TrieNodeCache for the chunk cache and L1TrieNodeCache for the shard cache. This makes it clear what is being cached and what the hierarchy is. When a cache is cleared doesn't really belong in the name IMO. But I don't care enough to change it...
| @@ -72,7 +71,7 @@ pub struct TrieCacheInner { | |||
| /// Upper bound for the total size. | |||
| total_size_limit: u64, | |||
| /// Shard id of the nodes being cached. | |||
| shard_id: u32, | |||
| shard_id: ShardId, | |||
There was a problem hiding this comment.
Yeah this is one is weirder than it might look. ShardId is u64 but in NEP241 we specified shard_id inside ShardUId to be u32, such that it fits in 8 bytes. Changing everything to u32 doesn't work, as there are places where we deserialize it from u64.
I considered creating an issue on GH for a general cleanup. But I'm not sure there is anything worth while doing at this point. Probably it's best to just use ShardId = u64 for shard_id everywhere except inside ShardUId.
| pub fn new(config: &TrieConfig, shard_uid: ShardUId, is_view: bool) -> Self { | ||
| let capacity = config.shard_cache_capacity(shard_uid, is_view); | ||
| let total_size_limit = config.shard_cache_total_size_limit(shard_uid, is_view); | ||
| let queue_capacity = config.deletions_queue_capacity(); | ||
| Self(Arc::new(Mutex::new(TrieCacheInner::new( | ||
| cap, | ||
| DEFAULT_SHARD_CACHE_DELETIONS_QUEUE_CAPACITY, | ||
| DEFAULT_SHARD_CACHE_TOTAL_SIZE_LIMIT, | ||
| shard_id, | ||
| capacity as usize, | ||
| queue_capacity, | ||
| total_size_limit, | ||
| shard_uid.shard_id(), |
There was a problem hiding this comment.
Consider adding a TrieCacheConfig with capacity, total_size_limit, queue_capacity, passing in just that, and removing shard_id and is_viewparametres altogether (we only need them for log_assert, and I am fairly sure we can live without this info there).
Ahhh, no we need those to create metrics as well. 🤔, yeah, I think this is ok then.
- `cfg!() if else` instead of two `#[cfg]` - derive default for `TrieConfig` - converge `ShardTries` constructors
This is a pure refactor, including two main refactors: - replace `TrieCacheFactory` with `TrieConfig` - move config constants into `TrieConfig` The motivation is to facilitate wider configuration and changes to the config format in follow-up PRs. See also #7564.
This is a pure refactor, including two main refactors: - replace `TrieCacheFactory` with `TrieConfig` - move config constants into `TrieConfig` The motivation is to facilitate wider configuration and changes to the config format in follow-up PRs. See also near#7564.
This is a pure refactor, including two main refactors: - replace `TrieCacheFactory` with `TrieConfig` - move config constants into `TrieConfig` The motivation is to facilitate wider configuration and changes to the config format in follow-up PRs. See also #7564.
This is a pure refactor, including two main refactors:
TrieCacheFactorywithTrieConfigTrieConfigThe motivation is to facilitate wider configuration and changes to the
config format in follow-up PRs.
See also #7564.