Repository navigation
Improve giga execution performance - #4317
Conversation
PR SummaryMedium Risk Overview Execution performance on all store-backed paths: single-row evmonly-loadtest gains Monitoring: Prometheus scrape job for loadtest metrics on :9698 and a new EVMOnly Loadtest Grafana dashboard (throughput, executor wall time by phase/stage, pipeline upstream work, OCC, view cache, commit breakdown). Tests cover overlay/pipeline equivalence, reorder bounds, account read coalescing, and pool sizing rules. Reviewed by Cursor Bugbot for commit c97e110. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4317 +/- ##
==========================================
- Coverage 67.58% 66.87% -0.72%
==========================================
Files 2163 2068 -95
Lines 167636 159076 -8560
==========================================
- Hits 113299 106375 -6924
+ Misses 54328 52692 -1636
Partials 9 9
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
The pipelined commit, the overlay, and the single-row account reads look correct. The ordering, the latched failure, and the read-pending-before-OpenView race are each covered by a focused test. The remaining notes are robustness gaps in Close and in the load-test streaming path. None of them blocks the merge.
Findings: 0 blocking | 3 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- None at the file/PR level.
- 3 suggestion(s)/nit(s) flagged inline on specific lines.
cody-littley
left a comment
cody-littley
left a comment
There was a problem hiding this comment.
LGTM. I only reviewed the DB stuff, probably good to have somebody with EVM knowledge review the execution stuff.
| type AccountReader interface { | ||
| // ReadAccount returns addr's row fields, or false when addr has no account. CodeHash is the | ||
| // empty-code hash for an account that holds no code, matching GetCodeHash. | ||
| ReadAccount(addr Address) (AccountSnapshot, bool) | ||
| } |
There was a problem hiding this comment.
Personal preference. Would it be possible to just put this method into the EVMStateView interface? In general, I prefer to avoid creating lots of interfaces there is a specific reason.
There was a problem hiding this comment.
Yeah, make sense to merge this into EVMStateView interface
shemnon
left a comment
shemnon
left a comment
There was a problem hiding this comment.
Looks reasonable to me. Just the one style nit.
We will need to get some way to test the durability of in-flight but unlanded pipeline stages in presence of unexpected failure (disk error, evm panic) but I'm not sure how to induce those in a test environment or how it would impact the code.
| timer := phases.Build() | ||
| defer timer.Reset() | ||
| for groupCtx.Err() == nil { | ||
| timer.SetPhase(phaseWaitingForWork) |
There was a problem hiding this comment.
There is an inconsistency in calls to SetPhase where some are done via magic string and some are constants. And some strings are reused, some are not, and lastPhase is only read for metrics.
Calls to SetPhase should be consistent, and the quickest path to victory is string constants.
There was a problem hiding this comment.
Make sense, will fix
| // predates them, leaving nothing to supply them. Read first, the worst case is a view that | ||
| // already holds them and an overlay that replays the same values over the top. | ||
| pending := e.pipelinePending() | ||
| e.blockPhases.SetPhase("open_view") |
There was a problem hiding this comment.
nit - can we standardize using constants for these phases? i.e.
const (
phaseExecute = "execute"
phaseOpenView = "open_view"
phaseAwaitCommit = "await_commit"
phaseWaitingForBlock = "waiting_for_block"
)
maybe defined here
There was a problem hiding this comment.
there are a few other ones like encode_receipts
| @@ -0,0 +1,121 @@ | |||
| package evmonly | |||
There was a problem hiding this comment.
do you think it makes sense to add some more test cases i.e.:
- pipeline execution == synchronous execution
- a bunch of writes, then clear. we should not see any dirty entries
There was a problem hiding this comment.
Definitely worth it, will add more tests here to cover more edge cases
* main: (21 commits) Backport evmonly parse, app-hash and changeset perf fixes from giga-1 (#4345) Remove the oracle module behind a v6.8 upgrade (#4319) fix(seidb): report only the current migration boundary on the snapshot gauge (#4327) fix(flatkv): keep 10 old checkpoints instead of mirroring memIAVL's count (#4322) Backport Autobahn execute-loop and produced-tx metrics from giga-1 (#4330) Add giga.storage.receipts to toggle the Autobahn receipt store (#4333) optimize gather phase (#4326) Regenerate the Unreleased changelog as a plain PR list (#4336) Bump sei-protocol/go-ethereum to v1.15.7-sei-21 (#4332) Fail dynamic-gas precompile out-of-gas as an EVM out-of-gas call (#4318) Add dashboard and topology option for Autobahn e2e (#4167) Add Giga fetch/serve and BlockDB prune metrics (#4329) Add eth_getLogs to the EVM-only Giga RPC (#4308) Generate v6.8 precompiles (#4320) Add [giga] app.toml section and honor it on the Autobahn node (#4323) feat(evmonly): add eth_estimateGas via existing libraries (#4325) Use Pebble batch directly in SS (#4300) Fix pruning issue in SS causing huge disk spike (#4321) Make Autobahn always run the EVM-only executor, disable/remove some integration tests (#4316) reduce seal lock contention (#4314) ...
A failed commit is remembered and returned by every later AwaitCommits. The load test calls that before reporting success, and TestFailedPipelinedCommitIsReportedToEveryWaiter covers it. Testing a disk error or an EVM panic in the middle of a commit is worth tracking separately. Note that this pipelining optimization is for evmonly app, the production code path is different |
|
@seidroid review |
There was a problem hiding this comment.
All three findings from the previous review are fixed and covered by tests. The Close path now waits on storeMu, run-ahead in streamBlocks is capped at 2 * builders slots, and the account pool is seeded for bounded runs. One new gap remains: a block executed after Close fails at receipt encoding once it has 64 or more transactions, which breaks the promise in Close's own godoc.
Findings: 0 blocking | 2 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- None at the file/PR level.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
- 1 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion]
forwardPreparedBlocksInOrderingiga/evmonly/cmd/evmonly-loadtest/pipeline.gohas no cap on itspendingreorder map. If one prepare worker stalls, the others keep filling it. The new builder slot limit does not bound this downstream buffer. Streaming--blocks=0runs are unbounded, so this buffer is more likely to grow there. Load-test only. (Raised by Codex.)
| // the same records in the same order as receiptRecords. | ||
| func (e *Executor) receiptRecordsParallel(ctx context.Context, blockNumber uint64, result *BlockResult) ([]receipt.ReceiptRecord, error) { | ||
| count := len(result.Receipts) | ||
| if e.occPool == nil || count < occParallelReceiptThreshold { |
There was a problem hiding this comment.
[suggestion] This guard misses a closed pool. Executor.Close calls occPool.Close() but leaves e.occPool non-nil, so once Close returns, any store-backed block with a receipt store and ≥64 transactions reaches e.occPool.Run. That returns errOCCWorkerPoolClosed, and the block fails with encode receipts for block N. Close's godoc says such blocks "commit synchronously", and TestExecutorCloseDisablesOCC relies on blocks still running after Close. The shutdown tests use fewer than 64 transactions, so they do not catch this. Suggested fix: add e.closed.Load() to this condition, as useOCC does, or fall back to receiptRecords on errOCCWorkerPoolClosed. Add a test that executes a block of 64 or more transactions after Close with a receipt store. (Also raised by Codex.)
…tocol#4366) The EVM-only executor on main can already pipeline commits (`PrepareBlock`, `ExecutePreparedBlock`, `AwaitCommits`, from sei-protocol#4317), but `evmOnlyApplication.FinalizeBlock` still calls the synchronous `ExecuteBlock`. That call waits for the FlatKV commit of block N before it returns, so the node can't start N+1 until the write lands. This PR brings the application-layer half of sei-protocol#4270 over from `giga-1` (PLT-1312), adapted to main's `evmOnlyState` / sender-cache layout instead of the older cursor/executor split that sei-protocol#4270 assumes. `FinalizeBlock` now runs `PrepareBlock` and then `ExecutePreparedBlock` (in `executeBlockPipelined`), and returns once the state commit has started. The next block still waits on the previous commit inside the executor before it starts its own. Committed-state readers settle the in-flight commit first. `EvmNonce`, `EvmBalance` and `EvmCode` go through `openSettledView`, which calls `AwaitCommits` on the executor published through a lock-free `settler`, so they never block on `FinalizeBlock`. `currentExecutionContext`, used by `EvmCall` and `EvmEstimateGas`, awaits while it holds `state`, so no new block can start between the settle and the snapshot, and it returns a failed commit as an error. `openSettledView` logs a failed commit once and serves the last version that landed. Its return values have no error channel, and the failure halts the node through the next `FinalizeBlock`. `Proxy.AwaitCommits` exposes the settle, and `nodeImpl.closeGigaStorage` calls it before `manager.Close()` so shutdown never closes the store under an in-flight write. Consensus and app hash are unchanged: the app hash is still computed from the execution result, not the store write. The behaviour change is that a failed store commit now surfaces one block later, from the next `FinalizeBlock`, rather than from its own. Between `FinalizeBlock` and `Commit`, `EvmNonce` and friends already see the finalized block's state; before this PR they could not, because the write had already landed by then anyway. New tests cover reads settling across four pipelined blocks and a commit failure surfacing from the next block, from `EvmCall` and from `AwaitCommits`, and a `-race` test that drives `EvmNonce`/`EvmCall` from a goroutine while blocks are finalized and committed, asserting the nonce never goes backwards. The test helpers now settle before closing storage. I validated with `scripts/ramtest.sh -race` over `evmonlyapp`, `proxy`, `node` and `giga/evmonly`, plus `make fmtcheck` and `make lint`. Link to Devin session: https://app.devin.ai/sessions/d808db90106341278344623de10b92c0 Open in Devin Desktop: https://app.devin.ai/desktop/session/d808db90106341278344623de10b92c0?variant=devin Requested by: @bdchatham --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
4 participants
Ports the execution optimizations from #4219 (
giga-1) onto main, adapted to changes that have landed on main since.CommitStateChangesruns in the background while the next block executes, reading the previous block's writes through a pending overlay.ExecuteBlockstill waits for its commit, so Autobahn'sevmonlyapp, which calls it fromFinalizeBlock, keeps committing synchronously; wiring the pipeline into Autobahn is a follow-up.AwaitCommitsis the sync point for the pipelined path.EVMStateView.ReadAccountreturns an account's balance, nonce and code hash in one lookup, used bynativeStateDBand the OCC merge.--accountssender pool with streaming--blocks=0,--gc-percent, OTel phase timers, plus a Grafana dashboard and Prometheus scrape target.AccountExistsshort-circuit from Improve EVM execution performance #4219, since main's Skip the redundant AccountExists probe in gigaSnapshotStateReader #4239 already covers it.Scope and risk: Autobahn e2e gets the read, merge and receipt improvements but not the pipelined commit. On the pipelined path a block's state becomes visible later, and
ResultSinkruns once the commit has started rather than after it lands.Results from
evmonly-loadtest(pipelined path) on a 12-core M2 Pro, 0 failed txs, 0 OCC fallbacks:--accounts=200000 --blocks=0 --executor-workers=8 --gc-percent=400: ~73–77k TPS steady state