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

Perf epns #280

Merged
merged 4 commits into from
Jan 1, 2025
Merged

Perf epns #280

merged 4 commits into from
Jan 1, 2025

Conversation

shramee
Copy link
Contributor

@shramee shramee commented Jan 1, 2025

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
  • Testing
  • Other (please describe):

What are the changes in the behavior?

  1. Number of affected tests: 63
  2. The largest improvements were in the neg_3 utility tests, with nearly 40% reduction in steps
  3. BLS12_381 MSM (Multi-Scalar Multiplication) tests showed consistent improvements around 33%
  4. Average improvement across all changed tests: 23.77%

Top 5 most improved tests:

  1. garaga::utils::neg_3::tests::test_scalar_to_epns:

    • Change: 16,622 steps -> 9,986 steps
    • Improvement: 39.92%
  2. garaga::utils::neg_3::tests::test_scalar_to_epns_single:

    • Change: 5,326 steps -> 3,209 steps
    • Improvement: 39.75%
  3. garaga::tests::msm_tests::msm_tests::test_msm_BLS12_381_12P:

    • Change: 138,245 steps -> 92,206 steps
    • Improvement: 33.30%
  4. garaga::tests::msm_tests::msm_tests::test_msm_BLS12_381_11P:

    • Change: 126,070 steps -> 84,131 steps
    • Improvement: 33.27%
  5. garaga::tests::msm_tests::msm_tests::test_msm_BLS12_381_10P:

    • Change: 113,502 steps -> 75,813 steps
    • Improvement: 33.21%

Does this introduce a breaking change?

  • Yes
  • No

Other information

Might be nicer as separate commit for separation of optims.

@shramee shramee requested a review from feltroidprime as a code owner January 1, 2025 10:14
@feltroidprime
Copy link
Collaborator

feltroidprime commented Jan 1, 2025

For the failing E2e test, simply replace the class hash you see in the error in the CI here https://github.com/keep-starknet-strange/garaga/actions/runs/12569635626/job/35038768468?pr=280#step:8:81 which has changed as result of the neg_3 changes.

(you can run it locally replicating

- name: Install dependencies
run: make setup
- name: Install devnet
run: ./tests/contracts_e2e/install_devnet.sh
- name: Run tests
run: source venv/bin/activate && pytest -s -x tests/contracts_e2e/e2e_test.py
)

ECIP_OPS_CLASS_HASH = 0x684D2756A4440C190A5FE54E367C0ABE33AEFA75084DEC2FFFC791B620C80E3

@feltroidprime
Copy link
Collaborator

feltroidprime commented Jan 1, 2025

Also dont forget to do a make rewrite after changing the class hash and commit to the changes

@shramee
Copy link
Contributor Author

shramee commented Jan 1, 2025

I'm getting some errors doing make rewrite

(venv) ➜  garaga git:(perf-epns) ✗ make rewrite
./tools/make/rewrite.sh
Traceback (most recent call last):
  File "/Users/shramee/www/starknet/garaga/hydra/garaga/starknet/honk_contract_generator/generator_honk.py", line 5, in <module>
    from garaga.definitions import CurveID
ModuleNotFoundError: No module named 'garaga'
Error in generator_honk.py
make: *** [rewrite] Error 1

⚠️ But I've updated the classhashes at other places reflexively.

@feltroidprime
Copy link
Collaborator

feltroidprime commented Jan 1, 2025

I'm getting some errors doing make rewrite

(venv) ➜  garaga git:(perf-epns) ✗ make rewrite
./tools/make/rewrite.sh
Traceback (most recent call last):
  File "/Users/shramee/www/starknet/garaga/hydra/garaga/starknet/honk_contract_generator/generator_honk.py", line 5, in <module>
    from garaga.definitions import CurveID
ModuleNotFoundError: No module named 'garaga'
Error in generator_honk.py
make: *** [rewrite] Error 1

⚠️ But I've updated the classhashes at other places reflexively.

I suggest to rm the venv/ folder, do a make setup again and scan for errors there if any, then activate the venv and retry make rewrite

@feltroidprime
Copy link
Collaborator

feltroidprime commented Jan 1, 2025

For the last error we need to replace

pub const RISC_ZERO_VERIFIER_CLASS_HASH: felt252 =
0x7f3157e83dc0d636c462c77fd8f309351b4a2c710f9a23a44305830a29b0b27;
by the new class hash of the risc0 verifier : 0x11c3cd9b36eb302d4b5a27a79f78d065c783995000cfe571f60408d11708dc4 https://github.com/keep-starknet-strange/garaga/actions/runs/12569877475/job/35039093457?pr=280#step:8:337

and it should be good

its a bit annoying but at least everything is in sync

Thank you

@shramee
Copy link
Contributor Author

shramee commented Jan 1, 2025

Thanks!
Done. make setup thing didn't wanna work for me. My Python c bindings are probably fucked up.

@feltroidprime feltroidprime merged commit 0356bb3 into keep-starknet-strange:main Jan 1, 2025
27 checks passed
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.

2 participants