Add SnapshotArchiveInfo and refactor functions in snapshot_utils #18232
Conversation
brooksprumo
force-pushed
the
snapshot-utils
branch
5 times, most recently
from
d5402f9
to
1f87a2d
Compare
Codecov Report
@@ Coverage Diff @@
## master #18232 +/- ##
=========================================
+ Coverage 82.3% 82.6% +0.2%
=========================================
Files 435 435
Lines 121424 121554 +130
=========================================
+ Hits 100020 100408 +388
+ Misses 21404 21146 -258 |
ledger/src/bank_forks_utils.rs
Outdated
| archive_slot: Slot, | ||
| archive_hash: Hash, | ||
| archive_format: ArchiveFormat, | ||
| snapshot_archive_info: SnapshotArchiveInfo, |
There was a problem hiding this comment.
I'm basically just replacing the (destructured) tuple up above at line 56 with this struct. The original version was borrowed/moved, so I just kept it the same. I was under the impression that Rust would optimize away any copies and just perform a move; do you know if there's a performance benefit here for references?
There was a problem hiding this comment.
I did some experiments and examine the assembly code generated in a site Jeff W. pointed me to -- it does not appear the compiler is doing the move. I still see for each fields pushing to the stack instead of pushing one pointer and referencing the fields by offset later. But I agree you did not make it worse compared with before. And let's just see if you need copy the struct.
There was a problem hiding this comment.
OK thanks for checking. I've changed the code to use a reference now.
ledger/src/bank_forks_utils.rs
Outdated
| error!( | ||
| "Snapshot has mismatch:\narchive: {:?}\ndeserialized: {:?}", | ||
| archive_hash, deserialized_bank_slot_and_hash | ||
| snapshot_archive_info.hash, deserialized_bank_slot_and_hash |
There was a problem hiding this comment.
Maybe logging (snapshot_archive_info.slot, snapshot_archive_info.hash) makes more sense. 1. it is checked. 2. making the two entries logged symmetric. But I know it exists before your changes.
There was a problem hiding this comment.
Good point, and I agree. I like slot first, then hash second. I'll make this change.
| } | ||
| None | ||
| }) | ||
| .collect(), |
There was a problem hiding this comment.
What happens to the sorting? No longer needed or handled by caller?
There was a problem hiding this comment.
I've moved the sorting into its own function (see get_sorted_snapshot_archives() below). This way when a caller doesn't need sorted archives they don't have to pay for the sorting.
There was a problem hiding this comment.
Seems like it's been replaced with get_sorted_snapshot_archives
brooksprumo
force-pushed
the
snapshot-utils
branch
from
efde6f7
to
e10bfd3
Compare
While working on Incremental Snapshots (#17088), I put up an initial PR (#17875) to get ISS working. However, there was a lot of code in that PR, so I'm pulling out smaller chunks to make it easier to review and merge.
This PR adds
SnapshotArchiveInfo, refactors functions, and adds tests tosnapshot_utils.