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

Optimize sqrts #2029

Closed
wants to merge 2 commits into from
Closed

Optimize sqrts #2029

wants to merge 2 commits into from

Conversation

chipshort
Copy link
Collaborator

@chipshort chipshort commented Feb 23, 2024

closes #1897

  • The upper bound for isqrt can be lowered using ilog2.
  • The max precision for the Decimal::sqrt can also be calculated directly using ilog10

TODOs:

  • use better bound for isqrt
  • calculate max precision for Decimal::sqrt (I added a section to the HackMD)
  • poove that the max precision calculated here actually is the maximum. I ran some tests and didn't find any counter-examples and the resulting function also looks like this is the case, but given all the intermediate rounding it is not obvious and it would be good to have a proof.
  • benchmark the number of wasm instructions it takes. Some notes:
    • Rust std's ilog10 looks very efficient (and crazy), but no idea how good bnum's implementation is
    • even with an efficient log, it might still be faster to try the checked math

@chipshort
Copy link
Collaborator Author

Benchmark results for the new Decimal sqrt look very lackluster.

NB: There is some overhead here, since I call the code inside instantiate and use cosmwasm-vm to measure the gas of that, so it overcounts a bit, but by a similar amount for each of them

sqrt(MAX)

Gas 128-bit 256-bit
old 6770250 12130050
new 5896950 10237050

=> 15% less gas

sqrt(1.0)

Gas 128-bit 256-bit
old 6106200 7081950
new 6512250 11291550

=> 6% more gas for Decimal::one() and 60% (!) more gas for Decimal256::one()

I'll adjust my benchmark code to find the break-even point, so we can see if adding a check for that would improve the gas usage.

@webmaster128
Copy link
Member

Thank you for the benchmarking work! Even in the best case those 15% are not exactly a great improvement. Maybe we should just ditch this idea.

@chipshort
Copy link
Collaborator Author

Yes, that's also a good point. I doubt many people take square roots of such big numbers.
Let's ditch it then. Figuring out the math was quite fun, though 😁

@chipshort chipshort closed this Mar 11, 2024
@webmaster128
Copy link
Member

I was thinking of doing binary search in the range [0, 9] instead of going one by one. This might be more efficient. But let's put that in the nice-to-have bucket.

This was referenced Apr 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Optimize sqrt
2 participants