perf: avoid expensive lookup of storage metrics #7495
Conversation
Finding the counter of a metric is a relatively expensive operation, when compared to just updating its value. A performance regression of about 20% on read perfromance has been observed due to additional metrics added in near#7429. This commit stores the counters in `TrieCacheInne` and `TrieCachingStorag` to avoid the lookup in hot paths like when we hit the shard or chunk cache.
There was a problem hiding this comment.
LGTM, but maybe consider bunching these up into
struct TrieCacheMetrics {
...
}
struct, so that call-site looks like
self.metrics.chunk_cache_misses.inc();
and makes it abundantly clear that we are dealing with metrics, and not with something affecting semantics.
There was a problem hiding this comment.
+1 to @matklad. We actually need two groups of metrics - TrieCacheMetrics and TrieCacheInnerMetrics
core/store/src/trie/trie_storage.rs
Outdated
| @@ -73,6 +75,14 @@ pub struct TrieCacheInner { | |||
| shard_id: u32, | |||
| /// Whether cache is used for view calls execution. | |||
| is_view: bool, | |||
|
|
|||
| // Store counteres here to avoid overhead of looking them up on hot paths. | |||
There was a problem hiding this comment.
| // Store counteres here to avoid overhead of looking them up on hot paths. | |
| // Counters tracking operations happening inside the shard cache. Stored here to avoid overhead of looking them up on hot paths. |
core/store/src/trie/trie_storage.rs
Outdated
| @@ -369,6 +381,16 @@ pub struct TrieCachingStorage { | |||
| pub(crate) db_read_nodes: Cell<u64>, | |||
| /// Counts trie nodes retrieved from the chunk cache. | |||
| pub(crate) mem_read_nodes: Cell<u64>, | |||
|
|
|||
| // Store counteres here to avoid overhead of looking them up on hot paths. | |||
There was a problem hiding this comment.
| // Store counteres here to avoid overhead of looking them up on hot paths. | |
| // Counters tracking events happening while querying shard cache. Stored here to avoid overhead of looking them up on hot paths. |
- group metrics together in two new structs - update comments
|
Thinking out loud: is it possible here to have wrappers over |
|
Hm, I see, that does sound kind of nice. But I think you are spot on with the problem that it still requires something like a hash map internally. Doing some manual mapping into a vector seems like a good solution performance wise but it seems a bit fragile to future changes. And not as broadly applicable as I would like it to be. The current implementation certainly is not the most elegant. But for a fix that we need fast, I prefer blunt and simple code. I would suggest we think about refactoring this in the larger context. There are still more metrics that use the inefficient pattern and maybe we can find a solution that fits them all. If nobody raises any concern quick enough, I will merge this PR soon. I would like it to be in before the nightly CE job triggers. |
There was a problem hiding this comment.
LGTM!
Opened https://nearinc.atlassian.net/browse/ND-203 to track a larger problem.
* perf: avoid expensive lookup of storage metrics Finding the counter of a metric is a relatively expensive operation, when compared to just updating its value. A performance regression of about 20% on read perfromance has been observed due to additional metrics added in #7429. This commit stores the counters in `TrieCacheInne` and `TrieCachingStorag` to avoid the lookup in hot paths like when we hit the shard or chunk cache. * apply reviewer suggestions - group metrics together in two new structs - update comments
* perf: avoid expensive lookup of storage metrics Finding the counter of a metric is a relatively expensive operation, when compared to just updating its value. A performance regression of about 20% on read perfromance has been observed due to additional metrics added in #7429. This commit stores the counters in `TrieCacheInne` and `TrieCachingStorag` to avoid the lookup in hot paths like when we hit the shard or chunk cache. * apply reviewer suggestions - group metrics together in two new structs - update comments
* perf: avoid expensive lookup of storage metrics Finding the counter of a metric is a relatively expensive operation, when compared to just updating its value. A performance regression of about 20% on read perfromance has been observed due to additional metrics added in #7429. This commit stores the counters in `TrieCacheInne` and `TrieCachingStorag` to avoid the lookup in hot paths like when we hit the shard or chunk cache. * apply reviewer suggestions - group metrics together in two new structs - update comments
Finding the counter of a metric is a relatively expensive operation,
when compared to just updating its value. A performance regression
of about 20% on read perfromance has been observed due to additional
metrics added in #7429.
This commit stores the counters in
TrieCacheInneandTrieCachingStoragto avoid the lookup in hot paths like whenwe hit the shard or chunk cache.