go/worker/storage/committee: Refactor diff sync#6451
Draft
martintomazic wants to merge 1 commit intomasterfrom
Draft
go/worker/storage/committee: Refactor diff sync#6451martintomazic wants to merge 1 commit intomasterfrom
martintomazic wants to merge 1 commit intomasterfrom
Conversation
Improve worker performance and make it more readable, idiomatic, testable and bechmarkable.
✅ Deploy Preview for oasisprotocol-oasis-core canceled.
|
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Improve worker performance and make it more readable, idiomatic, testable and benchmarkable. The idea of this PR is to first assess whether refactor is worth it.
Looking at
25.6and25.9logs, the queuing of the tasks was not optimal. In practice even after this change we are still blocked by data availability when syncing older storage diffs (e.g. genesis sync) and or pruning at the same time**. So in practice performance hasn't changed (benchmarked already), except for maybe when syncing recent state (cca50%faster but need to reproduce to confirm).Things I like:
Got rid of two redundant structures; syncing rounds and summary cache together with corresponding locking. Given this is IO-bound task, and the work is primarily about concurrency (not parallelism) I believe moving from mutexes to channels is desirable.
Things I dislike: I will probably get rid of the new
diffsyncnested package, also overall it still feels to complex. Writing another fetcher, that will read from another local DB for the purpose of benchmarking will probably clarify design.** As pointed out , and benchmarked again, runtime state pruning is incredibly slow even with only few versions locally. As
ndb.Finalizeandndb.Pruneare completely sequential due to metadata lock and with pruning critical section easily lasting0.1-0.5sthis means syncing interleaved with pruning is bounded with1-5rounds/s assuming all other operations would be negligible. As this is not the case, and there is likely big write amplification/compaction load on the DB in practice we are closer to1-2rounds/s.