close
The Wayback Machine - https://web.archive.org/web/20210603123917/https://github.com/ethereum/solidity/pull/10863
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

[isoltest] Add gas costs to function call expectations #10863

Merged
merged 13 commits into from Mar 10, 2021
Merged

Conversation

@mijovic
Copy link
Contributor

@mijovic mijovic commented Jan 28, 2021

Fixes #10474

TODO:

  • Add expected gas costs, not only obtained
  • Add deploy costs
  • Run ir gas cost only if explicitly added to expectations
@mijovic mijovic force-pushed the isoltestGasCosts branch from 19ad899 to 1be4899 Jan 28, 2021
@chriseth
Copy link
Contributor

@chriseth chriseth commented Jan 28, 2021

I'm not sure it's a good idea to add the gas costs always for all test.

@stackenbotten
Copy link

@stackenbotten stackenbotten commented Jan 28, 2021

There was an error when running chk_coding_style for commit 1be489917b2431f2d7e93c5319f64529e628325c:

Error: Trailing whitespace found:
test/libsolidity/semanticTests/ecrecover/ecrecover_abiV2.sol:13:// 0x73b1693892219d736caba55bdb67216e485557ea6b6af75f37096c9aa6a5a75f, 

Please check that your changes are working as intended.

@chriseth
Copy link
Contributor

@chriseth chriseth commented Jan 28, 2021

We should also add deploy gas costs, or maybe better: code size

@mijovic mijovic changed the title Isoltest gas costs [isoltest] Add gas costs to function call expectations Jan 28, 2021
test/libsolidity/SemanticTest.cpp Outdated Show resolved Hide resolved
test/libsolidity/SemanticTest.cpp Outdated Show resolved Hide resolved
test/libsolidity/SemanticTest.cpp Outdated Show resolved Hide resolved
test/libsolidity/SemanticTest.cpp Outdated Show resolved Hide resolved
test/libsolidity/SemanticTest.cpp Outdated Show resolved Hide resolved
@mijovic
Copy link
Contributor Author

@mijovic mijovic commented Jan 29, 2021

I'll change PR now, so that gas is checked only for these tests where expectation is present and will try filtering out tests where we want to run at the beginning.

@mijovic mijovic force-pushed the isoltestGasCosts branch from 1be4899 to 5c4fe64 Jan 29, 2021
@mijovic
Copy link
Contributor Author

@mijovic mijovic commented Jan 29, 2021

@cameel Updated
Gas tests will be run only when expectation is present.
I removed gas expectations from all the tests that have less than 100K gas usage (still 600+ tests are affected)
Also, updated to use only latest version of evm for gas usage

@mijovic mijovic force-pushed the isoltestGasCosts branch from 5c4fe64 to cf6bcc8 Jan 29, 2021
@mijovic mijovic force-pushed the isoltestGasCosts branch from cf6bcc8 to e16c5d3 Jan 29, 2021
@mijovic mijovic force-pushed the isoltestGasCosts branch from e16c5d3 to 64e6138 Jan 29, 2021
Copy link
Member

@cameel cameel left a comment

Here's another batch of comments.

I also skimmed through the updated tests and commented on the ones where the cost is particularly high (>= 1 million gas) so that we can discuss them but I didn't really analyze them to check if the high cost is warranted or not. Feel free to mark any obvious ones as resolved.

liblangutil/EVMVersion.h Outdated Show resolved Hide resolved
test/libsolidity/SemanticTest.cpp Outdated Show resolved Hide resolved
test/Common.cpp Outdated Show resolved Hide resolved
test/TestCase.h Outdated Show resolved Hide resolved
test/libsolidity/SemanticTest.h Outdated Show resolved Hide resolved
test/libsolidity/util/TestFunctionCall.h Outdated Show resolved Hide resolved
test/libsolidity/util/TestFunctionCall.h Outdated Show resolved Hide resolved
test/tools/IsolTestOptions.cpp Outdated Show resolved Hide resolved
test/libsolidity/util/TestFileParser.cpp Outdated Show resolved Hide resolved
@mijovic mijovic force-pushed the isoltestGasCosts branch 3 times, most recently from 90c792d to e6aa739 Jan 29, 2021
@mijovic
Copy link
Contributor Author

@mijovic mijovic commented Jan 29, 2021

I think I addressed all comments, will wait for points we need more feedback.

@aarlt
Copy link
Member

@aarlt aarlt commented Jan 29, 2021

