Optimize db lookup extensions while sync & fast-reply-database #2269
Conversation
…nly on "receive notifications" goroutines.
andreibancioiu
requested review from
SebastianMarian,
AdoAdoAdo and
LucianMincu
…for both, shard and metachain nodes
| @@ -50,6 +50,7 @@ type historyRepository struct { | |||
| // that could mistakenly override the "patch()" operations performed when consuming notarization notifications. | |||
| deduplicationCacheForInsertMiniblockMetadata storage.Cacher | |||
|
|
|||
| // Question for review: this can be removed now, using the new approach (without the block tracker), right? | |||
There was a problem hiding this comment.
The only situation when the notifications will be redundant will be when a node will do a rollback. In this situation the same final block/blocks will be notified again, after another new block will be constructed upon it. But this case should not be a regular pattern and could be considered as an exception. If this redundant notifications will not break something in the implementation logic and the below method was used just as an optimization for this case, I would say that you can remove it now, considering the new approach.
There was a problem hiding this comment.
The implementation of OnNotarizedBlocks() is all right with being called twice with some redundant data in case of rollbacks. Removed the extra optimization, thanks!
| @@ -246,12 +247,14 @@ func (hr *historyRepository) OnNotarizedBlocks(shardID uint32, headers []data.He | |||
| for i, headerHandler := range headers { | |||
| headerHash := headersHashes[i] | |||
|
|
|||
| log.Debug("onNotarizedBlocks():", "shardID", shardID, "nonce", headerHandler.GetNonce(), "headerHash", headerHash, "type", fmt.Sprintf("%T", headerHandler)) | |||
|
|
|||
| // Question for review: this optimization is not required anymore, right? | |||
There was a problem hiding this comment.
The same response as above
cmd/node/config/config.toml
Outdated
| @@ -645,7 +645,7 @@ | |||
| PollingIntervalInMinutes = 65 | |||
|
|
|||
| [DbLookupExtensions] | |||
| Enabled = false | |||
| Enabled = true # TODO DISABLE AFTER TESTS | |||
| @@ -158,6 +176,14 @@ func (hr *historyRepository) computeMiniblockHash(miniblock *block.MiniBlock) ([ | |||
| return core.CalculateHash(hr.marshalizer, hr.hasher, miniblock) | |||
| } | |||
|
|
|||
| func (hr *historyRepository) hasRecentlyInsertedMiniblockMetadata(miniblockHash []byte) bool { | |||
| return hr.deduplicationCacheForInsertMiniblockMetadata.Has(miniblockHash) | |||
There was a problem hiding this comment.
In case of a miniblock added in epoch x on a fork, but added again in epoch x+1 on canonical chain, the "miniblock hash to epoch" will not be updated with the correct epoch.
maybe add also the epoch in this cache as value, and only deduplicate if the epoch value is the same, otherwise update.
There was a problem hiding this comment.
Very very nice catch 🥳
Fixed by using a composite key for the deduplication cache. Is it all right?
RecordBlocksynchronously withCommit(). Consume notifications only on "receive notifications" goroutines.Hold LRU caches for recently handled records so that performance at sync is improvednot needed anymoreshardBlock/metaBlockprocessor.Handle repeated commits of same block (protect against overriding the
miniblockMetadata) / EN-7520. Edge case was:Foo, start processing notifications (in progress), notification is seen, still in mapFooFoofrom map