From 152ac249315a50f428dce490a8103d93177fadd7 Mon Sep 17 00:00:00 2001 From: Emmanuel T Odeke Date: Sat, 11 Jul 2020 04:44:10 -0700 Subject: [PATCH] rootmulti/internal/maps: remove duplicated code, simplify and speed up KVPair (#6689) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Noticed during an audit, we've got duplicated code that's unused. There were 2 type definitions: * type kvPair types.Pair * type KVPair types.Pair and each had a .bytes() and .Bytes() method respectively that then used some extra code. This change deletes the duplicated/unnecessary code but also while here improves the performance by removing the use of a bytes.Buffer which is unnecessary given that we are only length prefixed the key, length prefixing the value hence the various helpers are unnecessary. The added benchmarks shows the performance boost ```shell $ benchstat before after name old time/op new time/op delta KVPairBytes-8 146µs ± 1% 142µs ± 2% -3.05% (p=0.000 n=18+17) name old speed new speed delta KVPairBytes-8 6.84GB/s ± 1% 7.06GB/s ± 2% +3.15% (p=0.000 n=18+17) name old alloc/op new alloc/op delta KVPairBytes-8 1.01MB ± 0% 1.01MB ± 0% -0.04% (p=0.000 n=17+20) name old allocs/op new allocs/op delta KVPairBytes-8 6.00 ± 0% 1.00 ± 0% -83.33% (p=0.000 n=20+20) ``` Closes: #6688 --- store/rootmulti/internal/maps/bench_test.go | 13 ++++ store/rootmulti/internal/maps/maps.go | 66 ++++++--------------- 2 files changed, 31 insertions(+), 48 deletions(-) create mode 100644 store/rootmulti/internal/maps/bench_test.go diff --git a/store/rootmulti/internal/maps/bench_test.go b/store/rootmulti/internal/maps/bench_test.go new file mode 100644 index 0000000000..4d7f680c70 --- /dev/null +++ b/store/rootmulti/internal/maps/bench_test.go @@ -0,0 +1,13 @@ +package maps + +import "testing" + +func BenchmarkKVPairBytes(b *testing.B) { + kvp := NewKVPair(make([]byte, 128), make([]byte, 1e6)) + b.ResetTimer() + b.ReportAllocs() + + for i := 0; i < b.N; i++ { + b.SetBytes(int64(len(kvp.Bytes()))) + } +} diff --git a/store/rootmulti/internal/maps/maps.go b/store/rootmulti/internal/maps/maps.go index 91edd11773..1dfdca6ff2 100644 --- a/store/rootmulti/internal/maps/maps.go +++ b/store/rootmulti/internal/maps/maps.go @@ -1,9 +1,7 @@ package maps import ( - "bytes" "encoding/binary" - "io" "github.com/tendermint/tendermint/crypto/merkle" "github.com/tendermint/tendermint/crypto/tmhash" @@ -55,47 +53,12 @@ func (sm *merkleMap) sort() { sm.sorted = true } -// kvPair defines a type alias for kv.Pair so that we can create bytes to hash -// when constructing the merkle root. Note, key and values are both length-prefixed. -type kvPair kv.Pair - -// bytes returns a byte slice representation of the kvPair where the key and value -// are length-prefixed. -func (kv kvPair) bytes() []byte { - var b bytes.Buffer - - err := encodeByteSlice(&b, kv.Key) - if err != nil { - panic(err) - } - - err = encodeByteSlice(&b, kv.Value) - if err != nil { - panic(err) - } - - return b.Bytes() -} - -func encodeByteSlice(w io.Writer, bz []byte) error { - var buf [8]byte - n := binary.PutUvarint(buf[:], uint64(len(bz))) - - _, err := w.Write(buf[:n]) - if err != nil { - return err - } - - _, err = w.Write(bz) - return err -} - // hashKVPairs hashes a kvPair and creates a merkle tree where the leaves are // byte slices. func hashKVPairs(kvs kv.Pairs) []byte { kvsH := make([][]byte, len(kvs)) for i, kvp := range kvs { - kvsH[i] = kvPair(kvp).bytes() + kvsH[i] = KVPair(kvp).Bytes() } return merkle.SimpleHashFromByteSlices(kvsH) @@ -177,16 +140,23 @@ func NewKVPair(key, value []byte) KVPair { // Bytes returns key || value, with both the // key and value length prefixed. func (kv KVPair) Bytes() []byte { - var b bytes.Buffer - err := encodeByteSlice(&b, kv.Key) - if err != nil { - panic(err) - } - err = encodeByteSlice(&b, kv.Value) - if err != nil { - panic(err) - } - return b.Bytes() + // In the worst case: + // * 8 bytes to Uvarint encode the length of the key + // * 8 bytes to Uvarint encode the length of the value + // So preallocate for the worst case, which will in total + // be a maximum of 14 bytes wasted, if len(key)=1, len(value)=1, + // but that's going to rare. + buf := make([]byte, 8+len(kv.Key)+8+len(kv.Value)) + + // Encode the key, prefixed with its length. + nlk := binary.PutUvarint(buf, uint64(len(kv.Key))) + nk := copy(buf[nlk:], kv.Key) + + // Encode the value, prefixing with its length. + nlv := binary.PutUvarint(buf[nlk+nk:], uint64(len(kv.Value))) + nv := copy(buf[nlk+nk+nlv:], kv.Value) + + return buf[:nlk+nk+nlv+nv] } // SimpleHashFromMap computes a merkle tree from sorted map and returns the merkle