Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

exp/orderbook: Use uint256s for pool calculations to improve performance #4113

Merged

Conversation

2opremio
Copy link
Contributor

@2opremio 2opremio commented Dec 1, 2021

Second attempt at #4104 by explicitly checking every possible overflow.

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	  10375399 ns/op	 2758209 B/op	   64732 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   	     127	   9447364 ns/op	 2251733 B/op	   39260 allocs/op
PASS

@tamirms
Copy link
Contributor

tamirms commented Dec 1, 2021

I tried comparing the uint128 calculations with the old code which uses math/big ones:


func TestCalculatePoolExpectations(t *testing.T) {
	for i := 0; i < 1000000; i++ {
		reserveA := xdr.Int64(rand.Int63())
		reserveB := xdr.Int64(rand.Int63())
		disbursed := xdr.Int64(rand.Int63())

		result, ok := calculatePoolExpectationBig(reserveA, reserveB, disbursed, 30)
		result1, ok1 := calculatePoolExpectation(reserveA, reserveB, disbursed, 30)
		if assert.Equal(t, ok, ok1) {
			assert.Equal(t, result, result1)
		}
	}
}

func TestCalculatePoolPayout(t *testing.T) {
	for i := 0; i < 1000000; i++ {
		reserveA := xdr.Int64(rand.Int63())
		reserveB := xdr.Int64(rand.Int63())
		received := xdr.Int64(rand.Int63())

		result, ok := calculatePoolPayoutBig(reserveA, reserveB, received, 30)
		result1, ok1 := calculatePoolPayout(reserveA, reserveB, received, 30)
		if assert.Equal(t, ok, ok1) {
			assert.Equal(t, result, result1)
		}
	}
}

Unfortunately there were a lot of cases where the results differed because there were cases where the numerator overflowed but after dividing by the denominator the result was in the bounds of an int64.

But I did find https://github.com/holiman/uint256 . The library is mostly compatible with the big.Int API so it was easy to swap in. When trying out the library I was able to get all the tests to pass and there was a decent performance improvement as well:

Before:

goos: darwin
goarch: amd64
pkg: github.com/stellar/go/exp/orderbook
cpu: Intel(R) Core(TM) i7-8850H CPU @ 2.60GHz
BenchmarkVibrantPath
BenchmarkVibrantPath-12    	     100	  11879873 ns/op	 2794719 B/op	   66595 allocs/op
PASS

After:

goos: darwin
goarch: amd64
pkg: github.com/stellar/go/exp/orderbook
cpu: Intel(R) Core(TM) i7-8850H CPU @ 2.60GHz
BenchmarkVibrantPath
BenchmarkVibrantPath-12    	     100	  10841301 ns/op	 2259508 B/op	   39745 allocs/op
PASS

@tamirms tamirms force-pushed the uint128-orderbook-pools branch 3 times, most recently from b2ed4a1 to 65d0615 Compare December 1, 2021 22:13
Copy link
Contributor

@tamirms tamirms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tested this by running through path finding requests on this branch and comparing the output with the result from the old code. Everything matches

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

// hacky way to ceil(): if there's a remainder, add 1
if rem.Cmp(big.NewInt(0)) > 0 {
result.Add(result, big.NewInt(1))
if !rem.IsZero() {
Copy link
Contributor Author

@2opremio 2opremio Dec 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In some implementations, the reminder can be negative. But I think that only happens with negative integers. We should double-check to be sure since !IsZero() may not be semantically equivalent.


// sanity check: disbursing shouldn't underflow the reserve
if disbursed >= reserveB {
return 0, false
}

// We do all of the math in bips, so it's all upscaled by this value.
maxBips := big.NewInt(10000)
f := new(big.Int).Sub(maxBips, F) // upscaled 1 - F
maxBips := uint256.NewInt(10000)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be worth having a global var/constant for maxBips

Comment on lines +151 to +152
result.Div(numer, denom)
rem.Mod(numer, denom)
Copy link
Contributor Author

@2opremio 2opremio Dec 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get why the library doesn't expose DivMod() since that's how it implements Div() and Mod() internally:

https://github.com/holiman/uint256/blob/38e2c56bf6d3ee96b9d391351a612336b0b552a5/uint256.go#L485

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created a ticket at holiman/uint256#103

@@ -115,8 +115,8 @@ func calculatePoolPayout(reserveA, reserveB, received xdr.Int64, feeBips xdr.Int
// divide & check overflow
result := numer.Div(numer, denom)

i := xdr.Int64(result.Int64())
return i, result.IsInt64() && i > 0
val := xdr.Int64(result.Uint64())
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder it it would be simpler and more performant to use Uint64WithOverflow()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(we would still need to check that the result is <= than math.MaxInt64 though)

@2opremio 2opremio changed the title exp/orderbook: Use uint128s for pool calculations to improve performance exp/orderbook: Use uint256s for pool calculations to improve performance Dec 3, 2021
@2opremio
Copy link
Contributor Author

2opremio commented Dec 3, 2021

@tamirms https://github.com/holiman/uint256 seems to have overflow/underflow-checking alternatives for all the operations.

I wonder if we should be using those, since, unlike https://lukechampine.com/uint128, https://github.com/holiman/uint256 doesn't seem to panic on overflows.

@2opremio
Copy link
Contributor Author

2opremio commented Dec 3, 2021

I wonder if we should be using those

After reading the code, it seems that the overflow-checking may be considerably less performant. So we should make sure to double-check performance if/when doing it.

@2opremio 2opremio deleted the uint128-orderbook-pools branch January 24, 2022 23:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants