Skip to content

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.

Sign up for GitHub

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

Merged
brooksprumo merged 1 commit into from
Aug 5, 2025
Merged

Removes snapshot_bank_utils::get_snapshot_storages() #7302

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions runtime/src/accounts_background_service.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ use {
crate::{
bank::{Bank, BankSlotDelta, DropCallback},
bank_forks::BankForks,
snapshot_bank_utils,
snapshot_controller::SnapshotController,
snapshot_package::{SnapshotKind, SnapshotPackage},
snapshot_utils::SnapshotError,
Expand Down Expand Up @@ -284,7 +283,7 @@ impl SnapshotRequestHandler {
let snapshot_package = SnapshotPackage::new(
snapshot_kind,
&snapshot_root_bank,
snapshot_bank_utils::get_snapshot_storages(&snapshot_root_bank),
snapshot_root_bank.get_snapshot_storages(None),
status_cache_slot_deltas,
);
self.pending_snapshot_packages
Expand Down
19 changes: 1 addition & 18 deletions runtime/src/snapshot_bank_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ use {
bincode::{config::Options, serialize_into},
log::*,
solana_accounts_db::{
accounts_db::{AccountStorageEntry, AccountsDbConfig, AtomicAccountsFileId},
accounts_db::{AccountsDbConfig, AtomicAccountsFileId},
accounts_update_notifier_interface::AccountsUpdateNotifier,
utils::remove_dir_contents,
},
Expand Down Expand Up @@ -719,23 +719,6 @@ fn _verify_epoch_stakes(
Ok(())
}

/// Get the snapshot storages for this bank
pub fn get_snapshot_storages(bank: &Bank) -> Vec<Arc<AccountStorageEntry>> {
let mut measure_snapshot_storages = Measure::start("snapshot-storages");
let snapshot_storages = bank.get_snapshot_storages(None);
measure_snapshot_storages.stop();
datapoint_info!(
"get_snapshot_storages",
(
"snapshot-storages-time-ms",
measure_snapshot_storages.as_ms(),
i64
),
);
Comment on lines -727 to -734
Copy link
Copy Markdown
Author

@brooksprumo brooksprumo Aug 5, 2025

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.


snapshot_storages
}

/// Convenience function to create a full snapshot archive out of any Bank, regardless of state.
/// The Bank will be frozen during the process.
/// This is only called from ledger-tool or tests. Warping is a special case as well.
Expand Down
Loading