-
Notifications
You must be signed in to change notification settings - Fork 166
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
Improve scheduling in account-based migration #5901
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
I only left a suggestion about avoiding overhead of creating and sorting allAccountRegisters
slice by only tracking the largest accounts above a threshold.
@fxamacker @SupunS PTAL at the improved version which does not sort: https://github.com/onflow/flow-go/pull/5901/files/53fe9a69d901d7c15846a6a2cc8879c08f122c82..bdba7af3eb1daf165ca8b2fee86cc2a43f75c162 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5901 +/- ##
==========================================
+ Coverage 55.58% 55.61% +0.02%
==========================================
Files 1127 1128 +1
Lines 88840 88926 +86
==========================================
+ Hits 49380 49453 +73
- Misses 34751 34761 +10
- Partials 4709 4712 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Thanks for updating PR to replace very large sorted slice with a tiny queue!
I only left 1 comment about Less()
for topDurations
.
Co-authored-by: Faye Amacker <33205765+fxamacker@users.noreply.github.com>
Depends on #5897
Closes onflow/cadence#3308
Currently, the migrations of accounts are scheduled in address order. If there are large accounts at high addresses, the migration might end up waiting for a these accounts, with most available workers idling.
Schedule the migration of large accounts first to increase utilization of available workers.