BCI-3668 Optimise HeadTracker's memory usage #14130
Conversation
# Conflicts: # core/chains/evm/txmgr/confirmer_test.go # core/chains/evm/txmgr/txmgr_test.go
…tcontractkit/chainlink into feature/BCI-3668-optimise-ht-memory
This reverts commit 76343e5.
dhaidashenko
requested review from
cedric-cordenier
and removed request for
a team
| } | ||
| if parent, ok := h.headsByHash[newHead.ParentHash]; ok { | ||
| if parent.Number >= newHead.Number { | ||
| return fmt.Errorf("potential cycle detected while adding newHead as child: %w", newPotentialCycleError(parent, newHead)) |
There was a problem hiding this comment.
I understand this is a cycle problem in the context of Head Tracker, but this is more of a general problem as the parent's block number is higher than the child that references it. I would consider this as invalid head/data from the chain.
There was a problem hiding this comment.
newPotentialCycleError already signals that we expect block number to strictly decrease in 'child -> parent' relation. Do you suggest changing this msg?
There was a problem hiding this comment.
newPotentialCycleError error message is accurate. Perhaps I would rephrase the potential cycle... part because if the hashes are right, from a user perspective the issue would be better categorized as bad data on the block number rather than a cycle problem, but it's just me nitpicking. You can ignore this.
core/chains/evm/headtracker/heap.go
Outdated
| func (h *headsHeap) Pop() any { | ||
| old := h.values[len(h.values)-1] | ||
| h.values = h.values[:len(h.values)-1] |
There was a problem hiding this comment.
Given the heap is structured in ascending order, shouldn't old pop the smallest/first item of the array?
There was a problem hiding this comment.
headsHeap implements heap.Interface, which requires Pop to remove the last element from the slice.
core/chains/evm/types/models.go
Outdated
| if h == nil { | ||
| return 0 | ||
| } |
There was a problem hiding this comment.
Should we remove this since you already check for nil pointer below?
There was a problem hiding this comment.
This is fine, but I wonder if we have a generic heap type anywhere already.
# Conflicts: # core/chains/evm/logpoller/log_poller_internal_test.go
dhaidashenko
dismissed stale reviews from jmank88, DylanTinianov, and dimriou
via
73c7c9a
|
…develop * origin/develop: (233 commits) update Smoke Test TestAutomationBasic to load pre-deployed contracts if given (#14537) CCIP-2881 USDC Reader integration tests (#14516) [TT-1624] link PR to solidity review issue (#14521) Fix skipped notification in E2E test workflow (#14538) Add regression testing for pruning bug (#14454) Bump owner dep (#14531) Fix state view (#14532) Deployment tooling (#14533) CCIP 3388 - add offramp generation (#14526) CCIP-3416 paginate token admin registry get all tokens call (#14514) Change Polygon zkEVM to use FeeHistory estimator (#14161) [TT-1747] fix core changeset (#14524) TT-1459 Use CTF actions from .github repo (#14522) [TT-1693] try more universal Solidity scripts (#14436) Remove unused workflow for e2e tests (#14520) BCI-3668 Optimise HeadTracker's memory usage (#14130) BCFR-888 LP support chains that have not reached finality yet (#14366) support new heads polling over http rpc client (#14373) Bump CTF (#14518) TT-1550 Migrate remaining E2E workflows to the reusable workflow (#14403) ...
Ticket.
Previously HeadTracker performed deep copy of the canonical chain every time new block was added to avoid race condition on .Parent field, when old blocks were removed. This causes large number of allocations and as result increases CPU usage due to GC.
PR replaces deep copy with atomic pointer, which results in slower iteration through the chain, but significantly reduces number of allocations.
Benchmark results:
Iterate through whole chain:
Add new head to the chain: