Store: batching writes to the block and state stores#1152
Store: batching writes to the block and state stores#1152
Conversation
adizere
left a comment
There was a problem hiding this comment.
Great work Anton!
@thanethomson should we use main as the base branch for this PR? Unclear if there's a feature branch with storage improvements or not.
@werty144 Shall we also add a table with performance results so we can understand the impact of this batching mechanism?
state/store.go
Outdated
| } | ||
|
|
||
| err = store.db.Set(calcValidatorsKey(height), bz) | ||
| //err = store.db.Set(calcValidatorsKey(height), bz) |
There was a problem hiding this comment.
Should we delete this comment ?
state/store.go
Outdated
| } | ||
|
|
||
| err = store.db.Set(calcConsensusParamsKey(nextHeight), bz) | ||
| //err = store.db.Set(calcConsensusParamsKey(nextHeight), bz) |
|
I've also performed some measurements using
|
|
I quickly went through this and looks pretty good to me. I would say we can merge this to |
| defer func(batch dbm.Batch) { | ||
| err := batch.Close() | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| }(batch) |
There was a problem hiding this comment.
I would extract this as a function:
func closeBatchOrPanic(batch dbm.Batch) {
if err := batch.Close(); err != nil {
panic(err)
}
}Then you could simply:
| defer func(batch dbm.Batch) { | |
| err := batch.Close() | |
| if err != nil { | |
| panic(err) | |
| } | |
| }(batch) | |
| defer closeBatchOrPanic(batch) |
Makes the enclosing function a little easier to read. Similarly for this same type of pattern in other packages.
If the performance worsens with GoLevelDB (the most popular underlying DB for Comet atm, afaict) then I'm not sure we'd want to merge this. I still find it a very useful exercise to have the data. Might be worthwhile to transcribe the sheet into a table that can be added into a comment on this PR for posterity. |
I see, I have to believe the measurements as that's all we have, so yes, it's probably better to wait. However, I'm surprised that simply batching DB transactions incurs an extra performance cost, as batching is done precisely to have to sync less often. Also, remember that the IG team had implemented batching in |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
This might be related to why goleveldb is having issues: cosmos/iavl#619 TLDR; There is an optimal batch size. Having the batch be too large causes garbage collector overhead since GoLevelDB simply has a bytes slice where it stores data until commit. |
|
Reopening this because we are still to decide whether to go with this after we benchmark it further. |
|
Superseded by #1735 |
Closes #1040
PR checklist
.changelog(we use unclog to manage our changelog)docs/orspec/) and code comments