-
Notifications
You must be signed in to change notification settings - Fork 499
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
exp/orderbook: Use uint256s for pool calculations to improve performance #4113
Conversation
1d4b53f
to
6d961d6
Compare
6d961d6
to
10c56f5
Compare
I tried comparing the uint128 calculations with the old code which uses math/big ones:
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:
After:
|
b2ed4a1
to
65d0615
Compare
65d0615
to
a0aa6cc
Compare
4b94728
to
bcbf7ca
Compare
There was a problem hiding this 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
|
||
// 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() { |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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
result.Div(numer, denom) | ||
rem.Mod(numer, denom) |
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
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()) |
There was a problem hiding this comment.
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()
There was a problem hiding this comment.
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)
@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. |
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. |
Second attempt at #4104 by explicitly checking every possible overflow.
Before:
After: