Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Optimize compute_top_candidates #2123

Merged
merged 4 commits into from
Mar 14, 2023
Merged

Optimize compute_top_candidates #2123

merged 4 commits into from
Mar 14, 2023

Conversation

tmpolaczyk
Copy link
Contributor

Optimize compute_top_candidates function:

  • The documentation was wrong, the returned vec is sorted by AccountId
  • In that case, there is no need to sort by amount if the number of candidates is smaller than or equal to TotalSelected, because they will all be returned
  • In the case that we do need to select the top candidates by amount, we also do not need to fully sort the list, we can use quickselect to get the top_n unordered
  • Added test to ensure that the sort is stable and matches the previous behavior

@tmpolaczyk tmpolaczyk added B0-silent Changes should not be mentioned in any release notes D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Feb 22, 2023
@librelois librelois added B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes not-breaking Does not need to be mentioned in breaking changes and removed B0-silent Changes should not be mentioned in any release notes labels Mar 9, 2023
@tmpolaczyk
Copy link
Contributor Author

Simplified the code a bit, by noticing that the owner can be used instead of the index to achieve a stable sort, and that allows the select_nth to happen in place.

@tmpolaczyk tmpolaczyk merged commit 51818a8 into master Mar 14, 2023
@tmpolaczyk tmpolaczyk deleted the tomasz-refactor-compute-top-candidates branch March 14, 2023 12:57
@notlesh notlesh added D1-audited👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Apr 26, 2023
imstar15 pushed a commit to AvaProtocol/moonbeam that referenced this pull request May 16, 2023
@tmpolaczyk @imstar15
Optimize compute_top_candidates (moonbeam-foundation#2123)
f6b4364
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@librelois librelois librelois approved these changes

@4meta5 4meta5 4meta5 approved these changes

Assignees
No one assigned
Labels
B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D1-audited👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited not-breaking Does not need to be mentioned in breaking changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tmpolaczyk @librelois @4meta5 @notlesh