Upstream: "core/rawdb: freezer batch write" #1908
Conversation
This change is a rewrite of the freezer code. When writing ancient chain data to the freezer, the previous version first encoded each individual item to a temporary buffer, then wrote the buffer. For small item sizes (for example, in the block hash freezer table), this strategy causes a lot of system calls for writing tiny chunks of data. It also allocated a lot of temporary []byte buffers. In the new version, we instead encode multiple items into a re-useable batch buffer, which is then written to the file all at once. This avoids performing a system call for every inserted item. To make the internal batching work, the ancient database API had to be changed. While integrating this new API in BlockChain.InsertReceiptChain, additional optimizations were also added there. Co-authored-by: Felix Lange <fjl@twurst.com>
…/upstream_1.10.9_part2
hbandura
requested review from
gastonponti,
37ng and
piersy
and removed request for
a team
|
Coverage from tests in coverage: 46.4% of statements across all listed packagescoverage: 57.3% of statements in consensus/istanbul coverage: 23.7% of statements in consensus/istanbul/announce coverage: 54.3% of statements in consensus/istanbul/backend coverage: 0.0% of statements in consensus/istanbul/backend/backendtest coverage: 24.3% of statements in consensus/istanbul/backend/internal/replica coverage: 65.5% of statements in consensus/istanbul/core coverage: 45.0% of statements in consensus/istanbul/db coverage: 0.0% of statements in consensus/istanbul/proxy coverage: 64.4% of statements in consensus/istanbul/uptime coverage: 51.8% of statements in consensus/istanbul/validator coverage: 79.2% of statements in consensus/istanbul/validator/randomCommentID: 03ec6ea4e3 |
Codecov Report
@@ Coverage Diff @@
## master #1908 +/- ##
==========================================
+ Coverage 54.24% 54.41% +0.16%
==========================================
Files 678 679 +1
Lines 89317 89514 +197
==========================================
+ Hits 48453 48710 +257
+ Misses 37226 37157 -69
- Partials 3638 3647 +9
Continue to review full report at Codecov.
|
| if first.NumberU64() == 1 { | ||
| if frozen, _ := bc.db.Ancients(); frozen == 0 { | ||
| b := bc.genesisBlock | ||
| writeSize, err := rawdb.WriteAncientBlocks(bc.db, []*types.Block{b}, []types.Receipts{nil}, big.NewInt(1)) |
There was a problem hiding this comment.
Given that WriteAncientBlocks is ignoring the td parameter I think we should just remove it from the parameter list, otherwise providing values here is quite confusing
There was a problem hiding this comment.
@hbandura Is there an issue for this?
| @@ -1204,120 +1183,116 @@ func (bc *BlockChain) InsertReceiptChain(blockChain types.Blocks, receiptChain [ | |||
| } | |||
| return false | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
This PR makes significant changes to InsertReceiptChain but it is also missing a test that was included in the upstream PR TestInsertReceiptChainRollback. Looking at the results of coverage I can see that none of the error cases of insertReceiptChain are executed so I think it would be good to include a test here that exercises some of the error cases for InsertReceiptChain.
There was a problem hiding this comment.
The new test uses sidechains, which is why we can't run it. But the old test uses a test field/test mechanism "testInsert" to provoke the tests to be tested, which was removed in this refactor. The errors are tested using sidechains, which we don't have.
Re-adding the mechanism to make it testeable was probably a change that should be made in a different PR for the same reason you mentioned the redundant parameter one
|
Hey @hbandura, I left 2 comments. I think we could leave the removal of the redundant parameter to another PR, because making that change here would complicate understanding this merge. Further testing of insert receipt chain could however be added without risk of conflict so I think that change should be made here, unless you don't think it should be tested. Aside from that all the e2e tests on ci are failing. Also why did you merge the original branch back into this branch? That's going to make for an overly complicated history? |
Merging commit ethereum/go-ethereum@794c613#diff-d78b498ba524953c810b53b2f1e866245d052a063863ed820c9b4852f8762d69 from upstream into the 1.10.9 merge PR
It took me a while to make this one work. Had lots of issues with the total difficulty storage. I hacked it by forcing it to write number+1 in the batch write functions.
Opening this PR against the partial 1.10.9 PR since that one hasn't been merged yet.