osmomath: Mutative version for QuoRoundUp #6437
Conversation
hieuvubk
requested review from
czarcas7ic,
p0mvn and
AlpinYukseloglu
as code owners
|
Important Notice This PR modifies an in-repo Go module. It is one of:
The dependent Go modules, especially the root one, will have to be Please follow the instructions below:
Please let us know if you need any help. |
hieuvubk
added
the
V:state/compatible/no_backport
hieuvubk
changed the title
|
I made some changes to reduce reallocations max, the new benchmark results: Diff sign: Same sign: |
Nice! Non-blocking tip: there is a tool called benchstat that helps to compare benchmark results. Check out: https://docs.osmosis.zone/osmosis-core/guides/performance |
osmomath/decimal.go
Outdated
| func chopPrecisionAndRoundUpMut(d *big.Int, precisionReuse *big.Int) *big.Int { | ||
| // remove the negative and add it back when returning | ||
| if d.Sign() == -1 { | ||
| // make d positive, compute chopped value, and then un-mutate d | ||
| d = d.Neg(d) | ||
| // truncate since d is negative... | ||
| d = chopPrecisionAndTruncateMut(d) | ||
| d = d.Neg(d) | ||
| return d | ||
| } | ||
|
|
||
| // get the truncated quotient and remainder | ||
| _, rem := d.QuoRem(d, precisionReuse, big.NewInt(0)) | ||
|
|
||
| if rem.Sign() == 0 { // remainder is zero | ||
| return d | ||
| } | ||
|
|
||
| return d.Add(d, oneInt) | ||
| } | ||
|
|
There was a problem hiding this comment.
How does this differ from chopPrecisionAndRoundUp? I'm not seeing any difference
There was a problem hiding this comment.
I'm using chopPrecisionAndTruncateMut that uses d as result. Also use d as QuoRem res instead of declaring new quo variable
There was a problem hiding this comment.
Thanks for the context. I've made commits directly to the branch with changes that were missing from approval.
What was added:
- remove the non-mutative chop version since we can now use the mutative one + copy the input
- covered non-mutative QuoRoundUp with a test
Commit: 0622c30
p0mvn
added
V:state/compatible/backport
* mutative version for QuoRoundUp * change log & go mod * actual mutative input * testing * no reuse global var zeroInt * update go.mod * reduce duplication and cover non-mutative with a test * update go.mod --------- Co-authored-by: roman <[email protected]> (cherry picked from commit b6db671) # Conflicts: # CHANGELOG.md # go.mod # go.sum # x/concentrated-liquidity/math/math.go
* osmomath: Mutative version for QuoRoundUp (backport #6437) * fix bug * go mod * go mod --------- Co-authored-by: roman <[email protected]>
Closes: #6370
What is the purpose of the change
Create new mutative version of
QuoRoundUpBenchmark
Testing and Verifying
(Please pick one of the following options)
This change is a trivial rework / code cleanup without any test coverage.
(or)
This change is already covered by existing tests, such as (please describe tests).
(or)
This change added tests and can be verified as follows:
(example:)
Documentation and Release Note
Unreleasedsection ofCHANGELOG.md?Where is the change documented?
x/{module}/README.md)