Skip to content

Removes snapshot_bank_utils::get_snapshot_storages()#7302

Merged
brooksprumo merged 1 commit intoanza-xyz:masterfrom
brooksprumo:snap/get-storages
Aug 5, 2025
Merged

Removes snapshot_bank_utils::get_snapshot_storages()#7302
brooksprumo merged 1 commit intoanza-xyz:masterfrom
brooksprumo:snap/get-storages

Conversation

@brooksprumo
Copy link
Copy Markdown

@brooksprumo brooksprumo commented Aug 4, 2025

Problem

snapshot_bank_utils::get_snapshot_storages() is unnecessary.

Summary of Changes

Remove it.

@brooksprumo brooksprumo self-assigned this Aug 4, 2025
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Aug 4, 2025

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 82.8%. Comparing base (a8e0a02) to head (f00d725).

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:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment on lines -727 to -734
datapoint_info!(
"get_snapshot_storages",
(
"snapshot-storages-time-ms",
measure_snapshot_storages.as_ms(),
i64
),
);
Copy link
Copy Markdown
Author

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:

// 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.

@brooksprumo brooksprumo marked this pull request as ready for review August 5, 2025 00:09
@brooksprumo brooksprumo requested a review from roryharr August 5, 2025 00:09
@brooksprumo brooksprumo merged commit 166044c into anza-xyz:master Aug 5, 2025
41 checks passed
@brooksprumo brooksprumo deleted the snap/get-storages branch August 5, 2025 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants