Optimise merge registers for migrations - Revert of Revert #5613

janezpodhostnik · 2024-04-02T14:26:21Z

This reverts commit 7503026.

This adds a fix for flakey tests.

I called the two snapshot types:

mapBased (better when only reading or when expected changes are a large portion of all registers)
indexMapBase (better when changed registers are a smaller portion of all registers)

This reverts commit 7503026.

codecov-commenter · 2024-04-02T14:32:52Z

Codecov Report

Attention: Patch coverage is 3.82166% with 151 lines in your changes are missing coverage. Please review.

Project coverage is 55.85%. Comparing base (11040f7) to head (bce5dc5).

Files	Patch %	Lines
cmd/util/ledger/util/payload_snapshot.go	0.00%	140 Missing ⚠️
cmd/util/ledger/util/util.go	0.00%	7 Missing ⚠️
...util/ledger/migrations/atree_register_migration.go	25.00%	3 Missing ⚠️
...til/ledger/migrations/account_storage_migration.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@                    Coverage Diff                     @@
##           feature/stable-cadence    #5613      +/-   ##
==========================================================
- Coverage                   55.89%   55.85%   -0.05%     
==========================================================
  Files                        1049     1049              
  Lines                      103232   103305      +73     
==========================================================
- Hits                        57706    57696      -10     
- Misses                      41132    41219      +87     
+ Partials                     4394     4390       -4

