Moves status cache serde-for-snapshots code into own module#7604
Conversation
brooksprumo
force-pushed
the
snap/serde-transaction-error
branch
from
8130933 to
cbc6e3c
Compare
brooksprumo
force-pushed
the
snap/serde-transaction-error
branch
from
cbc6e3c to
5171076
Compare
| let path = tempfile::NamedTempFile::new().unwrap().into_temp_path(); | ||
| bencher.iter(|| { | ||
| let _ = serialize_status_cache(&status_cache.root_slot_deltas(), &path).unwrap(); | ||
| let _ = serialize(&status_cache.root_slot_deltas()).unwrap(); |
There was a problem hiding this comment.
This was added in #7593 to cleanup some tasks from #7559. One was adding this bench. This bench is calling serialize_status_cache() (the fn I'm moving in this PR to a new module), and I realized it is writing to a file.
Microbenchmarks are notoriously finicky, and ones that hit disk are even more so. I've moved this one back to using the stock serialize method here (just like the other status cache bench above, bench_status_cache_serialize()). This also saves us from having to conditionally make serialize_status_cache() public beyond just the crate.
We have metrics for the actual runtime of serialize_status_cache(), and I feel that is sufficient for our needs here.
There was a problem hiding this comment.
Ok no worries, that's fine. Just be aware that bincode::serialize on the status cache isn't really what's being run in snapshots with the current code -- we're converting to the serde types and then calling bincode::serialize on those.
|
@joncinque I realize that I led you astray in #7593, sorry about that! Please accept my work here as penance. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #7604 +/- ##
=========================================
- Coverage 83.4% 83.4% -0.1%
=========================================
Files 812 813 +1
Lines 364968 364971 +3
=========================================
- Hits 304439 304387 -52
- Misses 60529 60584 +55 🚀 New features to boost your workflow:
|
joncinque
left a comment
joncinque
left a comment
There was a problem hiding this comment.
Looks good to me! Thanks for cleaning this up
| let path = tempfile::NamedTempFile::new().unwrap().into_temp_path(); | ||
| bencher.iter(|| { | ||
| let _ = serialize_status_cache(&status_cache.root_slot_deltas(), &path).unwrap(); | ||
| let _ = serialize(&status_cache.root_slot_deltas()).unwrap(); |
There was a problem hiding this comment.
Ok no worries, that's fine. Just be aware that bincode::serialize on the status cache isn't really what's being run in snapshots with the current code -- we're converting to the serde types and then calling bincode::serialize on those.
3 participants
Problem
The SDK's TransactionError and InstructionError changed, which caused snapshots to break due to the serialization of the status cache. This was mitigated in #7559 by airgap-ing the TransactionError type, and performing conversions when serializing and deserializing the status cache.
This new code was living in
snapshot_bank_utils.rs, which was not ideal. For one, it requiredsnapshot_utils.rsto import fromsnapshot_bank_utils, and the whole point of these two files being separate was so thatsnapshot_utils.rsdid not import fromsnapshot_bank_utils.rsnor anything from Bank.For two, there is already precedent for putting these "serde" types within the
serde_snapshotmodule.Summary of Changes
Move the serde code of the status cache into its own module under
serde_snapshot.Note, no functionality was changed in this PR, minus the bench (see comments inline below).