-
Notifications
You must be signed in to change notification settings - Fork 68
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 chainbase mapped and heap behavior #1691
Merged
Changes from 11 commits
Commits
Show all changes
23 commits
Select commit
Hold shift + click to select a range
a098946
Use Chainbase's branch with `mapped` mode updates.
greg7mdp 121b0f7
Update chainbase to branch tip.
greg7mdp 2914f46
Use `mapped_shared` mode for chainbase when loading snapshot.
greg7mdp 63e6bef
Try making the new mode not the default one (renaming new mode `mappe…
greg7mdp 73a6d22
Load snapshot in `mapped_shared` in leap-util, and revert to new mode…
greg7mdp 7da7862
Merge branch 'main' of github.com:AntelopeIO/leap into gh_1650
greg7mdp 8140958
Update to appbase branch tip.
greg7mdp 418a570
Update chainbase to tip:
greg7mdp 811192f
Update chainbase to tip.
greg7mdp 797f045
Update appbase to branch tip.
greg7mdp ca9b6a9
If `mapped` mode was requested, revert to it after loading snapshot.
greg7mdp 5618bd1
Update chainbase to branch tip.
greg7mdp bb86cf0
Call chainbase API to give the opportunity to flush some dirty pages.
greg7mdp fc421a9
Address PR comments (log info message in controller, naming)
greg7mdp 43006e2
Make the new mode non-default; rename it `mapped_private`.
greg7mdp b02fa00
Call `check_memory_and_flush_if_needed()` only in write window as Mat…
greg7mdp 6f0782d
Add snapshot load time to info log
greg7mdp 958bc01
Merge branch 'main' of github.com:AntelopeIO/leap into gh_1650
greg7mdp 5c9ebe7
Address PR comment (remove unneeded line)
greg7mdp eefda26
Add `mapped_private` description to `--help` and docs.
greg7mdp e023358
Merge branch 'main' of github.com:AntelopeIO/leap into gh_1650
greg7mdp f427935
Update to chainbase tip (`main` branch)
greg7mdp d8fb38c
Merge branch 'main' of github.com:AntelopeIO/leap into gh_1650
greg7mdp File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Submodule chainbase
updated
9 files
+1 −1 | .github/workflows/build.yaml | |
+2 −2 | CMakeLists.txt | |
+1 −1 | README.md | |
+4 −0 | include/chainbase/chainbase.hpp | |
+170 −0 | include/chainbase/pagemap_accessor.hpp | |
+21 −8 | include/chainbase/pinnable_mapped_file.hpp | |
+171 −49 | src/pinnable_mapped_file.cpp | |
+5 −1 | test/grow_shrink.cpp | |
+1 −1 | test/test.cpp |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 would be nice to maintain the new
mapped
mode for loading snapshots because I think it would be a huge perf boost (I've seen comments from some users it takes 30 minutes to load a WAX snapshot, but it takes me less than 5 minutes inheap
mode.. I'm pretty sure it's disk grinding for those users).If we're worried about leaving 100% dirty pages after loading a snapshot, maybe one option is chainbase could expose a
flush()
call that (synchronously) performs the write out so that all the pages are clean. and nodeos calls that after loading the snapshot but before continuing.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.
This would be useful when you have just the right amount of RAM that can hold all the state in RAM, but not quite enough for
heap
mode. I'm a little bit concerned that we may get more crashes this way. Maybe I can detect the available RAM, and according to the size of the disk db configured decide if it makes sense to use the newmapped
mode (for example go for it if RAM_size > 1.1 xchain-state-db-size-mb
).Yes that's a great idea. We can still use the soft-dirty thing as is the state-db-size is still configured much greater than the actual db_size used, it will make the flush faster.
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.
Should add an
ilog
or maybe even awlog
that the mode was changed from specified.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.
Upon reflection, I think the best compromise when loading a snapshot is:
mapped_shared
mode. Yes it is probably slower, but it minimizes the odds of running out of memory. With the newmapped
mode, we need memory for both the currently read data from the snapshot + the full chainbase db.copy_on_write
mapping.Do you guys agree?
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 just checked in the implementation of 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.
@heifner I updated the change to
mapped_shared
to be temporary (just while loading the snapshot), so I don't think ailog
is necessary, but I can add it if you think it might be useful still.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.
No, not needed if it honors the configuration after snapshot load.