Flag	Coverage Δ
unittests	`55.85% <3.82%> (-0.05%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Currently, storage health check always fails with "slabs not referenced from account storage" because storage maps are not loaded in Cadence runtime storage even though payloads are loaded. This commit fixes this problem by loading storage map explicitly after loading payloads in storage.

turbolent · 2024-04-10T17:13:55Z

@janezpodhostnik Awesome work! What was the problem with the old, reverted variant, and what is the fix for it in this PR?

janezpodhostnik · 2024-04-12T09:35:34Z

What was the problem with the old, reverted variant

The registers loaded in the test (from emulator) were not unique. So creating the mapping in a different order created different test results.
This was fixed by sorting the registers descending by their block height when they were edited and only taking the first encountered register for each key.

turbolent

Great work!

Should this also be on master?

Might be good to get more eyes on it. Also, it would be great to test this exhaustively with the race detector on.

janezpodhostnik · 2024-04-15T15:35:34Z

@turbolent this is only for migrations so it does not need to be ported back to master.

I will test it with the race detector!

turbolent · 2024-04-15T16:02:58Z

@janezpodhostnik right, I assumed we wanted to also use it in the atree register inlining migration?

janezpodhostnik · 2024-04-15T16:31:01Z

Actually no. This optimisation is relevant for the cases where the amount of changed registers is a smaller portion of all the registers in a snapshot. If used in the atree migration it makes it worse.

janezpodhostnik · 2024-04-15T18:08:53Z

Tested this with the race detector on, and looks good.

fxamacker

Nice! I left some comments.

cmd/util/ledger/util/snapshot/payload_snapshot.go

fxamacker · 2024-04-17T15:29:12Z

cmd/util/ledger/util/snapshot/payload_snapshot.go

+type IndexMapSnapshot struct {
+	reverseMap map[flow.RegisterID]int
+	payloads   []*ledger.Payload
+}


Do we want to unexport IndexMapSnapshot, since mapSnapshot isn't exported?

cmd/util/ledger/util/snapshot/payload_snapshot.go

cmd/util/ledger/migrations/cadence_value_diff_test.go

cmd/util/ledger/migrations/cadence_value_validation_test.go

cmd/util/ledger/migrations/migrator_runtime.go

fxamacker · 2024-04-17T16:20:13Z

cmd/util/ledger/util/snapshot/payload_snapshot.go

+	MapBased MigrationSnapshotType = iota
+	// IndexMapBased is more efficient when the number of payloads that are going to change is small
+	// compared to the total number of payloads (less than 30% of the current number of payloads).
+	IndexMapBased


nit: MapBased and IndexMapBased can be confusing to differentiate without reading comments. Maybe we can rename them.

I didn't have any good ideas I renamed them according to suggested usage. Let me know what you think.

Maybe something like this to help user to choose the snapshot type (without implementation detail):

ReadOnlySnapshotType

LargeChangeSetSnapshotType

SmallChangeSetSnapshotType

Both ReadOnlySnapshotType and LargeChangeSetSnapshotType create MapBased snapshot.

SmallChangeSetSnapshotType creates IndexMapBased snapshot.

This just an idea, so please feel free to choose any name you prefer.

turbolent · 2024-04-17T19:11:41Z

@janezpodhostnik Sorry about all the conflicts, I didn't realize we were touching a lot of the same code. Happy to help with resolving the conflicts if you want to!

janezpodhostnik · 2024-04-17T19:13:40Z

Oh, no :)
I just fixed some.

Its ok I think its not that bad.

turbolent

Great work! Mainly reviewed the changes to the existing code

fxamacker

Great work! 👍

This function was added in PR #5613 by Janez for feature/stable-cadence and was copied into PR #5765. Co-authored-by: Janez Podhostnik <67895329+janezpodhostnik@users.noreply.github.com>

janezpodhostnik added 2 commits April 2, 2024 15:31

Revert "Revert "Optimise merge registers for migrations""

b6d677f

This reverts commit 7503026.

fix tests

bce5dc5

janezpodhostnik requested review from SupunS, fxamacker and a team April 2, 2024 14:26

janezpodhostnik self-assigned this Apr 2, 2024

janezpodhostnik marked this pull request as draft April 2, 2024 14:26

joshuahannan and others added 9 commits April 9, 2024 15:24

update core contract dependencies and state commitments

f14f23a

Update to Cadence v1.0.0-preview.19

eda3caf

adjust Cadence API usage

4d2b9f8

switch contract update problems from failures to logged errors

450a6ac

fix storage health check

1de9f75

clean up options

e47d048

Export user defined type change checker

9a3ce01

cleanup

201861a

janezpodhostnik marked this pull request as ready for review April 9, 2024 13:37

Merge branch 'feature/stable-cadence' into janez/index-map-based-payload

e7a6377

janezpodhostnik force-pushed the janez/index-map-based-payload branch from 5632f10 to e7a6377 Compare April 9, 2024 14:38

Merge branch 'feature/stable-cadence' into janez/index-map-based-payload

fb9a4bb

turbolent approved these changes Apr 12, 2024

View reviewed changes

Merge branch 'feature/stable-cadence' into janez/index-map-based-payload

ba59e26

fix lint

de493cb

fxamacker reviewed Apr 17, 2024

View reviewed changes

apply review comments

7295f3f

janezpodhostnik force-pushed the janez/index-map-based-payload branch from f5b20c5 to 7295f3f Compare April 17, 2024 18:43

janezpodhostnik requested a review from fxamacker April 17, 2024 18:44

Merge branch 'feature/stable-cadence' into janez/index-map-based-payload

ca5e8d0

merge fix

3f3792d

Merge branch 'feature/stable-cadence' into janez/index-map-based-payload

fdf18aa

janezpodhostnik requested a review from turbolent April 17, 2024 19:28

turbolent approved these changes Apr 17, 2024

View reviewed changes

turbolent requested a review from a team April 17, 2024 20:08

fxamacker approved these changes Apr 17, 2024

View reviewed changes

janezpodhostnik merged commit a36bb74 into feature/stable-cadence Apr 18, 2024
55 checks passed

janezpodhostnik deleted the janez/index-map-based-payload branch April 18, 2024 13:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimise merge registers for migrations - Revert of Revert #5613

Optimise merge registers for migrations - Revert of Revert #5613

janezpodhostnik commented Apr 2, 2024 •

edited

Loading

codecov-commenter commented Apr 2, 2024 •

edited

Loading

turbolent commented Apr 10, 2024

janezpodhostnik commented Apr 12, 2024

turbolent left a comment •

edited

Loading

janezpodhostnik commented Apr 15, 2024

turbolent commented Apr 15, 2024

janezpodhostnik commented Apr 15, 2024 •

edited

Loading

janezpodhostnik commented Apr 15, 2024

fxamacker left a comment

fxamacker Apr 17, 2024

fxamacker Apr 17, 2024

janezpodhostnik Apr 17, 2024

fxamacker Apr 17, 2024

turbolent commented Apr 17, 2024

janezpodhostnik commented Apr 17, 2024

turbolent left a comment

fxamacker left a comment

Optimise merge registers for migrations - Revert of Revert #5613

Optimise merge registers for migrations - Revert of Revert #5613

Conversation

janezpodhostnik commented Apr 2, 2024 • edited Loading

codecov-commenter commented Apr 2, 2024 • edited Loading

Codecov Report

turbolent commented Apr 10, 2024

janezpodhostnik commented Apr 12, 2024

turbolent left a comment • edited Loading

Choose a reason for hiding this comment

janezpodhostnik commented Apr 15, 2024

turbolent commented Apr 15, 2024

janezpodhostnik commented Apr 15, 2024 • edited Loading

janezpodhostnik commented Apr 15, 2024

fxamacker left a comment

Choose a reason for hiding this comment

fxamacker Apr 17, 2024

Choose a reason for hiding this comment

fxamacker Apr 17, 2024

Choose a reason for hiding this comment

janezpodhostnik Apr 17, 2024

Choose a reason for hiding this comment

fxamacker Apr 17, 2024

Choose a reason for hiding this comment

turbolent commented Apr 17, 2024

janezpodhostnik commented Apr 17, 2024

turbolent left a comment

Choose a reason for hiding this comment

fxamacker left a comment

Choose a reason for hiding this comment

janezpodhostnik commented Apr 2, 2024 •

edited

Loading

codecov-commenter commented Apr 2, 2024 •

edited

Loading

turbolent left a comment •

edited

Loading

janezpodhostnik commented Apr 15, 2024 •

edited

Loading