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.

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

PowApprox mutable internal logic #324

Merged
merged 5 commits into from
Jun 23, 2021
Merged

Conversation

mconcat
Copy link
Collaborator

@mconcat mconcat commented Jun 16, 2021

closes: #56

term = term.Mul(c.Mul(x))
term = term.Quo(bigK)
for i := int64(1); term.GTE(precision); i++ {
a.Set(exp)
Copy link
Member

Choose a reason for hiding this comment

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

whats going on with a.Set(exp) and bigK. These don't seem the same across the update

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I thought absDifferenceWithSign was mutable but it isn't, will fix this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually we could gain speed if we make abs mutable.

@mconcat
Copy link
Collaborator Author

mconcat commented Jun 16, 2021

mconcat/56-powapprox-opt

goos: darwin
goarch: amd64
pkg: github.com/osmosis-labs/osmosis/x/gamm/keeper
cpu: Intel(R) Core(TM) i7-8750H CPU @ 2.20GHz
BenchmarkPow                  33          37631000 ns/op         4247544 B/op     191873 allocs/op
BenchmarkPow-2                32          35742374 ns/op         4247695 B/op     191875 allocs/op
BenchmarkPow-4                33          37513079 ns/op         4247849 B/op     191875 allocs/op
BenchmarkPow-8                28          36158232 ns/op         4248223 B/op     191878 allocs/op
BenchmarkPow-16               32          35642177 ns/op         4248668 B/op     191878 allocs/op
BenchmarkSqrtPow           95598             12724 ns/op            3624 B/op        199 allocs/op
BenchmarkSqrtPow-2        101456             11737 ns/op            3624 B/op        199 allocs/op
BenchmarkSqrtPow-4         90078             11598 ns/op            3624 B/op        199 allocs/op
BenchmarkSqrtPow-8        102190             11701 ns/op            3624 B/op        199 allocs/op
BenchmarkSqrtPow-16       101715             11598 ns/op            3624 B/op        199 allocs/op

main

goos: darwin
goarch: amd64
pkg: github.com/osmosis-labs/osmosis/x/gamm/keeper
cpu: Intel(R) Core(TM) i7-8750H CPU @ 2.20GHz
BenchmarkPow                  20          67177389 ns/op        33736621 B/op     970000 allocs/op
BenchmarkPow-2                19          59532866 ns/op        33737600 B/op     970008 allocs/op
BenchmarkPow-4                19          57907179 ns/op        33739167 B/op     970019 allocs/op
BenchmarkPow-8                20          58669216 ns/op        33741359 B/op     970028 allocs/op
BenchmarkPow-16               19          58099349 ns/op        33745091 B/op     970036 allocs/op
BenchmarkSqrtPow           64537             16292 ns/op            7384 B/op        309 allocs/op
BenchmarkSqrtPow-2         80757             14496 ns/op            7384 B/op        309 allocs/op
BenchmarkSqrtPow-4         78423             15511 ns/op            7384 B/op        309 allocs/op
BenchmarkSqrtPow-8         78834             14515 ns/op            7384 B/op        309 allocs/op
BenchmarkSqrtPow-16        83053             14379 ns/op            7384 B/op        309 allocs/op

@ValarDragon
Copy link
Member

Awesome, so it is like a 2x speed improvement!(Also the thing with heap improvements is that they are more impactful as he overall memory situation gets worse.

Lets sync with Sunny whether this gets merged today, or like this weekend in a version that just makes nodes faster

@codecov-commenter
Copy link

codecov-commenter commented Jun 22, 2021

Codecov Report

Merging #324 (e4bbe29) into main (91887b6) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #324   +/-   ##
=======================================
  Coverage   18.28%   18.28%           
=======================================
  Files         133      133           
  Lines       21471    21473    +2     
=======================================
+ Hits         3925     3927    +2     
  Misses      16891    16891           
  Partials      655      655           
Impacted Files Coverage Δ
x/gamm/keeper/math.go 92.72% <100.00%> (+0.13%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 91887b6...e4bbe29. Read the comment docs.

@ValarDragon ValarDragon merged commit e78fd3c into main Jun 23, 2021
@ValarDragon ValarDragon deleted the mconcat/56-powapprox-opt branch June 23, 2021 00:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Optimize powApprox
3 participants