diff --git a/execution/commitment/commitment.go b/execution/commitment/commitment.go index b55e2ccba22..6efb209d8ab 100644 --- a/execution/commitment/commitment.go +++ b/execution/commitment/commitment.go @@ -210,6 +210,9 @@ type DeferredBranchUpdate struct { raw BranchData prev []byte encoded BranchData + // Backing store for a merged encoded value. encoded either aliases raw or points + // here, so it is never itself reused — this is the buffer that survives pooling. + encodedBuf []byte } var deferredUpdatePool = &sync.Pool{ @@ -234,10 +237,7 @@ func getDeferredUpdate(prefix []byte, raw, prev []byte) *DeferredBranchUpdate { upd.prefix = reuseBytes(upd.prefix, prefix) upd.raw = reuseBytes(upd.raw, raw) - // prev stays cloned: it is the one argument that is legitimately nil or empty, and - // callers read a nil prev as "look up the previous value". Deriving that shape from a - // recycled buffer's capacity rather than from the input is not worth one allocation. - upd.prev = bytes.Clone(prev) + upd.prev = reuseBytes(upd.prev, prev) upd.encoded = nil return upd @@ -268,7 +268,8 @@ func capLen(b []byte) []byte { // putDeferredUpdate returns a DeferredBranchUpdate to the global pool. func putDeferredUpdate(upd *DeferredBranchUpdate) { if upd != nil { - upd.prev = nil + // encoded can alias raw, so it is dropped rather than recycled; prefix, raw, + // prev and encodedBuf keep their backing arrays for the next checkout. upd.encoded = nil deferredUpdatePool.Put(upd) } @@ -354,7 +355,8 @@ func mergeDeferredUpdate(upd *DeferredBranchUpdate, merger *BranchMerger) error if err != nil { return err } - upd.encoded = bytes.Clone(merged) + upd.encodedBuf = reuseBytes(upd.encodedBuf, merged) + upd.encoded = upd.encodedBuf return nil } upd.encoded = upd.raw diff --git a/execution/commitment/commitmentdb/commitment_context.go b/execution/commitment/commitmentdb/commitment_context.go index 982fb0ab4c4..4df2a4def13 100644 --- a/execution/commitment/commitmentdb/commitment_context.go +++ b/execution/commitment/commitmentdb/commitment_context.go @@ -790,7 +790,8 @@ func (sdc *SharedDomainsCommitmentContext) LatestCommitmentState(trieContext *Tr } txNum, blockNum = DecodeTxBlockNums(state) - return blockNum, txNum, state, nil + // Outlives this call: Branch hands back a buffer it reuses on the next read. + return blockNum, txNum, bytes.Clone(state), nil } // SeekCommitment searches for last encoded state from DomainCommitted @@ -945,6 +946,8 @@ type TrieContext struct { traceW io.Writer // nil = disabled; traces branch reads/writes (see [SDC] lines) stateReader StateReader localCollector *etl.Collector // per-goroutine collector for concurrent PutBranch + + branchBuf []byte // reused across Branch calls; see the ownership note on Branch } // NewTrieContextRo creates a read-only TrieContext for Branch-only lookups. @@ -958,15 +961,24 @@ func (sdc *TrieContext) Branch(pref []byte) ([]byte, kv.Step, error) { if err != nil { return nil, 0, err } - // Branch reads feed Merge(prev,update), branchEncoder/merger internal buffers, - // deferred-update queues, and unfoldBranchNode reads. The slice returned by the - // underlying state cache / getter aliases shared storage that another goroutine - // (concurrent commitment workers) can recycle. Own the bytes at the trie-context - // boundary so all downstream consumers are safe. if sdc.traceW != nil { fmt.Fprintf(sdc.traceW, "[SDC] Branch read %x => %x\n", pref, enc) } - return bytes.Clone(enc), step, nil + // The slice from the underlying state cache / getter aliases storage another + // commitment worker can recycle, so the bytes have to be owned here. They are + // copied into a per-context buffer rather than a fresh allocation: a trie + // context belongs to exactly one goroutine (every mount worker builds its own + // via TrieContextFactory), and every caller finishes with one branch before + // reading the next. LatestCommitmentState is the sole caller that keeps the + // bytes past its own return, and clones them itself. + if enc == nil { + return nil, step, nil + } + if sdc.branchBuf == nil { + sdc.branchBuf = make([]byte, 0, len(enc)) + } + sdc.branchBuf = append(sdc.branchBuf[:0], enc...) + return sdc.branchBuf, step, nil } func (sdc *TrieContext) PutBranch(prefix []byte, data []byte, prevData []byte) error { diff --git a/execution/commitment/commitmentdb/commitment_context_test.go b/execution/commitment/commitmentdb/commitment_context_test.go index 11d04181f11..1bc9ccd9583 100644 --- a/execution/commitment/commitmentdb/commitment_context_test.go +++ b/execution/commitment/commitmentdb/commitment_context_test.go @@ -83,3 +83,53 @@ func Test_TrieContext_BranchCopiesData(t *testing.T) { branch[1] = 8 require.Equal(t, []byte{9, 2, 3}, reader.branchData) } + +func Test_TrieContext_BranchReusesBufferAcrossReads(t *testing.T) { + t.Parallel() + + reader := &testStateReader{branchData: []byte{1, 2, 3}, step: 7} + ctx := NewTrieContextRo(reader, 1) + + got1, _, err := ctx.Branch([]byte{0xaa}) + require.NoError(t, err) + require.Equal(t, []byte{1, 2, 3}, got1) + + reader.branchData = []byte{4, 5, 6} + got2, _, err := ctx.Branch([]byte{0xbb}) + require.NoError(t, err) + require.Equal(t, []byte{4, 5, 6}, got2) + + // The contract Branch's callers rely on: the returned bytes live only until the + // next read on this context. A caller that keeps them sees the newer branch. + require.Equal(t, []byte{4, 5, 6}, got1, "second read must land in the same buffer") + require.Equal(t, &got1[0], &got2[0], "second read must not allocate a new buffer") +} + +func Test_TrieContext_BranchKeepsNilAndEmptyDistinct(t *testing.T) { + t.Parallel() + + // A nil branch means "absent"; callers test it with == nil, so reusing a buffer + // must not turn it into an empty non-nil slice. + reader := &testStateReader{} + ctx := NewTrieContextRo(reader, 1) + + got, _, err := ctx.Branch([]byte{0xaa}) + require.NoError(t, err) + require.Nil(t, got, "absent branch must stay nil") + + reader.branchData = []byte{1, 2, 3} + if _, _, err = ctx.Branch([]byte{0xbb}); err != nil { + t.Fatal(err) + } + + reader.branchData = []byte{} + got, _, err = ctx.Branch([]byte{0xcc}) + require.NoError(t, err) + require.NotNil(t, got, "present-but-empty branch must stay non-nil after the buffer is warm") + require.Empty(t, got) + + reader.branchData = nil + got, _, err = ctx.Branch([]byte{0xdd}) + require.NoError(t, err) + require.Nil(t, got, "absent branch must stay nil after the buffer is warm") +}