Optimise merge registers for migrations #5522
Conversation
There was a problem hiding this comment.
Thanks for optimizing! I agree that this can speedup large snapshots having only a small portion changed.
Given this optimization has tradeoffs (uses more memory), can you include memory usage info in your benchmarks (B/op and allocs/op columns) and also add benchmark(s) for 100+ million payloads since largest account reached ~170 million payloads last summer?
I'm concerned Atree migration may have some performance regression for large accounts due to increased memory use. I left some comments about this.
nit: payload_shnapshot_test.go file probably needs to be renamed.
|
@fxamacker I got some surpising results from the benchmarks They show that the reverse map based approach is equal to the original approach regarding allocations. |
@janezpodhostnik Amazing! You mentioned a "~44% improvement" but these results from your benchmarks look awesome! 🚀 🎉 /merge_10000000/changes_10000000/MapBasedPayloadSnapshot-12 37 45845741 ns/op 100008808 B/op 19 allocs/op
/merge_10000000/changes_10000000/PayloadSnapshot-12 2 531950084 ns/op 180011696 B/op 17 allocs/opI'll take a closer look after wrapping up some urgent items (I'm out next week). Very curious to see maxRSS and total duration of migration using mainnet24 snapshot as input when run on gcp (with enough RAM). |
|
There was an error in the benchmark naming, so here are the results again: |
|
Before I merge this I want to see how do we feel about only having the |
There was a problem hiding this comment.
@janezpodhostnik Nice! 👍 Thanks for updating PR!
Before I merge this I want to see how do we feel about only having the MapBasedPayloadSnapshot instead of having 2 implementations, since it look like there are no allocation penalties.
I'm in favor of it (based on your amazing benchmark results) 🚀 👍 It looks like this PR can speedup both atree migration and Cadence 1.0 migration with far less memory overhead than initially thought (for the payload counts benchmarked).
One remaining concern is that we should confirm our expectations by running a test migration of all payloads from all accounts using mainnet snapshot/checkpoint as input.
If you want, we can merge this PR and I can try migration on large gcp vm when I return.
382a70c to
9c86bab
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## feature/stable-cadence #5522 +/- ##
==========================================================
+ Coverage 57.35% 60.38% +3.03%
==========================================================
Files 885 703 -182
Lines 88700 69686 -19014
==========================================================
- Hits 50870 42077 -8793
+ Misses 33917 24328 -9589
+ Partials 3913 3281 -632
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
9c86bab to
639573e
Compare
| @@ -69,38 +68,6 @@ func (a *AccountsAtreeLedger) AllocateStorageIndex(owner []byte) (atree.StorageI | |||
| return v, nil | |||
| } | |||
|
|
|||
| type PayloadSnapshot struct { | |||
I used Atree inlining + Cadence 0.42 using mainnet data (Jan 19, 2024 checkpoint) as input. Test vm was large enough RAM=3.84 TB and I used nworkers=40 (for consistency with old benchmarks). |
|
Thank you for the info. I'm not quite sure why this is. The benchmarks looked promising... I will prepare a PR to revert this. |
|
Though it may have slight adverse effects on one migration (atree register inlining), maybe it's still helpful for others (e.g. updating contracts in the Cadence 1.0 migration)? |
@janezpodhostnik Maybe revert only for atree migration because I didn't test this optimization with Cadence 1.0 migration. It is probably best to test with mainnet data as input for Cadence 1.0 migrations. |
@turbolent I agree and I also think we should test with mainnet data as input for Cadence 1.0 migrations to determine impact of this optimization. Both approaches have tradeoffs and we may benefit by using the most appropriate approach for each migration or account size. |
|
I'm going to try using account size at runtime to choose merge implementation. I'll let you know how it goes! |
I have changed the snapshot so that it uses a map of key to array index. Because of that when merging in changes we don't need to go through all entries of the array.
This doesn't seem to have an impact on snapshot creation, or on the case where a large portion of the snapshot has been changed.
But for the case where a small portion of a large snapshot is changed, this is a ~44% improvement
Benchmarks
Before
After