From 85464cc021186a97336270cd6c572586ca93c0b1 Mon Sep 17 00:00:00 2001 From: Faye Amacker <33205765+fxamacker@users.noreply.github.com> Date: Tue, 24 May 2022 09:16:51 -0500 Subject: [PATCH 1/8] Add Ledger.GetSingleValue for speedups and memory MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Added Ledger.GetSingleValue() to improve speed, alloc/op, and allocs/op - Modified delta.View.readFunc to use Ledger.GetSingleValue() - Optimized existing mtrie batch read for single path Bench comparision between current Ledger.Get() vs Ledger.GetSingleValue() Benchstat results: name old time/op new time/op delta LedgerGetOneValue/batch_get-4 6.54µs ± 0% 5.24µs ± 0% -19.92% (p=0.000 n=9+10) name old alloc/op new alloc/op delta LedgerGetOneValue/batch_get-4 1.74kB ± 0% 1.62kB ± 0% -6.85% (p=0.000 n=10+10) name old allocs/op new allocs/op delta LedgerGetOneValue/batch_get-4 21.0 ± 0% 15.0 ± 0% -28.57% (p=0.000 n=10+10) Bench comparision between optimized new Ledger.Get() vs Ledger.GetSingleValue() Benchstat results: name old time/op new time/op delta LedgerGetOneValue/batch_get-4 5.70µs ± 0% 5.23µs ± 0% -8.24% (p=0.000 n=9+10) name old alloc/op new alloc/op delta LedgerGetOneValue/batch_get-4 1.69kB ± 0% 1.62kB ± 0% -3.79% (p=0.000 n=10+10) name old allocs/op new allocs/op delta LedgerGetOneValue/batch_get-4 18.0 ± 0% 15.0 ± 0% -16.67% (p=0.000 n=10+10) --- engine/execution/state/state.go | 22 ++--- ledger/complete/ledger.go | 23 +++++ ledger/complete/ledger_benchmark_test.go | 73 +++++++++++++++ ledger/complete/ledger_test.go | 90 ++++++++++++++++++ ledger/complete/mtrie/forest.go | 18 ++++ ledger/complete/mtrie/forest_test.go | 60 ++++++++++++ ledger/complete/mtrie/trie/trie.go | 35 +++++++ ledger/complete/mtrie/trie/trie_test.go | 83 +++++++++++++++++ ledger/ledger.go | 26 +++++- ledger/partial/ledger.go | 16 ++++ ledger/partial/ledger_test.go | 37 +++++++- ledger/partial/ptrie/partialTrie.go | 9 ++ ledger/partial/ptrie/partialTrie_test.go | 114 +++++++++++++++++++++++ ledger/trie.go | 6 ++ module/chunks/chunkVerifier.go | 6 +- 15 files changed, 598 insertions(+), 20 deletions(-) diff --git a/engine/execution/state/state.go b/engine/execution/state/state.go index c922ea0014a..44e22f1cf28 100644 --- a/engine/execution/state/state.go +++ b/engine/execution/state/state.go @@ -133,11 +133,10 @@ func NewExecutionState( } -func makeSingleValueQuery(commitment flow.StateCommitment, owner, controller, key string) (*ledger.Query, error) { - return ledger.NewQuery(ledger.State(commitment), - []ledger.Key{ - RegisterIDToKey(flow.NewRegisterID(owner, controller, key)), - }) +func makeSingleValueQuery(commitment flow.StateCommitment, owner, controller, key string) (*ledger.QuerySingleValue, error) { + return ledger.NewQuerySingleValue(ledger.State(commitment), + RegisterIDToKey(flow.NewRegisterID(owner, controller, key)), + ) } func makeQuery(commitment flow.StateCommitment, ids []flow.RegisterID) (*ledger.Query, error) { @@ -187,26 +186,21 @@ func LedgerGetRegister(ldg ledger.Ledger, commitment flow.StateCommitment) delta return nil, fmt.Errorf("cannot create ledger query: %w", err) } - values, err := ldg.Get(query) + value, err := ldg.GetSingleValue(query) if err != nil { return nil, fmt.Errorf("error getting register (%s) value at %x: %w", key, commitment, err) } - // We expect 1 element in the returned slice of values because query is from makeSingleValueQuery() - if len(values) != 1 { - return nil, fmt.Errorf("error getting register (%s) value at %x: number of returned values (%d) != number of queried keys (%d)", key, commitment, len(values), len(query.Keys())) - } - // Prevent caching of value with len zero - if len(values[0]) == 0 { + if len(value) == 0 { return nil, nil } // don't cache value with len zero - readCache[regID] = flow.RegisterEntry{Key: regID, Value: values[0]} + readCache[regID] = flow.RegisterEntry{Key: regID, Value: value} - return values[0], nil + return value, nil } } diff --git a/ledger/complete/ledger.go b/ledger/complete/ledger.go index f6a01037d15..f824ac00523 100644 --- a/ledger/complete/ledger.go +++ b/ledger/complete/ledger.go @@ -133,6 +133,29 @@ func (l *Ledger) ValueSizes(query *ledger.Query) (valueSizes []int, err error) { return valueSizes, err } +// GetSingleValue reads value of a single given key at the given state. +func (l *Ledger) GetSingleValue(query *ledger.QuerySingleValue) (value ledger.Value, err error) { + start := time.Now() + path, err := pathfinder.KeyToPath(query.Key(), l.pathFinderVersion) + if err != nil { + return nil, err + } + trieRead := &ledger.TrieReadSinglePayload{RootHash: ledger.RootHash(query.State()), Path: path} + payload, err := l.forest.ReadSinglePayload(trieRead) + if err != nil { + return nil, err + } + + l.metrics.ReadValuesNumber(1) + readDuration := time.Since(start) + l.metrics.ReadDuration(readDuration) + + durationPerValue := time.Duration(readDuration.Nanoseconds()) * time.Nanosecond + l.metrics.ReadDurationPerItem(durationPerValue) + + return payload.Value, nil +} + // Get read the values of the given keys at the given state // it returns the values in the same order as given registerIDs and errors (if any) func (l *Ledger) Get(query *ledger.Query) (values []ledger.Value, err error) { diff --git a/ledger/complete/ledger_benchmark_test.go b/ledger/complete/ledger_benchmark_test.go index 98b5a2f5d40..519aee54e7c 100644 --- a/ledger/complete/ledger_benchmark_test.go +++ b/ledger/complete/ledger_benchmark_test.go @@ -239,6 +239,79 @@ func BenchmarkTrieRead(b *testing.B) { b.StopTimer() } +func BenchmarkLedgerGetOneValue(b *testing.B) { + // key updates per iteration + numInsPerStep := 10000 + keyNumberOfParts := 10 + keyPartMinByteSize := 1 + keyPartMaxByteSize := 100 + valueMaxByteSize := 32 + rand.Seed(1) + + dir, err := os.MkdirTemp("", "test-mtrie-") + defer os.RemoveAll(dir) + if err != nil { + b.Fatal(err) + } + + diskWal, err := wal.NewDiskWAL(zerolog.Nop(), nil, metrics.NewNoopCollector(), dir, 101, pathfinder.PathByteSize, wal.SegmentSize) + require.NoError(b, err) + defer func() { + <-diskWal.Done() + }() + + led, err := complete.NewLedger(diskWal, 101, &metrics.NoopCollector{}, zerolog.Logger{}, complete.DefaultPathFinderVersion) + defer led.Done() + if err != nil { + b.Fatal("can't create a new complete ledger") + } + + state := led.InitialState() + + keys := utils.RandomUniqueKeys(numInsPerStep, keyNumberOfParts, keyPartMinByteSize, keyPartMaxByteSize) + values := utils.RandomValues(numInsPerStep, 1, valueMaxByteSize) + + update, err := ledger.NewUpdate(state, keys, values) + if err != nil { + b.Fatal(err) + } + + newState, _, err := led.Set(update) + if err != nil { + b.Fatal(err) + } + + b.Run("batch get", func(b *testing.B) { + query, err := ledger.NewQuery(newState, []ledger.Key{keys[0]}) + if err != nil { + b.Fatal(err) + } + + b.ResetTimer() + for i := 0; i < b.N; i++ { + _, err = led.Get(query) + if err != nil { + b.Fatal(err) + } + } + }) + + b.Run("single get", func(b *testing.B) { + query, err := ledger.NewQuerySingleValue(newState, keys[0]) + if err != nil { + b.Fatal(err) + } + + b.ResetTimer() + for i := 0; i < b.N; i++ { + _, err = led.GetSingleValue(query) + if err != nil { + b.Fatal(err) + } + } + }) +} + // BenchmarkTrieUpdate benchmarks the performance of a trie prove func BenchmarkTrieProve(b *testing.B) { // key updates per iteration diff --git a/ledger/complete/ledger_test.go b/ledger/complete/ledger_test.go index 27cb57c978e..efe6020651a 100644 --- a/ledger/complete/ledger_test.go +++ b/ledger/complete/ledger_test.go @@ -122,6 +122,96 @@ func TestLedger_Get(t *testing.T) { }) } +// TestLedger_GetSingleValue tests reading value from a single path. +func TestLedger_GetSingleValue(t *testing.T) { + + wal := &fixtures.NoopWAL{} + led, err := complete.NewLedger( + wal, + 100, + &metrics.NoopCollector{}, + zerolog.Logger{}, + complete.DefaultPathFinderVersion, + ) + require.NoError(t, err) + + state := led.InitialState() + + t.Run("non-existent key", func(t *testing.T) { + + keys := utils.RandomUniqueKeys(10, 2, 1, 10) + + for _, k := range keys { + qs, err := ledger.NewQuerySingleValue(state, k) + require.NoError(t, err) + + retValue, err := led.GetSingleValue(qs) + require.NoError(t, err) + assert.Equal(t, 0, len(retValue)) + } + }) + + t.Run("existent key", func(t *testing.T) { + + u := utils.UpdateFixture() + u.SetState(state) + + newState, _, err := led.Set(u) + require.NoError(t, err) + assert.NotEqual(t, state, newState) + + for i, k := range u.Keys() { + q, err := ledger.NewQuerySingleValue(newState, k) + require.NoError(t, err) + + retValue, err := led.GetSingleValue(q) + require.NoError(t, err) + assert.Equal(t, u.Values()[i], retValue) + } + }) + + t.Run("mix of existent and non-existent keys", func(t *testing.T) { + + u := utils.UpdateFixture() + u.SetState(state) + + newState, _, err := led.Set(u) + require.NoError(t, err) + assert.NotEqual(t, state, newState) + + // Save expected values for existent keys + expectedValues := make(map[string]ledger.Value) + for i, key := range u.Keys() { + encKey := encoding.EncodeKey(&key) + expectedValues[string(encKey)] = u.Values()[i] + } + + // Create a randomly ordered mix of existent and non-existent keys + var queryKeys []ledger.Key + queryKeys = append(queryKeys, u.Keys()...) + queryKeys = append(queryKeys, utils.RandomUniqueKeys(10, 2, 1, 10)...) + + rand.Shuffle(len(queryKeys), func(i, j int) { + queryKeys[i], queryKeys[j] = queryKeys[j], queryKeys[i] + }) + + for _, k := range queryKeys { + qs, err := ledger.NewQuerySingleValue(newState, k) + require.NoError(t, err) + + retValue, err := led.GetSingleValue(qs) + require.NoError(t, err) + + encKey := encoding.EncodeKey(&k) + if value, ok := expectedValues[string(encKey)]; ok { + require.Equal(t, value, retValue) + } else { + require.Equal(t, 0, len(retValue)) + } + } + }) +} + func TestLedgerValueSizes(t *testing.T) { t.Run("empty query", func(t *testing.T) { diff --git a/ledger/complete/mtrie/forest.go b/ledger/complete/mtrie/forest.go index 50947ec6bf9..7fb4a4e9b9a 100644 --- a/ledger/complete/mtrie/forest.go +++ b/ledger/complete/mtrie/forest.go @@ -123,6 +123,18 @@ func (f *Forest) ValueSizes(r *ledger.TrieRead) ([]int, error) { return orderedValueSizes, nil } +// ReadSinglePayload reads value for a single path and returns value and error (if any) +func (f *Forest) ReadSinglePayload(r *ledger.TrieReadSinglePayload) (*ledger.Payload, error) { + // lookup the trie by rootHash + trie, err := f.GetTrie(r.RootHash) + if err != nil { + return nil, err + } + + payload := trie.ReadSinglePayload(r.Path) + return payload.DeepCopy(), nil +} + // Read reads values for an slice of paths and returns values and error (if any) // TODO: can be optimized further if we don't care about changing the order of the input r.Paths func (f *Forest) Read(r *ledger.TrieRead) ([]*ledger.Payload, error) { @@ -137,6 +149,12 @@ func (f *Forest) Read(r *ledger.TrieRead) ([]*ledger.Payload, error) { return nil, err } + // call ReadSinglePayload if there is only one path + if len(r.Paths) == 1 { + payload := trie.ReadSinglePayload(r.Paths[0]) + return []*ledger.Payload{payload.DeepCopy()}, nil + } + // deduplicate keys: // Generally, we expect the VM to deduplicate reads and writes. Hence, the following is a pre-caution. // TODO: We could take out the following de-duplication logic diff --git a/ledger/complete/mtrie/forest_test.go b/ledger/complete/mtrie/forest_test.go index 694b0c97f9e..1ef855a24d1 100644 --- a/ledger/complete/mtrie/forest_test.go +++ b/ledger/complete/mtrie/forest_test.go @@ -578,6 +578,66 @@ func TestReadNonExistingPath(t *testing.T) { require.True(t, retPayloads[0].IsEmpty()) } +// TestReadSinglePayload tests reading a single payload of set/unset register. +func TestReadSinglePayload(t *testing.T) { + forest, err := NewForest(5, &metrics.NoopCollector{}, nil) + require.NoError(t, err) + + // path: 01111101... + path1 := pathByUint8s([]uint8{uint8(125), uint8(23)}) + payload1 := payloadBySlices([]byte{'A'}, []byte{'A'}) + + // path: 10110010... + path2 := pathByUint8s([]uint8{uint8(178), uint8(152)}) + payload2 := payloadBySlices([]byte{'B'}, []byte{'B'}) + + paths := []ledger.Path{path1, path2} + payloads := []*ledger.Payload{payload1, payload2} + + update := &ledger.TrieUpdate{RootHash: forest.GetEmptyRootHash(), Paths: paths, Payloads: payloads} + baseRoot, err := forest.Update(update) + require.NoError(t, err) + + // path: 01101110... + path3 := pathByUint8s([]uint8{uint8(110), uint8(48)}) + payload3 := ledger.EmptyPayload() + + // path: 00010111... + path4 := pathByUint8s([]uint8{uint8(23), uint8(82)}) + payload4 := ledger.EmptyPayload() + + expectedPayloads := make(map[ledger.Path]*ledger.Payload) + expectedPayloads[path1] = payload1 + expectedPayloads[path2] = payload2 + expectedPayloads[path3] = payload3 + expectedPayloads[path4] = payload4 + + // Batch read one payload at a time (less efficient) + for path, payload := range expectedPayloads { + read := &ledger.TrieRead{RootHash: baseRoot, Paths: []ledger.Path{path}} + retPayloads, err := forest.Read(read) + require.NoError(t, err) + require.Equal(t, 1, len(retPayloads)) + if payload.IsEmpty() { + require.True(t, retPayloads[0].IsEmpty()) + } else { + require.Equal(t, payload, retPayloads[0]) + } + } + + // Read single payload + for path, payload := range expectedPayloads { + read := &ledger.TrieReadSinglePayload{RootHash: baseRoot, Path: path} + retPayload, err := forest.ReadSinglePayload(read) + require.NoError(t, err) + if payload.IsEmpty() { + require.True(t, retPayload.IsEmpty()) + } else { + require.Equal(t, payload, retPayload) + } + } +} + // TestForkingUpdates updates a base trie in two different ways. We expect // that for each update, a new trie is added to the forest preserving the // updated values independently of the other update. diff --git a/ledger/complete/mtrie/trie/trie.go b/ledger/complete/mtrie/trie/trie.go index d1db346a496..6bcd3068b68 100644 --- a/ledger/complete/mtrie/trie/trie.go +++ b/ledger/complete/mtrie/trie/trie.go @@ -195,6 +195,33 @@ func valueSizes(sizes []int, paths []ledger.Path, head *node.Node) { } } +// ReadSinglePayload reads and returns a payload for a single path. +func (mt *MTrie) ReadSinglePayload(path ledger.Path) *ledger.Payload { + return readSinglePayload(path, mt.root) +} + +// readSinglePayload reads and returns a payload for a single path in subtree with `head` as root node. +func readSinglePayload(path ledger.Path, head *node.Node) *ledger.Payload { + pathBytes := path[:] + + // Traverse nodes following the path until a leaf node or nil node is reached. + for !head.IsLeaf() { + depth := ledger.NodeMaxHeight - head.Height() // distance to the tree root + bit := bitutils.ReadBit(pathBytes, depth) + if bit == 0 { + head = head.LeftChild() + } else { + head = head.RightChild() + } + } + + if head != nil && *head.Path() == path { + return head.Payload() + } + + return ledger.EmptyPayload() +} + // UnsafeRead reads payloads for the given paths. // UNSAFE: requires _all_ paths to have a length of mt.Height bits. // CAUTION: while reading the payloads, `paths` is permuted IN-PLACE for optimized processing. @@ -228,6 +255,7 @@ func read(payloads []*ledger.Payload, paths []ledger.Path, head *node.Node) { } return } + // reached a leaf node if head.IsLeaf() { for i, p := range paths { @@ -240,6 +268,13 @@ func read(payloads []*ledger.Payload, paths []ledger.Path, head *node.Node) { return } + // reached an interim node + if len(paths) == 1 { + // call readSinglePayload to skip partition and recursive calls when there is only one path + payloads[0] = readSinglePayload(paths[0], head) + return + } + // partition step to quick sort the paths: // lpaths contains all paths that have `0` at the partitionIndex // rpaths contains all paths that have `1` at the partitionIndex diff --git a/ledger/complete/mtrie/trie/trie_test.go b/ledger/complete/mtrie/trie/trie_test.go index 4a4e7cab6b0..227975f8f7b 100644 --- a/ledger/complete/mtrie/trie/trie_test.go +++ b/ledger/complete/mtrie/trie/trie_test.go @@ -1094,3 +1094,86 @@ func TestTrieAllocatedRegCountRegSizeWithMixedPruneFlag(t *testing.T) { require.Equal(t, expectedAllocatedRegCount, updatedTrie.AllocatedRegCount()) require.Equal(t, expectedAllocatedRegSize, updatedTrie.AllocatedRegSize()) } + +// TestReadSinglePayload tests reading a single payload of existent/non-existent path for trie of different layouts. +func TestReadSinglePayload(t *testing.T) { + + emptyTrie := trie.NewEmptyMTrie() + + // Test reading payload in empty trie + t.Run("empty trie", func(t *testing.T) { + path := utils.PathByUint16LeftPadded(0) + payload := emptyTrie.ReadSinglePayload(path) + require.True(t, payload.IsEmpty()) + }) + + // Test reading payload for existent/non-existent path + // in trie with compact leaf as root node. + t.Run("compact leaf as root", func(t *testing.T) { + path1 := utils.PathByUint16LeftPadded(0) + payload1 := utils.RandomPayload(1, 100) + + paths := []ledger.Path{path1} + payloads := []ledger.Payload{*payload1} + + newTrie, maxDepthTouched, err := trie.NewTrieWithUpdatedRegisters(emptyTrie, paths, payloads, true) + require.NoError(t, err) + require.Equal(t, uint16(0), maxDepthTouched) + + // Get payload for existent path path + retPayload := newTrie.ReadSinglePayload(path1) + require.Equal(t, payload1, retPayload) + + // Get payload for non-existent path + path2 := utils.PathByUint16LeftPadded(1) + retPayload = newTrie.ReadSinglePayload(path2) + require.True(t, retPayload.IsEmpty()) + }) + + // Test reading payload for existent/non-existent path in an unpruned trie. + t.Run("trie", func(t *testing.T) { + path1 := utils.PathByUint16(1 << 12) // 000100... + path2 := utils.PathByUint16(1 << 13) // 001000... + path3 := utils.PathByUint16(1 << 14) // 010000... + + payload1 := utils.RandomPayload(1, 100) + payload2 := utils.RandomPayload(1, 100) + payload3 := ledger.EmptyPayload() + + paths := []ledger.Path{path1, path2, path3} + payloads := []ledger.Payload{*payload1, *payload2, *payload3} + + // Create an unpruned trie with 3 leaf nodes (n1, n2, n3). + newTrie, maxDepthTouched, err := trie.NewTrieWithUpdatedRegisters(emptyTrie, paths, payloads, false) + require.NoError(t, err) + require.Equal(t, uint16(3), maxDepthTouched) + + // n5 + // / + // / + // n4 + // / \ + // / \ + // n3 n3 (path3/ + // / \ payload3) + // / \ + // n1 (path1/ n2 (path2/ + // payload1) payload2) + // + + // Test reading payload for all possible paths for the first 4 bits. + for i := 0; i < 16; i++ { + path := utils.PathByUint16(uint16(i << 12)) + + retPayload := newTrie.ReadSinglePayload(path) + switch path { + case path1: + require.Equal(t, payload1, retPayload) + case path2: + require.Equal(t, payload2, retPayload) + default: + require.True(t, retPayload.IsEmpty()) + } + } + }) +} diff --git a/ledger/ledger.go b/ledger/ledger.go index 6b5578f1945..54c8e36fb4b 100644 --- a/ledger/ledger.go +++ b/ledger/ledger.go @@ -24,6 +24,9 @@ type Ledger interface { // InitialState returns the initial state of the ledger InitialState() State + // GetSingleValue returns value for a given key at specific state + GetSingleValue(query *QuerySingleValue) (value Value, err error) + // Get returns values for the given slice of keys at specific state Get(query *Query) (values []Value, err error) @@ -45,7 +48,7 @@ func NewEmptyQuery(sc State) (*Query, error) { return &Query{state: sc}, nil } -// NewQuery constructs a new ledger query +// NewQuery constructs a new ledger query func NewQuery(sc State, keys []Key) (*Query, error) { return &Query{state: sc, keys: keys}, nil } @@ -70,6 +73,27 @@ func (q *Query) SetState(s State) { q.state = s } +// QuerySingleValue contains ledger query for a single value +type QuerySingleValue struct { + state State + key Key +} + +// NewQuerySingleValue constructs a new ledger query for a single value +func NewQuerySingleValue(sc State, key Key) (*QuerySingleValue, error) { + return &QuerySingleValue{state: sc, key: key}, nil +} + +// Key returns key of the query +func (q *QuerySingleValue) Key() Key { + return q.key +} + +// State returns the state part of the query +func (q *QuerySingleValue) State() State { + return q.state +} + // Update holds all data needed for a ledger update type Update struct { state State diff --git a/ledger/partial/ledger.go b/ledger/partial/ledger.go index 89ab41ce1ef..cae3bc5fe37 100644 --- a/ledger/partial/ledger.go +++ b/ledger/partial/ledger.go @@ -63,6 +63,22 @@ func (l *Ledger) InitialState() ledger.State { return l.state } +// GetSingleValue reads value of a given key at the given state +func (l *Ledger) GetSingleValue(query *ledger.QuerySingleValue) (value ledger.Value, err error) { + path, err := pathfinder.KeyToPath(query.Key(), l.pathFinderVersion) + if err != nil { + return nil, err + } + payload, err := l.ptrie.GetSinglePayload(path) + if err != nil { + if _, ok := err.(*ptrie.ErrMissingPath); ok { + return nil, &ledger.ErrMissingKeys{Keys: []ledger.Key{query.Key()}} + } + return nil, err + } + return payload.Value, err +} + // Get read the values of the given keys at the given state // it returns the values in the same order as given registerIDs and errors (if any) func (l *Ledger) Get(query *ledger.Query) (values []ledger.Value, err error) { diff --git a/ledger/partial/ledger_test.go b/ledger/partial/ledger_test.go index b9515de2783..0f1485f0bd0 100644 --- a/ledger/partial/ledger_test.go +++ b/ledger/partial/ledger_test.go @@ -42,18 +42,51 @@ func TestFunctionalityWithCompleteTrie(t *testing.T) { assert.NoError(t, err) assert.Equal(t, pled.InitialState(), newState) - // test missing keys (get) + // test batch querying existent keys + query, err = ledger.NewQuery(newState, keys[0:2]) + require.NoError(t, err) + + retValues, err := pled.Get(query) + require.NoError(t, err) + require.Equal(t, 2, len(retValues)) + for i := 0; i < len(retValues); i++ { + require.Equal(t, values[i], retValues[i]) + } + + // test querying single existent key + querySingleValue, err := ledger.NewQuerySingleValue(newState, keys[0]) + require.NoError(t, err) + + retValue, err := pled.GetSingleValue(querySingleValue) + require.NoError(t, err) + require.Equal(t, values[0], retValue) + + // test batch getting missing keys query, err = ledger.NewQuery(newState, keys[1:3]) require.NoError(t, err) - _, err = pled.Get(query) + retValues, err = pled.Get(query) require.Error(t, err) + require.Nil(t, retValues) e, ok := err.(*ledger.ErrMissingKeys) require.True(t, ok) assert.Equal(t, len(e.Keys), 1) require.True(t, e.Keys[0].Equals(&keys[2])) + // test querying single non-existent key + querySingleValue, err = ledger.NewQuerySingleValue(newState, keys[2]) + require.NoError(t, err) + + retValue, err = pled.GetSingleValue(querySingleValue) + require.Error(t, err) + require.Nil(t, retValue) + + e, ok = err.(*ledger.ErrMissingKeys) + require.True(t, ok) + assert.Equal(t, len(e.Keys), 1) + require.True(t, e.Keys[0].Equals(&keys[2])) + // test missing keys (set) update, err = ledger.NewUpdate(state, keys[1:3], values[1:3]) require.NoError(t, err) diff --git a/ledger/partial/ptrie/partialTrie.go b/ledger/partial/ptrie/partialTrie.go index 3c8bf2a4f9a..5f4a3b95cfb 100644 --- a/ledger/partial/ptrie/partialTrie.go +++ b/ledger/partial/ptrie/partialTrie.go @@ -27,6 +27,15 @@ func (p *PSMT) RootHash() ledger.RootHash { return ledger.RootHash(p.root.Hash()) } +// GetSinglePayload returns payload of a given path +func (p *PSMT) GetSinglePayload(path ledger.Path) (*ledger.Payload, error) { + node, found := p.pathLookUp[path] + if !found { + return nil, &ErrMissingPath{Paths: []ledger.Path{path}} + } + return node.payload, nil +} + // Get returns an slice of payloads (same order), an slice of failed paths and errors (if any) // TODO return list of indecies instead of paths func (p *PSMT) Get(paths []ledger.Path) ([]*ledger.Payload, error) { diff --git a/ledger/partial/ptrie/partialTrie_test.go b/ledger/partial/ptrie/partialTrie_test.go index 3e9006240fd..3c9cd896f8a 100644 --- a/ledger/partial/ptrie/partialTrie_test.go +++ b/ledger/partial/ptrie/partialTrie_test.go @@ -68,6 +68,120 @@ func TestPartialTrieEmptyTrie(t *testing.T) { }) } +// TestPartialTrieGet gets payloads from existent and non-existent paths. +func TestPartialTrieGet(t *testing.T) { + + pathByteSize := 32 + withForest(t, pathByteSize, 10, func(t *testing.T, f *mtrie.Forest) { + + path1 := utils.PathByUint16(0) + payload1 := utils.LightPayload('A', 'a') + + path2 := utils.PathByUint16(1) + payload2 := utils.LightPayload('B', 'b') + + paths := []ledger.Path{path1, path2} + payloads := []*ledger.Payload{payload1, payload2} + + u := &ledger.TrieUpdate{RootHash: f.GetEmptyRootHash(), Paths: paths, Payloads: payloads} + rootHash, err := f.Update(u) + require.NoError(t, err, "error updating trie") + + r := &ledger.TrieRead{RootHash: rootHash, Paths: paths} + bp, err := f.Proofs(r) + require.NoError(t, err, "error getting batch proof") + + psmt, err := NewPSMT(rootHash, bp) + require.NoError(t, err, "error building partial trie") + ensureRootHash(t, rootHash, psmt) + + t.Run("non-existent key", func(t *testing.T) { + path3 := utils.PathByUint16(2) + path4 := utils.PathByUint16(4) + + nonExistentPaths := []ledger.Path{path3, path4} + retPayloads, err := psmt.Get(nonExistentPaths) + require.Nil(t, retPayloads) + + e, ok := err.(*ErrMissingPath) + require.True(t, ok) + assert.Equal(t, 2, len(e.Paths)) + require.Equal(t, path3, e.Paths[0]) + require.Equal(t, path4, e.Paths[1]) + }) + + t.Run("existent key", func(t *testing.T) { + retPayloads, err := psmt.Get(paths) + require.NoError(t, err) + require.Equal(t, len(paths), len(retPayloads)) + require.Equal(t, payload1, retPayloads[0]) + require.Equal(t, payload2, retPayloads[1]) + }) + + t.Run("mix of existent and non-existent keys", func(t *testing.T) { + path3 := utils.PathByUint16(2) + path4 := utils.PathByUint16(4) + + retPayloads, err := psmt.Get([]ledger.Path{path1, path2, path3, path4}) + require.Nil(t, retPayloads) + + e, ok := err.(*ErrMissingPath) + require.True(t, ok) + assert.Equal(t, 2, len(e.Paths)) + require.Equal(t, path3, e.Paths[0]) + require.Equal(t, path4, e.Paths[1]) + }) + }) +} + +// TestPartialTrieGetSinglePayload gets single payload from existent/non-existent path. +func TestPartialTrieGetSinglePayload(t *testing.T) { + + pathByteSize := 32 + withForest(t, pathByteSize, 10, func(t *testing.T, f *mtrie.Forest) { + + path1 := utils.PathByUint16(0) + payload1 := utils.LightPayload('A', 'a') + + path2 := utils.PathByUint16(1) + payload2 := utils.LightPayload('B', 'b') + + paths := []ledger.Path{path1, path2} + payloads := []*ledger.Payload{payload1, payload2} + + u := &ledger.TrieUpdate{RootHash: f.GetEmptyRootHash(), Paths: paths, Payloads: payloads} + rootHash, err := f.Update(u) + require.NoError(t, err, "error updating trie") + + r := &ledger.TrieRead{RootHash: rootHash, Paths: paths} + bp, err := f.Proofs(r) + require.NoError(t, err, "error getting batch proof") + + psmt, err := NewPSMT(rootHash, bp) + require.NoError(t, err, "error building partial trie") + ensureRootHash(t, rootHash, psmt) + + retPayload, err := psmt.GetSinglePayload(path1) + require.NoError(t, err) + require.Equal(t, payload1, retPayload) + + retPayload, err = psmt.GetSinglePayload(path2) + require.NoError(t, err) + require.Equal(t, payload2, retPayload) + + path3 := utils.PathByUint16(2) + + retPayload, err = psmt.GetSinglePayload(path3) + require.Nil(t, retPayload) + + var errMissingPath *ErrMissingPath + require.ErrorAs(t, err, &errMissingPath) + missingPath := err.(*ErrMissingPath) + require.Equal(t, 1, len(missingPath.Paths)) + require.Equal(t, path3, missingPath.Paths[0]) + }) +} + func TestPartialTrieLeafUpdates(t *testing.T) { pathByteSize := 32 diff --git a/ledger/trie.go b/ledger/trie.go index 78431eb8f11..fcd6411d74b 100644 --- a/ledger/trie.go +++ b/ledger/trie.go @@ -89,6 +89,12 @@ type TrieRead struct { Paths []Path } +// TrieReadSinglePayload contains trie read query for a single payload +type TrieReadSinglePayload struct { + RootHash RootHash + Path Path +} + // TrieUpdate holds all data for a trie update type TrieUpdate struct { RootHash RootHash diff --git a/module/chunks/chunkVerifier.go b/module/chunks/chunkVerifier.go index 63a7ccdc83a..2f249eb7155 100644 --- a/module/chunks/chunkVerifier.go +++ b/module/chunks/chunkVerifier.go @@ -133,13 +133,13 @@ func (fcv *ChunkVerifier) verifyTransactionsInContext(context fvm.Context, chunk registerKey := executionState.RegisterIDToKey(registerID) - query, err := ledger.NewQuery(ledger.State(chunkDataPack.StartState), []ledger.Key{registerKey}) + query, err := ledger.NewQuerySingleValue(ledger.State(chunkDataPack.StartState), registerKey) if err != nil { return nil, fmt.Errorf("cannot create query: %w", err) } - values, err := psmt.Get(query) + value, err := psmt.GetSingleValue(query) if err != nil { if errors.Is(err, ledger.ErrMissingKeys{}) { @@ -158,7 +158,7 @@ func (fcv *ChunkVerifier) verifyTransactionsInContext(context fvm.Context, chunk return nil, fmt.Errorf("cannot query register: %w", err) } - return values[0], nil + return value, nil } chunkView := delta.NewView(getRegister) From 729e26fd8de2e40e8a5dcd4eb30d7ccbf750beb5 Mon Sep 17 00:00:00 2001 From: Faye Amacker <33205765+fxamacker@users.noreply.github.com> Date: Tue, 24 May 2022 10:54:22 -0500 Subject: [PATCH 2/8] Update ledger mock --- ledger/mock/ledger.go | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/ledger/mock/ledger.go b/ledger/mock/ledger.go index 258789d2171..6f900bae5c6 100644 --- a/ledger/mock/ledger.go +++ b/ledger/mock/ledger.go @@ -30,6 +30,29 @@ func (_m *Ledger) Done() <-chan struct{} { return r0 } +// GetSingleValue provides a mock function with given fields: query +func (_m *Ledger) GetSingleValue(query *ledger.QuerySingleValue) (ledger.Value, error) { + ret := _m.Called(query) + + var r0 ledger.Value + if rf, ok := ret.Get(0).(func(*ledger.QuerySingleValue) ledger.Value); ok { + r0 = rf(query) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(ledger.Value) + } + } + + var r1 error + if rf, ok := ret.Get(1).(func(*ledger.QuerySingleValue) error); ok { + r1 = rf(query) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + // Get provides a mock function with given fields: query func (_m *Ledger) Get(query *ledger.Query) ([]ledger.Value, error) { ret := _m.Called(query) From d9a57fa336eb8ebc1372a3176e346713ea09a9b0 Mon Sep 17 00:00:00 2001 From: Faye Amacker <33205765+fxamacker@users.noreply.github.com> Date: Tue, 24 May 2022 12:11:12 -0500 Subject: [PATCH 3/8] Check Mtrie root integrity after read in tests --- ledger/complete/mtrie/trie/trie_test.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/ledger/complete/mtrie/trie/trie_test.go b/ledger/complete/mtrie/trie/trie_test.go index 227975f8f7b..ecf02cbb94c 100644 --- a/ledger/complete/mtrie/trie/trie_test.go +++ b/ledger/complete/mtrie/trie/trie_test.go @@ -1102,9 +1102,12 @@ func TestReadSinglePayload(t *testing.T) { // Test reading payload in empty trie t.Run("empty trie", func(t *testing.T) { + savedRootHash := emptyTrie.RootHash() + path := utils.PathByUint16LeftPadded(0) payload := emptyTrie.ReadSinglePayload(path) require.True(t, payload.IsEmpty()) + require.Equal(t, savedRootHash, emptyTrie.RootHash()) }) // Test reading payload for existent/non-existent path @@ -1120,14 +1123,18 @@ func TestReadSinglePayload(t *testing.T) { require.NoError(t, err) require.Equal(t, uint16(0), maxDepthTouched) + savedRootHash := newTrie.RootHash() + // Get payload for existent path path retPayload := newTrie.ReadSinglePayload(path1) require.Equal(t, payload1, retPayload) + require.Equal(t, savedRootHash, newTrie.RootHash()) // Get payload for non-existent path path2 := utils.PathByUint16LeftPadded(1) retPayload = newTrie.ReadSinglePayload(path2) require.True(t, retPayload.IsEmpty()) + require.Equal(t, savedRootHash, newTrie.RootHash()) }) // Test reading payload for existent/non-existent path in an unpruned trie. @@ -1148,6 +1155,8 @@ func TestReadSinglePayload(t *testing.T) { require.NoError(t, err) require.Equal(t, uint16(3), maxDepthTouched) + savedRootHash := newTrie.RootHash() + // n5 // / // / @@ -1166,6 +1175,7 @@ func TestReadSinglePayload(t *testing.T) { path := utils.PathByUint16(uint16(i << 12)) retPayload := newTrie.ReadSinglePayload(path) + require.Equal(t, savedRootHash, newTrie.RootHash()) switch path { case path1: require.Equal(t, payload1, retPayload) From b7cb77bf0ca28bd803579205e98adef5bf1efa9f Mon Sep 17 00:00:00 2001 From: Faye Amacker <33205765+fxamacker@users.noreply.github.com> Date: Tue, 24 May 2022 14:58:54 -0500 Subject: [PATCH 4/8] Reduce allocs/op by 79% in ledger read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Change Forest.Read to return []ledger.Value without deep copying payload keys. This avoids 4 heap allocation per key. This change doesn't affect the caller (Ledger.Get) because it discards the payload keys. name old time/op new time/op delta TrieRead-4 524µs ± 1% 420µs ± 1% -19.77% (p=0.000 n=10+10) name old alloc/op new alloc/op delta TrieRead-4 190kB ± 0% 95kB ± 0% -50.04% (p=0.000 n=10+10) name old allocs/op new allocs/op delta TrieRead-4 1.52k ± 0% 0.32k ± 0% -79.17% (p=0.000 n=10+10) --- ledger/complete/ledger.go | 6 +- ledger/complete/mtrie/forest.go | 10 +-- ledger/complete/mtrie/forest_test.go | 103 +++++++++++++-------------- 3 files changed, 57 insertions(+), 62 deletions(-) diff --git a/ledger/complete/ledger.go b/ledger/complete/ledger.go index f824ac00523..a081a54cc8d 100644 --- a/ledger/complete/ledger.go +++ b/ledger/complete/ledger.go @@ -165,11 +165,7 @@ func (l *Ledger) Get(query *ledger.Query) (values []ledger.Value, err error) { return nil, err } trieRead := &ledger.TrieRead{RootHash: ledger.RootHash(query.State()), Paths: paths} - payloads, err := l.forest.Read(trieRead) - if err != nil { - return nil, err - } - values, err = pathfinder.PayloadsToValues(payloads) + values, err = l.forest.Read(trieRead) if err != nil { return nil, err } diff --git a/ledger/complete/mtrie/forest.go b/ledger/complete/mtrie/forest.go index 7fb4a4e9b9a..67a2f851c60 100644 --- a/ledger/complete/mtrie/forest.go +++ b/ledger/complete/mtrie/forest.go @@ -137,10 +137,10 @@ func (f *Forest) ReadSinglePayload(r *ledger.TrieReadSinglePayload) (*ledger.Pay // Read reads values for an slice of paths and returns values and error (if any) // TODO: can be optimized further if we don't care about changing the order of the input r.Paths -func (f *Forest) Read(r *ledger.TrieRead) ([]*ledger.Payload, error) { +func (f *Forest) Read(r *ledger.TrieRead) ([]ledger.Value, error) { if len(r.Paths) == 0 { - return []*ledger.Payload{}, nil + return []ledger.Value{}, nil } // lookup the trie by rootHash @@ -174,20 +174,20 @@ func (f *Forest) Read(r *ledger.TrieRead) ([]*ledger.Payload, error) { payloads := trie.UnsafeRead(deduplicatedPaths) // this sorts deduplicatedPaths IN-PLACE // reconstruct the payloads in the same key order that called the method - orderedPayloads := make([]*ledger.Payload, len(r.Paths)) + orderedValues := make([]ledger.Value, len(r.Paths)) totalPayloadSize := 0 for i, p := range deduplicatedPaths { payload := payloads[i] indices := pathOrgIndex[p] for _, j := range indices { - orderedPayloads[j] = payload.DeepCopy() + orderedValues[j] = payload.Value.DeepCopy() } totalPayloadSize += len(indices) * payload.Size() } // TODO rename the metrics f.metrics.ReadValuesSize(uint64(totalPayloadSize)) - return orderedPayloads, nil + return orderedValues, nil } // Update updates the Values for the registers and returns rootHash and error (if any). diff --git a/ledger/complete/mtrie/forest_test.go b/ledger/complete/mtrie/forest_test.go index 1ef855a24d1..11123c9d622 100644 --- a/ledger/complete/mtrie/forest_test.go +++ b/ledger/complete/mtrie/forest_test.go @@ -11,7 +11,6 @@ import ( "github.com/stretchr/testify/require" "github.com/onflow/flow-go/ledger" - "github.com/onflow/flow-go/ledger/common/encoding" prf "github.com/onflow/flow-go/ledger/common/proof" "github.com/onflow/flow-go/ledger/common/utils" "github.com/onflow/flow-go/ledger/complete/mtrie/trie" @@ -67,9 +66,9 @@ func TestTrieUpdate(t *testing.T) { require.NoError(t, err) read := &ledger.TrieRead{RootHash: updatedRoot, Paths: paths} - retPayloads, err := forest.Read(read) + retValues, err := forest.Read(read) require.NoError(t, err) - require.True(t, bytes.Equal(encoding.EncodePayload(retPayloads[0]), encoding.EncodePayload(payloads[0]))) + require.Equal(t, retValues[0], payloads[0].Value) } // TestLeftEmptyInsert tests inserting a new value into an empty sub-trie: @@ -122,10 +121,10 @@ func TestLeftEmptyInsert(t *testing.T) { paths = []ledger.Path{p1, p2, p3} payloads = []*ledger.Payload{v1, v2, v3} read := &ledger.TrieRead{RootHash: updatedRoot, Paths: paths} - retPayloads, err := forest.Read(read) + retValues, err := forest.Read(read) require.NoError(t, err) for i := range paths { - require.True(t, bytes.Equal(encoding.EncodePayload(retPayloads[i]), encoding.EncodePayload(payloads[i]))) + require.Equal(t, retValues[i], payloads[i].Value) } } @@ -180,10 +179,10 @@ func TestRightEmptyInsert(t *testing.T) { paths = []ledger.Path{p1, p2, p3} payloads = []*ledger.Payload{v1, v2, v3} read := &ledger.TrieRead{RootHash: updatedRoot, Paths: paths} - retPayloads, err := forest.Read(read) + retValues, err := forest.Read(read) require.NoError(t, err) for i := range paths { - require.True(t, bytes.Equal(encoding.EncodePayload(retPayloads[i]), encoding.EncodePayload(payloads[i]))) + require.Equal(t, retValues[i], payloads[i].Value) } } @@ -236,10 +235,10 @@ func TestExpansionInsert(t *testing.T) { paths = []ledger.Path{p1, p2} payloads = []*ledger.Payload{v1, v2} read := &ledger.TrieRead{RootHash: updatedRoot, Paths: paths} - retPayloads, err := forest.Read(read) + retValues, err := forest.Read(read) require.NoError(t, err) for i := range paths { - require.True(t, bytes.Equal(encoding.EncodePayload(retPayloads[i]), encoding.EncodePayload(payloads[i]))) + require.Equal(t, retValues[i], payloads[i].Value) } } @@ -304,10 +303,10 @@ func TestFullHouseInsert(t *testing.T) { paths = []ledger.Path{p1, p2, p3} payloads = []*ledger.Payload{v1, v2, v3} read := &ledger.TrieRead{RootHash: updatedRoot, Paths: paths} - retPayloads, err := forest.Read(read) + retValues, err := forest.Read(read) require.NoError(t, err) for i := range paths { - require.True(t, bytes.Equal(encoding.EncodePayload(retPayloads[i]), encoding.EncodePayload(payloads[i]))) + require.Equal(t, retValues[i], payloads[i].Value) } } @@ -346,10 +345,10 @@ func TestLeafInsert(t *testing.T) { require.Equal(t, uint64(v1.Size()+v2.Size()), updatedTrie.AllocatedRegSize()) read := &ledger.TrieRead{RootHash: updatedRoot, Paths: paths} - retPayloads, err := forest.Read(read) + retValues, err := forest.Read(read) require.NoError(t, err) for i := range paths { - require.True(t, bytes.Equal(encoding.EncodePayload(retPayloads[i]), encoding.EncodePayload(payloads[i]))) + require.Equal(t, retValues[i], payloads[i].Value) } } @@ -384,9 +383,9 @@ func TestOverrideValue(t *testing.T) { require.NoError(t, err) read := &ledger.TrieRead{RootHash: updatedRoot, Paths: paths} - retPayloads, err := forest.Read(read) + retValues, err := forest.Read(read) require.NoError(t, err) - require.True(t, bytes.Equal(encoding.EncodePayload(retPayloads[0]), encoding.EncodePayload(payloads[0]))) + require.Equal(t, retValues[0], payloads[0].Value) } @@ -417,9 +416,9 @@ func TestDuplicateOverride(t *testing.T) { paths = []ledger.Path{p0} read := &ledger.TrieRead{RootHash: updatedRoot, Paths: paths} - retPayloads, err := forest.Read(read) + retValues, err := forest.Read(read) require.NoError(t, err) - require.True(t, bytes.Equal(encoding.EncodePayload(retPayloads[0]), encoding.EncodePayload(v2))) + require.Equal(t, retValues[0], v2.Value) } @@ -443,16 +442,16 @@ func TestReadSafety(t *testing.T) { require.NoError(t, err) require.Len(t, data, 1) - require.Equal(t, v0, data[0]) + require.Equal(t, v0.Value, data[0]) // modify returned slice - data[0].Value = []byte("new value") + data[0] = []byte("new value") // read again data2, err := forest.Read(read) require.NoError(t, err) require.Len(t, data2, 1) - require.Equal(t, v0, data2[0]) + require.Equal(t, v0.Value, data2[0]) } // TestReadOrder tests that payloads from reading a trie are delivered in the order as specified by the paths @@ -474,18 +473,18 @@ func TestReadOrder(t *testing.T) { require.NoError(t, err) read := &ledger.TrieRead{RootHash: testRoot, Paths: []ledger.Path{p1, p2}} - retPayloads, err := forest.Read(read) + retValues, err := forest.Read(read) require.NoError(t, err) - require.Equal(t, len(read.Paths), len(retPayloads)) - require.True(t, bytes.Equal(encoding.EncodePayload(retPayloads[0]), encoding.EncodePayload(payloads[0]))) - require.True(t, bytes.Equal(encoding.EncodePayload(retPayloads[1]), encoding.EncodePayload(payloads[1]))) + require.Equal(t, len(read.Paths), len(retValues)) + require.Equal(t, retValues[0], payloads[0].Value) + require.Equal(t, retValues[1], payloads[1].Value) read = &ledger.TrieRead{RootHash: testRoot, Paths: []ledger.Path{p2, p1}} - retPayloads, err = forest.Read(read) + retValues, err = forest.Read(read) require.NoError(t, err) - require.Equal(t, len(read.Paths), len(retPayloads)) - require.True(t, bytes.Equal(encoding.EncodePayload(retPayloads[1]), encoding.EncodePayload(payloads[0]))) - require.True(t, bytes.Equal(encoding.EncodePayload(retPayloads[0]), encoding.EncodePayload(payloads[1]))) + require.Equal(t, len(read.Paths), len(retValues)) + require.Equal(t, retValues[1], payloads[0].Value) + require.Equal(t, retValues[0], payloads[1].Value) } // TestMixRead tests reading a mixture of set and unset registers. @@ -521,10 +520,10 @@ func TestMixRead(t *testing.T) { expectedPayloads := []*ledger.Payload{v1, v2, v3, v4} read := &ledger.TrieRead{RootHash: baseRoot, Paths: readPaths} - retPayloads, err := forest.Read(read) + retValues, err := forest.Read(read) require.NoError(t, err) for i := range paths { - require.True(t, bytes.Equal(encoding.EncodePayload(retPayloads[i]), encoding.EncodePayload(expectedPayloads[i]))) + require.Equal(t, retValues[i], expectedPayloads[i].Value) } } @@ -549,11 +548,11 @@ func TestReadWithDuplicatedKeys(t *testing.T) { paths = []ledger.Path{p1, p2, p3} expectedPayloads := []*ledger.Payload{v1, v2, v1} read := &ledger.TrieRead{RootHash: updatedRoot, Paths: paths} - retPayloads, err := forest.Read(read) + retValues, err := forest.Read(read) require.NoError(t, err) - require.Equal(t, len(read.Paths), len(retPayloads)) + require.Equal(t, len(read.Paths), len(retValues)) for i := range paths { - require.True(t, bytes.Equal(encoding.EncodePayload(retPayloads[i]), encoding.EncodePayload(expectedPayloads[i]))) + require.Equal(t, retValues[i], expectedPayloads[i].Value) } } @@ -573,9 +572,9 @@ func TestReadNonExistingPath(t *testing.T) { p2 := pathByUint8s([]uint8{uint8(116), uint8(129)}) read := &ledger.TrieRead{RootHash: updatedRoot, Paths: []ledger.Path{p2}} - retPayloads, err := forest.Read(read) + retValues, err := forest.Read(read) require.NoError(t, err) - require.True(t, retPayloads[0].IsEmpty()) + require.Equal(t, 0, len(retValues[0])) } // TestReadSinglePayload tests reading a single payload of set/unset register. @@ -677,24 +676,24 @@ func TestForkingUpdates(t *testing.T) { // Verify payloads are preserved read := &ledger.TrieRead{RootHash: baseRoot, Paths: paths} - retPayloads, err := forest.Read(read) // reading from original Trie + retValues, err := forest.Read(read) // reading from original Trie require.NoError(t, err) for i := range paths { - require.True(t, bytes.Equal(encoding.EncodePayload(retPayloads[i]), encoding.EncodePayload(payloads[i]))) + require.Equal(t, retValues[i], payloads[i].Value) } readA := &ledger.TrieRead{RootHash: updatedRootA, Paths: pathsA} - retPayloads, err = forest.Read(readA) // reading from updatedTrieA + retValues, err = forest.Read(readA) // reading from updatedTrieA require.NoError(t, err) for i := range paths { - require.True(t, bytes.Equal(encoding.EncodePayload(retPayloads[i]), encoding.EncodePayload(payloadsA[i]))) + require.Equal(t, retValues[i], payloadsA[i].Value) } readB := &ledger.TrieRead{RootHash: updatedRootB, Paths: pathsB} - retPayloads, err = forest.Read(readB) // reading from updatedTrieB + retValues, err = forest.Read(readB) // reading from updatedTrieB require.NoError(t, err) for i := range paths { - require.True(t, bytes.Equal(encoding.EncodePayload(retPayloads[i]), encoding.EncodePayload(payloadsB[i]))) + require.Equal(t, retValues[i], payloadsB[i].Value) } } @@ -728,17 +727,17 @@ func TestIdenticalUpdateAppliedTwice(t *testing.T) { paths = []ledger.Path{p1, p2, p3} payloads = []*ledger.Payload{v1, v2, v3} read := &ledger.TrieRead{RootHash: updatedRootA, Paths: paths} - retPayloadsA, err := forest.Read(read) + retValuesA, err := forest.Read(read) require.NoError(t, err) for i := range paths { - require.True(t, bytes.Equal(encoding.EncodePayload(retPayloadsA[i]), encoding.EncodePayload(payloads[i]))) + require.Equal(t, retValuesA[i], payloads[i].Value) } read = &ledger.TrieRead{RootHash: updatedRootB, Paths: paths} - retPayloadsB, err := forest.Read(read) + retValuesB, err := forest.Read(read) require.NoError(t, err) for i := range paths { - require.True(t, bytes.Equal(encoding.EncodePayload(retPayloadsB[i]), encoding.EncodePayload(payloads[i]))) + require.Equal(t, retValuesB[i], payloads[i].Value) } } @@ -780,10 +779,10 @@ func TestRandomUpdateReadProofValueSizes(t *testing.T) { } } read := &ledger.TrieRead{RootHash: activeRoot, Paths: nonExistingPaths} - retPayloads, err := forest.Read(read) + retValues, err := forest.Read(read) require.NoError(t, err, "error reading - non existing paths") - for _, p := range retPayloads { - require.True(t, p.IsEmpty()) + for _, p := range retValues { + require.Equal(t, 0, len(p)) } // test value sizes for non-existent keys @@ -801,10 +800,10 @@ func TestRandomUpdateReadProofValueSizes(t *testing.T) { // test read read = &ledger.TrieRead{RootHash: activeRoot, Paths: paths} - retPayloads, err = forest.Read(read) + retValues, err = forest.Read(read) require.NoError(t, err, "error reading") for i := range payloads { - require.True(t, bytes.Equal(encoding.EncodePayload(retPayloads[i]), encoding.EncodePayload(payloads[i]))) + require.Equal(t, retValues[i], payloads[i].Value) } // test value sizes for existing keys @@ -852,10 +851,10 @@ func TestRandomUpdateReadProofValueSizes(t *testing.T) { } read = &ledger.TrieRead{RootHash: activeRoot, Paths: allPaths} - retPayloads, err = forest.Read(read) + retValues, err = forest.Read(read) require.NoError(t, err) for i, v := range allPayloads { - assert.True(t, v.Equals(retPayloads[i])) + assert.Equal(t, retValues[i], v.Value) } // check value sizes for all existing paths From f9b80769687d7caaa2e0ece039ad38e7211b2622 Mon Sep 17 00:00:00 2001 From: Faye Amacker <33205765+fxamacker@users.noreply.github.com> Date: Tue, 24 May 2022 16:28:45 -0500 Subject: [PATCH 5/8] Fix tests --- ledger/complete/wal/checkpointer_test.go | 28 ++++++++++++------------ ledger/complete/wal/compactor_test.go | 8 +++---- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/ledger/complete/wal/checkpointer_test.go b/ledger/complete/wal/checkpointer_test.go index 54e4078251a..53db783bbdf 100644 --- a/ledger/complete/wal/checkpointer_test.go +++ b/ledger/complete/wal/checkpointer_test.go @@ -242,19 +242,19 @@ func Test_Checkpointing(t *testing.T) { paths = append(paths, path) } - payloads1, err := f.Read(&ledger.TrieRead{RootHash: rootHash, Paths: paths}) + values1, err := f.Read(&ledger.TrieRead{RootHash: rootHash, Paths: paths}) require.NoError(t, err) - payloads2, err := f2.Read(&ledger.TrieRead{RootHash: rootHash, Paths: paths}) + values2, err := f2.Read(&ledger.TrieRead{RootHash: rootHash, Paths: paths}) require.NoError(t, err) - payloads3, err := f3.Read(&ledger.TrieRead{RootHash: rootHash, Paths: paths}) + values3, err := f3.Read(&ledger.TrieRead{RootHash: rootHash, Paths: paths}) require.NoError(t, err) for i, path := range paths { - require.True(t, data[path].Equals(payloads1[i])) - require.True(t, data[path].Equals(payloads2[i])) - require.True(t, data[path].Equals(payloads3[i])) + require.Equal(t, data[path].Value, values1[i]) + require.Equal(t, data[path].Value, values2[i]) + require.Equal(t, data[path].Value, values3[i]) } } }) @@ -325,15 +325,15 @@ func Test_Checkpointing(t *testing.T) { trieRead, err := pathfinder.QueryToTrieRead(query, pathFinderVersion) require.NoError(t, err) - payloads, err := f.Read(trieRead) + values, err := f.Read(trieRead) require.NoError(t, err) - payloads5, err := f5.Read(trieRead) + values5, err := f5.Read(trieRead) require.NoError(t, err) for i := range keys2 { - require.Equal(t, values2[i], payloads[i].Value) - require.Equal(t, values2[i], payloads5[i].Value) + require.Equal(t, values2[i], values[i]) + require.Equal(t, values2[i], values5[i]) } }) @@ -415,15 +415,15 @@ func Test_Checkpointing(t *testing.T) { trieRead, err := pathfinder.QueryToTrieRead(query, pathFinderVersion) require.NoError(t, err) - payloads, err := f.Read(trieRead) + values, err := f.Read(trieRead) require.NoError(t, err) - payloads6, err := f6.Read(trieRead) + values6, err := f6.Read(trieRead) require.NoError(t, err) for i := range keys2 { - require.Equal(t, values2[i], payloads[i].Value) - require.Equal(t, values2[i], payloads6[i].Value) + require.Equal(t, values2[i], values[i]) + require.Equal(t, values2[i], values6[i]) } }) diff --git a/ledger/complete/wal/compactor_test.go b/ledger/complete/wal/compactor_test.go index ffec556fc15..4a29aad8ff5 100644 --- a/ledger/complete/wal/compactor_test.go +++ b/ledger/complete/wal/compactor_test.go @@ -194,15 +194,15 @@ func Test_Compactor(t *testing.T) { } read := &ledger.TrieRead{RootHash: rootHash, Paths: paths} - payloads, err := f.Read(read) + values, err := f.Read(read) require.NoError(t, err) - payloads2, err := f2.Read(read) + values2, err := f2.Read(read) require.NoError(t, err) for i, path := range paths { - require.True(t, data[path].Equals(payloads[i])) - require.True(t, data[path].Equals(payloads2[i])) + require.Equal(t, data[path].Value, values[i]) + require.Equal(t, data[path].Value, values2[i]) } } From ee5ab90a1331fc54f387a88b3ddfad8e9f03597e Mon Sep 17 00:00:00 2001 From: Faye Amacker <33205765+fxamacker@users.noreply.github.com> Date: Tue, 24 May 2022 16:36:34 -0500 Subject: [PATCH 6/8] Update Forest.Read() callers to use new API --- cmd/util/cmd/read-execution-state/list-accounts/cmd.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/util/cmd/read-execution-state/list-accounts/cmd.go b/cmd/util/cmd/read-execution-state/list-accounts/cmd.go index 18555b3bbe1..cef6e1adf65 100644 --- a/cmd/util/cmd/read-execution-state/list-accounts/cmd.go +++ b/cmd/util/cmd/read-execution-state/list-accounts/cmd.go @@ -89,12 +89,12 @@ func run(*cobra.Command, []string) { }, } - payload, err := forest.Read(read) + values, err := forest.Read(read) if err != nil { return nil, err } - return payload[0].Value, nil + return values[0], nil }) sth := state.NewStateHolder(state.NewState(ldg)) From 31d144c4a8727961f030e6743c1e587d7685966c Mon Sep 17 00:00:00 2001 From: Faye Amacker <33205765+fxamacker@users.noreply.github.com> Date: Tue, 24 May 2022 19:24:03 -0500 Subject: [PATCH 7/8] Speedup and reduce allocs/op in ledger single read Changed Forest.ReadSingleValue to return ledger.Value without deep copying payload keys. This avoid 4 heap allocation per key. This change doesn't affect the caller (Ledger.GetSingleValue) because it discards the payload key. --- ledger/complete/ledger.go | 6 +++--- ledger/complete/mtrie/forest.go | 8 ++++---- ledger/complete/mtrie/forest_test.go | 18 +++++++++--------- ledger/trie.go | 2 +- 4 files changed, 17 insertions(+), 17 deletions(-) diff --git a/ledger/complete/ledger.go b/ledger/complete/ledger.go index a081a54cc8d..0eaadcb014b 100644 --- a/ledger/complete/ledger.go +++ b/ledger/complete/ledger.go @@ -140,8 +140,8 @@ func (l *Ledger) GetSingleValue(query *ledger.QuerySingleValue) (value ledger.Va if err != nil { return nil, err } - trieRead := &ledger.TrieReadSinglePayload{RootHash: ledger.RootHash(query.State()), Path: path} - payload, err := l.forest.ReadSinglePayload(trieRead) + trieRead := &ledger.TrieReadSingleValue{RootHash: ledger.RootHash(query.State()), Path: path} + value, err = l.forest.ReadSingleValue(trieRead) if err != nil { return nil, err } @@ -153,7 +153,7 @@ func (l *Ledger) GetSingleValue(query *ledger.QuerySingleValue) (value ledger.Va durationPerValue := time.Duration(readDuration.Nanoseconds()) * time.Nanosecond l.metrics.ReadDurationPerItem(durationPerValue) - return payload.Value, nil + return value, nil } // Get read the values of the given keys at the given state diff --git a/ledger/complete/mtrie/forest.go b/ledger/complete/mtrie/forest.go index 67a2f851c60..c03a619bf5c 100644 --- a/ledger/complete/mtrie/forest.go +++ b/ledger/complete/mtrie/forest.go @@ -123,8 +123,8 @@ func (f *Forest) ValueSizes(r *ledger.TrieRead) ([]int, error) { return orderedValueSizes, nil } -// ReadSinglePayload reads value for a single path and returns value and error (if any) -func (f *Forest) ReadSinglePayload(r *ledger.TrieReadSinglePayload) (*ledger.Payload, error) { +// ReadSingleValue reads value for a single path and returns value and error (if any) +func (f *Forest) ReadSingleValue(r *ledger.TrieReadSingleValue) (ledger.Value, error) { // lookup the trie by rootHash trie, err := f.GetTrie(r.RootHash) if err != nil { @@ -132,7 +132,7 @@ func (f *Forest) ReadSinglePayload(r *ledger.TrieReadSinglePayload) (*ledger.Pay } payload := trie.ReadSinglePayload(r.Path) - return payload.DeepCopy(), nil + return payload.Value.DeepCopy(), nil } // Read reads values for an slice of paths and returns values and error (if any) @@ -152,7 +152,7 @@ func (f *Forest) Read(r *ledger.TrieRead) ([]ledger.Value, error) { // call ReadSinglePayload if there is only one path if len(r.Paths) == 1 { payload := trie.ReadSinglePayload(r.Paths[0]) - return []*ledger.Payload{payload.DeepCopy()}, nil + return []ledger.Value{payload.Value.DeepCopy()}, nil } // deduplicate keys: diff --git a/ledger/complete/mtrie/forest_test.go b/ledger/complete/mtrie/forest_test.go index 11123c9d622..bf7c9b6eda4 100644 --- a/ledger/complete/mtrie/forest_test.go +++ b/ledger/complete/mtrie/forest_test.go @@ -614,25 +614,25 @@ func TestReadSinglePayload(t *testing.T) { // Batch read one payload at a time (less efficient) for path, payload := range expectedPayloads { read := &ledger.TrieRead{RootHash: baseRoot, Paths: []ledger.Path{path}} - retPayloads, err := forest.Read(read) + retValues, err := forest.Read(read) require.NoError(t, err) - require.Equal(t, 1, len(retPayloads)) + require.Equal(t, 1, len(retValues)) if payload.IsEmpty() { - require.True(t, retPayloads[0].IsEmpty()) + require.Equal(t, 0, len(retValues[0])) } else { - require.Equal(t, payload, retPayloads[0]) + require.Equal(t, payload.Value, retValues[0]) } } - // Read single payload + // Read single value for path, payload := range expectedPayloads { - read := &ledger.TrieReadSinglePayload{RootHash: baseRoot, Path: path} - retPayload, err := forest.ReadSinglePayload(read) + read := &ledger.TrieReadSingleValue{RootHash: baseRoot, Path: path} + retValue, err := forest.ReadSingleValue(read) require.NoError(t, err) if payload.IsEmpty() { - require.True(t, retPayload.IsEmpty()) + require.Equal(t, 0, len(retValue)) } else { - require.Equal(t, payload, retPayload) + require.Equal(t, payload.Value, retValue) } } } diff --git a/ledger/trie.go b/ledger/trie.go index fcd6411d74b..2e1b99dbc40 100644 --- a/ledger/trie.go +++ b/ledger/trie.go @@ -90,7 +90,7 @@ type TrieRead struct { } // TrieReadSinglePayload contains trie read query for a single payload -type TrieReadSinglePayload struct { +type TrieReadSingleValue struct { RootHash RootHash Path Path } From c8b2bc58551cbfe4e2c6e9274922f68b9b72a666 Mon Sep 17 00:00:00 2001 From: Faye Amacker <33205765+fxamacker@users.noreply.github.com> Date: Wed, 25 May 2022 11:03:27 -0500 Subject: [PATCH 8/8] Move a line outside for-loop in readSinglePayload This optimization improves speed by 0.49% on Haswell CPU. Thanks for suggestion @tarakby. Co-authored-by: Tarak Ben Youssef --- ledger/complete/mtrie/trie/trie.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/ledger/complete/mtrie/trie/trie.go b/ledger/complete/mtrie/trie/trie.go index 6bcd3068b68..59581b3aa0e 100644 --- a/ledger/complete/mtrie/trie/trie.go +++ b/ledger/complete/mtrie/trie/trie.go @@ -204,15 +204,21 @@ func (mt *MTrie) ReadSinglePayload(path ledger.Path) *ledger.Payload { func readSinglePayload(path ledger.Path, head *node.Node) *ledger.Payload { pathBytes := path[:] + if head == nil { + return ledger.EmptyPayload() + } + + depth := ledger.NodeMaxHeight - head.Height() // distance to the tree root + // Traverse nodes following the path until a leaf node or nil node is reached. for !head.IsLeaf() { - depth := ledger.NodeMaxHeight - head.Height() // distance to the tree root bit := bitutils.ReadBit(pathBytes, depth) if bit == 0 { head = head.LeftChild() } else { head = head.RightChild() } + depth++ } if head != nil && *head.Path() == path {