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: fast const pow #336

Merged
merged 5 commits into from
Oct 15, 2024

Conversation

Th0rgal
Copy link
Contributor

@Th0rgal Th0rgal commented Oct 14, 2024

Implement efficient pow2 and pow10 using const arrays

Pull Request type

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?

Currently, the implementation of pow2 and pow10 functions uses if/else branches, which is less efficient than using lookup tables.

Issue Number: N/A

What is the new behavior?

  • Implement pow2 and pow10 using const arrays for efficient lookup
  • Update keccak code to use the new pow2 implementation
  • Significant gas usage reduction (e.g., keccak test gas usage reduced from 2,399,388 to 1,289,988)

Does this introduce a breaking change?

  • Yes
  • No

Other information

This PR takes advantage of Cairo 2.7.0's reintroduction of "dw like hardcoded function results" via const arrays to implement efficient lookup tables for pow2 and pow10. These functions are commonly used in various projects, including Alexandria for bitshifting and in DeFi applications for handling ERC20 decimals. I introduced a u128 version (should be the one used in most usecases), and a u256 version for both pow2 and pow10. The u256 version of pow2 uses the u128 one.

The new implementation significantly reduces gas usage. For example, I updated keccak implementation to use this new function and the test_bytes_keccak test saw a reduction in estimated gas usage from 2,399,388 to 1,289,988.

closes #323

Copy link
Collaborator

@gaetbout gaetbout left a comment

Choose a reason for hiding this comment

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

I see some places where pow is used pow(2, ...) could you also update those?
Same for pow(10, ...)

@Th0rgal
Copy link
Contributor Author

Th0rgal commented Oct 14, 2024

I see some places where pow is used pow(2, ...) could you also update those? Same for pow(10, ...)

I can do it but I am not sure it makes sense because pow(2, ...) uses a generic, so it returns T. If I try to replace its usages by pow2 I will usually have to .try_into().unwrap() which implies a range_check which could be avoided by making a variant of pow2 for each type, but it's probably better to wait for a feature of the language allowing us to do it automatically. What do you prefer?

@gaetbout
Copy link
Collaborator

@Th0rgal Guess it is case specific then 🤔 , do as you believe is the best 😄

@Th0rgal
Copy link
Contributor Author

Th0rgal commented Oct 15, 2024

Okay, here is a summary:

  1. I updated traverse from storage_proof. Initially I used pow2_u256 but we got only about ~8% steps decrease vs ~15% using a new pow2_felt252 which could be useful in other usecases.

  2. I fixed the comment of count_digits_of_base which said it returned a felt252 while it returned a u128 and I returned a u32.

  3. I updated karatsuba to use pow10 (and adapted the typings to use u32 when possible)

  4. I updated armstrong_numbers to use u32 when possible (implied by previous change).

  5. We use pow(2, x) in the tests, I think that's fine.

  6. I look into BitShift traits, we could update them so they take a u32 n. We should be able to use pow2 (u128 version) without additional range_check on them because we do a & MAX and then try_into() after. Should I do it or do you want to keep the exact type? I think it would cause a lot of breaking changes.

  7. Reimplementing rotate_right: again we should take a u32 n but here we will have to reimplement pow2 for each binary type except u128. I would suggest to wait for a way to do it automatically.

@gaetbout
Copy link
Collaborator

Any idea why the root lock file is changed?

@Th0rgal
Copy link
Contributor Author

Th0rgal commented Oct 15, 2024

Any idea why the root lock file is changed?

Yes, I added math dependency to alexandria_merkle:

[dependencies]
alexandria_math = { path = "../math" }

I thought it would be cleaner than copy pasting the code, but I can do it if you prefer.

@gaetbout
Copy link
Collaborator

Oh wow... My bad on that one, missed that toml file 😅

@gaetbout gaetbout enabled auto-merge (squash) October 15, 2024 12:14
@gaetbout gaetbout merged commit 0adfaab into keep-starknet-strange:main Oct 15, 2024
3 of 4 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Oct 17, 2024
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.

feat: add pow2 and pow10 utilizing const arrays
2 participants