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 a way to query gas usage #10474

Closed
axic opened this issue Dec 2, 2020 · 16 comments · Fixed by #10863
Closed

[isoltest] Add a way to query gas usage #10474

axic opened this issue Dec 2, 2020 · 16 comments · Fixed by #10863
Assignees

Comments

@axic
Copy link
Member

axic commented Dec 2, 2020

As discussed on today's call, a potential way is to have a line similar to storage: called gasUsed: <value>. This line would refer to the gas usage of the preceding call (including the explicit constructor call).

If there is not preceding call, that is a misformatted expectation. If it is the very first line, would that however count as the constructor cost?

@ekpyron
Copy link
Member

ekpyron commented Dec 2, 2020

If it's easier, then checking constructor costs could also only be allowed if there's an explicit constructor() call in the implementation. [by the way: nice: I hadn't seen the storage: thing until now :-)]

@chriseth
Copy link
Contributor

chriseth commented Dec 2, 2020

approval

@mijovic
Copy link
Contributor

mijovic commented Dec 2, 2020

What about gas usage in different runs (with IR and with old codegen)?

@axic
Copy link
Member Author

axic commented Dec 2, 2020

What about gas usage in different runs (with IR and with old codegen)?

I think one needs to duplicate files and explicitly disable specific options, e.g. compileViaYul: false is a file for old codegen.

At the start we do not want to run gas tests for every single semantic tests, but rather have a bunch of larger examples which we use.

@chriseth
Copy link
Contributor

chriseth commented Dec 2, 2020

It might be better to have multiple fields for the different runs, so they can also be compared. It could be multiple lines.

@mijovic
Copy link
Contributor

mijovic commented Dec 2, 2020

I would prefer it to be in a single file, as we don't need to change enfoce-via-yul feature. We can write two lines in expectations, one for gas usage for old codegen, and one via IR.

@axic
Copy link
Member Author

axic commented Dec 2, 2020

It might be better to have multiple fields for the different runs, so they can also be compared. It could be multiple lines.

I'd argue if we do that, then we should also consider doing the same for the other flags...

One option could be grouping expectations for different flags, e.g.:

// ===
// compileViaYul: false
// ----
// f() -> 0
// gasUsed: 11

// ===
// compileViaYul: true
// ----
// f() -> 0
// gasUsed: 1

@mijovic
Copy link
Contributor

mijovic commented Dec 2, 2020

@axic
That would make sense, as we will also have features that will be supported by both runs, and give different results.

One example is constructor_inheritance_init_order.sol from semanticTests (returns 0 in old codegen and 42 with new codegen, and both are correct). That way we can also track differences in two implementations in a better way than now.

But than, this should be done in two steps:

  1. splitting expectations and refactoring enforce via yul feature
  2. Implementing gasUsed

@chriseth
Copy link
Contributor

chriseth commented Dec 3, 2020

// gas YulOptimized: 11
// gas Yul: 200
// gas Legacy: 20
// gas LegacyOptimized: 900

@mijovic
Copy link
Contributor

mijovic commented Dec 7, 2020

I would start working on this, I can report gas usage as @chriseth suggested, and we can change it later

@mijovic mijovic self-assigned this Dec 7, 2020
@axic
Copy link
Member Author

axic commented Dec 7, 2020

In my opinion that is a bad direction and #10474 (comment) is a better one to take.

@ekpyron
Copy link
Member

ekpyron commented Dec 7, 2020

I was about to argue against #10474 (comment) by saying that there are options that don't affect gas costs... but in fact the only option for semantic tests that's generally independent of gas costs is allowNonExistingFunctions and that's a rarely used exotic one...
evmVersion doesn't fit exaclty either (e.g. gas cost can differ depending on SHL being available), but one could just not use larger sets of evm versions, but restrict to only one evm version per settings block to mitigate that.

So given that, the main drawback of #10474 (comment) is the need to repeat the calls, which is not so nice for complex signatures...
I'm not sure myself, though.

In any case: the fact that the selected evm version affects the gas cost will come up using #10474 (comment) as is only...

@axic
Copy link
Member Author

axic commented Dec 7, 2020

My proposal is not only useful for this particular use case, but for numerous other settings we have: IR/wasm/evmVersion/abiversion/etc.

Currently we are duplicating files for these.

@ekpyron
Copy link
Member

ekpyron commented Dec 7, 2020

Your proposal has been a plan for a while... there's an issue for it I'm sure...

@ekpyron
Copy link
Member

ekpyron commented Dec 7, 2020

Maybe not, strange, I was sure... the idea at some point was to also add syntax warnings to semantic tests as one such expectation block...

@chriseth
Copy link
Contributor

chriseth commented Dec 8, 2020

I would still prefer adding a filter to the gas cost statement instead of repeating all calls.

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

Successfully merging a pull request may close this issue.

4 participants