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.

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
merged 1 commit into from
Aug 5, 2025

Conversation

brooksprumo
Copy link

@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

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