[wip] commitment: reuse pooled trie workers across Process calls - #22839
[wip] commitment: reuse pooled trie workers across Process calls#22839AskAlexSharov wants to merge 7 commits into
Conversation
resetPool() replaced the whole sync.Pool object on every Reset and Release, discarding every cached worker trie. On a mainnet node heap profile (alloc_space) the resulting refills cost 943MB — 27% of all sync.Pool refill bytes process-wide, ~9.8K refills at ~93KB each, one full NewHexPatriciaHashed per refill — and the drain was self-inflicted, not GC: Reset runs once per commitment round via SharedDomains.ClearRam. Workers are already scrubbed with resetForReuse() before every Put, so pool contents are safe to reuse. Replace the raw sync.Pool with a small trieWorkerPool that: - keeps cached workers across Reset (accountKeyLen/cfg never change after construction, so there is nothing to invalidate); - re-applies accountKeyLen + cfg on every pooled get, making a pooled hit identical to fresh construction (previously an intra-Process recycled worker kept zeroed cfg/memoizationOff while a fresh one got the configured values); - on Release drains cached workers back to the global hphPool, so a successor instance (fresh SharedDomains after a commit cycle) refills from cached objects instead of allocating. put() centralizes the resetForReuse-before-Put scrub that every call site previously repeated by hand. Claude-Session: https://claude.ai/code/session_01UYCsHj9HYTJnqUCy8a7W91
There was a problem hiding this comment.
Pull request overview
This PR optimizes the execution commitment pipeline by avoiding sync.Pool churn: pooled HexPatriciaHashed workers are preserved across Reset()/Process() lifecycles and only drained back to the shared constructor pool on Release(), reducing repeated trie construction and refill allocations during commitment rounds.
Changes:
- Introduces
trieWorkerPoolwithget()/put()/drain()to reuse scrubbed trie workers acrossProcess()calls and instances. - Updates Parallel and Streaming commitment implementations to use the new pool and to stop replacing the pool on
Reset(). - Adds tests to assert worker reuse across
Reset()and parity across the “Reset + SetState(blob)” lifecycle.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| execution/commitment/streaming_deep_fold.go | Switches deferred storage folding to use trieWorkerPool get/put instead of raw sync.Pool. |
| execution/commitment/streaming_commitment.go | Replaces sync.Pool reset/replacement with trieWorkerPool init + drain-on-Release, and updates worker checkout/return sites. |
| execution/commitment/streaming_commitment_test.go | Adds a test asserting worker caching survives Reset(). |
| execution/commitment/parallel_patricia_hashed.go | Adds trieWorkerPool and wires it into ParallelPatriciaHashed Reset/Release and worker acquisition. |
| execution/commitment/parallel_patricia_hashed_test.go | Adds tests for worker pool survival across Reset() and for parity when reusing an instance across Reset+SetState. |
| execution/commitment/parallel_mount.go | Updates mount processing to use trieWorkerPool.get/put (centralizing scrub-on-put). |
Comments suppressed due to low confidence (2)
execution/commitment/parallel_patricia_hashed.go:66
- trieWorkerPool.drain() has the same typed-nil risk as get(): if a typed-nil *HexPatriciaHashed is present, w.Release() will panic. Add a w==nil guard (and consider wording the comment as best-effort since sync.Pool is GC-managed).
// drain hands cached workers back to the shared constructor pool instead of the GC.
func (wp *trieWorkerPool) drain() {
for {
w, ok := wp.pool.Get().(*HexPatriciaHashed)
if !ok {
return
}
w.Release()
}
}
execution/commitment/parallel_patricia_hashed.go:55
- trieWorkerPool.put() should defensively ignore nil to avoid inserting a typed-nil pointer into the underlying sync.Pool, which would later trigger a nil dereference in get()/drain().
func (wp *trieWorkerPool) put(w *HexPatriciaHashed) {
w.resetForReuse()
wp.pool.Put(w)
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…nvariant The tagged-worker identity assertion depends on sync.Pool per-P slot placement; goroutine migration (frequent under -race) makes it flaky. The property it guarded is structural now, so pin the checkout invariant instead: every get() is config-correct regardless of pool contents. Claude-Session: https://claude.ai/code/session_01UYCsHj9HYTJnqUCy8a7W91
get() re-applies config on every checkout, so pooled workers are fungible across instances — the instance-local sync.Pool tier and its drain() duplicated what NewHexPatriciaHashed/Release already do against hphPool. Workers now become globally shareable at put() time instead of waiting for instance teardown. Claude-Session: https://claude.ai/code/session_01UYCsHj9HYTJnqUCy8a7W91
|
Follow-up commit: |
… calls The handle only carried accountKeyLen+cfg; call NewHexPatriciaHashed and Release directly, with cfg stored on the owning structs. Claude-Session: https://claude.ai/code/session_01UYCsHj9HYTJnqUCy8a7W91
Problem
On a mainnet node heap profile (alloc_space),
ParallelPatriciaHashed.workerPoolrefills cost 943MB — 27% of all sync.Pool refill bytes process-wide (~9.8K refills at ~93KB each, one fullNewHexPatriciaHashedper refill).The drain was self-inflicted, not GC pressure:
resetPool()replaced the entiresync.Poolobject, instantly discarding every cached worker trie. It ran fromReset()— once per commitment round viaSharedDomains.ClearRam— and fromRelease()on every SharedDomains teardown, in bothParallelPatriciaHashedandStreamingCommitter.Fix
Workers were already scrubbed with
resetForReuse()before everyPut(the same-Process recycle path has always relied on that), so pool contents are safe to reuse across Process calls. The rawsync.Poolis replaced by a smalltrieWorkerPoolthat:Reset()—accountKeyLen/cfgare set once at construction and never mutated afterwards (no assignment site exists), so there is nothing to invalidate and no pool-replacement path remains;accountKeyLen+applyConfig(cfg)on every pooledget(), making a pooled hit state-identical to fresh construction (previously an intra-Process recycled worker kept zeroedcfg/memoizationOffwhile a fresh one carried the configured values);hphPoolonRelease()(viaHexPatriciaHashed.Release()), so a successor instance — e.g. the fresh SharedDomains built after a commit cycle — refills from cached objects instead of allocating;resetForReuse-before-Putscrub input()instead of repeating it at every call site.Why per-use scrubbing is sufficient
resetForReuse()clears everything a use can observe: root cell + root flags,activeRows/currentKeyLen, per-rowdepths/branchBefore/touchMap/afterMap,ctx(nil),hadToLoadL, mount state, trace/collapse/witness tracers,leaveDeferredForCaller/memoizationOff,auxBuffer, branchEncoder deferred set + buffer,cfg,depthsToTxNum. Grid cells are intentionally not zeroed —activeRows == 0means no cell is live andmountTo/unfold re-initialize cells before any read — and cells hold no heap references (cell/Updateare pure value types), so no DB/tx data can leak across transactions. Everyget()site then establishes context, trace writer, and defer flags before use. This is the exact contract same-Process worker recycling has always run under; this PR extends it across Process calls and instances without changing it.When the pool is still emptied
Only on
Release(), where cached workers are handed to the shared constructor pool (hphPool) rather than dropped for the GC. There is no config-change replacement path because the config provably cannot change after construction.Tests
TestParallelWorkerPoolSurvivesReset/TestStreamingWorkerPoolSurvivesReset— written red-first against the old code (a tagged pooled worker must surviveReset(); previously a fresh worker came back).TestParallelReuseAcrossResetParity— two batches on one instance with the production per-blockReset()+SetState(blob)lifecycle between them, for bothVariantParallelHexPatriciaandVariantStreamingHexPatricia, asserting root parity with the sequential engine so batch 2 runs on workers cached by batch 1../execution/commitment/...suite green in both short and non-short mode;./db/state/...short green; parallel commitment benches smoke-run.https://claude.ai/code/session_01UYCsHj9HYTJnqUCy8a7W91