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

Add BatchWithFlusher to iavl tree #771

Closed
wants to merge 3 commits into from

Conversation

catShaark
Copy link
Contributor

@catShaark catShaark commented May 18, 2023

Closes #619

  • This Pr allow iavl tree to use BatchWithFlusher to limit size of commit to db. By including flushThreshold to nodeDB, we can configure the size limit of each commit batch.
  • This Pr changes nodeDB struct and NewMutableTreeWithOpts to replace uses of dbm.Batch to BatchWithFlusher
  • We'll have to make changes to sdk accordingly to adapt this changes.

@catShaark
Copy link
Contributor Author

still needs profiling on live osmosis node to compare the results

batch.go Show resolved Hide resolved
@tac0turtle
Copy link
Member

will this be able to be part of the next release or should we wait for the next release (unclear when that will be)

@catShaark
Copy link
Contributor Author

This won't be state breaking so if it's ready we can add this to the next release. But I get errors backporting this to v0.19.x

@tac0turtle
Copy link
Member

tac0turtle commented Jun 13, 2023

gentle ping

@catShaark
Copy link
Contributor Author

catShaark commented Jun 18, 2023

I figure out the deadlock issue is due to the mem db iterator being channel based, the iterator will lock the db during iteration which inturn cause the deadlock. There're already issue on cosmos-db related to this cosmos/cosmos-db#56, cosmos/cosmos-db#47

@tac0turtle
Copy link
Member

The sdk doesn't use memdb anymore. Maybe we have our own memdb here

@catShaark
Copy link
Contributor Author

So I have run benchmark on a live osmosis node. Comparing normal iavl batch vs batch with flusher (flush to state at 100KB), the result is almost the same for the case of normal blocks ( blocks that don't cause heavy commit to state ) but for epoch blocks that cause heavy commit, the commit time is reduced by 25%.

@catShaark
Copy link
Contributor Author

catShaark commented Jul 14, 2023

Screen Shot 2023-07-14 at 14 08 14

running 10 blocks including epoch block with 100KB flush

Screen Shot 2023-07-14 at 14 17 39

running 10 blocks including epoch block with normal iavl batch

@catShaark
Copy link
Contributor Author

the benchmark also proves that iavl with batch is not state breaking. I've run 10000 blocks with no app hash error so far

@tac0turtle
Copy link
Member

amazing this is super useful!!

@tac0turtle
Copy link
Member

gentle ping on this pr, if you are running this on mainnets then it should be good to merge?

@catShaark
Copy link
Contributor Author

yes, we can have this on mainnets. Do we want the flush thresh hold to be configurable. If so then we have to change iavl.NewMutableTreeWithOpts which is used in sdk root multi logic, that means we also need to make changes to the sdk

@catShaark
Copy link
Contributor Author

catShaark commented Jul 25, 2023

here is the commit time of a osmosis epoch block :

Flush threshold Commit time
25 27.17s
50 26.72s
100 26.8s
250 26.19s
500 27.5s
750 26.76s
1000 41.83s
no-flush 49.61s

Seems like we got the same performance in the range from 25 KB to 750KB. I think we should choose 100KB to be the flush threshold.

@tac0turtle
Copy link
Member

gentle ping, need help completing this?

@catShaark
Copy link
Contributor Author

thanks for pinging, I will finish this by tomorow

@catShaark
Copy link
Contributor Author

I've made a new PR for this
#807

@catShaark catShaark closed this Aug 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor One Large Commit Into Smaller Batches
2 participants