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

ethdb: improve batch size calculation #23790

Merged
merged 1 commit into from
Oct 26, 2021

Conversation

rjl493456442
Copy link
Member

@rjl493456442 rjl493456442 commented Oct 22, 2021

This PR fixes the batch size calculation a bit.

In Geth there is a function called ValueSize of db batch, the number returned can act as an indicator to prevent accumulate too much data in a single batch.

However it only counts the value size, instead of counting both key and value size. It's OK in most of cases but may lead to critical issue in some special scenarios, e.g. there are only Batch.Put(key, nil) operations queued up in the batch, then they will all be ignored.

So in this PR, key size is also counted. It won't have a big impact to the system, but try to avoid some unexpected error.

@MariusVanDerWijden
Copy link
Member

Wouldn't this change also change how often we commit batches? By halving the amount of data in a batch thus doubling how often we call Write? With that we might need to change the ethdb.IdealBatchSize to keep the previous behavior?

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@holiman
Copy link
Contributor

holiman commented Oct 25, 2021

With that we might need to change the ethdb.IdealBatchSize to keep the previous behavior?

I wouldn't put too much value on that. The idealbatchsize is an old magic constant, and I see it as more of an indicator than an scientific exact optimal value. I think we should do the change in this PR, and yes, at some point investigate if our batch sizes seem appropriate or not.

@holiman holiman added this to the 1.10.12 milestone Oct 26, 2021
@holiman holiman merged commit 53f8157 into ethereum:master Oct 26, 2021
sidhujag pushed a commit to syscoin/go-ethereum that referenced this pull request Oct 26, 2021
This PR also counts the size of the key when calculating the size of a db batch
yongjun925 pushed a commit to DODOEX/go-ethereum that referenced this pull request Dec 3, 2022
This PR also counts the size of the key when calculating the size of a db batch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants