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

dev: move pow to constants #332

Merged
merged 4 commits into from
Sep 12, 2023
Merged

dev: move pow to constants #332

merged 4 commits into from
Sep 12, 2023

Conversation

lambda-0x
Copy link
Contributor

@lambda-0x lambda-0x commented Sep 8, 2023

Pull Request type

Please check the type of change your PR introduces:

  • Refactoring (no functional changes, no API changes)

What is the current behavior?

Resolves: #185

What is the new behavior?

  • move all variables whose value can be calculated at compile time to contants
  • move couple constant variables from math to constants module

Does this introduce a breaking change?

  • No

crates/evm/src/memory.cairo Outdated Show resolved Hide resolved
crates/utils/src/u256_signed_math.cairo Show resolved Hide resolved
@lambda-0x lambda-0x marked this pull request as ready for review September 9, 2023 14:10
@lambda-0x
Copy link
Contributor Author

i skimmed through all the modules, lmk if i am missing any.

Copy link
Collaborator

@enitrat enitrat left a comment

Choose a reason for hiding this comment

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

Great thanks 🔥 just a few comments

crates/evm/src/memory.cairo Outdated Show resolved Hide resolved
crates/utils/src/constants.cairo Outdated Show resolved Hide resolved
crates/utils/src/u256_signed_math.cairo Show resolved Hide resolved
@enitrat
Copy link
Collaborator

enitrat commented Sep 11, 2023

Just for reference, here are gas costs changes:

****IMPROVEMENTS****
evm::tests::test_instructions::test_environment_information::test_codecopy_basic 2741246 --> 2141666 | -21.87 %
evm::tests::test_instructions::test_environment_information::test_codecopy_type_conversion_error 619720 --> 619700 | -0.00 %
evm::tests::test_instructions::test_environment_information::test_codecopy_with_offset 2628446 --> 2028866 | -22.81 %
evm::tests::test_instructions::test_environment_information::test_codecopy_with_out_of_bound_bytes 2962516 --> 2362936 | -20.24 %
evm::tests::test_instructions::test_environment_information::test_codecopy_with_out_of_bound_offset 2006756 --> 1992836 | -0.69 %
evm::tests::test_instructions::test_environment_information::test_returndata_copy_basic 5148972 --> 4724522 | -8.24 %
evm::tests::test_instructions::test_environment_information::test_returndata_copy_overflowing_add_error 578940 --> 574180 | -0.82 %
evm::tests::test_instructions::test_environment_information::test_returndata_copy_type_conversion_error 414980 --> 414970 | -0.00 %
evm::tests::test_instructions::test_environment_information::test_returndata_copy_with_multiple_words 5005422 --> 4580972 | -8.48 %
evm::tests::test_instructions::test_environment_information::test_returndata_copy_with_offset 5053272 --> 4628822 | -8.40 %
evm::tests::test_instructions::test_environment_information::test_returndata_copy_with_out_of_bound_bytes 578940 --> 574180 | -0.82 %
evm::tests::test_instructions::test_memory_operations::test_exec_mload_should_load_a_value_from_memory 505420 --> 496020 | -1.86 %
evm::tests::test_instructions::test_memory_operations::test_exec_mload_should_load_a_value_from_memory_with_memory_expansion 802300 --> 500070 | -37.67 %
evm::tests::test_instructions::test_memory_operations::test_exec_mload_should_load_a_value_from_memory_with_offset_larger_than_msize 806350 --> 504120 | -37.48 %
evm::tests::test_instructions::test_memory_operations::test_exec_mstore8_should_store_last_uint8_offset_31 635534 --> 630724 | -0.76 %
evm::tests::test_instructions::test_memory_operations::test_exec_mstore8_should_store_last_uint8_offset_63 928364 --> 630724 | -32.06 %
evm::tests::test_instructions::test_memory_operations::test_exec_mstore8_should_store_uint8_offset_30 635534 --> 630724 | -0.76 %
evm::tests::test_instructions::test_memory_operations::test_exec_mstore8_should_store_uint8_offset_31 635534 --> 630724 | -0.76 %
evm::tests::test_instructions::test_memory_operations::test_exec_mstore8_should_store_uint8_offset_31_then_uint8_offset_30 1022258 --> 1017438 | -0.47 %
evm::tests::test_instructions::test_memory_operations::test_exec_mstore_should_store_max_uint256_offset_0 490030 --> 480930 | -1.86 %
evm::tests::test_instructions::test_memory_operations::test_exec_mstore_should_store_max_uint256_offset_1 809300 --> 468780 | -42.08 %
evm::tests::test_memory::test__ensure_length__should_return_expanded_memory_and_cost 2449916 --> 2445106 | -0.20 %
evm::tests::test_memory::test__ensure_length__should_return_the_same_memory_and_no_cost 2449646 --> 2444836 | -0.20 %
evm::tests::test_memory::test__expand__should_return_expanded_memory_and_cost 2446246 --> 2441436 | -0.20 %
evm::tests::test_memory::test__expand__should_return_the_same_memory_and_no_cost 2445976 --> 2441166 | -0.20 %
evm::tests::test_memory::test__expand_and_load__should_return_expanded_memory_and_element_and_cost 3289776 --> 2689606 | -18.24 %
evm::tests::test_memory::test__load__should_load_an_element_from_the_memory 5473032 --> 4872952 | -10.96 %
evm::tests::test_memory::test__load__should_load_an_element_from_the_memory_with_offset_1 4988792 --> 4642922 | -6.93 %
evm::tests::test_memory::test__load__should_load_an_element_from_the_memory_with_offset_2 4984252 --> 4642922 | -6.85 %
evm::tests::test_memory::test__load__should_load_an_element_from_the_memory_with_offset_3 4990882 --> 4649552 | -6.84 %
evm::tests::test_memory::test__load__should_load_an_element_from_the_memory_with_offset_4 4967692 --> 4653602 | -6.32 %
evm::tests::test_memory::test__load__should_load_an_element_from_the_memory_with_offset_5 4987572 --> 4650022 | -6.77 %
evm::tests::test_memory::test__load__should_load_an_element_from_the_memory_with_offset_6 4944092 --> 4646442 | -6.02 %
evm::tests::test_memory::test_load_n_internal_same_word 1029570 --> 1025170 | -0.43 %
evm::tests::test_memory::test_store_n_no_aligned_words 350330 --> 350320 | -0.00 %
evm::tests::test_memory::test_store_should_add_an_element_to_the_memory 207590 --> 203090 | -2.17 %
evm::tests::test_memory::test_store_should_add_an_element_with_offset_to_the_memory 238080 --> 194990 | -18.10 %
evm::tests::test_memory::test_store_should_add_n_elements_to_the_memory 2263836 --> 2263826 | -0.00 %


