-
Notifications
You must be signed in to change notification settings - Fork 5.4k
Improve Contract Call by not encoding method name #7455
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
Conversation
CodSpeed Performance ReportMerging #7455 will not alter performanceComparing Summary
|
d5df284 to
7d49ce3
Compare
e056208 to
c1a5a19
Compare
ironcev
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be interesting to see the performance comparison before and after the optimization, if it's not too much effort to generate it. I assume just pe2e and just pd should be sufficient to get it.
sway-core/src/semantic_analysis/ast_node/expression/typed_expression/method_application.rs
Outdated
Show resolved
Hide resolved
Done. See the description again. |
ironcev
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The improvements numbers are great! 🚀
zees-dev
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggested optional/minor nit.
LGTM! 👍🚀
747d9e8 to
6832509
Compare
## Description This PR continues the optimisation of contract calls on top of #7455. Now the contract method selector is optimised. Ideally, the method selection would just be a `match` and we would let the compiler generate the best code. But we are not there yet. So, for now, we are going to first test the called method length. And based on its length we are going to check the whole string. Something like: ```sway if called_method.len() == 5 { if called_method == "abcde" { ... } if called_method == "fghij" { ... } } if called_method.len() == 10 { if called_method == "..." { .... } } ``` This alone reduces gas a lot. To understand why `match_expressions_all` gas usage is much worse see below. | Test | Before | After | Percentage | |------|--------|-------|------------| | should_pass/empty_fields_in_storage_struct (test.toml)::test_read_write_bytes | 17533 | 12080 | 31.10% | | should_pass/empty_fields_in_storage_struct (test.toml)::test_read_write_map | 13986 | 8502 | 39.21% | | should_pass/empty_fields_in_storage_struct (test.toml)::test_read_write_vec | 29288 | 12797 | 56.31% | | should_pass/language/associated_const_abi (test.toml)::test | 7817 | 4547 | 41.83% | | should_pass/language/associated_const_abi_multiple (test.toml)::test | 2097 | 1511 | 27.94% | | should_pass/language/associated_const_in_decls_of_other_constants (test.toml)::test | 709 | 537 | 24.26% | | should_pass/language/contract_ret_intrinsic (test.toml)::test | 1809 | 1465 | 19.02% | | should_pass/language/match_expressions_all (test.toml) | 764 | 3708 | -385.34% | | should_pass/language/pusha_popa_multiple_defreg (test.toml)::incorrect_pusha_popa | 624 | 451 | 27.72% | | should_pass/language/raw_identifiers (test.error_type.toml)::test | 1075 | 903 | 16.00% | | should_pass/language/raw_identifiers (test.toml)::test | 1075 | 903 | 16.00% | | should_pass/language/references/mutability_of_references_memcpy_bug (test.toml)::test | 1203 | 1031 | 14.30% | | should_pass/language/slice/slice_contract (test.toml)::test_success | 1145 | 920 | 19.65% | | should_pass/language/string_slice/string_slice_contract (test.toml)::test_success | 1322 | 1079 | 18.38% | | should_pass/stdlib/storage_vec_insert (test.toml)::test_test_function | 3831 | 3669 | 4.23% | | should_pass/storage_element_key_modification (test.toml)::test_storage_key_address | 1138 | 943 | 17.14% | | should_pass/storage_element_key_modification (test.toml)::test_storage_key_modification | 757 | 465 | 38.57% | | should_pass/storage_slot_key_calculation (test.toml)::test | 2979 | 2785 | 6.51% | | should_pass/superabi_contract_calls (test.toml)::tests | 1812 | 1310 | 27.70% | | should_pass/superabi_supertrait_external_call (test.toml) | 95 | 76 | 20.00% | | should_pass/superabi_supertrait_same_methods (test.toml)::tests | 936 | 634 | 32.26% | | should_pass/test_abis/abi_impl_methods_callable (test.toml)::tests | 772 | 599 | 22.41% | | should_pass/test_abis/contract_abi-auto_impl (test.toml)::tests | 772 | 599 | 22.41% | | should_pass/unit_tests/aggr_indexing (test.toml)::test1 | 5443 | 4282 | 21.33% | | should_pass/unit_tests/contract-multi-contract-calls (test.toml)::test_contract_2_call | 804 | 631 | 21.52% | | should_pass/unit_tests/contract-multi-contract-calls (test.toml)::test_contract_call | 807 | 634 | 21.44% | | should_pass/unit_tests/contract-multi-contract-calls (test.toml)::test_contract_multi_call | 1592 | 1246 | 21.73% | | should_pass/unit_tests/contract_multi_test (test.toml)::test_fail | 808 | 635 | 21.41% | | should_pass/unit_tests/contract_multi_test (test.toml)::test_success | 806 | 633 | 21.46% | | should_pass/unit_tests/script-contract-calls (test.toml)::test_contract_call | 736 | 563 | 23.51% | | should_pass/unit_tests/workspace_test (test.toml)::test_fail | 808 | 635 | 21.41% | | should_pass/unit_tests/workspace_test (test.toml)::test_success | 807 | 634 | 21.44% | # Radix Trie This PR also turns off the radix trie optimisation at `sway-core/src/semantic_analysis/ast_node/expression/match_expression/typed/typed_match_expression.rs`. I am not 100% sure of its correctness and heuristics. We need a better way to decide when not to use it. One of the examples where the generated code is worse is when all strings are very similar. For example: `get_a` , `get_b`. The generated trie will first test its length. Both have length 5. Not much was gained. Then it tests if for `get_`, which makes sense as it is the common substring. And the last step will test for `a` or `b`. The issue is that all branches and jumps of the last step are not worthy to test just one character. Is probably cheaper to just test the whole string for each option. In the end, we need a better heuristic to determine when to use this optimisation. ## Checklist - [x] I have linked to any relevant issues. - [x] I have commented my code, particularly in hard-to-understand areas. - [ ] I have updated the documentation where relevant (API docs, the reference, and the Sway book). - [ ] If my change requires substantial documentation changes, I have [requested support from the DevRel team](https://github.com/FuelLabs/devrel-requests/issues/new/choose) - [x] I have added tests that prove my fix is effective or that my feature works. - [ ] I have added (or requested a maintainer to add) the necessary `Breaking*` or `New Feature` labels where relevant. - [x] I have done my best to ensure that my PR adheres to [the Fuel Labs Code Review Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md). - [ ] I have requested a review from the relevant team or maintainers. --------- Co-authored-by: Joshua Batty <[email protected]>
Description
This is an optimisation of contract calls on top of #7440.
The old version would pass a string with the method name and encode it, which is
<LEN><BYTES. Now the compiler generates these bytes directly, avoiding encoding the method name in the first place.For that this PR creats the concept of
Literal::Binarythat does not exist in the language, yet. Would be something likeb""in Rust.Improvements
Failed Test
Only now I realized that the test for array of size zero
[u64; 0]is not working. As this is not a useful case, I will solve it in another PR.Checklist
Breaking*orNew Featurelabels where relevant.