Maybe it would be interesting to add test cases that are doing gas measurements for all the different evm versions. E.g. setting the same tests that only differ in the defined EVMVersion. With this we could more easily detect if e.g. a new future was accidentally changing the gas-costs of older evm versions. But I'm not sure whether it would really make sense. I just imagine, if someone adds a feature for the latest evm version, that should only influence the gas cost in that particular version, but it also change the gas cost of other version(s), that this could be interesting to see.

test/Common.cpp Outdated Show resolved Hide resolved
@mijovic
Copy link
Contributor Author

@mijovic mijovic commented Jan 29, 2021

@aarlt Regarding what gas costs are added to expectations by enforce feature, I did it in a way that I add only gas costs greater than X(which was set to 100000 gas). So if, for example ir* is missing, it means that it has gas cost of less than 100k gas...

If it is confusing, I'll change it

@aarlt
Copy link
Member

@aarlt aarlt commented Jan 29, 2021

I just checked out your branch and ran the tests locally. It looks like that I have always different gas measurement results.

If i run test/tools/isoltest -t semanticTests/\* --vm /Users/alex/evmone/lib/libevmone.dylib --no-smt I got this:

semanticTests/constructor_ihneritance_init_order_2.sol: FAIL
  Contract:
    contract A {
        uint x = 42;
        function f() public returns(uint256) {
            return x;
        }
    }
    contract B is A {
        uint public y = f();
    }
    // ====
    // compileToEwasm: also
    // compileViaYul: also

  Gas results missing or wrong, obtained result:
  // constructor() ->
  // gas LegacyOptimized: 138540
  // gas Yul: 232112
  // gas YulOptimized: 153851
  // gas Legacy: 151224

  // y() -> 42
  // gas LegacyOptimized: 22092
  // gas Yul: 22744
  // gas YulOptimized: 22262
  // gas Legacy: 22193

and in the test file

❯ cat ../test/libsolidity/semanticTests/constructor_ihneritance_init_order_2.sol
contract A {
    uint x = 42;
    function f() public returns(uint256) {
        return x;
    }
}
contract B is A {
    uint public y = f();
}
// ====
// compileToEwasm: also
// compileViaYul: also
// ----
// constructor() ->
// gas Legacy: 152088
// gas LegacyOptimized: 138540
// gas Yul: 232112
// gas YulOptimized: 153851
// y() -> 42
// gas Legacy: 22193
// gas LegacyOptimized: 22092
// gas Yul: 22744
// gas YulOptimized: 22262

What could be the reason for this?

@mijovic mijovic force-pushed the isoltestGasCosts branch 6 times, most recently from a0046a7 to 0c4dcaf Mar 9, 2021
Copy link
Member

@cameel cameel left a comment

I suggested adding an assert in one place and there's a question about some tests running out of gas from @chriseth but overall the PR looks ready to merge to me.

@mijovic mijovic force-pushed the isoltestGasCosts branch from 0c4dcaf to ed6c999 Mar 10, 2021
@mijovic
Copy link
Contributor Author

@mijovic mijovic commented Mar 10, 2021

@cameel Updated :)

@mijovic mijovic force-pushed the isoltestGasCosts branch 2 times, most recently from da3d73a to afcc044 Mar 10, 2021
@mijovic mijovic force-pushed the isoltestGasCosts branch from afcc044 to b73e9f3 Mar 10, 2021
@cameel
cameel approved these changes Mar 10, 2021
@chriseth chriseth merged commit 89946b1 into develop Mar 10, 2021
47 checks passed
47 checks passed
Image
ci/circleci: b_archlinux Your tests passed on CircleCI!
Details
Image
ci/circleci: b_bytecode_ems Your tests passed on CircleCI!
Details
Image
ci/circleci: b_bytecode_osx Your tests passed on CircleCI!
Details
Image
ci/circleci: b_bytecode_ubu Your tests passed on CircleCI!
Details
Image
ci/circleci: b_bytecode_win Your tests passed on CircleCI!
Details
Image
ci/circleci: b_docs Your tests passed on CircleCI!
Details
Image
ci/circleci: b_ems Your tests passed on CircleCI!
Details
Image
ci/circleci: b_osx Your tests passed on CircleCI!
Details
Image
ci/circleci: b_ubu Your tests passed on CircleCI!
Details
Image
ci/circleci: b_ubu_clang Your tests passed on CircleCI!
Details
Image
ci/circleci: b_ubu_cxx20 Your tests passed on CircleCI!
Details
Image
ci/circleci: b_ubu_ossfuzz Your tests passed on CircleCI!
Details
Image
ci/circleci: b_ubu_release Your tests passed on CircleCI!
Details
Image
ci/circleci: b_ubu_static Your tests passed on CircleCI!
Details
Image
ci/circleci: b_win Your tests passed on CircleCI!
Details
Image
ci/circleci: b_win_release Your tests passed on CircleCI!
Details
Image
ci/circleci: chk_antlr_grammar Your tests passed on CircleCI!
Details
Image
ci/circleci: chk_buglist Your tests passed on CircleCI!
Details
Image
ci/circleci: chk_coding_style Your tests passed on CircleCI!
Details
Image
ci/circleci: chk_docs_pragma_min_version Your tests passed on CircleCI!
Details
Image
ci/circleci: chk_errorcodes Your tests passed on CircleCI!
Details
Image
ci/circleci: chk_proofs Your tests passed on CircleCI!
Details
Image
ci/circleci: chk_pylint Your tests passed on CircleCI!
Details
Image
ci/circleci: chk_spelling Your tests passed on CircleCI!
Details
Image
ci/circleci: t_archlinux_soltest Your tests passed on CircleCI!
Details
Image
ci/circleci: t_bytecode_compare Your tests passed on CircleCI!
Details
Image
ci/circleci: t_ems_compile_ext_colony Your tests passed on CircleCI!
Details
Image
ci/circleci: t_ems_compile_ext_ens Your tests passed on CircleCI!
Details
Image
ci/circleci: t_ems_compile_ext_gnosis Your tests passed on CircleCI!
Details
Image
ci/circleci: t_ems_compile_ext_gnosis_v2 Your tests passed on CircleCI!
Details
Image
ci/circleci: t_ems_compile_ext_zeppelin Your tests passed on CircleCI!
Details
Image
ci/circleci: t_ems_solcjs Your tests passed on CircleCI!
Details
Image
ci/circleci: t_ems_test_ext_ens Your tests passed on CircleCI!
Details
Image
ci/circleci: t_ems_test_ext_gnosis_v2 Your tests passed on CircleCI!
Details
Image
ci/circleci: t_ems_test_ext_zeppelin Your tests passed on CircleCI!
Details
Image
ci/circleci: t_osx_cli Your tests passed on CircleCI!
Details
Image
ci/circleci: t_osx_soltest Your tests passed on CircleCI!
Details
Image
ci/circleci: t_pyscripts_ubu Your tests passed on CircleCI!
Details
Image
ci/circleci: t_pyscripts_win Your tests passed on CircleCI!
Details
Image
ci/circleci: t_ubu_clang_soltest Your tests passed on CircleCI!
Details
Image
ci/circleci: t_ubu_cli Your tests passed on CircleCI!
Details
Image
ci/circleci: t_ubu_release_cli Your tests passed on CircleCI!
Details
Image
ci/circleci: t_ubu_release_soltest Your tests passed on CircleCI!
Details
Image
ci/circleci: t_ubu_soltest Your tests passed on CircleCI!
Details
Image
ci/circleci: t_ubu_soltest_enforce_yul Your tests passed on CircleCI!
Details
Image
ci/circleci: t_win Your tests passed on CircleCI!
Details
Image
ci/circleci: t_win_release Your tests passed on CircleCI!
Details
@chriseth chriseth deleted the isoltestGasCosts branch Mar 10, 2021
@@ -25,3 +25,5 @@ contract B {
// compileViaYul: also
// ----
// g() -> 42
// gas irOptimized: 107179
// gas legacy: 117797

This comment has been minimized.

@axic

axic Mar 10, 2021
Member

Why are some contracts missing legacyOptimized or irOptimized?

This comment has been minimized.

@mijovic

mijovic Mar 10, 2021
Author Contributor

If they are less then 100K gas, as that was original threshold for including gas expectation

This comment has been minimized.

@axic

axic Mar 10, 2021
Member

Ah, this is rather confusing then. Ideally if any of the runs hit the threshold then all of them should be included in the file. But due to the implementation this may be hard to achieve.

This comment has been minimized.

@mijovic

mijovic Mar 10, 2021
Author Contributor

I think it is possible to add that upgrade with not too much of effort. If you think it is useful, I'll implement it in that way.

This comment has been minimized.

@mijovic

mijovic Mar 10, 2021
Author Contributor

For optimized it is easier to add both in case that one went over 100k, as both of them run in same setup of soltest.
maybe it is worth trying maling this better

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

8 participants