Skip to content

Ordered account insertion into merkle trie (I)#1697

Merged
tsachiherman merged 12 commits into
algorand:masterfrom
tsachiherman:tsachi/ordered_catchpoint_file
Nov 17, 2020
Merged

Ordered account insertion into merkle trie (I)#1697
tsachiherman merged 12 commits into
algorand:masterfrom
tsachiherman:tsachi/ordered_catchpoint_file

Conversation

@tsachiherman
Copy link
Copy Markdown
Contributor

@tsachiherman tsachiherman commented Nov 13, 2020

Summary

Inserting ordered hashes into the merkle trie improve its insertion speed dramatically. The goal of this change is to modify the insertion that doing so that these would always be in-order.

We currently have two such use cases :

  • Rebuilding a local merkle trie from the accounts themselves
  • Node catching up using the fast catchup. This would be handled on the server side by ordering the accounts on the catchpoint file.

This PR addresses the first part, using an ordered accounts hashes to be inserted into the trie.
The new orderedAccountsIter is intended to replace the encodedAccountsBatchIter, but that would only be possible on the subsequent PR.

Performance Plan

I added a benchmark to test the gains:

Number of accounts Before the change With the change Performance gains
6,000,000 293 seconds 80 seconds 266%

The 6M accounts were chosen since it's roughly the number of accounts we currently have on mainnet.

Test Plan

Existing tests already cover the functional portion of this PR.

@tsachiherman tsachiherman self-assigned this Nov 13, 2020
@tsachizehub tsachizehub added this to the Sprint 13 milestone Nov 13, 2020
Copy link
Copy Markdown

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some minor comments. Question: how efficient is using sql's orderby / indexing as a sorting algorithm?

Comment thread ledger/accountdb.go Outdated
Comment thread ledger/accountdb.go Outdated
Comment thread ledger/accountdb.go Outdated
@tsachiherman
Copy link
Copy Markdown
Contributor Author

Added some minor comments. Question: how efficient is using sql's orderby / indexing as a sorting algorithm?

I'm not sure that we have a good alternative. We don't have enough memory to load it all in anyway.

ghost
ghost previously approved these changes Nov 16, 2020
Copy link
Copy Markdown

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated changes look good

Copy link
Copy Markdown
Contributor

@algonautshant algonautshant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.
I feel like this can be simplified. So I need to understand why all these cases are needed.

Comment thread ledger/accountdb.go Outdated
Comment thread ledger/accountdb.go
Comment thread ledger/accountdb.go
Comment thread ledger/accountdb.go
iterator.step = oaiStepCreateOrderingAccountIndex
return
}
if iterator.step == oaiStepCreateOrderingAccountIndex {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this step needs to return? Can't it directly call Next to advance to oaiStepSelectFromOrderedTable?

Comment thread ledger/accountdb.go
Comment thread ledger/accountdb.go Outdated
Comment thread ledger/acctupdates.go Outdated
Comment thread ledger/accountdb.go Outdated
algonautshant
algonautshant previously approved these changes Nov 17, 2020
Copy link
Copy Markdown
Contributor

@algonautshant algonautshant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great.
Thanks for the changes.
Just one comment about closing the iterator.

Comment thread ledger/accountdb.go
@tsachiherman tsachiherman changed the title Ordered account insertion into merkle trie Ordered account insertion into merkle trie (I) Nov 17, 2020
Copy link
Copy Markdown
Contributor

@algorandskiy algorandskiy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, not sure although why do we need such level of granularity/steps within the Next() function

Comment thread ledger/accountdb.go
@tsachiherman tsachiherman merged commit 5a05207 into algorand:master Nov 17, 2020
@tsachiherman tsachiherman deleted the tsachi/ordered_catchpoint_file branch November 17, 2020 17:11
tsachiherman added a commit to tsachiherman/go-algorand that referenced this pull request Jul 7, 2021
Refactor merkle trie rebuild to use ordered accounts insertions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants