-
Notifications
You must be signed in to change notification settings - Fork 146
Implement dynamic adjustment of ForkedChain persistBatchSize #3750
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
base: master
Are you sure you want to change the base?
Conversation
runTime = finishTime - startTime | ||
|
||
|
||
if runTime < targetTime and c.persistBatchSize <= batchSizeUpperBound: |
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.
EthTime.now()
resolution is one second. runTime < targetTime(1 second)
will not works. And persistBatchsize
will never increased, right?
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 does work because if the run time is less than one second then runTime = 0. In other words it appears to truncate or round down the value since the precision only supports seconds.
I've been testing it and verified that both increasing the size and decreasing the batch size does indeed work.
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.
Maybe add some comment to avoid confusion?
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.
Sure, will do
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.
Actually, I might update this to measure the time in milliseconds instead of seconds.
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've updated the code to use milliseconds precision instead.
After testing these changes I found that increasing/decreasing the batch size by a factor of 2 produced better results than just increasing/decreasing by 5. This was because it allows the batch size to grow or shrink fast enough in response to changes in the size of the blocks during the sync process. I'm also going to leave this feature disabled by default for now considering that it is a bit experimental. It can be manually enabled and can be set as the default in a future PR if all goes well. |
This implements dynamic adjustment of the forked chain persist batch size so that we can set the best size to optimize for performance while also taking into account how long we can safely block the event loop when computing the stateroot and persisting to the database.
The idea here is that we have a target run time of 1 second and we increase the batch size if the run time is below the target and decrease it if the run time is over the target. It looks like the various p2p networking timeouts are set to between 5-10 seconds and so using a target of 1 second seems reasonable to avoid hitting these timeouts.