-
Notifications
You must be signed in to change notification settings - Fork 549
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
Performance improvement: fetch merkle paths from masks instead of disk #14570
Performance improvement: fetch merkle paths from masks instead of disk #14570
Conversation
!ci-build-me |
!ci-nightly-me |
!ci-build-me |
1 similar comment
!ci-build-me |
!ci-nightly-me |
!ci-build-me |
!ci-nightly-me |
| Error _ -> | ||
(* Caught by [try_with] above. *) assert false | ||
in | ||
Direction.map sibling_dir ~left:(`Right hash) ~right:(`Left hash) |
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.
The order of direction seems to be flipped here. Is there any reason for this?
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.
It seems off to me too, but inverting the direction causes the tests to fail. Not sure why it's like this.
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.
I think this actually should not be inverted. The reason why we get the test failure is because the parent's merkle_path
function is wrong. And the root cause of that is here: https://github.com/MinaProtocol/mina/pull/14513/files#r1407811510
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.
Pushed a fix for the above
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.
Looks good to me. I just have 2 minor issues explained belowe.
15a4df4
to
b9c0b92
Compare
0dca053
to
5085720
Compare
| Error _ -> | ||
(* Caught by [try_with] above. *) assert false | ||
in | ||
Direction.map sibling_dir ~left:(`Right hash) ~right:(`Left hash) |
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.
I think this actually should not be inverted. The reason why we get the test failure is because the parent's merkle_path
function is wrong. And the root cause of that is here: https://github.com/MinaProtocol/mina/pull/14513/files#r1407811510
b9c0b92
to
07af60b
Compare
5085720
to
67b4f1d
Compare
!ci-build-me |
6828275
to
5f56888
Compare
07af60b
to
f644690
Compare
5f56888
to
d1e5ed8
Compare
f644690
to
5147f71
Compare
d1e5ed8
to
a636e02
Compare
Includes a bugfix in Merkle_ledger.Database.wide_merkle_path_batch
…d-merkle-masks-for-staged-ledger
…eature/merkle-mask-preloading
…k-empty-account-preloading
…ure/sparse-ledger-of-subset-no-mutability
…ture/sparse-ledger-wide-merkle-paths
Replace tables with maps in merkle masks
…e-merkle-paths Use 'wide merkle paths' to optimize `Sparse_ledger.of_ledger_subset_exn`
…subset-no-mutability Avoid ledger copy and mutation in `Sparse_ledger.of_ledger_subset_exn`
…-account-preloading Allow merkle masks to handle empty accounts directly
…ading Preload accounts into merkle path for staged ledger diff application
This PR improves the performance of merkle path requests when the mask already has the full merkle path. When this case is hit, we avoid walking through all of the intermediate masks, as well as disk access to the database.
Checklist: