Removes AccountStorageEntry::approx_store_count #2953
Conversation
| @@ -8031,16 +8014,14 @@ impl AccountsDb { | |||
|
|
|||
| fn is_shrinking_productive(store: &AccountStorageEntry) -> bool { | |||
| let alive_count = store.count(); | |||
| let stored_count = store.approx_stored_count(); | |||
There was a problem hiding this comment.
We read the approx_store_count atomic here, but it is only used for a trace! log, not determining if shrinking is productive or not.
This would be the only useful place to load this field, but since it is only used for logging, it is not actually useful.
| self.approx_store_count | ||
| .fetch_add(num_accounts, Ordering::Relaxed); |
There was a problem hiding this comment.
Since we never read this field in a useful way, we also do not need to write this field here.
| /// | ||
| /// This is used as a rough estimate for slot shrinking. As such a relaxed | ||
| /// use case, this value ARE NOT strictly synchronized with count_and_status! | ||
| approx_store_count: AtomicUsize, |
There was a problem hiding this comment.
...thus the whole field is useless and can be removed.
| @@ -1137,14 +1127,13 @@ impl AccountStorageEntry { | |||
| slot: Slot, | |||
| id: AccountsFileId, | |||
| accounts: AccountsFile, | |||
| num_accounts: usize, | |||
| _num_accounts: usize, | |||
There was a problem hiding this comment.
The next PR will remove the now-unused num_accounts param from this function.
There was a problem hiding this comment.
To me, 'approx_stored_count' seems to be a redundant counter. And its usage
is for logging and assert. Probably, it was added initially for debugging or
as sanity check to make sure that the account storage has the number of
expected accounts.
Logically, I think it is fine to remove it.
Alternatively, if we think there is value to maintain this redundant counter
check, maybe we can have add a mode to enable this? which can be done through
a CLI arg or an environment variable?
Problem
The AccountStorageEntry::approx_store_count field is unused.
The comments describe it was used during/for shrink, but that is no longer the case. I end up having to re-remember this field isn't actually used though while going through the shrink/storage code. And, it's an atomic, so eventually it requires cache coherence updates ("eventually" due to the Relaxed accesses). Again, since it is not used, we can avoid doing that unnecessary work.
Summary of Changes
Remove it.