perf(state): avoid full header decode in StateAtBlockNumber#3808
perf(state): avoid full header decode in StateAtBlockNumber#38080xsamalt wants to merge 3 commits intoNethermindEth:mainNethermindEth/juno:mainfrom 0xsamalt:perf/state-at-block-number-skip-header-decode0xsamalt/juno:perf/state-at-block-number-skip-header-decodeCopy head branch name to clipboard
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3808 +/- ##
==========================================
- Coverage 75.30% 75.16% -0.15%
==========================================
Files 438 438
Lines 39380 39318 -62
==========================================
- Hits 29655 29552 -103
- Misses 7641 7687 +46
+ Partials 2084 2079 -5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
This is a good possible optimization, but we still need to see the benchmarks (similar to the ones here) so we actually know this is an optimization.
I know you are not implicitly claiming this to be a performance improvement, but this PR is of the perf type.
And I cannot approve performance without measurements.
I'm not sure if you are just starting your career and looking for "first PRs in open source", but if that is the case, and you are interested in performance, I recommend this very small guide. It is focused in C++, but the principles are good and any modern compiled or lower-level language should behave similarly.
Also, there is another possible performance issue you need to address.
| state, err := core.GetGlobalStateRootByBlockNumber(r, blockNumber) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if _, err := core.GetBlockHeaderNumberByHash(r, header.Hash); err != nil { | ||
| hash, err := core.GetBlockHeaderHashByNumber(r, blockNumber) |
There was a problem hiding this comment.
This needs measurements and fixes
Since this is a perf PR, so I'm being more picky related to performance stuff.
Both GetBlockHeaderHashByNumber and GetGlobalStateRootByBlockNumber internally call:
err := r.Get(db.BlockHeaderByNumberKey(blockNum), func(data []byte) error {
return encoder.Unmarshal(data, &header)
})so now we are querying the DB and marshaling twice.
One way to fix this is creating a new accessor.
There was a problem hiding this comment.
I have now included this in the following commit.
SummaryConsolidates the header hash and global state root reads used by Previously this path called two separate reads — This adds BenchmarksI ran local benchmarks ( Runs were interleaved (base/new/base/new) with CPU pinning (
Only I also saw a significant delta on I don't have a reliable perf number to report from local testing, despite several rounds of interleaved, CPU-pinned runs. Happy to have this re-benchmarked on other hardware or in CI if a hard number is needed before merge otherwise this PR should stand on correctness/code-clarity grounds (one DB read + unmarshal instead of two). |
| func HeaderByNumberIfStateRetained(r db.KeyValueReader, blockNumber uint64) (*core.Header, error) { | ||
| header, err := core.GetBlockHeaderByNumber(r, blockNumber) | ||
| func StateRootByNumberIfStateRetained(r db.KeyValueReader, blockNumber uint64) (*felt.Felt, error) { | ||
| state, hash, err := core.GetStateRootAndHashByBlockNumber(r, blockNumber) |
There was a problem hiding this comment.
The return values are swapped. GetStateRootAndHashByBlockNumber returns (hash, stateRoot), but it's bound here as state, hash. So the retention check runs on the state root instead of the hash → ErrKeyNotFound for every block, and StateAtBlockNumber breaks. TestStateRootByNumberIfStateRetained fails on this branch because of it.
Fix: hash, state, err := core.GetStateRootAndHashByBlockNumber(r, blockNumber)
There was a problem hiding this comment.
Thanks for pointing it out! Fixing it to hash, state, err := core.GetStateRootAndHashByBlockNumber(r, blockNumber) and re-running TestStateRootByNumberIfStateRetained locally before pushing.
|
Fixed the swapped binding bug you flagged, Ehsan confirmed @RafaelGranza , re: the benchmarks I ran them properly interleaved/CPU-pinned but couldn't get a clean signal beyond noise, wrote up the details in the PR. I only have the one machine to test on, so I can't rule out local noise myself. If there's a way to get this benchmarked somewhere more controlled, happy to do that. Otherwise I'd lean on the correctness/clarity argument (one read+unmarshal instead of two) rather than force a number that isn't really there. |
Summary
Closes #3784.
StateAtBlockNumberonly needs a block's global state root to check retention and construct aStateHistory, but it was going throughpruner.HeaderByNumberIfStateRetained, which fully decodes the block header including fields likeEventsBloomthat this path never uses.Background
#3782 already introduced partial-decode accessors for this exact purpose:
core.GetGlobalStateRootByBlockNumbercore.GetBlockHeaderHashByNumberand
pruner.RequireStateRetainedByBlockNumberalready used the hash-based retention check pattern. The only remaining gap was thatStateAtBlockNumberinblockchain/statebackend/statebackend.gohadn't been updated to use them.Changes
pruner/retention.go: renamedHeaderByNumberIfStateRetained→StateRootByNumberIfStateRetained. It now returns*felt.Felt(the state root) instead of*core.Header, built from:GetGlobalStateRootByBlockNumber→GetBlockHeaderHashByNumber→GetBlockHeaderNumberByHash(retention check).blockchain/statebackend/statebackend.go:StateAtBlockNumbernow callsStateRootByNumberIfStateRetaineddirectly and passes the state root straight tostate.NewStateHistory, avoiding the header decode.pruner/retention_test.go: renamedTestHeaderByNumberIfStateRetained→TestStateRootByNumberIfStateRetained; the "fully retained" case now asserts on the state root instead of a header field. The pruned/missing-block error cases only needed the function name updated no logic changes.Testing
go build ./...— cleango test ./pruner/... ./blockchain/statebackend/... ./core/...—all pass
from perf(state): decode only state root when opening head state #3782's
TestPartialBlockHeaderAccessorsByNumber, and no new decode logic was introduced here.Next
Following up with #3785 (
StateAtBlockHash), same pattern.