-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
Conversation
test/libsolidity/semanticTests/abiEncoderV1/abi_decode_static_array.sol
Outdated
Show resolved
Hide resolved
19ad899
to
1be4899
Compare
I'm not sure it's a good idea to add the gas costs always for all test. |
There was an error when running
Please check that your changes are working as intended. |
test/libsolidity/semanticTests/abiEncoderV1/abi_decode_dynamic_array.sol
Outdated
Show resolved
Hide resolved
We should also add deploy gas costs, or maybe better: code size |
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. |
1be4899
to
5c4fe64
Compare
@cameel Updated |
5c4fe64
to
cf6bcc8
Compare
test/libsolidity/semanticTests/abiEncoderV2/cleanup/dynamic_array.sol
Outdated
Show resolved
Hide resolved
cf6bcc8
to
e16c5d3
Compare
e16c5d3
to
64e6138
Compare
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.
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.
test/libsolidity/semanticTests/viaYul/array_storage_push_pop.sol
Outdated
Show resolved
Hide resolved
90c792d
to
e6aa739
Compare
I think I addressed all comments, will wait for points we need more feedback. |
test/libsolidity/semanticTests/array/copying/copy_byte_array_in_struct_to_storage.sol
Outdated
Show resolved
Hide resolved
test/libsolidity/semanticTests/array/copying/copy_internal_function_array_to_storage.sol
Show resolved
Hide resolved
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 |
@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 |
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
and in the test file
What could be the reason for this? |
Co-authored-by: Daniel Kirchner <[email protected]>
Co-authored-by: Kamil Śliwak <[email protected]>
a0046a7
to
0c4dcaf
Compare
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.
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.
0c4dcaf
to
ed6c999
Compare
@cameel Updated :) |
da3d73a
to
afcc044
Compare
afcc044
to
b73e9f3
Compare
@@ -25,3 25,5 @@ contract B { | |||
// compileViaYul: also | |||
// ---- | |||
// g() -> 42 | |||
// gas irOptimized: 107179 | |||
// gas legacy: 117797 |
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.
Why are some contracts missing legacyOptimized
or irOptimized
?
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.
If they are less then 100K gas, as that was original threshold for including gas expectation
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.
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.
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.
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.
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.
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
Fixes #10474
TODO: