-
Notifications
You must be signed in to change notification settings - Fork 636
Removes snapshot_bank_utils::get_snapshot_storages() #7302
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #7302 +/- ##
=======================================
Coverage 82.8% 82.8%
=======================================
Files 801 801
Lines 363263 363255 -8
=======================================
- Hits 300848 300842 -6
+ Misses 62415 62413 -2 🚀 New features to boost your workflow:
|
7ad0de7
to
f00d725
Compare
datapoint_info!( | ||
"get_snapshot_storages", | ||
( | ||
"snapshot-storages-time-ms", | ||
measure_snapshot_storages.as_ms(), | ||
i64 | ||
), | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll notice that we'll no longer be submitting this metric. I think that's fine because there is already another datapoint here:
agave/runtime/src/accounts_background_service.rs
Lines 281 to 318 in f00d725
// Snapshot the bank and send over a snapshot package | |
let mut snapshot_time = Measure::start("snapshot_time"); | |
let snapshot_package = SnapshotPackage::new( | |
snapshot_kind, | |
&snapshot_root_bank, | |
snapshot_root_bank.get_snapshot_storages(None), | |
status_cache_slot_deltas, | |
); | |
self.pending_snapshot_packages | |
.lock() | |
.unwrap() | |
.push(snapshot_package); | |
snapshot_time.stop(); | |
info!( | |
"Handled snapshot request. snapshot kind: {:?}, slot: {}, bank hash: {}", | |
snapshot_kind, | |
snapshot_root_bank.slot(), | |
snapshot_root_bank.hash(), | |
); | |
total_time.stop(); | |
datapoint_info!( | |
"handle_snapshot_requests-timing", | |
( | |
"flush_accounts_cache_time", | |
flush_accounts_cache_time.as_us(), | |
i64 | |
), | |
("shrink_time", shrink_time.as_us(), i64), | |
("clean_time", clean_time.as_us(), i64), | |
("snapshot_time", snapshot_time.as_us(), i64), | |
("total_us", total_time.as_us(), i64), | |
("non_snapshot_time_us", non_snapshot_time_us, i64), | |
("shrink_ancient_time_us", shrink_ancient_time_us, i64), | |
); | |
Ok(snapshot_root_bank.slot()) | |
} |
The handle_snapshot_requests-timing.snapshot_time
now encapsulates getting the snapshot storages. And getting the snapshot storages is almost 100% of this metric anyway. (Note there are spikes that correspond with the full snapshot interval, so we are seeing some other timing influence, but that's the minority of the time.) That and I don't think anyone was checking this metric anyway.
Problem
snapshot_bank_utils::get_snapshot_storages()
is unnecessary.Summary of Changes
Remove it.