feat: cache validator workload in BuildLastCommitInfo for 28x performance improvement #5421
Conversation
state/execution.go
Outdated
| } | ||
|
|
||
| // ExecCommitBlockWithCache is an optimized version that uses caching for better performance | ||
| func (blockExec *BlockExecutor) ExecCommitBlockWithCache( |
There was a problem hiding this comment.
this is unused. looks like the previous ExecCommitBlock is only used when replaying blocks. do we want to use that there?
state/execution.go
Outdated
| } | ||
|
|
||
| // ClearValidatorCache clears the validator caches to free memory | ||
| func (blockExec *BlockExecutor) ClearValidatorCache() { |
There was a problem hiding this comment.
this is unused except in tests, so the part of your test that is testing cache clearing isn't really testing anything
| validatorCache: make(map[int64]*types.ValidatorSet), | ||
| abciValidatorCache: make(map[string]abci.Validator), |
There was a problem hiding this comment.
in real execution, no values from these caches are ever removed, so these will grow forever. We will need someway to periodically remove elements from them.
state/execution.go
Outdated
| commitSig := block.LastCommit.Signatures[i] | ||
|
|
||
| // Create cache key for this validator | ||
| cacheKey := fmt.Sprintf("%s_%d", val.PubKey.Address(), val.VotingPower) |
There was a problem hiding this comment.
from my understanding the point of the abciValidatorCache is to not have to do the expensive val.PubKey.Address() call. however you are calling val.PubKey.Address() when generating the cacheKey. doesn't this eliminate most of the point of the cache?
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
|
||
| // Cleanup old cache entries if needed | ||
| blockExec.cleanupOldCacheEntries() | ||
| } |
There was a problem hiding this comment.
Bug: Validator Cache Issues in ABCI
The abciValidatorCache in BuildLastCommitInfoWithCache has a few problems: it uses a manual abci.Validator conversion that may miss important logic, its cache key (validator address) can lead to stale voting power if power changes, and it has a race condition allowing redundant conversions and cache writes.
Summary
Implements caching for
state.BuildLastCommitInfoto eliminate performance bottlenecks in consensus operations.Changes
BlockExecutorBuildLastCommitInfoFromStoreWithCacheandBuildLastCommitInfoWithCachemethodsapplyBlock,ExtendVote,ProcessProposalClearValidatorCache()for memory managementPerformance Impact
Fixes #3152
Note
Introduce cached validator/ABCI validator lookups in
BuildLastCommitInfoand update call sites for faster block processing.state/execution.go):validatorCacheandabciValidatorCachewith RW locks toBlockExecutor.BuildLastCommitInfoFromStoreWithCache,BuildLastCommitInfoWithCache, andClearValidatorCache.buildLastCommitInfoFromStoreinProcessProposal,FinalizeBlockpath (applyBlock), andExtendVotewith cached variants.NewBlockExecutor.ExecCommitBlockWithCachehelper.state/execution_test.go):Written by Cursor Bugbot for commit 5663a3f. This will update automatically on new commits. Configure here.