Adds hit/miss stats for cache hash data #34954
Conversation
|
Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis. |
brooksprumo
force-pushed
the
hash-cache/stats/hits-misses
branch
from
5c3f951
to
8c510ef
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #34954 +/- ##
=========================================
- Coverage 81.6% 81.6% -0.1%
=========================================
Files 828 830 +2
Lines 224339 224670 +331
=========================================
+ Hits 183274 183508 +234
- Misses 41065 41162 +97 |
| ScanAccountStorageResult::CacheFileNeedsToBeCreated(_) => misses += 1, | ||
| }; | ||
| } | ||
| cache_hash_data |
There was a problem hiding this comment.
can 'hit' and 'miss' be just u64 instead?
There was a problem hiding this comment.
Ish, but it's probably not worth it.
The CacheHashData struct has the CacheHashDataStats field wrapped in an Arc so that it can be accessed/updated concurrently during the parallel loops that use CacheHashData.
If we wanted hits and misses to be non-atomic, then I'd need to mutably borrow the CacheHashDataStats field. In order to do that, I'd need to add a Mutex (or equivalent) to the stats type, resulting in a Arc<Mutex<CacheHashDataStats>>. This means all the stats updates would then need to grab that mutex inside the parallel processing loops.
Maybe the parallel loops can be refactored to not update the stats inside the loop itself, and thus make it possible to remove the Arc on the CacheHashDataStats field. But as of now, this is what prevents us from having hits and misses be non-atomic.
There was a problem hiding this comment.
👍 I see. Some of the stats are updated multithread.
One way is to split the Stats into two parts: 1. Arc<_atomics>, 2. non_automic.
but yes. doesn't worth it.
Heh, looks like this didn't actually approve the PR |
(cherry picked from commit f0d67d7)
…34964) Adds hit/miss stats for cache hash data (#34954) (cherry picked from commit f0d67d7) Co-authored-by: Brooks <[email protected]>
Problem
When create/using the account hash data cache, we don't know easily how many hits and misses there were.
Summary of Changes
Add metrics for the number of hits and misses.