New header body pair cache#7620
Conversation
# Conflicts: # process/asyncExecution/headersExecutor.go # process/asyncExecution/headersExecutor_test.go # testscommon/processMocks/executionManagerMock.go
|
|
||
| headerNonce := pair.Header.GetNonce() | ||
| c.cacheByNonce[headerNonce] = pair | ||
| c.lastAddedNonce = headerNonce |
There was a problem hiding this comment.
what if this is called with a lower nonce? in this case, I think we should remove all headers with higher nonces from cache
There was a problem hiding this comment.
refactored this
last added nonce is no longer needed, also is no need to remove the pairs with higher nonce
because with this implementation we will fetch pairs by providing the correct nonce.
There was a problem hiding this comment.
ok, indeed we have RemoveAtNonceAndHigher that does that
| c.mutex.Lock() | ||
| defer c.mutex.Unlock() | ||
|
|
||
| delete(c.cacheByNonce, nonce) |
There was a problem hiding this comment.
update lastAddedNonce nonce as well?
| require.False(t, found) | ||
| } | ||
|
|
||
| func TestHeaderBodyCache_ConcurrentAccess(t *testing.T) { |
There was a problem hiding this comment.
maybe add one more test that calls all methods concurrently
|
|
||
| // blocking operation | ||
| headerBodyPair, ok := he.blocksQueue.Pop() | ||
| /// get pair by nonce from blockchain |
There was a problem hiding this comment.
| /// get pair by nonce from blockchain | |
| // get pair by nonce from blockchain |
| headerBodyPair, ok := he.blocksQueue.Pop() | ||
| /// get pair by nonce from blockchain | ||
| lastExecutedHeader := he.blockChain.GetLastExecutedBlockHeader() | ||
| if lastExecutedHeader == nil { |
There was a problem hiding this comment.
| if lastExecutedHeader == nil { | |
| if check.IfNil(lastExecutedHeader) { |
| continue | ||
| } | ||
|
|
||
| if check.IfNil(headerBodyPair.Header) || check.IfNil(headerBodyPair.Body) { |
There was a problem hiding this comment.
I think this can be removed now
| } | ||
|
|
||
| lastExecutionResult := he.blockChain.GetLastExecutionResult() | ||
| if lastExecutionResult != nil { |
There was a problem hiding this comment.
| if lastExecutionResult != nil { | |
| if check.IfNil(lastExecutionResult) { |
| args := createMockArgs() | ||
| blocksQueue := queue.NewBlocksQueue() | ||
| args.BlocksQueue = blocksQueue | ||
| blocksQueue := cache.NewHeaderBodyCache() |
There was a problem hiding this comment.
| blocksQueue := cache.NewHeaderBodyCache() | |
| blocksCache := cache.NewHeaderBodyCache() |
| RemoveAtNonceAndHigher(nonce uint64) []uint64 | ||
| ValidateQueueIntegrity() error | ||
| Clean(lastAddedNonce uint64) | ||
| GetByNonce(nonce uint64) (cache.HeaderBodyPair, bool) |
There was a problem hiding this comment.
rename interface as well
|
|
||
| executionResultsTracker := executionTrack.NewExecutionResultsTracker() | ||
| tpn.BlocksQueue = queue.NewBlocksQueue() | ||
| tpn.BlocksQueue = headersCache.NewHeaderBodyCache() |
There was a problem hiding this comment.
rename to BlocksCache
| c.mutex.Lock() | ||
| defer c.mutex.Unlock() | ||
|
|
||
| nonces := make([]uint64, 0) | ||
| for nonce := range c.cacheByNonce { | ||
| if nonce >= providedNonce { | ||
| delete(c.cacheByNonce, nonce) | ||
| nonces = append(nonces, nonce) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
You can move the mutex to be used only for this for, up until sort.Slice
| // Should be pair2 (note: they are different pointers for Header/Body so Equal check works if pointer comparison) | ||
| // Wait, slice/map/func in structs make Go equality tricky unless pointers. | ||
| // These are pointers to structs, so it should be fine. |
There was a problem hiding this comment.
Perhaps remove these AI generated comments
| require.True(t, found) | ||
| // Should be pair2 (note: they are different pointers for Header/Body so Equal check works if pointer comparison) | ||
| // Wait, slice/map/func in structs make Go equality tricky unless pointers. | ||
| // These are pointers to structs, so it should be fine. |
There was a problem hiding this comment.
This doesn't actually tests pointer comparison.
It should require that retrievedPair == pair
| log.Warn("executionManager.Close - failed to close headers executor", "error", err) | ||
| } | ||
|
|
||
| em.blocksQueue.Close() |
There was a problem hiding this comment.
Should we clean the cache here? Does it make sense?
There was a problem hiding this comment.
it is not needed, because at the restart the cache will be initialized again.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feat/supernova-async-exec #7620 +/- ##
=============================================================
- Coverage 77.62% 77.60% -0.03%
=============================================================
Files 877 877
Lines 121763 121752 -11
=============================================================
- Hits 94515 94480 -35
- Misses 20987 21004 +17
- Partials 6261 6268 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
# Conflicts: # process/asyncExecution/queue/blocksQueue.go # process/asyncExecution/queue/blocksQueue_test.go
# Conflicts: # node/chainSimulator/process/processor.go
…balance fix unexecutable tx due to initial balance
fix on concurrent header execution and revert
# Conflicts: # process/sync/baseSync.go
# Conflicts: # factory/processing/processComponents.go # integrationTests/testFullNode.go # integrationTests/testProcessorNode.go # integrationTests/testSyncNode.go # integrationTests/vm/staking/metaBlockProcessorCreator.go # process/asyncExecution/headersExecutor_test.go # process/asyncExecution/queue/blocksQueue.go # process/asyncExecution/queue/blocksQueue_test.go # process/block/baseProcess_test.go # process/block/export_test.go
Header body queue capacity in config.toml
Reasoning behind the pull request
Pre-requisites
Based on the Contributing Guidelines the PR author and the reviewers must check the following requirements are met:
featbranch created?featbranch merging, do all satellite projects have a proper tag insidego.mod?