From 6ff491ed55246a34ea7d9c6fdcb8d1a4b8bdd215 Mon Sep 17 00:00:00 2001 From: Alfonso Acosta Date: Mon, 29 Nov 2021 13:21:06 +0100 Subject: [PATCH 1/7] Piggyback on append() to optimize pathNode allocations --- exp/orderbook/search.go | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/exp/orderbook/search.go b/exp/orderbook/search.go index fa7ad738f1..461a11b5f4 100644 --- a/exp/orderbook/search.go +++ b/exp/orderbook/search.go @@ -128,6 +128,8 @@ func search( bestPath := make([]*pathNode, totalAssets) updatePath := make([]*pathNode, totalAssets) updatedAssets := make([]int32, 0, totalAssets) + // Used to minimize allocations + slab := make([]pathNode, 0, totalAssets) bestAmount[sourceAsset] = sourceAssetAmount updateAmount[sourceAsset] = sourceAssetAmount bestPath[sourceAsset] = &pathNode{ @@ -177,11 +179,15 @@ func search( updateAmount[nextAsset] = nextAssetAmount if newEntry { - updatePath[nextAsset] = &pathNode{ + updatedAssets = append(updatedAssets, nextAsset) + // By piggybacking on slice appending (which uses exponential allocation) + // we avoid allocating each node individually, which is much slower and + // puts more pressure on the garbage collector. + slab = append(slab, pathNode{ asset: nextAsset, prev: pathToCurrentAsset, - } - updatedAssets = append(updatedAssets, nextAsset) + }) + updatePath[nextAsset] = &slab[len(slab)-1] } else { updatePath[nextAsset].prev = pathToCurrentAsset } From d806ada34f36492538505387cb28955a52ffede1 Mon Sep 17 00:00:00 2001 From: Alfonso Acosta Date: Thu, 25 Nov 2021 01:17:04 +0100 Subject: [PATCH 2/7] Preallocate slices and introduce strkey.Encoder --- exp/orderbook/graph.go | 60 ++++++++++++++++++------ exp/orderbook/graph_benchmark_test.go | 3 +- exp/orderbook/search.go | 10 ++-- strkey/internal/crc16/main.go | 13 ++---- strkey/internal/crc16/main_test.go | 6 +-- strkey/main.go | 67 ++++++++++++++++++++++----- xdr/asset.go | 38 +++++++++++++-- 7 files changed, 149 insertions(+), 48 deletions(-) diff --git a/exp/orderbook/graph.go b/exp/orderbook/graph.go index 6fa5019cef..4dd5a39991 100644 --- a/exp/orderbook/graph.go +++ b/exp/orderbook/graph.go @@ -5,6 +5,7 @@ import ( "sort" "sync" + "github.com/stellar/go/strkey" "github.com/stellar/go/support/errors" "github.com/stellar/go/xdr" ) @@ -84,6 +85,8 @@ type OrderBookGraph struct { lock sync.RWMutex // the orderbook graph is accurate up to lastLedger lastLedger uint32 + + strkeyEncoder *strkey.Encoder } var _ OBGraph = (*OrderBookGraph)(nil) @@ -91,6 +94,7 @@ var _ OBGraph = (*OrderBookGraph)(nil) // NewOrderBookGraph constructs an empty OrderBookGraph func NewOrderBookGraph() *OrderBookGraph { graph := &OrderBookGraph{} + graph.strkeyEncoder = strkey.NewEncoder() graph.Clear() return graph } @@ -205,7 +209,7 @@ func (graph *OrderBookGraph) batch() *orderBookBatchedUpdates { } func (graph *OrderBookGraph) getOrCreateAssetID(asset xdr.Asset) int32 { - assetString := asset.String() + assetString := asset.StringWithEncoder(graph.strkeyEncoder) id, ok := graph.assetStringToID[assetString] if ok { return id @@ -367,10 +371,10 @@ func (graph *OrderBookGraph) FindPaths( maxAssetsPerPath int, includePools bool, ) ([]Path, uint32, error) { - destinationAssetString := destinationAsset.String() + destinationAssetString := destinationAsset.StringWithEncoder(graph.strkeyEncoder) sourceAssetsMap := make(map[int32]xdr.Int64, len(sourceAssets)) for i, sourceAsset := range sourceAssets { - sourceAssetString := sourceAsset.String() + sourceAssetString := sourceAsset.StringWithEncoder(graph.strkeyEncoder) sourceAssetsMap[graph.assetStringToID[sourceAssetString]] = sourceAssetBalances[i] } @@ -406,6 +410,23 @@ func (graph *OrderBookGraph) FindPaths( return paths, lastLedger, err } +type sortablePaths struct { + paths []Path + less func(paths []Path, i, j int) bool +} + +func (s sortablePaths) Swap(i, j int) { + s.paths[i], s.paths[j] = s.paths[j], s.paths[i] +} + +func (s sortablePaths) Less(i, j int) bool { + return s.less(s.paths, i, j) +} + +func (s sortablePaths) Len() int { + return len(s.paths) +} + // FindFixedPaths returns a list of payment paths where the source and // destination assets are fixed. // @@ -425,16 +446,19 @@ func (graph *OrderBookGraph) FindFixedPaths( ) ([]Path, uint32, error) { target := map[int32]bool{} for _, destinationAsset := range destinationAssets { - destinationAssetString := destinationAsset.String() + destinationAssetString := destinationAsset.StringWithEncoder(graph.strkeyEncoder) target[graph.assetStringToID[destinationAssetString]] = true } + // Initialize the capacity of paths to minimize allocations + // TODO: choose value wisely + pathsWithCapacity := make([]Path, 0, 64) searchState := &buyingGraphSearchState{ graph: graph, - sourceAssetString: sourceAsset.String(), + sourceAssetString: sourceAsset.StringWithEncoder(graph.strkeyEncoder), sourceAssetAmount: amountToSpend, targetAssets: target, - paths: []Path{}, + paths: pathsWithCapacity, includePools: includePools, } graph.lock.RLock() @@ -442,7 +466,7 @@ func (graph *OrderBookGraph) FindFixedPaths( ctx, searchState, maxPathLength, - graph.assetStringToID[sourceAsset.String()], + graph.assetStringToID[sourceAsset.StringWithEncoder(graph.strkeyEncoder)], amountToSpend, ) lastLedger := graph.lastLedger @@ -451,9 +475,11 @@ func (graph *OrderBookGraph) FindFixedPaths( return nil, lastLedger, errors.Wrap(err, "could not determine paths") } - sort.Slice(searchState.paths, func(i, j int) bool { - return searchState.paths[i].DestinationAmount > searchState.paths[j].DestinationAmount - }) + sPaths := sortablePaths{ + paths: searchState.paths, + less: compareDestinationAmount, + } + sort.Sort(sPaths) paths, err := sortAndFilterPaths( searchState.paths, @@ -491,6 +517,10 @@ func compareDestinationAsset(allPaths []Path, i, j int) bool { return allPaths[i].DestinationAsset < allPaths[j].DestinationAsset } +func compareDestinationAmount(allPaths []Path, i, j int) bool { + return allPaths[i].DestinationAmount > allPaths[j].DestinationAmount +} + func sourceAssetEquals(p, otherPath Path) bool { return p.SourceAsset == otherPath.SourceAsset } @@ -520,11 +550,13 @@ func sortAndFilterPaths( return nil, errors.New("invalid sort by type") } - sort.Slice(allPaths, func(i, j int) bool { - return comparePaths(allPaths, i, j) - }) + sPaths := sortablePaths{ + paths: allPaths, + less: comparePaths, + } + sort.Sort(sPaths) - filtered := []Path{} + filtered := make([]Path, 0, len(allPaths)) countForAsset := 0 for _, entry := range allPaths { if len(filtered) == 0 || !assetsEqual(filtered[len(filtered)-1], entry) { diff --git a/exp/orderbook/graph_benchmark_test.go b/exp/orderbook/graph_benchmark_test.go index 7d5a260522..8588a2f36a 100644 --- a/exp/orderbook/graph_benchmark_test.go +++ b/exp/orderbook/graph_benchmark_test.go @@ -124,7 +124,8 @@ func BenchmarkVibrantPath(b *testing.B) { b.ResetTimer() b.ReportAllocs() // https://horizon.stellar.org/paths/strict-send?source_asset_type=credit_alphanum4&source_asset_code=USDC&source_asset_issuer=GA5ZSEJYB37JRC5AVCIA5MOP4RHTM335X2KGX3IHOJAPP5RE34K4KZVN&source_amount=10&destination_assets=ARST%3AGCSAZVWXZKWS4XS223M5F54H2B6XPIIXZZGP7KEAIU6YSL5HDRGCI3DG - + // Uncomment in order to get a detailed heap allocations profile + // runtime.MemProfileRate = 1 for i := 0; i < b.N; i++ { _, _, err := graph.FindFixedPaths( context.Background(), diff --git a/exp/orderbook/search.go b/exp/orderbook/search.go index 461a11b5f4..7bb55ff15c 100644 --- a/exp/orderbook/search.go +++ b/exp/orderbook/search.go @@ -106,7 +106,9 @@ func reversePath(path []int32) { } func (e *pathNode) path() []int32 { - var result []int32 + // Initialize slice capacity to minimize allocations + // TODO: choose value wisely + result := make([]int32, 0, 8) for cur := e; cur != nil; cur = cur.prev { result = append(result, cur.asset) } @@ -265,9 +267,9 @@ func (state *sellingGraphSearchState) betterPathAmount(currentAmount, alternativ } func assetIDsToAssetStrings(graph *OrderBookGraph, path []int32) []string { - var result []string - for _, asset := range path { - result = append(result, graph.idToAssetString[asset]) + result := make([]string, len(path)) + for i := 0; i < len(path); i++ { + result[i] = graph.idToAssetString[path[i]] } return result } diff --git a/strkey/internal/crc16/main.go b/strkey/internal/crc16/main.go index ccf5b2630a..647ebfa275 100644 --- a/strkey/internal/crc16/main.go +++ b/strkey/internal/crc16/main.go @@ -47,8 +47,6 @@ package crc16 import ( - "bytes" - "encoding/binary" "errors" ) @@ -92,26 +90,23 @@ var crc16tab = [256]uint16{ } // Checksum returns the 2-byte checksum for the provided data -func Checksum(data []byte) []byte { +func Checksum(data []byte) uint16 { var crc uint16 - var out [2]byte for _, b := range data { crc = ((crc << 8) & 0xffff) ^ crc16tab[((crc>>8)^uint16(b))&0x00FF] } - binary.LittleEndian.PutUint16(out[:], crc) - - return out[:] + return crc } // Validate returns an error if the provided checksum does not match // the calculated checksum of the provided data -func Validate(data []byte, expected []byte) error { +func Validate(data []byte, expected uint16) error { actual := Checksum(data) // validate the provided checksum against the calculated - if !bytes.Equal(actual, expected) { + if actual != expected { return ErrInvalidChecksum } diff --git a/strkey/internal/crc16/main_test.go b/strkey/internal/crc16/main_test.go index a1314a9fa1..c1a64f3611 100644 --- a/strkey/internal/crc16/main_test.go +++ b/strkey/internal/crc16/main_test.go @@ -8,13 +8,13 @@ import ( func TestChecksum(t *testing.T) { result := Checksum([]byte{0x12, 0x34, 0x56, 0x78, 0x90}) - assert.Equal(t, []byte{0xe6, 0x48}, result) + assert.Equal(t, uint16(0x48e6), result) } func TestValidate(t *testing.T) { - err := Validate([]byte{0x12, 0x34, 0x56, 0x78, 0x90}, []byte{0xe6, 0x48}) + err := Validate([]byte{0x12, 0x34, 0x56, 0x78, 0x90}, uint16(0x48e6)) assert.NoError(t, err) - err = Validate([]byte{0x12, 0x34, 0x56, 0x78, 0x90}, []byte{0xe7, 0x48}) + err = Validate([]byte{0x12, 0x34, 0x56, 0x78, 0x90}, uint16(0x48e7)) assert.ErrorIs(t, err, ErrInvalidChecksum) } diff --git a/strkey/main.go b/strkey/main.go index 743c8afd22..c2611e68ca 100644 --- a/strkey/main.go +++ b/strkey/main.go @@ -56,7 +56,7 @@ func DecodeAny(src string) (VersionByte, []byte, error) { } // ensure checksum is valid - if err := crc16.Validate(vp, checksum); err != nil { + if err := crc16.Validate(vp, binary.LittleEndian.Uint16(checksum)); err != nil { return 0, nil, err } @@ -94,7 +94,7 @@ func Decode(expected VersionByte, src string) ([]byte, error) { } // ensure checksum is valid - if err := crc16.Validate(vp, checksum); err != nil { + if err := crc16.Validate(vp, binary.LittleEndian.Uint16(checksum)); err != nil { return nil, err } @@ -111,33 +111,76 @@ func MustDecode(expected VersionByte, src string) []byte { return d } -// Encode encodes the provided data to a StrKey, using the provided version -// byte. -func Encode(version VersionByte, src []byte) (string, error) { +type Encoder struct { + encoding *base32.Encoding + buf *bytes.Buffer + scratchBuf []byte +} + +func NewEncoder() *Encoder { + encoding := base32.StdEncoding.WithPadding(base32.NoPadding) + // Initialize buffer storage in order to avoid allocations + initialCap := 64 + return &Encoder{ + encoding: encoding, + buf: bytes.NewBuffer(make([]byte, 0, initialCap)), + scratchBuf: make([]byte, encoding.EncodedLen(initialCap)), + } +} + +func growSlice(old []byte, newSize int) []byte { + oldCap := cap(old) + if newSize <= oldCap { + return old[:newSize] + } + // the array doesn't fit, lets return a new one with double the capacity + // to avoid further resizing + return make([]byte, newSize, 2*newSize) +} + +func (e *Encoder) Encode(version VersionByte, src []byte) (string, error) { if err := checkValidVersionByte(version); err != nil { return "", err } - var raw bytes.Buffer + e.buf.Reset() // write version byte - if err := binary.Write(&raw, binary.LittleEndian, version); err != nil { + if err := e.buf.WriteByte(byte(version)); err != nil { return "", err } // write payload - if _, err := raw.Write(src); err != nil { + if _, err := e.buf.Write(src); err != nil { return "", err } // calculate and write checksum - checksum := crc16.Checksum(raw.Bytes()) - if _, err := raw.Write(checksum); err != nil { + checksum := crc16.Checksum(e.buf.Bytes()) + var crc [2]byte + binary.LittleEndian.PutUint16(crc[:], checksum) + if _, err := e.buf.Write(crc[:]); err != nil { return "", err } - result := base32.StdEncoding.WithPadding(base32.NoPadding).EncodeToString(raw.Bytes()) - return result, nil + e.scratchBuf = growSlice(e.scratchBuf, e.encoding.EncodedLen(len(e.buf.Bytes()))) + e.encoding.Encode(e.scratchBuf, e.buf.Bytes()) + return string(e.scratchBuf), nil +} + +func (e *Encoder) MustEncode(version VersionByte, src []byte) string { + res, err := e.Encode(version, src) + if err != nil { + panic(err) + } + return res +} + +// Encode encodes the provided data to a StrKey, using the provided version +// byte. +func Encode(version VersionByte, src []byte) (string, error) { + encoder := NewEncoder() + return encoder.Encode(version, src) } // MustEncode is like Encode, but panics on error diff --git a/xdr/asset.go b/xdr/asset.go index 4749e49940..7538f65b9f 100644 --- a/xdr/asset.go +++ b/xdr/asset.go @@ -251,7 +251,20 @@ func (a Asset) String() string { return t } - return fmt.Sprintf("%s/%s/%s", t, c, i) + return t + "/" + c + "/" + i +} + +// StringWithEncoder works like String() but uses an strkey.Encoder +func (a Asset) StringWithEncoder(encoder *strkey.Encoder) string { + var t, c, i string + + a.MustExtractWithEncoder(&t, &c, &i, encoder) + + if a.Type == AssetTypeAssetTypeNative { + return t + } + + return t + "/" + c + "/" + i } // StringCanonical returns a display friendly form of the asset following its @@ -295,6 +308,12 @@ func (a Asset) Equals(other Asset) bool { // code and issuer to `code` and `issuer` respectively if they are of type // *string and the asset is non-native func (a Asset) Extract(typ interface{}, code interface{}, issuer interface{}) error { + encoder := strkey.NewEncoder() + return a.ExtractWithEncoder(typ, code, issuer, encoder) +} + +// ExtractWithEncoder works like Extract but uses an strkey.Encoder +func (a Asset) ExtractWithEncoder(typ interface{}, code interface{}, issuer interface{}, encoder *strkey.Encoder) error { switch typ := typ.(type) { case *AssetType: *typ = a.Type @@ -310,10 +329,10 @@ func (a Asset) Extract(typ interface{}, code interface{}, issuer interface{}) er switch a.Type { case AssetTypeAssetTypeCreditAlphanum4: an := a.MustAlphaNum4() - *code = strings.TrimRight(string(an.AssetCode[:]), "\x00") + *code = string(trimRightZeros(an.AssetCode[:])) case AssetTypeAssetTypeCreditAlphanum12: an := a.MustAlphaNum12() - *code = strings.TrimRight(string(an.AssetCode[:]), "\x00") + *code = string(trimRightZeros(an.AssetCode[:])) } default: return errors.New("can't extract code") @@ -327,11 +346,11 @@ func (a Asset) Extract(typ interface{}, code interface{}, issuer interface{}) er case AssetTypeAssetTypeCreditAlphanum4: an := a.MustAlphaNum4() raw := an.Issuer.MustEd25519() - *issuer = strkey.MustEncode(strkey.VersionByteAccountID, raw[:]) + *issuer = encoder.MustEncode(strkey.VersionByteAccountID, raw[:]) case AssetTypeAssetTypeCreditAlphanum12: an := a.MustAlphaNum12() raw := an.Issuer.MustEd25519() - *issuer = strkey.MustEncode(strkey.VersionByteAccountID, raw[:]) + *issuer = encoder.MustEncode(strkey.VersionByteAccountID, raw[:]) } default: return errors.New("can't extract issuer") @@ -341,6 +360,15 @@ func (a Asset) Extract(typ interface{}, code interface{}, issuer interface{}) er return nil } +// MustExtractWithEncoder behaves as ExtractWithEncoder, but panics if an error occurs. +func (a Asset) MustExtractWithEncoder(typ interface{}, code interface{}, issuer interface{}, encoder *strkey.Encoder) { + err := a.ExtractWithEncoder(typ, code, issuer, encoder) + + if err != nil { + panic(err) + } +} + // MustExtract behaves as Extract, but panics if an error occurs. func (a Asset) MustExtract(typ interface{}, code interface{}, issuer interface{}) { err := a.Extract(typ, code, issuer) From fe8e2c78af8f4e2e0add20021793e4d5007ffa4e Mon Sep 17 00:00:00 2001 From: Alfonso Acosta Date: Mon, 29 Nov 2021 15:46:47 +0100 Subject: [PATCH 3/7] Rename Encoder to EncodingBuffer --- exp/orderbook/graph.go | 16 ++++++++-------- strkey/main.go | 14 ++++++++------ xdr/asset.go | 20 ++++++++++---------- xdr/main.go | 1 + 4 files changed, 27 insertions(+), 24 deletions(-) diff --git a/exp/orderbook/graph.go b/exp/orderbook/graph.go index 4dd5a39991..b9539bb678 100644 --- a/exp/orderbook/graph.go +++ b/exp/orderbook/graph.go @@ -86,7 +86,7 @@ type OrderBookGraph struct { // the orderbook graph is accurate up to lastLedger lastLedger uint32 - strkeyEncoder *strkey.Encoder + strkeyBuffer *strkey.EncodingBuffer } var _ OBGraph = (*OrderBookGraph)(nil) @@ -94,7 +94,7 @@ var _ OBGraph = (*OrderBookGraph)(nil) // NewOrderBookGraph constructs an empty OrderBookGraph func NewOrderBookGraph() *OrderBookGraph { graph := &OrderBookGraph{} - graph.strkeyEncoder = strkey.NewEncoder() + graph.strkeyBuffer = strkey.NewEncodingBuffer() graph.Clear() return graph } @@ -209,7 +209,7 @@ func (graph *OrderBookGraph) batch() *orderBookBatchedUpdates { } func (graph *OrderBookGraph) getOrCreateAssetID(asset xdr.Asset) int32 { - assetString := asset.StringWithEncoder(graph.strkeyEncoder) + assetString := asset.StringWithEncoder(graph.strkeyBuffer) id, ok := graph.assetStringToID[assetString] if ok { return id @@ -371,10 +371,10 @@ func (graph *OrderBookGraph) FindPaths( maxAssetsPerPath int, includePools bool, ) ([]Path, uint32, error) { - destinationAssetString := destinationAsset.StringWithEncoder(graph.strkeyEncoder) + destinationAssetString := destinationAsset.StringWithEncoder(graph.strkeyBuffer) sourceAssetsMap := make(map[int32]xdr.Int64, len(sourceAssets)) for i, sourceAsset := range sourceAssets { - sourceAssetString := sourceAsset.StringWithEncoder(graph.strkeyEncoder) + sourceAssetString := sourceAsset.StringWithEncoder(graph.strkeyBuffer) sourceAssetsMap[graph.assetStringToID[sourceAssetString]] = sourceAssetBalances[i] } @@ -446,7 +446,7 @@ func (graph *OrderBookGraph) FindFixedPaths( ) ([]Path, uint32, error) { target := map[int32]bool{} for _, destinationAsset := range destinationAssets { - destinationAssetString := destinationAsset.StringWithEncoder(graph.strkeyEncoder) + destinationAssetString := destinationAsset.StringWithEncoder(graph.strkeyBuffer) target[graph.assetStringToID[destinationAssetString]] = true } @@ -455,7 +455,7 @@ func (graph *OrderBookGraph) FindFixedPaths( pathsWithCapacity := make([]Path, 0, 64) searchState := &buyingGraphSearchState{ graph: graph, - sourceAssetString: sourceAsset.StringWithEncoder(graph.strkeyEncoder), + sourceAssetString: sourceAsset.StringWithEncoder(graph.strkeyBuffer), sourceAssetAmount: amountToSpend, targetAssets: target, paths: pathsWithCapacity, @@ -466,7 +466,7 @@ func (graph *OrderBookGraph) FindFixedPaths( ctx, searchState, maxPathLength, - graph.assetStringToID[sourceAsset.StringWithEncoder(graph.strkeyEncoder)], + graph.assetStringToID[sourceAsset.StringWithEncoder(graph.strkeyBuffer)], amountToSpend, ) lastLedger := graph.lastLedger diff --git a/strkey/main.go b/strkey/main.go index c2611e68ca..1e42bd62be 100644 --- a/strkey/main.go +++ b/strkey/main.go @@ -111,17 +111,19 @@ func MustDecode(expected VersionByte, src string) []byte { return d } -type Encoder struct { +// EncodingBuffer reuses internal buffers between invocations to minimize allocations. +// For that reason, it is not thread-safe. +type EncodingBuffer struct { encoding *base32.Encoding buf *bytes.Buffer scratchBuf []byte } -func NewEncoder() *Encoder { +func NewEncodingBuffer() *EncodingBuffer { encoding := base32.StdEncoding.WithPadding(base32.NoPadding) // Initialize buffer storage in order to avoid allocations initialCap := 64 - return &Encoder{ + return &EncodingBuffer{ encoding: encoding, buf: bytes.NewBuffer(make([]byte, 0, initialCap)), scratchBuf: make([]byte, encoding.EncodedLen(initialCap)), @@ -138,7 +140,7 @@ func growSlice(old []byte, newSize int) []byte { return make([]byte, newSize, 2*newSize) } -func (e *Encoder) Encode(version VersionByte, src []byte) (string, error) { +func (e *EncodingBuffer) Encode(version VersionByte, src []byte) (string, error) { if err := checkValidVersionByte(version); err != nil { return "", err } @@ -168,7 +170,7 @@ func (e *Encoder) Encode(version VersionByte, src []byte) (string, error) { return string(e.scratchBuf), nil } -func (e *Encoder) MustEncode(version VersionByte, src []byte) string { +func (e *EncodingBuffer) MustEncode(version VersionByte, src []byte) string { res, err := e.Encode(version, src) if err != nil { panic(err) @@ -179,7 +181,7 @@ func (e *Encoder) MustEncode(version VersionByte, src []byte) string { // Encode encodes the provided data to a StrKey, using the provided version // byte. func Encode(version VersionByte, src []byte) (string, error) { - encoder := NewEncoder() + encoder := NewEncodingBuffer() return encoder.Encode(version, src) } diff --git a/xdr/asset.go b/xdr/asset.go index 7538f65b9f..7fa6270285 100644 --- a/xdr/asset.go +++ b/xdr/asset.go @@ -254,11 +254,11 @@ func (a Asset) String() string { return t + "/" + c + "/" + i } -// StringWithEncoder works like String() but uses an strkey.Encoder -func (a Asset) StringWithEncoder(encoder *strkey.Encoder) string { +// StringWithEncoder works like String() but uses an strkey.EncodingBuffer +func (a Asset) StringWithEncoder(encoder *strkey.EncodingBuffer) string { var t, c, i string - a.MustExtractWithEncoder(&t, &c, &i, encoder) + a.MustExtractWithBuffer(&t, &c, &i, encoder) if a.Type == AssetTypeAssetTypeNative { return t @@ -308,12 +308,12 @@ func (a Asset) Equals(other Asset) bool { // code and issuer to `code` and `issuer` respectively if they are of type // *string and the asset is non-native func (a Asset) Extract(typ interface{}, code interface{}, issuer interface{}) error { - encoder := strkey.NewEncoder() - return a.ExtractWithEncoder(typ, code, issuer, encoder) + encoder := strkey.NewEncodingBuffer() + return a.ExtractWithBuffer(typ, code, issuer, encoder) } -// ExtractWithEncoder works like Extract but uses an strkey.Encoder -func (a Asset) ExtractWithEncoder(typ interface{}, code interface{}, issuer interface{}, encoder *strkey.Encoder) error { +// ExtractWithBuffer works like Extract but uses an strkey.EncodingBuffer +func (a Asset) ExtractWithBuffer(typ interface{}, code interface{}, issuer interface{}, encoder *strkey.EncodingBuffer) error { switch typ := typ.(type) { case *AssetType: *typ = a.Type @@ -360,9 +360,9 @@ func (a Asset) ExtractWithEncoder(typ interface{}, code interface{}, issuer inte return nil } -// MustExtractWithEncoder behaves as ExtractWithEncoder, but panics if an error occurs. -func (a Asset) MustExtractWithEncoder(typ interface{}, code interface{}, issuer interface{}, encoder *strkey.Encoder) { - err := a.ExtractWithEncoder(typ, code, issuer, encoder) +// MustExtractWithBuffer behaves as ExtractWithBuffer, but panics if an error occurs. +func (a Asset) MustExtractWithBuffer(typ interface{}, code interface{}, issuer interface{}, encoder *strkey.EncodingBuffer) { + err := a.ExtractWithBuffer(typ, code, issuer, encoder) if err != nil { panic(err) diff --git a/xdr/main.go b/xdr/main.go index 43de34d6e0..77618af842 100644 --- a/xdr/main.go +++ b/xdr/main.go @@ -125,6 +125,7 @@ func MarshalHex(v interface{}) (string, error) { } // EncodingBuffer reuses internal buffers between invocations to minimize allocations. +// For that reason, it is not thread-safe. // It intentionally only allows EncodeTo method arguments, to guarantee high performance encoding. type EncodingBuffer struct { encoder *xdr.Encoder From 94e92e846d026e094b7ab31b75e2cdc90474ccce Mon Sep 17 00:00:00 2001 From: Alfonso Acosta Date: Mon, 29 Nov 2021 15:59:51 +0100 Subject: [PATCH 4/7] Address review feedback --- exp/orderbook/graph.go | 5 +---- exp/orderbook/search.go | 4 ++-- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/exp/orderbook/graph.go b/exp/orderbook/graph.go index b9539bb678..6b3c595ec8 100644 --- a/exp/orderbook/graph.go +++ b/exp/orderbook/graph.go @@ -450,15 +450,12 @@ func (graph *OrderBookGraph) FindFixedPaths( target[graph.assetStringToID[destinationAssetString]] = true } - // Initialize the capacity of paths to minimize allocations - // TODO: choose value wisely - pathsWithCapacity := make([]Path, 0, 64) searchState := &buyingGraphSearchState{ graph: graph, sourceAssetString: sourceAsset.StringWithEncoder(graph.strkeyBuffer), sourceAssetAmount: amountToSpend, targetAssets: target, - paths: pathsWithCapacity, + paths: []Path{}, includePools: includePools, } graph.lock.RLock() diff --git a/exp/orderbook/search.go b/exp/orderbook/search.go index 7bb55ff15c..dd840f3791 100644 --- a/exp/orderbook/search.go +++ b/exp/orderbook/search.go @@ -106,8 +106,8 @@ func reversePath(path []int32) { } func (e *pathNode) path() []int32 { - // Initialize slice capacity to minimize allocations - // TODO: choose value wisely + // Initialize slice capacity to minimize allocations. + // 8 is the maximum path supported by stellar. result := make([]int32, 0, 8) for cur := e; cur != nil; cur = cur.prev { result = append(result, cur.asset) From cd56af59018a866342649d00e9e38650be232b4d Mon Sep 17 00:00:00 2001 From: Alfonso Acosta Date: Mon, 29 Nov 2021 16:25:33 +0100 Subject: [PATCH 5/7] Protect encoding buffer in `Find*Paths()` --- exp/orderbook/graph.go | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/exp/orderbook/graph.go b/exp/orderbook/graph.go index 6b3c595ec8..18a68df7ad 100644 --- a/exp/orderbook/graph.go +++ b/exp/orderbook/graph.go @@ -371,12 +371,15 @@ func (graph *OrderBookGraph) FindPaths( maxAssetsPerPath int, includePools bool, ) ([]Path, uint32, error) { + + graph.lock.Lock() // for graph.strkeyBuffer destinationAssetString := destinationAsset.StringWithEncoder(graph.strkeyBuffer) sourceAssetsMap := make(map[int32]xdr.Int64, len(sourceAssets)) for i, sourceAsset := range sourceAssets { sourceAssetString := sourceAsset.StringWithEncoder(graph.strkeyBuffer) sourceAssetsMap[graph.assetStringToID[sourceAssetString]] = sourceAssetBalances[i] } + graph.lock.Unlock() searchState := &sellingGraphSearchState{ graph: graph, @@ -445,14 +448,18 @@ func (graph *OrderBookGraph) FindFixedPaths( includePools bool, ) ([]Path, uint32, error) { target := map[int32]bool{} + + graph.lock.Lock() // for graph.strkeyBuffer + sourceAssetString := sourceAsset.StringWithEncoder(graph.strkeyBuffer) for _, destinationAsset := range destinationAssets { destinationAssetString := destinationAsset.StringWithEncoder(graph.strkeyBuffer) target[graph.assetStringToID[destinationAssetString]] = true } + graph.lock.Unlock() searchState := &buyingGraphSearchState{ graph: graph, - sourceAssetString: sourceAsset.StringWithEncoder(graph.strkeyBuffer), + sourceAssetString: sourceAssetString, sourceAssetAmount: amountToSpend, targetAssets: target, paths: []Path{}, @@ -463,7 +470,7 @@ func (graph *OrderBookGraph) FindFixedPaths( ctx, searchState, maxPathLength, - graph.assetStringToID[sourceAsset.StringWithEncoder(graph.strkeyBuffer)], + graph.assetStringToID[sourceAssetString], amountToSpend, ) lastLedger := graph.lastLedger From 1e20816ea28838aa733ee2ff2ed1e51c66def35f Mon Sep 17 00:00:00 2001 From: Alfonso Acosta Date: Mon, 29 Nov 2021 16:29:16 +0100 Subject: [PATCH 6/7] Renaming leftover --- exp/orderbook/graph.go | 10 +++++----- xdr/asset.go | 4 ++-- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/exp/orderbook/graph.go b/exp/orderbook/graph.go index 18a68df7ad..416552502d 100644 --- a/exp/orderbook/graph.go +++ b/exp/orderbook/graph.go @@ -209,7 +209,7 @@ func (graph *OrderBookGraph) batch() *orderBookBatchedUpdates { } func (graph *OrderBookGraph) getOrCreateAssetID(asset xdr.Asset) int32 { - assetString := asset.StringWithEncoder(graph.strkeyBuffer) + assetString := asset.StringWithBuffer(graph.strkeyBuffer) id, ok := graph.assetStringToID[assetString] if ok { return id @@ -373,10 +373,10 @@ func (graph *OrderBookGraph) FindPaths( ) ([]Path, uint32, error) { graph.lock.Lock() // for graph.strkeyBuffer - destinationAssetString := destinationAsset.StringWithEncoder(graph.strkeyBuffer) + destinationAssetString := destinationAsset.StringWithBuffer(graph.strkeyBuffer) sourceAssetsMap := make(map[int32]xdr.Int64, len(sourceAssets)) for i, sourceAsset := range sourceAssets { - sourceAssetString := sourceAsset.StringWithEncoder(graph.strkeyBuffer) + sourceAssetString := sourceAsset.StringWithBuffer(graph.strkeyBuffer) sourceAssetsMap[graph.assetStringToID[sourceAssetString]] = sourceAssetBalances[i] } graph.lock.Unlock() @@ -450,9 +450,9 @@ func (graph *OrderBookGraph) FindFixedPaths( target := map[int32]bool{} graph.lock.Lock() // for graph.strkeyBuffer - sourceAssetString := sourceAsset.StringWithEncoder(graph.strkeyBuffer) + sourceAssetString := sourceAsset.StringWithBuffer(graph.strkeyBuffer) for _, destinationAsset := range destinationAssets { - destinationAssetString := destinationAsset.StringWithEncoder(graph.strkeyBuffer) + destinationAssetString := destinationAsset.StringWithBuffer(graph.strkeyBuffer) target[graph.assetStringToID[destinationAssetString]] = true } graph.lock.Unlock() diff --git a/xdr/asset.go b/xdr/asset.go index 7fa6270285..71fe2a3369 100644 --- a/xdr/asset.go +++ b/xdr/asset.go @@ -254,8 +254,8 @@ func (a Asset) String() string { return t + "/" + c + "/" + i } -// StringWithEncoder works like String() but uses an strkey.EncodingBuffer -func (a Asset) StringWithEncoder(encoder *strkey.EncodingBuffer) string { +// StringWithBuffer works like String() but uses an strkey.EncodingBuffer +func (a Asset) StringWithBuffer(encoder *strkey.EncodingBuffer) string { var t, c, i string a.MustExtractWithBuffer(&t, &c, &i, encoder) From fdfc5215beb244cc99ddbf1f02b65737a7767520 Mon Sep 17 00:00:00 2001 From: Alfonso Acosta Date: Wed, 1 Dec 2021 16:44:25 +0100 Subject: [PATCH 7/7] Revert strkey.EncodingBuffer --- exp/orderbook/graph.go | 18 +++----- strkey/internal/crc16/main.go | 13 ++++-- strkey/internal/crc16/main_test.go | 6 +-- strkey/main.go | 69 ++++++------------------------ xdr/asset.go | 32 +------------- 5 files changed, 31 insertions(+), 107 deletions(-) diff --git a/exp/orderbook/graph.go b/exp/orderbook/graph.go index 416552502d..91b7f0e73c 100644 --- a/exp/orderbook/graph.go +++ b/exp/orderbook/graph.go @@ -5,7 +5,6 @@ import ( "sort" "sync" - "github.com/stellar/go/strkey" "github.com/stellar/go/support/errors" "github.com/stellar/go/xdr" ) @@ -85,8 +84,6 @@ type OrderBookGraph struct { lock sync.RWMutex // the orderbook graph is accurate up to lastLedger lastLedger uint32 - - strkeyBuffer *strkey.EncodingBuffer } var _ OBGraph = (*OrderBookGraph)(nil) @@ -94,7 +91,6 @@ var _ OBGraph = (*OrderBookGraph)(nil) // NewOrderBookGraph constructs an empty OrderBookGraph func NewOrderBookGraph() *OrderBookGraph { graph := &OrderBookGraph{} - graph.strkeyBuffer = strkey.NewEncodingBuffer() graph.Clear() return graph } @@ -209,7 +205,7 @@ func (graph *OrderBookGraph) batch() *orderBookBatchedUpdates { } func (graph *OrderBookGraph) getOrCreateAssetID(asset xdr.Asset) int32 { - assetString := asset.StringWithBuffer(graph.strkeyBuffer) + assetString := asset.String() id, ok := graph.assetStringToID[assetString] if ok { return id @@ -372,14 +368,12 @@ func (graph *OrderBookGraph) FindPaths( includePools bool, ) ([]Path, uint32, error) { - graph.lock.Lock() // for graph.strkeyBuffer - destinationAssetString := destinationAsset.StringWithBuffer(graph.strkeyBuffer) + destinationAssetString := destinationAsset.String() sourceAssetsMap := make(map[int32]xdr.Int64, len(sourceAssets)) for i, sourceAsset := range sourceAssets { - sourceAssetString := sourceAsset.StringWithBuffer(graph.strkeyBuffer) + sourceAssetString := sourceAsset.String() sourceAssetsMap[graph.assetStringToID[sourceAssetString]] = sourceAssetBalances[i] } - graph.lock.Unlock() searchState := &sellingGraphSearchState{ graph: graph, @@ -449,14 +443,12 @@ func (graph *OrderBookGraph) FindFixedPaths( ) ([]Path, uint32, error) { target := map[int32]bool{} - graph.lock.Lock() // for graph.strkeyBuffer - sourceAssetString := sourceAsset.StringWithBuffer(graph.strkeyBuffer) for _, destinationAsset := range destinationAssets { - destinationAssetString := destinationAsset.StringWithBuffer(graph.strkeyBuffer) + destinationAssetString := destinationAsset.String() target[graph.assetStringToID[destinationAssetString]] = true } - graph.lock.Unlock() + sourceAssetString := sourceAsset.String() searchState := &buyingGraphSearchState{ graph: graph, sourceAssetString: sourceAssetString, diff --git a/strkey/internal/crc16/main.go b/strkey/internal/crc16/main.go index 647ebfa275..ccf5b2630a 100644 --- a/strkey/internal/crc16/main.go +++ b/strkey/internal/crc16/main.go @@ -47,6 +47,8 @@ package crc16 import ( + "bytes" + "encoding/binary" "errors" ) @@ -90,23 +92,26 @@ var crc16tab = [256]uint16{ } // Checksum returns the 2-byte checksum for the provided data -func Checksum(data []byte) uint16 { +func Checksum(data []byte) []byte { var crc uint16 + var out [2]byte for _, b := range data { crc = ((crc << 8) & 0xffff) ^ crc16tab[((crc>>8)^uint16(b))&0x00FF] } - return crc + binary.LittleEndian.PutUint16(out[:], crc) + + return out[:] } // Validate returns an error if the provided checksum does not match // the calculated checksum of the provided data -func Validate(data []byte, expected uint16) error { +func Validate(data []byte, expected []byte) error { actual := Checksum(data) // validate the provided checksum against the calculated - if actual != expected { + if !bytes.Equal(actual, expected) { return ErrInvalidChecksum } diff --git a/strkey/internal/crc16/main_test.go b/strkey/internal/crc16/main_test.go index c1a64f3611..a1314a9fa1 100644 --- a/strkey/internal/crc16/main_test.go +++ b/strkey/internal/crc16/main_test.go @@ -8,13 +8,13 @@ import ( func TestChecksum(t *testing.T) { result := Checksum([]byte{0x12, 0x34, 0x56, 0x78, 0x90}) - assert.Equal(t, uint16(0x48e6), result) + assert.Equal(t, []byte{0xe6, 0x48}, result) } func TestValidate(t *testing.T) { - err := Validate([]byte{0x12, 0x34, 0x56, 0x78, 0x90}, uint16(0x48e6)) + err := Validate([]byte{0x12, 0x34, 0x56, 0x78, 0x90}, []byte{0xe6, 0x48}) assert.NoError(t, err) - err = Validate([]byte{0x12, 0x34, 0x56, 0x78, 0x90}, uint16(0x48e7)) + err = Validate([]byte{0x12, 0x34, 0x56, 0x78, 0x90}, []byte{0xe7, 0x48}) assert.ErrorIs(t, err, ErrInvalidChecksum) } diff --git a/strkey/main.go b/strkey/main.go index 1e42bd62be..743c8afd22 100644 --- a/strkey/main.go +++ b/strkey/main.go @@ -56,7 +56,7 @@ func DecodeAny(src string) (VersionByte, []byte, error) { } // ensure checksum is valid - if err := crc16.Validate(vp, binary.LittleEndian.Uint16(checksum)); err != nil { + if err := crc16.Validate(vp, checksum); err != nil { return 0, nil, err } @@ -94,7 +94,7 @@ func Decode(expected VersionByte, src string) ([]byte, error) { } // ensure checksum is valid - if err := crc16.Validate(vp, binary.LittleEndian.Uint16(checksum)); err != nil { + if err := crc16.Validate(vp, checksum); err != nil { return nil, err } @@ -111,78 +111,33 @@ func MustDecode(expected VersionByte, src string) []byte { return d } -// EncodingBuffer reuses internal buffers between invocations to minimize allocations. -// For that reason, it is not thread-safe. -type EncodingBuffer struct { - encoding *base32.Encoding - buf *bytes.Buffer - scratchBuf []byte -} - -func NewEncodingBuffer() *EncodingBuffer { - encoding := base32.StdEncoding.WithPadding(base32.NoPadding) - // Initialize buffer storage in order to avoid allocations - initialCap := 64 - return &EncodingBuffer{ - encoding: encoding, - buf: bytes.NewBuffer(make([]byte, 0, initialCap)), - scratchBuf: make([]byte, encoding.EncodedLen(initialCap)), - } -} - -func growSlice(old []byte, newSize int) []byte { - oldCap := cap(old) - if newSize <= oldCap { - return old[:newSize] - } - // the array doesn't fit, lets return a new one with double the capacity - // to avoid further resizing - return make([]byte, newSize, 2*newSize) -} - -func (e *EncodingBuffer) Encode(version VersionByte, src []byte) (string, error) { +// Encode encodes the provided data to a StrKey, using the provided version +// byte. +func Encode(version VersionByte, src []byte) (string, error) { if err := checkValidVersionByte(version); err != nil { return "", err } - e.buf.Reset() + var raw bytes.Buffer // write version byte - if err := e.buf.WriteByte(byte(version)); err != nil { + if err := binary.Write(&raw, binary.LittleEndian, version); err != nil { return "", err } // write payload - if _, err := e.buf.Write(src); err != nil { + if _, err := raw.Write(src); err != nil { return "", err } // calculate and write checksum - checksum := crc16.Checksum(e.buf.Bytes()) - var crc [2]byte - binary.LittleEndian.PutUint16(crc[:], checksum) - if _, err := e.buf.Write(crc[:]); err != nil { + checksum := crc16.Checksum(raw.Bytes()) + if _, err := raw.Write(checksum); err != nil { return "", err } - e.scratchBuf = growSlice(e.scratchBuf, e.encoding.EncodedLen(len(e.buf.Bytes()))) - e.encoding.Encode(e.scratchBuf, e.buf.Bytes()) - return string(e.scratchBuf), nil -} - -func (e *EncodingBuffer) MustEncode(version VersionByte, src []byte) string { - res, err := e.Encode(version, src) - if err != nil { - panic(err) - } - return res -} - -// Encode encodes the provided data to a StrKey, using the provided version -// byte. -func Encode(version VersionByte, src []byte) (string, error) { - encoder := NewEncodingBuffer() - return encoder.Encode(version, src) + result := base32.StdEncoding.WithPadding(base32.NoPadding).EncodeToString(raw.Bytes()) + return result, nil } // MustEncode is like Encode, but panics on error diff --git a/xdr/asset.go b/xdr/asset.go index 71fe2a3369..85fef83f60 100644 --- a/xdr/asset.go +++ b/xdr/asset.go @@ -254,19 +254,6 @@ func (a Asset) String() string { return t + "/" + c + "/" + i } -// StringWithBuffer works like String() but uses an strkey.EncodingBuffer -func (a Asset) StringWithBuffer(encoder *strkey.EncodingBuffer) string { - var t, c, i string - - a.MustExtractWithBuffer(&t, &c, &i, encoder) - - if a.Type == AssetTypeAssetTypeNative { - return t - } - - return t + "/" + c + "/" + i -} - // StringCanonical returns a display friendly form of the asset following its // canonical representation func (a Asset) StringCanonical() string { @@ -308,12 +295,6 @@ func (a Asset) Equals(other Asset) bool { // code and issuer to `code` and `issuer` respectively if they are of type // *string and the asset is non-native func (a Asset) Extract(typ interface{}, code interface{}, issuer interface{}) error { - encoder := strkey.NewEncodingBuffer() - return a.ExtractWithBuffer(typ, code, issuer, encoder) -} - -// ExtractWithBuffer works like Extract but uses an strkey.EncodingBuffer -func (a Asset) ExtractWithBuffer(typ interface{}, code interface{}, issuer interface{}, encoder *strkey.EncodingBuffer) error { switch typ := typ.(type) { case *AssetType: *typ = a.Type @@ -346,11 +327,11 @@ func (a Asset) ExtractWithBuffer(typ interface{}, code interface{}, issuer inter case AssetTypeAssetTypeCreditAlphanum4: an := a.MustAlphaNum4() raw := an.Issuer.MustEd25519() - *issuer = encoder.MustEncode(strkey.VersionByteAccountID, raw[:]) + *issuer = strkey.MustEncode(strkey.VersionByteAccountID, raw[:]) case AssetTypeAssetTypeCreditAlphanum12: an := a.MustAlphaNum12() raw := an.Issuer.MustEd25519() - *issuer = encoder.MustEncode(strkey.VersionByteAccountID, raw[:]) + *issuer = strkey.MustEncode(strkey.VersionByteAccountID, raw[:]) } default: return errors.New("can't extract issuer") @@ -360,15 +341,6 @@ func (a Asset) ExtractWithBuffer(typ interface{}, code interface{}, issuer inter return nil } -// MustExtractWithBuffer behaves as ExtractWithBuffer, but panics if an error occurs. -func (a Asset) MustExtractWithBuffer(typ interface{}, code interface{}, issuer interface{}, encoder *strkey.EncodingBuffer) { - err := a.ExtractWithBuffer(typ, code, issuer, encoder) - - if err != nil { - panic(err) - } -} - // MustExtract behaves as Extract, but panics if an error occurs. func (a Asset) MustExtract(typ interface{}, code interface{}, issuer interface{}) { err := a.Extract(typ, code, issuer)