-
Notifications
You must be signed in to change notification settings - Fork 269
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
Introduce Pruning to IAVL #158
Conversation
Storing intermidiary IAVL versions in memory and not to disk Motivation: Both Cosmos and Loom Network save an IAVL version per block, then go back and delete these versions. So you have constant churn on the IAVL and underlying Leveldb database. When realistically what you want is to only store every X Blocks. At Berlin Tendermint Conference, Zaki and I surmised a plan where new versions are in memory, while still pointing back to nodes on disk to prevent needing to load entire IAVL into main memory. Loom IAVL tree is around 256gb so this is not feasible otherwise. Usage OLD Code would be like ```go hash, version, err := s.tree.SaveVersion() ``` New Caller code would look like ```go oldVersion := s.Version() var version int64 var hash []byte //Every X versions we should persist to disk if s.flushInterval == 0 || ((oldVersion+1)%s.flushInterval == 0) { if s.flushInterval != 0 { log.Error(fmt.Sprintf("Flushing mem to disk at version %d\n", oldVersion+1)) hash, version, err = s.tree.FlushMemVersionDisk() } else { hash, version, err = s.tree.SaveVersion() } } else { hash, version, err = s.tree.SaveVersionMem() } ``` FlushMemVersionDisk: Flushes the current memory version to disk SaveVersionMem: Saves the current tree to memory instead of disk and gives you back an apphash This is an opt in feature, you have to call new apis to get it. We also have a PR that demonstrates its usage https://github.com/loomnetwork/loomchain/pull/1232/files We are now commiting every 1000 blocks, so we store 1000x less. Also we have signficant improves in IO at least double from not having to Prune old versions of the IAVL Tree
Ready for initial review!! Ended up finding some pretty subtle bugs while testing this. Will need to add some more tests but it would be very helpful to get reviews both on the overall design of Pruning and also someone to look over my current tests and suggest any additional cases I should test for. TODO:
|
Co-Authored-By: Ismail Khoffi <[email protected]>
…nto aditya/pruning
@tnachen is taking this over I believe. |
Yes syncing with Aditya today about this patch
…On Tue, Dec 10, 2019 at 08:07 Zaki Manian ***@***.***> wrote:
@tnachen <https://github.com/tnachen> is taking this over I believe.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#158>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAQNLFNA3TZ3YGGWYDT3HDQX65FJANCNFSM4IF6I7ZQ>
.
|
I just did some benchmarking with the Cosmos SDK test-sim-benchmark with different configurations, and measure the benchmark completion time, and also looking into CPU / mem usage. It's not an exhaustive benchmark but overall can see the big improvements with pruning in terms of performance. https://docs.google.com/spreadsheets/d/1p9eZ4LIq5wSjogb03_l59AzOMK_Gfu8ncWT6Ujua4nU/edit?usp=sharing Overall CPU processing time and memory doesn't seem to differ that much regardless what recent/every setting chosen. The biggest different is time (compaction, etc). |
@tnachen is there a recommended config for users? We should document this. |
b.StartTimer() | ||
} | ||
|
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.
Add comment to pause timer so we don't measure db-closing time
Also, remove print statement comment
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.
Approved after Tim includes the latest benchmarking results he got from the SDK.
Also think my old benchmark results should be deleted since they weren't benchmarking very useful tasks.
Should be placed in the folder in format explained by Marko above
I believe {KeepEvery: 1, KeepRecent: 0} should also be benchmarked since this will test if there's been a major regression from the current master branch behavior with the new changes
* errors: add some error handling tm-db 0.4.0 introduced more returns with errors, bubbling up more errors because of this. Signed-off-by: Marko Baricevic <[email protected]> * wrap errors
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.
Addresses issue #144 with spec here: #144 (comment)
Only persist versions to memDB if they are snapshot versions and keep recent versions in memDB
NodeDB is responsible for handling pruning logic. Application developers simply specify pruning parameters