perf: Speedup sdk.Int overflow checks #19386
Conversation
Currently sdk.Int overflow checks compare against bigint.BitLen(), which is a complex method as it aims to be: - constant time - give granularity into how many bits are present in the final word. See: https://cs.opensource.google/go/go/+/refs/tags/go1.22.0:src/math/big/nat.go;l=654-674 We see this taking 1.1 seconds of the Osmosis epoch time! (Used to be 5+ seconds, until we moved more math to native bigInt math. Now its just 1.1 second in converting these final results into sdk.Int) This PR notices that we only need to check if its greater than 256 bits, which is equivalent to checking if it takes 4 words. Note in the code, we use bits.UintSize, so it works on 32 bit machines as well. So we change this to using int.Bits() which returns an []big.Word, with no copies, and hence will be much faster.
ValarDragon
changed the title
This comment has been minimized.
This comment has been minimized.
WalkthroughWalkthroughThis modification introduces a new constant, Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: .coderabbit.yml
Files selected for processing (1)
- math/int.go (10 hunks)
Additional comments: 8
math/int.go (8)
- 78-78: The overflow check using
len(i.Bits()) > maxWordLenis correct and improves performance by avoiding direct bit length comparisons. This change aligns with the PR's objective.- 135-135: The panic message in
NewIntFromBigInt()should be consistent with the one inNewIntFromBigIntMut()for clarity. Currently, both panic messages are identical, which is good for consistency.- 150-150: The use of
len(i.Bits()) > maxWordLenfor overflow detection inNewIntFromBigIntMut()is consistent with the approach inNewIntFromBigInt(). This maintains uniformity in handling big integers.- 164-164: The overflow check in
NewIntFromString()usinglen(i.Bits()) > maxWordLenis correctly implemented. It ensures that the integer does not exceed the maximum word length, aligning with the optimization goal.- 182-182: The overflow check in
NewIntWithDecimal()is correctly implemented usinglen(i.Bits()) > maxWordLen. This ensures that the resulting integer from the decimal conversion does not exceed the maximum word length.- 292-292: The overflow check in
SafeAdd()usinglen(res.i.Bits()) > maxWordLenis correctly implemented. This ensures that the addition operation does not result in an integer exceeding the maximum word length.- 317-317: The overflow/underflow check in
SafeSub()usinglen(res.i.Bits()) > maxWordLenis correctly implemented. This ensures that the subtraction operation does not result in an integer exceeding the maximum word length.- 342-342: The overflow check in
SafeMul()usinglen(res.i.Bits()) > maxWordLenis correctly implemented. This ensures that the multiplication operation does not result in an integer exceeding the maximum word length.
math/int.go
Outdated
| // maxWordLen defines the maximum word length supported by Int and Uint types. | ||
| // Note: This can only be used in checks if MaxBitLen % 64 == 0 | ||
| const maxWordLen = MaxBitLen / 64 |
There was a problem hiding this comment.
Ensure the assumption MaxBitLen % 64 == 0 is always valid. If MaxBitLen could change in the future, consider adding a compile-time assertion or runtime check to enforce this constraint.
|
I added a benchmark to show the speed difference on the existing table of small values (<= 78 bits). I expect this difference becomes more pronounced on the real Osmosis data set. Its an ~25% speedup, to this step which is part of every Int operation. Before: After: |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (3)
- math/CHANGELOG.md (1 hunks)
- math/int.go (12 hunks)
- math/int_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- math/int.go
Additional comments: 2
math/CHANGELOG.md (1)
- 45-45: LGTM!
math/int_test.go (1)
- 691-711: LGTM!
|
TYSM! I also learned this morning, that its impossible for the top word of a Nat to have 0's, so we could omit the fallback check -- but it doesn't really matter since its rarely called. |
This PR speedsup all sdk.Int math ops, especially for smaller Int arithmetic.
Currently sdk.Int overflow checks compare against bigint.BitLen(), which is a complex method as it aims to be:
We see this taking 1.1 seconds of the Osmosis epoch time! (Used to be 5+ seconds, until we moved more math to native bigInt math. Now its just 1.1 second in converting these final results into sdk.Int)
This PR notices that we only need to check if its greater than 256 bits, which is equivalent to checking if it takes 4 words. Note in the code, we use bits.UintSize, so it works on 32 bit machines as well.
So we change this to using int.Bits() which returns an []big.Word, with no copies, and hence will be much faster.
Notice that here in the tests we test the precise overflow bounds: https://github.com/cosmos/cosmos-sdk/blob/main/math/int_test.go#L175-L185
therefore this should be state compatible. (And I tested it with the tests under this)
Out of some abundance of caution, its possible that the most significant word could be all zeroes. To handle this edge case, we say that if the bigint uses too many words, then run the fallback (slower) BitLen logic, thus ensuring it will always be compatible.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
Summary by CodeRabbit