Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions execution/commitment/commitment.go
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,9 @@
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{
Expand All @@ -234,10 +237,7 @@

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
Expand Down Expand Up @@ -268,7 +268,8 @@
// 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)
}
Expand Down Expand Up @@ -354,7 +355,8 @@
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
Expand Down Expand Up @@ -1751,7 +1753,7 @@
}

// fn must not retain hk or pk slices after returning: they're backed by reusable arena memory.
func (t *Updates) HashSort(ctx context.Context, warmuper *Warmuper, fn func(hk, pk []byte, update *Update) error) error {

Check failure on line 1756 in execution/commitment/commitment.go

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Refactor this method to reduce its Cognitive Complexity from 75 to the 60 allowed.

See more on https://sonarcloud.io/project/issues?id=erigontech_erigon&issues=AaBB1-Y6nCXiaKqLPFdv&open=AaBB1-Y6nCXiaKqLPFdv&pullRequest=23617
switch t.mode {
case ModeDirect:
cnt := len(t.keys)
Expand Down
26 changes: 19 additions & 7 deletions execution/commitment/commitmentdb/commitment_context.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand All @@ -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 {
Expand Down
50 changes: 50 additions & 0 deletions execution/commitment/commitmentdb/commitment_context_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
Loading