Show Gas Used In Unit Tests & Allow Client Supplied Gas Cost Table #616
Conversation
|
I love the rendering, but I'd like to push back a little bit: the unit testing framework is still using the legacy GasStatus and a hard-coded dummy gas schedule, so the actual gas numbers here don't really make sense or match the actual ones downstream clients use. Displaying these numbers can confuse users a lot. I think correct long term solution is introduce a way for the client to plug in their own gas meter implementations somehow. I was aware of this problem when I added the |
@vgao1996 this does allow you plug in your own gas meter. See. The example output here was from Sui gas meter. We do not use the legacy gas anywhere. |
oxade
changed the title
There was a problem hiding this comment.
this does allow you plug in your own gas meter
Not really. That still requires one to use the legacy CostTable. We have a totally custom gas meter implementation that directly interfaces with the GasMeter trait and uses custom structure to store its gas parameters. The unit test framework needs to be refactored so that it is generic over the gas meter, just like the VM.
With that being said, I now realize that this is more of a legacy problem and we don't have to redesign it in this PR. (I knew it when I added the GasMeter trait, but didn't have the bandwidth to deal with this at that time) I also checked our own use cases and we currently do not enable statistics so this change shouldn't affect us.
|
Oh, I should also mention that even though the previous implementation claims it's counting the number of instructions, it's not really doing it correctly. By setting all gas parameters to |
oxade
changed the title
True. I misread CostTable for GasMeter. Yeah we need a heavy refactor. We can scope this for a follow up if you think its high pri |
Haha thats def not good. We should def scrap it. |
Probably not that high pri right now, but I'd imagine us running into this again when we start to polish some of the user-facing tools. |
…ove-language#616) * allow cost table in tests * allow cost table in tests * Clippy simplification * docs
* Show Gas Used In Unit Tests & Allow Client Supplied Gas Cost Table (#616) * allow cost table in tests * allow cost table in tests * Clippy simplification * docs * [stdlib] add `type_name` module for type reflection (#566) Move allows type-safe implementations of key types like fungible tokens using the Coin<T> standard. However, distinguishing coins at the type level only is sometimes limiting--e.g., you might want to write a DEX that has at most one pool for a given pair of currencies (need to compare types + reify the result in a value) or write a lending protocol that supports an extensible pool of currencies (need to reify the currency types + map them to the reserve coin supply). This PR seeks to add the weakest form of reflection that supports use-cases like the ones above in order to keep static reasoning as simple/predictable as possible. Similar modules exist in the Starcoin and Aptos frameworks, and Sui users have also been requesting this feature. - Add opaque `TypeName` value that can be produced from a Move type via `get<T>(): TypeName` - Add functions for converting a `TypeName`'s into its inner representation as an ASCII strings so it can be compared or inspected. This is safe because the Move binary format enforces ASCII encoding for all identifiers One issue worth discussing is whether we want to use the full-length representation of addresses (e.g., always produce a 16, 20, or 32 byte string, depending on the platform address length) or the "short" representation which strips leading 0's (e.g., the convention `0x1` for the Move stdlib). I think the former is slightly cleaner/more predictable, but I went with the latter because it already seems to be the standard in the `TypeTag` implementation of `Display`, as well as in the Aptos/Starcoin `TypeInfo` module that provides similar functionality to this code. * [build] Added additional diagnostics for fetching deps (#643) * [build] Added additional diagnostics for fetching deps * Fixes to EVM code and tests * Addressed review comments * [Tutorial] fix a minor inconsistency with code in step_4 (#623) * [move-cli] update description for 'move test --coverage' command (#620) * [unit tests] Improve DevX of unit tests diagnosis (#641) This does two improvements for UT diagnosis: - We now capture the stack trace (if according feature flags are enabled) for every interpreter error, not just aborts. The restriction to just aborts seem to be unnecessary. - We now treat execution failures (arithmetic errors, borrow errors, index out of bounds) as regular errors, not longer as internal VM errors. Effectively, were we had an output like this before: ``` │ ITE: An unknown error was reported. Location: error[E11001]: test failure │ ┌─ missing_data.move:6:9 │ │ │ 5 │ fun missing_data() acquires Missing { │ │ ------------ In this function in 0x1::MissingData │ 6 │ borrow_global<Missing>(@0x0); │ │ ^^^^^^^^^^^^^ │ │ │ VMError (if there is one): VMError { │ major_status: MISSING_DATA, │ sub_status: None, │ message: None, │ exec_state: None, │ location: Module( │ ModuleId { │ address: 00000000000000000000000000000001, │ name: Identifier( │ "MissingData", │ ), │ }, │ ), │ indices: [], │ offsets: [ │ ( │ FunctionDefinitionIndex(0), │ 1, │ ), │ ], │ } ``` ... we now have: ``` │ error[E11001]: test failure │ ┌─ missing_data.move:6:9 │ │ │ 5 │ fun missing_data() acquires Missing { │ │ ------------ In this function in 0x1::MissingData │ 6 │ borrow_global<Missing>(@0x0); │ │ ^^^^^^^^^^^^^ Execution failure MISSING_DATA │ │ │ stack trace │ MissingData::missing_data_from_other_function(tests/test_sources/missing_data.move:12) │ ``` Co-authored-by: oxade <93547199+oxade@users.noreply.github.com> Co-authored-by: Sam Blackshear <sam@mystenlabs.com> Co-authored-by: Adam Welc <adam@mystenlabs.com> Co-authored-by: wp-lai <techwplai@gmail.com>
Motivation
Knowing how much gas is used is very useful in unit tests.
We currently show instruction executed when the
--statisticsflag is supplied.However instruction count does not provide much use in real systems, so replacing this with gas used, where the client can supply the cost table for gas charging.
This approach allows the client to supply their gas table to the test suite, which then uses it for metering as it would during normal operation.
Note that we will have to increase the instruction bound as appropriate.
Note if we want the instruction count, we simply don't provide a gas table, which defaults each instr to 1 gas as before. This counts instructions.
Example outputs below use non-unity weighting in gas table (from Sui example).
(Write your motivation for proposed changes here.)
Have you read the Contributing Guidelines on pull requests?
(Write your answer here.)
Yes
Test Plan
(Share your test plan here. If you changed code, please provide us with clear instructions for verifying that your changes work.)
N/A