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.

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

MARF Performance Improvements #3044

Merged
merged 83 commits into from
Apr 13, 2022
Merged

MARF Performance Improvements #3044

merged 83 commits into from
Apr 13, 2022

Conversation

jcnelson
Copy link
Member

@jcnelson jcnelson commented Feb 10, 2022

This PR significantly improves the read and write paths for MARFs by between a factor of 10x and 200x, depending on how many paths are inserted into each trie. It fixes #3059, #3042, #3041, and does some work towards fixing #3014.

On the write path, this code adds the ability to defer calculating a trie's node hashes until when the trie is ready to be dumped to disk. This leads to a significant performance improvement because it means that an intermediate node's hash is only ever calculated at most once, whereas before, it could be calculated up to as many times as there were leaf inserts. Over 90% of the write path wall-clock time was spent in calculating node hashes that would later be overwritten by subsequent hash calculations.

On the read path, this code does two things:

  • It adds a caching layer for nodes, so that miners and node operators with lots of RAM can opt to cache some or all of the trie nodes in RAM. The current cache strategies (to be expanded in later PRs) are noop (default; does nothing), everything (caches all nodes in RAM once they are loaded), and node256 (caches all TrieNode256 nodes in RAM once loaded). The new cache layer also subsumes the caches for holding block hashes.

  • It allows the MARF owner to opt to store tries in a separate flat file, called a .blobs file. Before, the tries would be stored as sqlite blobs in the database. However, for large tries, such as those produced by the Clarity VM, the blob paging logic in sqlite significantly degraded performance on the read path when walking tries. Storing large tries in a flat file (i.e. tries bigger than one page size) removes sqlite from the trie node read path altogether, and yields up to a 14x improvement. In addition, this PR sets the sqlite page size to 32k (we were using the default of 4k), which is big enough to hold other MARF tries from the sortition DB and headers DB in a single sqlite page.

In addition, this PR introduces a simple profiler for measuring how long certain hot paths take. New measurements can be incrementally added in.

Without this PR, a node in master takes about 3 days to boot from genesis. With this PR, it takes about 28 hours.

Don't let the diff line count intimidate you. A lot of it is from indenting the unit test code needing to be indented by one tab in order to try each unit test with different MARFOpenOpt values.

…are used when storing key/value pairs; also, refactor all unit tests to use different variations of MARFOpenOpts
…re ready to dump a TrieRAM to disk, and to cache nodes from disk-baked tries. This considerably speeds up the write path for the MARF. A TrieRAM, now folded into a LastExtended enum, can now be in either a read/write state, or a sealed state; in the latter case, only reads and commit/abort are permitted, and the act of converting a read/write to a sealed TrieRAM will calculate all node hashes and return the MARF root hash.
…ng mode. Also, refactor all tests to use different hashing and node caching modes.
… of opening the block, storing key/value pairs, and sealing the block in put_indexed_all()
@jcnelson jcnelson marked this pull request as draft February 10, 2022 19:15
@codecov
Copy link

codecov bot commented Feb 10, 2022

Codecov Report

Merging #3044 (9ae7bb1) into develop (9a5e390) will decrease coverage by 0.09%.
The diff coverage is 79.51%.

@@             Coverage Diff             @@
##           develop    #3044      +/-   ##
===========================================
- Coverage    83.69%   83.60%   -0.10%     
===========================================
  Files          248      259      +11     
  Lines       197466   200544    +3078     
===========================================
+ Hits        165276   167670    +2394     
- Misses       32190    32874     +684     
Impacted Files Coverage Δ
src/chainstate/stacks/index/mod.rs 54.06% <0.00%> (-23.99%) ⬇️
src/chainstate/stacks/index/node.rs 85.66% <ø> (-12.18%) ⬇️
src/chainstate/stacks/index/test/node.rs 100.00% <ø> (ø)
src/main.rs 0.11% <0.00%> (-0.01%) ⬇️
stacks-common/src/types/mod.rs 93.22% <ø> (ø)
src/chainstate/stacks/index/proofs.rs 68.86% <14.28%> (-4.79%) ⬇️
src/chainstate/stacks/index/test/marf.rs 44.36% <44.36%> (ø)
testnet/stacks-node/src/tests/neon_integrations.rs 81.51% <50.00%> (-6.77%) ⬇️
src/chainstate/stacks/miner.rs 92.96% <53.84%> (-0.14%) ⬇️
testnet/stacks-node/src/node.rs 83.19% <61.53%> (+<0.01%) ⬆️
... and 82 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 280d574...9ae7bb1. Read the comment docs.

@gregorycoppola
Copy link
Contributor

Nice result. Will look. Thanks for sending.

Is there a command line to run the experiment?

@gregorycoppola
Copy link
Contributor

@jcnelson

I know how to run an experiment, I realized. You don't have to worry about sending it on any time frame.

Curious to know how you did it but I can also use the "mempool analyzer". (Except I think my current mempool state is not up to date.)

@kantai kantai changed the title Feat/marf node cache Draft: MARF Caching Feb 11, 2022
Copy link
Member

@kantai kantai left a comment

Choose a reason for hiding this comment

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

This all looks great to me, just a few comments and questions.

One high-level comment is around cache control via environment variables. It would be much better for that to be controllable via the stacks-node::config module -- almost all of the configuration variables are currently controlled there, and avoiding adding more environment variable switched behaviors would be wise.

Copy link
Contributor

@gregorycoppola gregorycoppola left a comment

Choose a reason for hiding this comment

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

Seems like this is working so LGTM.

Thanks for adding all this new functionality!

@jcnelson jcnelson requested a review from kantai April 13, 2022 17:14
Copy link
Member

@kantai kantai left a comment

Choose a reason for hiding this comment

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

LGTM! Excited to see these improvements in the node.

@jcnelson jcnelson merged commit 9dd09ab into develop Apr 13, 2022
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.

None yet

3 participants