Batch save blocks for initial sync. 80% faster BPS #5215
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5215 +/- ##
=========================================
Coverage ? 39.22%
=========================================
Files ? 231
Lines ? 19102
Branches ? 0
=========================================
Hits ? 7492
Misses ? 10274
Partials ? 1336 |
terencechain
changed the title
terencechain
changed the title
| var endBlock *ethpb.SignedBeaconBlock | ||
| if featureconfig.Get().InitSyncBatchSaveBlocks && s.hasInitSyncBlock(endRoot) { | ||
| endBlock = s.getInitSyncBlock(endRoot) | ||
| if featureconfig.Get().InitSyncBatchSaveBlocks { |
There was a problem hiding this comment.
Why is this condition here? Wouldn't it already be true?
| for _, block := range blocks { | ||
| if err := k.setBlockSlotBitField(ctx, tx, block.Block.Slot); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| blockRoot, err := ssz.HashTreeRoot(block.Block) | ||
| blockRoot, err := stateutil.BlockRoot(block.Block) |
| @@ -193,7 +203,7 @@ func (s *Service) roundRobinSync(genesis time.Time) error { | |||
| 1, // step | |||
| blockBatchSize, // count | |||
| peers, // peers | |||
| 0, // remainder | |||
| 0, // reminder | |||
There was a problem hiding this comment.
Reminder? What does that mean in this context?
There was a problem hiding this comment.
Sorry it should have been remainder
| log.Debugf("Beacon node doesn't have a block in db with root %#x", blk.Block.ParentRoot) | ||
| parentRoot := bytesutil.ToBytes32(blk.Block.ParentRoot) | ||
| if !s.db.HasBlock(ctx, parentRoot) && !s.chain.HasInitSyncBlock(parentRoot) { | ||
| log.Debugf("Beacon node doesn't have a block in db or cache with root %#x", parentRoot) |
There was a problem hiding this comment.
Should this be changed to WithField() for better log tracking?
There was a problem hiding this comment.
Also, not feature flag needed here?
There was a problem hiding this comment.
Updated to WithField(), feature flag not needed in this csae
| var endBlock *ethpb.SignedBeaconBlock | ||
| if featureconfig.Get().InitSyncBatchSaveBlocks && s.hasInitSyncBlock(endRoot) { | ||
| endBlock = s.getInitSyncBlock(endRoot) | ||
| if featureconfig.Get().InitSyncBatchSaveBlocks { |
There was a problem hiding this comment.
why check again ? this will always be true
| // This saves a beacon block to the initial sync blocks cache. | ||
| func (s *Service) saveInitSyncBlock(r [32]byte, b *ethpb.SignedBeaconBlock) { | ||
| s.initSyncBlocksLock.Lock() | ||
| defer s.initSyncBlocksLock.Unlock() |
There was a problem hiding this comment.
do we need a lock for this ? initial sync is single-threaded , so there isnt a risk of concurrent writes
There was a problem hiding this comment.
I tried without the lock, the performance is neglectable. Just figure be safe than sorry later if initial sync moves to multi threaded
| @@ -247,6 +252,14 @@ func (s *Service) onBlockInitialSyncStateTransition(ctx context.Context, signed | |||
| } | |||
| } | |||
|
|
|||
| // Rate limit how many blocks (2 epochs worth of blocks) a node keeps in the memory. | |||
| if len(s.getInitSyncBlocks()) > 2*int(params.BeaconConfig().SlotsPerEpoch) { | |||
There was a problem hiding this comment.
can we make this value a configurable constant at the start of the file
This PR saves blocks in batch during initial sync. It holds the blocks in cache and batch saves the cached blocks into DB when there's a new finalized check point or the blocks count exceeds 64 (whichever comes first)
This is gated behind feature flag
--init-sync-batch-save-blocks, I'm seeing on average 80-100% faster blocks per second versus current initial syncing.This works and tested with the following flags:
I dont think it's compatible with new state mgmt flag yet