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.

Sign up for GitHub

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

fix(math): revert to correct version of ApproxRoot from a former state breaking change #17725

Merged
merged 11 commits into from
Sep 18, 2023
Merged

fix(math): revert to correct version of ApproxRoot from a former state breaking change #17725

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
ca09655
fix: state-break in ApproxRoot
p0mvn Sep 13, 2023
dd04717
changelog
p0mvn Sep 13, 2023
35f2110
updates
p0mvn Sep 13, 2023
800b3d4
update
p0mvn Sep 13, 2023
2fd310f
Revert "perf: optimize math.Int.Size for small values (#17497)"
p0mvn Sep 13, 2023
b2f982b
Revert "Revert "perf: optimize math.Int.Size for small values (#17497)""
p0mvn Sep 13, 2023
26e36e3
updates
p0mvn Sep 13, 2023
9a39ad9
avoid redundant reallocation
p0mvn Sep 15, 2023
4e85854
better changelog
p0mvn Sep 15, 2023
db87bda
Merge branch 'main' into roman/revert-approx-root-state-break
p0mvn Sep 16, 2023
f4dff23
updates
p0mvn Sep 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions math/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,10 @@ Ref: https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.j

## [Unreleased]

### Bug Fixes

* [#17725](https://github.com/cosmos/cosmos-sdk/pull/17725) Fix state break in ApproxRoot.

### Improvements

* [#17497](https://github.com/cosmos/cosmos-sdk/pull/17497) Optimize math.Int.Size for values that fit in 53 bits.
Expand Down
28 changes: 7 additions & 21 deletions math/dec.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -459,38 +459,24 @@ func (d LegacyDec) ApproxRoot(root uint64) (guess LegacyDec, err error) {
return absRoot.NegMut(), err
}

// One decimal, that we invalidate later. Helps us save a heap allocation.
scratchOneDec := LegacyOneDec()
if root == 1 || d.IsZero() || d.Equal(scratchOneDec) {
if root == 1 || d.IsZero() || d.Equal(LegacyOneDec()) {
return d, nil
}

if root == 0 {
return scratchOneDec, nil
return LegacyOneDec(), nil
}

guess, delta := scratchOneDec, LegacyOneDec()
smallestDec := LegacySmallestDec()
guess, delta := LegacyOneDec(), LegacyOneDec()

for iter := 0; delta.AbsMut().GT(smallestDec) && iter < maxApproxRootIterations; iter++ {
// Set prev = guess^{root - 1}, with an optimization for sqrt
// where root=2 => prev = guess. (And thus no extra heap allocations)
prev := guess
if root != 2 {
prev = guess.Power(root - 1)
}
for iter := 0; delta.Abs().GT(LegacySmallestDec()) && iter < maxApproxRootIterations; iter++ {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can start by cheaper comparisons firstly of just integers then onto the .Abs().GT so

for iter := 0; iter < maxApproxRootIterations && delta.Abs().GT(legacySmallestDec); iter++ {

and even more LegacySmallestDec() never changes so we can initiate it as a global so

var legacySmallestDec = LegacySmallestDec()

Copy link
Member Author

Choose a reason for hiding this comment

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

f4dff23

prev := guess.Power(root - 1)
if prev.IsZero() {
prev = smallestDec
prev = LegacySmallestDec()
}
delta.Set(d).QuoMut(prev)
delta.SubMut(guess)
// delta = delta / root.
// We optimize for sqrt, where root=2 => delta = delta >> 1
if root == 2 {
delta.i.Rsh(delta.i, 1)
} else {
delta.QuoInt64Mut(int64(root))
}
delta.QuoInt64Mut(int64(root))

guess.AddMut(delta)
}
Expand Down
1 change: 1 addition & 0 deletions math/dec_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -480,6 +480,7 @@ func (s *decimalTestSuite) TestApproxSqrt() {
math.LegacyNewDec(2).Power(127).Sub(math.LegacyOneDec()),
math.LegacyMustNewDecFromStr("13043817825332782212.349571806252508369"),
},
{math.LegacyMustNewDecFromStr("1.000000011823380862"), math.LegacyMustNewDecFromStr("1.000000005911690414")},
}

for i, tc := range testCases {
Expand Down