Skip to content

Commit

Permalink
fix(trie): remove encoding buffers pool (#2929)
Browse files Browse the repository at this point in the history
Trie node encoding sizes vary greatly from a few bytes to megabytes. Therefore using a `sync.Pool` of buffers is wrong and leads to a build up of memory usage. See the PR body for a more detailed explanation.
  • Loading branch information
qdm12 committed Nov 9, 2022
1 parent 7131290 commit f4074cc
Show file tree
Hide file tree
Showing 4 changed files with 4 additions and 38 deletions.
15 changes: 1 addition & 14 deletions internal/trie/node/branch_encode.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import (
"io"
"runtime"

"github.com/ChainSafe/gossamer/internal/trie/pools"
"github.com/ChainSafe/gossamer/pkg/scale"
)

Expand All @@ -21,11 +20,7 @@ type encodingAsyncResult struct {

func runEncodeChild(child *Node, index int,
results chan<- encodingAsyncResult, rateLimit <-chan struct{}) {
buffer := pools.EncodingBuffers.Get().(*bytes.Buffer)
buffer.Reset()
// buffer is put back in the pool after processing its
// data in the select block below.

buffer := bytes.NewBuffer(nil)
err := encodeChild(child, buffer)

results <- encodingAsyncResult{
Expand Down Expand Up @@ -97,20 +92,12 @@ func encodeChildrenOpportunisticParallel(children []*Node, buffer io.Writer) (er
}
}

pools.EncodingBuffers.Put(resultBuffers[currentIndex])
resultBuffers[currentIndex] = nil

currentIndex++
}
}

for _, buffer := range resultBuffers {
if buffer == nil { // already emptied and put back in pool
continue
}
pools.EncodingBuffers.Put(buffer)
}

return err
}

Expand Down
12 changes: 2 additions & 10 deletions internal/trie/node/hash.go
Original file line number Diff line number Diff line change
Expand Up @@ -145,21 +145,13 @@ func (n *Node) encodeIfNeeded() (encoding []byte, err error) {
return n.Encoding, nil // no need to copy
}

buffer := pools.EncodingBuffers.Get().(*bytes.Buffer)
buffer.Reset()
defer pools.EncodingBuffers.Put(buffer)

buffer := bytes.NewBuffer(nil)
err = n.Encode(buffer)
if err != nil {
return nil, fmt.Errorf("encoding: %w", err)
}

bufferBytes := buffer.Bytes()

// TODO remove this copying since it defeats the purpose of `buffer`
// and the sync.Pool.
n.Encoding = make([]byte, len(bufferBytes))
copy(n.Encoding, bufferBytes)
n.Encoding = buffer.Bytes()

return n.Encoding, nil // no need to copy
}
9 changes: 0 additions & 9 deletions internal/trie/pools/pools.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,6 @@ var DigestBuffers = &sync.Pool{
},
}

// EncodingBuffers is a sync pool of buffers of capacity 1.9MB.
var EncodingBuffers = &sync.Pool{
New: func() interface{} {
const initialBufferCapacity = 1900000 // 1.9MB, from checking capacities at runtime
b := make([]byte, 0, initialBufferCapacity)
return bytes.NewBuffer(b)
},
}

// Hashers is a sync pool of blake2b 256 hashers.
var Hashers = &sync.Pool{
New: func() interface{} {
Expand Down
6 changes: 1 addition & 5 deletions lib/trie/trie.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import (

"github.com/ChainSafe/gossamer/internal/trie/codec"
"github.com/ChainSafe/gossamer/internal/trie/node"
"github.com/ChainSafe/gossamer/internal/trie/pools"
"github.com/ChainSafe/gossamer/lib/common"
)

Expand Down Expand Up @@ -196,10 +195,7 @@ func (t *Trie) MustHash() common.Hash {

// Hash returns the hashed root of the trie.
func (t *Trie) Hash() (rootHash common.Hash, err error) {
buffer := pools.EncodingBuffers.Get().(*bytes.Buffer)
buffer.Reset()
defer pools.EncodingBuffers.Put(buffer)

buffer := bytes.NewBuffer(nil)
err = encodeRoot(t.root, buffer)
if err != nil {
return [32]byte{}, err
Expand Down

0 comments on commit f4074cc

Please sign in to comment.