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

feat(math): binary exp #175

Merged
merged 1 commit into from
Sep 11, 2023

Conversation

milancermak
Copy link
Contributor

Updating the pow impl to use binary exponentiation, which is cheaper.

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Build-related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

pow is O(n)

What is the new behavior?

pow is O(log n)

Does this introduce a breaking change?

  • Yes
  • No

Other information

I did a simple benchmark. Stepwise performance is the same for a simple case, the greater the exponent the more efficient this new implementation is.

Added a testcase to check for the whole 2^N range.

FWIW, it's pretty much the same as BitShift::fpow 😁 but without the use of Bitwise ops.

Copy link
Collaborator

@0xLucqs 0xLucqs left a comment

Choose a reason for hiding this comment

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

I think that we want to keep slow pow for small numbers

@milancermak
Copy link
Contributor Author

slow pow for small numbers

What do you mean?

@0xLucqs
Copy link
Collaborator

0xLucqs commented Sep 8, 2023

@enitrat Were you the one saying that the slow power is more efficient than fast power ?

@enitrat
Copy link
Collaborator

enitrat commented Sep 8, 2023

yes but I didn't run the benchmarks yet. Basically the overhead cost brought by additional div/rem opérations outweigh the gains for small exponents. I think the difference is also bigger when ran on u256s

edit: Actually @milancermak ran the benchmarks and it looks similar for small exponents as well!

@milancermak
Copy link
Contributor Author

Yeah, if the test runner can be trusted, the small exponents gas usage stays the same.

@0xLucqs
Copy link
Collaborator

0xLucqs commented Sep 11, 2023

ok you win let's merge it

@0xLucqs 0xLucqs merged commit e863e5b into keep-starknet-strange:main Sep 11, 2023
3 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Sep 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants