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

@chriseth
Copy link
Contributor

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

@stackenbotten
Copy link

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

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 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
Copy link
Contributor Author

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

Copy link
Member

@cameel cameel left a comment

Choose a reason for hiding this comment

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

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 Compare January 29, 2021 15:50
@mijovic
Copy link
Contributor Author

mijovic commented Jan 29, 2021

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

@aarlt
Copy link
Member

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 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 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 Compare March 9, 2021 22:22
cameel
cameel previously approved these changes Mar 10, 2021
Copy link
Member

@cameel cameel left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

mijovic commented Mar 10, 2021

@cameel Updated :)

@mijovic mijovic force-pushed the isoltestGasCosts branch 2 times, most recently from da3d73a to afcc044 Compare March 10, 2021 13:18
@chriseth chriseth merged commit 89946b1 into develop Mar 10, 2021
@chriseth chriseth deleted the isoltestGasCosts branch March 10, 2021 14:11
@@ -25,3 25,5 @@ contract B {
// compileViaYul: also
// ----
// g() -> 42
// gas irOptimized: 107179
// gas legacy: 117797
Copy link
Member

Choose a reason for hiding this comment

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

Why are some contracts missing legacyOptimized or irOptimized?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Development

Successfully merging this pull request may close these issues.

[isoltest] Add a way to query gas usage
8 participants