perf(math): Significantly speedup Dec quo truncate and quo Roundup #20034
Conversation
WalkthroughWalkthroughThe recent updates focus on optimizing decimal operations by refining precision multiplication and rounding logic. Benchmark tests have also been enhanced for better accuracy and independence of calculations. Changes
Recent Review DetailsConfiguration used: .coderabbit.yml Files selected for processing (1)
Additional Context UsedPath-based Instructions (1)
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 (
|
This comment has been minimized.
This comment has been minimized.
ValarDragon
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| @@ -36,6 +36,8 @@ Ref: https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.j | |||
|
|
|||
| ## [Unreleased] | |||
|
|
|||
| * [#20034](https://github.com/cosmos/cosmos-sdk/pull/20034) Significantly speedup LegacyDec.QuoTruncate and LegacyDec.QuoRoundUp. | |||
There was a problem hiding this comment.
Correct the spelling of "speedup" to "speed up" as it should be two words when used as a verb.
- Significantly speedup LegacyDec.QuoTruncate and LegacyDec.QuoRoundUp.
+ Significantly speed up LegacyDec.QuoTruncate and LegacyDec.QuoRoundUp.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| * [#20034](https://github.com/cosmos/cosmos-sdk/pull/20034) Significantly speedup LegacyDec.QuoTruncate and LegacyDec.QuoRoundUp. | |
| * [#20034](https://github.com/cosmos/cosmos-sdk/pull/20034) Significantly speed up LegacyDec.QuoTruncate and LegacyDec.QuoRoundUp. |
Add a space after the period to separate the sentences properly.
- Significantly speed up LegacyDec.QuoTruncate and LegacyDec.QuoRoundUp.## [math/v1.3.0](https://github.com/cosmos/cosmos-sdk/releases/tag/math/v1.3.0) - 2024-02-22
+ Significantly speed up LegacyDec.QuoTruncate and LegacyDec.QuoRoundUp. ## [math/v1.3.0](https://github.com/cosmos/cosmos-sdk/releases/tag/math/v1.3.0) - 2024-02-22Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| * [#20034](https://github.com/cosmos/cosmos-sdk/pull/20034) Significantly speedup LegacyDec.QuoTruncate and LegacyDec.QuoRoundUp. | |
| * [#20034](https://github.com/cosmos/cosmos-sdk/pull/20034) Significantly speed up LegacyDec.QuoTruncate and LegacyDec.QuoRoundUp. ## [math/v1.3.0](https://github.com/cosmos/cosmos-sdk/releases/tag/math/v1.3.0) - 2024-02-22 |
julienrbrt
changed the title
| // multiply precision twice | ||
| d.i.Mul(d.i, squaredPrecisionReuse) | ||
| // multiply precision once | ||
| d.i.Mul(d.i, precisionReuse) |
There was a problem hiding this comment.
Although it was using higher precision before, the additional numbers where just cropped. Good finding!
| @@ -418,10 +417,12 @@ func (d LegacyDec) QuoRoundUp(d2 LegacyDec) LegacyDec { | |||
| // QuoRoundupMut mutable quotient, round up | |||
| func (d LegacyDec) QuoRoundupMut(d2 LegacyDec) LegacyDec { | |||
| // multiply precision twice | |||
| d.i.Mul(d.i, squaredPrecisionReuse) | |||
| d.i.Quo(d.i, d2.i) | |||
| d.i.Mul(d.i, precisionReuse) | |||
There was a problem hiding this comment.
nit: the code comment is not correct anymore
| d.i.Mul(d.i, precisionReuse) | ||
| _, rem := d.i.QuoRem(d.i, d2.i, big.NewInt(0)) | ||
| if rem.Sign() != 0 { | ||
| d.i.Add(d.i, oneInt) |
There was a problem hiding this comment.
This is not correct for negative numbers. Instead you can do the following to achieve the same behaviour as before:
if rem.Sign() > 0 && d.IsNegative() == d2.IsNegative() ||
rem.Sign() < 0 && d.IsNegative() != d2.IsNegative() {A test would be good to ensure this for regression. I wrote this for comparing with the old logic: https://gist.github.com/alpe/3eee84356d93b37be3e0e798c1c1464c
There was a problem hiding this comment.
Ah thank you, great catch!
| b2 := math.LegacyNewDec(4371) | ||
| b.ReportAllocs() | ||
| b.ResetTimer() | ||
| for i := 0; i < b.N; i++ { | ||
| sink = b1.QuoTruncateMut(b2) | ||
| sink = baseArr[i].QuoTruncateMut(b2) |
There was a problem hiding this comment.
good fix. Do you want to add more number examples to support your cases? You could run them via
b.Run("", func(b *testing.B) {}) like table tests
|
Good catch for negative numbers! |
|
gentle ping @ValarDragon |
Significantly speedsup Dec.QuoTruncate and Dec.QuoRoundUp.
This is state compatible, as Quo is always floor division. note that QuoRem is ~no overhead relative to Quo in the big.Int implementation for large words (which all Dec's in practice are) We have tested state compatability for this optimization on Osmosis' BigDec, which is the same implementation.
The speedup shown in the benchmark uses small values, the impact is much larger at larger values. (So a benchmark with bigger numbers would boast better speeds) (I also corrected the benchmark, before it would just be doing 0/b2 in the benchmark).
Performance on this small input (again impact much higher at real numbers we see) is already 33% improvement:
OLD perf on new benchmark code:
NEW:
Correctness of this is covered by existing tests. (And tested on Osmosis mainnet syncs for BigDecimal)
Summary by CodeRabbit