xdr and exp/orderbook: Reduce path search allocations #4105

2opremio · 2021-11-29T12:54:45Z

This considerably reduces the allocations of path search by:

Allocating pathNodes through a slab (which piggyback's on append()s exponential allocation).
Preallocating some slices
Improving asset extraction to strings.
Optimizing strkey encoding. This is done in two ways:
a. By creating strkey.EncodingBuffer which reuses the internal buffers between calls (much like Create xdr.EncodingBuffer, which reduces buffer allocations #4056 ).
b. By optimizing strkey encoding in general (there were a few things which were slow or caused uncessary allocartions, like using fmt.Sprintf() and using an array to obtain the crc16). This will be done at strkey: reduce allocs in Encode #4107 instead

Before:

goos: darwin
goarch: amd64
pkg: github.com/stellar/go/exp/orderbook
cpu: Intel(R) Core(TM) i7-1068NG7 CPU @ 2.30GHz
BenchmarkVibrantPath
BenchmarkVibrantPath-8               100          11918038 ns/op         2772506 B/op      65448 allocs/op
PASS

After:

goos: darwin
goarch: amd64
pkg: github.com/stellar/go/exp/orderbook
cpu: Intel(R) Core(TM) i7-1068NG7 CPU @ 2.30GHz
BenchmarkVibrantPath
BenchmarkVibrantPath-8   	     120	   9873280 ns/op	 2861917 B/op	   26235 allocs/op
PASS

This is an ~2.49x allocation reduction and a 20% speedup.

Most of the remaining allocations are removed at #4104

Depends on #4102

strkey/main.go

exp/orderbook/graph.go

exp/orderbook/search.go

tamirms · 2021-11-29T14:30:17Z

exp/orderbook/graph.go

@@ -348,10 +352,10 @@ func (graph *OrderBookGraph) FindPaths(
 	maxAssetsPerPath int,
 	includePools bool,
 ) ([]Path, uint32, error) {
-	destinationAssetString := destinationAsset.String()
+	destinationAssetString := destinationAsset.StringWithEncoder(graph.strkeyEncoder)


is graph.strkeyEncoder thread safe? FindPaths() can be called concurrently

Ops, no, it is not. I will make it thread-safe and see if it still performs well.

I added a small critical section in the Find*Paths() functions

PTAL

@2opremio is there still a performance improvement with the critical section?

I think it's worth it because of the loop.

leighmcculloch

I'm interested in strkey performance improvements because github.com/stellar/starlight uses strkey.Encode heavily and just looked at them, but I didn't review the other changes. I left a suggestion (💡) where I suggest an alternative optimization that benefits more folks. @2opremio told me this PR is urgent, so none of these comments are blocking if this is urgent, and I defer to Horizon folks for the PR as a whole.

strkey/main.go

2opremio · 2021-12-01T15:47:57Z

I removed strkey.EncodingBuffer() in favor of #4107

2opremio · 2021-12-01T15:48:08Z

@tamirms PTAL

@2opremio

Replace bytes.Buffer with working with a byte array directly. Also, contains the crc16 improvements by @2opremio that were in #4105. Reduce the allocations in strkey.Encode from 6 to 1, and reduce the heap use from 244 B/op to 64 B/op. In the past I have seen impact to performance of Starlight caused by the strkey.Encode functions. @2opremio also highlighted in #4105 there is an impact there. The use of a bytes.Buffer in this code is rather unnecessary given we know the size of everything, and given that the size of strkeys are small encouraging their data onto the stack is fine. It is worth noting that there is a functional impact by this change. Attempts to strkey.Encode a data value greater than the max payload size as defined by SEP-23 will now error. We've been wanting to add length checks (#1769) for sometime, so I think this is fine, and we can continue to improve those length checks in the future. Before: BenchmarkDecode_accountID-8 2576450 414.2 ns/op 130 B/op 3 allocs/op BenchmarkEncode_accountID-8 3657649 319.6 ns/op 244 B/op 6 allocs/op After: BenchmarkDecode_accountID-8 3563737 334.9 ns/op 128 B/op 2 allocs/op BenchmarkEncode_accountID-8 7365306 165.4 ns/op 64 B/op 1 allocs/op Co-authored-by: Alfonso Acosta <[email protected]>

@2opremio

Replace bytes.Buffer with working with a byte array directly. Also, contains the crc16 improvements by @2opremio that were in #4105. Reduce the allocations in strkey.Encode from 6 to 1, and reduce the heap use from 244 B/op to 64 B/op. In the past I have seen impact to performance of Starlight caused by the strkey.Encode functions. @2opremio also highlighted in #4105 there is an impact there. The use of a bytes.Buffer in this code is rather unnecessary given we know the size of everything, and given that the size of strkeys are small encouraging their data onto the stack is fine. It is worth noting that there is a functional impact by this change. Attempts to strkey.Encode a data value greater than the max payload size as defined by SEP-23 will now error. We've been wanting to add length checks (#1769) for sometime, so I think this is fine, and we can continue to improve those length checks in the future. Before: BenchmarkDecode_accountID-8 2576450 414.2 ns/op 130 B/op 3 allocs/op BenchmarkEncode_accountID-8 3657649 319.6 ns/op 244 B/op 6 allocs/op After: BenchmarkDecode_accountID-8 3563737 334.9 ns/op 128 B/op 2 allocs/op BenchmarkEncode_accountID-8 7365306 165.4 ns/op 64 B/op 1 allocs/op Co-authored-by: Alfonso Acosta <[email protected]>

Leftover from stellar#4105

Leftover from #4105

2opremio requested a review from tamirms November 29, 2021 12:55

2opremio commented Nov 29, 2021

View reviewed changes

strkey/main.go Outdated Show resolved Hide resolved

2opremio commented Nov 29, 2021

View reviewed changes

exp/orderbook/graph.go Outdated Show resolved Hide resolved

2opremio commented Nov 29, 2021

View reviewed changes

exp/orderbook/search.go Outdated Show resolved Hide resolved

2opremio marked this pull request as draft November 29, 2021 13:01

tamirms reviewed Nov 29, 2021

View reviewed changes

exp/orderbook/search.go Show resolved Hide resolved

2opremio force-pushed the reduce-path-search-allocations branch from bffe014 to f352677 Compare November 29, 2021 13:18

tamirms reviewed Nov 29, 2021

View reviewed changes

2opremio force-pushed the reduce-path-search-allocations branch 2 times, most recently from 15d2725 to 18b9aef Compare November 29, 2021 14:55

tamirms mentioned this pull request Nov 29, 2021

services/horizon: Improve performance of path finding endpoint #4106

Closed

5 tasks

tamirms linked an issue Nov 29, 2021 that may be closed by this pull request

services/horizon: Improve performance of path finding endpoint #4106

Closed

5 tasks

leighmcculloch mentioned this pull request Nov 29, 2021

strkey: reduce allocs in Encode #4107

Merged

leighmcculloch reviewed Nov 29, 2021

View reviewed changes

strkey/main.go Outdated Show resolved Hide resolved

strkey/main.go Outdated Show resolved Hide resolved

strkey/main.go Outdated Show resolved Hide resolved

tamirms changed the base branch from master to release-horizon-v2.12.0 November 30, 2021 09:19

2opremio added 7 commits December 1, 2021 16:45

Piggyback on append() to optimize pathNode allocations

6ff491e

Preallocate slices and introduce strkey.Encoder

d806ada

Rename Encoder to EncodingBuffer

fe8e2c7

Address review feedback

94e92e8

Protect encoding buffer in Find*Paths()

cd56af5

Renaming leftover

1e20816

Revert strkey.EncodingBuffer

fdfc521

2opremio force-pushed the reduce-path-search-allocations branch from 3145ec2 to fdfc521 Compare December 1, 2021 15:45

2opremio marked this pull request as ready for review December 1, 2021 15:47

2opremio changed the title ~~strkey and exp/orderbook: Reduce path search allocations~~ xdr and exp/orderbook: Reduce path search allocations Dec 1, 2021

tamirms approved these changes Dec 1, 2021

View reviewed changes

tamirms merged commit 0624517 into stellar:release-horizon-v2.12.0 Dec 1, 2021

2opremio deleted the reduce-path-search-allocations branch December 3, 2021 20:13

2opremio added a commit to 2opremio/go-2 that referenced this pull request Dec 3, 2021

Micro-optimization for paths search

3d0a530

Leftover from stellar#4105

2opremio mentioned this pull request Dec 3, 2021

exp/orderbook: Micro-optimization for paths search #4125

Merged

2opremio added a commit that referenced this pull request Dec 4, 2021

Micro-optimization for paths search (#4125)

01ba901

Leftover from #4105

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

xdr and exp/orderbook: Reduce path search allocations #4105

xdr and exp/orderbook: Reduce path search allocations #4105

2opremio commented Nov 29, 2021 •

edited

Loading

tamirms Nov 29, 2021

2opremio Nov 29, 2021 •

edited

Loading

2opremio Nov 29, 2021

tamirms Nov 29, 2021

2opremio Nov 29, 2021

2opremio Nov 29, 2021

leighmcculloch left a comment •

edited

Loading

2opremio commented Dec 1, 2021

2opremio commented Dec 1, 2021

xdr and exp/orderbook: Reduce path search allocations #4105

xdr and exp/orderbook: Reduce path search allocations #4105

Conversation

2opremio commented Nov 29, 2021 • edited Loading

tamirms Nov 29, 2021

Choose a reason for hiding this comment

2opremio Nov 29, 2021 • edited Loading

Choose a reason for hiding this comment

2opremio Nov 29, 2021

Choose a reason for hiding this comment

tamirms Nov 29, 2021

Choose a reason for hiding this comment

2opremio Nov 29, 2021

Choose a reason for hiding this comment

2opremio Nov 29, 2021

Choose a reason for hiding this comment

leighmcculloch left a comment • edited Loading

Choose a reason for hiding this comment

2opremio commented Dec 1, 2021

2opremio commented Dec 1, 2021

2opremio commented Nov 29, 2021 •

edited

Loading

2opremio Nov 29, 2021 •

edited

Loading

leighmcculloch left a comment •

edited

Loading