****WORSENED****
evm::tests::test_memory::test_store_n_2_aligned_words 3018220 --> 3572750 18.37 %
Overall gas change: performance improvement, gas consumption-2.59 %

As you can see changing the type of aligned store elements to u256 when performing the computation has a BIG overhead cost

@lambda-0x
Copy link
Contributor Author

(rebased on main)

@lambda-0x
Copy link
Contributor Author

@enitrat can you retry measuring gas usage? I tried using compare_snapshot.py but it depends on github CI things so cannot do it locally. I think it would be a good idea to make it work locally as well.

@enitrat
Copy link
Collaborator

enitrat commented Sep 11, 2023

@lambda-0x As long as you have a github token set in the dotenv file it should work locally

@lambda-0x
Copy link
Contributor Author

****IMPROVEMENTS****
evm::tests::test_instructions::test_environment_information::test_calldatacopy_basic 2057050 --> 1744440 | -15.20 %
evm::tests::test_instructions::test_environment_information::test_calldatacopy_type_conversion_error 620250 --> 620230 | -0.00 %
evm::tests::test_instructions::test_environment_information::test_calldatacopy_with_offset 1905190 --> 1592580 | -16.41 %
evm::tests::test_instructions::test_environment_information::test_calldatacopy_with_out_of_bound_bytes 2572770 --> 2260160 | -12.15 %
evm::tests::test_instructions::test_environment_information::test_calldatacopy_with_out_of_bound_bytes_multiple_words 5537070 --> 4816810 | -13.01 %
evm::tests::test_instructions::test_environment_information::test_codecopy_basic 2742446 --> 2142266 | -21.88 %
evm::tests::test_instructions::test_environment_information::test_codecopy_type_conversion_error 620320 --> 620300 | -0.00 %
evm::tests::test_instructions::test_environment_information::test_codecopy_with_offset 2629646 --> 2029466 | -22.82 %
evm::tests::test_instructions::test_environment_information::test_codecopy_with_out_of_bound_bytes 2963716 --> 2363536 | -20.25 %
evm::tests::test_instructions::test_environment_information::test_codecopy_with_out_of_bound_offset 2007956 --> 1993436 | -0.72 %
evm::tests::test_instructions::test_environment_information::test_returndata_copy_basic 5150172 --> 4148102 | -19.46 %
evm::tests::test_instructions::test_environment_information::test_returndata_copy_overflowing_add_error 580140 --> 575380 | -0.82 %
evm::tests::test_instructions::test_environment_information::test_returndata_copy_type_conversion_error 415580 --> 415570 | -0.00 %
evm::tests::test_instructions::test_environment_information::test_returndata_copy_with_multiple_words 5006622 --> 4004552 | -20.01 %
evm::tests::test_instructions::test_environment_information::test_returndata_copy_with_offset 5054472 --> 4052402 | -19.83 %
evm::tests::test_instructions::test_environment_information::test_returndata_copy_with_out_of_bound_bytes 580140 --> 575380 | -0.82 %
evm::tests::test_instructions::test_memory_operations::test_exec_mload_should_load_a_value_from_memory 506020 --> 496020 | -1.98 %
evm::tests::test_instructions::test_memory_operations::test_exec_mload_should_load_a_value_from_memory_with_memory_expansion 802900 --> 500070 | -37.72 %
evm::tests::test_instructions::test_memory_operations::test_exec_mload_should_load_a_value_from_memory_with_offset_larger_than_msize 806950 --> 504120 | -37.53 %
evm::tests::test_instructions::test_memory_operations::test_exec_mstore8_should_store_last_uint8_offset_31 635934 --> 631124 | -0.76 %
evm::tests::test_instructions::test_memory_operations::test_exec_mstore8_should_store_last_uint8_offset_63 928764 --> 631124 | -32.05 %
evm::tests::test_instructions::test_memory_operations::test_exec_mstore8_should_store_uint8_offset_30 635934 --> 631124 | -0.76 %
evm::tests::test_instructions::test_memory_operations::test_exec_mstore8_should_store_uint8_offset_31 635934 --> 631124 | -0.76 %
evm::tests::test_instructions::test_memory_operations::test_exec_mstore8_should_store_uint8_offset_31_then_uint8_offset_30 1023058 --> 1018238 | -0.47 %
evm::tests::test_instructions::test_memory_operations::test_exec_mstore_should_store_max_uint256_offset_0 490430 --> 480730 | -1.98 %
evm::tests::test_instructions::test_memory_operations::test_exec_mstore_should_store_max_uint256_offset_1 809700 --> 468580 | -42.13 %
evm::tests::test_memory::test__ensure_length__should_return_expanded_memory_and_cost 2449916 --> 2445106 | -0.20 %
evm::tests::test_memory::test__ensure_length__should_return_the_same_memory_and_no_cost 2449646 --> 2444836 | -0.20 %
evm::tests::test_memory::test__expand__should_return_expanded_memory_and_cost 2446246 --> 2441436 | -0.20 %
evm::tests::test_memory::test__expand__should_return_the_same_memory_and_no_cost 2445976 --> 2441166 | -0.20 %
evm::tests::test_memory::test__expand_and_load__should_return_expanded_memory_and_element_and_cost 3289776 --> 2689606 | -18.24 %
evm::tests::test_memory::test__load__should_load_an_element_from_the_memory 5473032 --> 4872952 | -10.96 %
evm::tests::test_memory::test__load__should_load_an_element_from_the_memory_with_offset_1 4988792 --> 4640922 | -6.97 %
evm::tests::test_memory::test__load__should_load_an_element_from_the_memory_with_offset_2 4984252 --> 4640922 | -6.89 %
evm::tests::test_memory::test__load__should_load_an_element_from_the_memory_with_offset_3 4990882 --> 4647552 | -6.88 %
evm::tests::test_memory::test__load__should_load_an_element_from_the_memory_with_offset_4 4967692 --> 4651602 | -6.36 %
evm::tests::test_memory::test__load__should_load_an_element_from_the_memory_with_offset_5 4987572 --> 4649022 | -6.79 %
evm::tests::test_memory::test__load__should_load_an_element_from_the_memory_with_offset_6 4944092 --> 4646442 | -6.02 %
evm::tests::test_memory::test_load_n_internal_same_word 1029570 --> 1024570 | -0.49 %
evm::tests::test_memory::test_store_n_2_aligned_words 3018220 --> 2706320 | -10.33 %
evm::tests::test_memory::test_store_n_no_aligned_words 350330 --> 350320 | -0.00 %
evm::tests::test_memory::test_store_should_add_an_element_to_the_memory 207590 --> 202490 | -2.46 %
evm::tests::test_memory::test_store_should_add_an_element_with_offset_to_the_memory 238080 --> 194390 | -18.35 %
evm::tests::test_memory::test_store_should_add_n_elements_to_the_memory 2263836 --> 2263826 | -0.00 %


****WORSENED****
utils::tests::test_u256_signed_math::test_u256_neg 61470 --> 62070 0.98 %
Overall gas change: performance improvement, gas consumption-3.97 %

seems to look good now

@lambda-0x
Copy link
Contributor Author

lambda-0x commented Sep 11, 2023

(didn't expect 42% improvement in few tests)

@enitrat
Copy link
Collaborator

enitrat commented Sep 12, 2023

(didn't expect 42% improvement in few tests)

yeah, pow can get ridiculously expensive

Copy link
Collaborator

@enitrat enitrat left a comment

Choose a reason for hiding this comment

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

lgtm

@enitrat enitrat added this pull request to the merge queue Sep 12, 2023
Merged via the queue into kkrt-labs:main with commit cf062eb Sep 12, 2023
1 of 2 checks passed
@lambda-0x lambda-0x deleted the fix-185 branch September 12, 2023 10:54
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.

dev: move all constant powers (e.g. 256.pow(16) ) variables in utils, evm and memory into their own constants
2 participants