feat(gasmeter): support detailed metering and reporting#4953
feat(gasmeter): support detailed metering and reporting#4953aeddi wants to merge 17 commits intognolang:masterfrom
Conversation
github-project-automation
bot
added this to
💪 Bounties & Worx and 🧙♂️Gno.land development
github-actions
bot
added
📦 🤖 gnovm
🛠 PR Checks SummaryAll Automated Checks passed. ✅ Manual Checks (for Reviewers):
Read More🤖 This bot helps streamline PR reviews by verifying automated checks and providing guidance for contributors and reviewers. ✅ Automated Checks (for Contributors):🟢 Maintainers must be able to edit this pull request (more info) ☑️ Contributor Actions:
☑️ Reviewer Actions:
📚 Resources:Debug
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
| metrics.VMCPUCycles.Record( | ||
| context.Background(), | ||
| cpuCycles, | ||
| gasUsed.CategoryDetails()["CPU"].Total.GasConsumed, |
There was a problem hiding this comment.
CategoryDetails is used to read the CPU total, but this method rebuild the entire map. So it seems like the map construction costs and memory allocation would occur every time the logTelemetry function is called.
There was a problem hiding this comment.
Yes, indeed, but since this function is called only once per maketx addPkg / call / run, it seems okay to me to build the map at that moment so we keep the metering as fast as possible.
Floats don’t overflow like integers, so putting them in the same
The current CPU metric is actually CPU gas rather than hardware cycles, the name could be misleading (I was confused by this initially as well). Rather than removing it entirely, renaming it to have a clearer name?
mixing auth params with hardcoded value elsewhere would reduce consistency. It seems better to choose one model and stick with it, but deciding which to choose is beyond my hands |
zivkovicmilos
requested review from
ajnavarro,
gfanton and
ltzmaxwell
and removed request for
ltzmaxwell
gfanton
left a comment
gfanton
left a comment
There was a problem hiding this comment.
looking good, only small typo
Co-authored-by: Guilhem Fanton <8671905+gfanton@users.noreply.github.com>
| Home string | ||
| Remote string | ||
| Quiet bool | ||
| Verbosity int // Verbosity level for gas detail: 0=none, 1=categories, 2=non-zero ops, 3=all ops |
There was a problem hiding this comment.
| Verbosity int // Verbosity level for gas detail: 0=none, 1=categories, 2=non-zero ops, 3=all ops | |
| Verbosity gas.Verbosity |
Should we create a specific enum for this?
type Verbosity int
const (
VerbosityNone Verbosity = iota // No gas detail
VerbosityCategories // Show gas categories
VerbosityNonZeroOps // Show non-zero ops
VerbosityAllOps // Show all ops
)
// Usage
// gas.VerbosityNone| func PrintTxInfo(tx std.Tx, res *ctypes.ResultBroadcastTxCommit, io commands.IO, verbosity int) { | ||
| io.Println(string(res.DeliverTx.Data)) | ||
| io.Println("OK!") | ||
| io.Println("GAS WANTED:", res.DeliverTx.GasWanted) | ||
|
|
||
| if verbosity == 0 { | ||
| io.Println("GAS USED: ", res.DeliverTx.GasUsed.Total.GasConsumed) | ||
| } else { | ||
| printGasDetail(res.DeliverTx.GasUsed, io, verbosity) | ||
| } |
There was a problem hiding this comment.
Example of a Gas logger, to avoid having all the if-else logic to determine what should be printed and what shouldn't
| func PrintTxInfo(tx std.Tx, res *ctypes.ResultBroadcastTxCommit, io commands.IO, verbosity int) { | |
| io.Println(string(res.DeliverTx.Data)) | |
| io.Println("OK!") | |
| io.Println("GAS WANTED:", res.DeliverTx.GasWanted) | |
| if verbosity == 0 { | |
| io.Println("GAS USED: ", res.DeliverTx.GasUsed.Total.GasConsumed) | |
| } else { | |
| printGasDetail(res.DeliverTx.GasUsed, io, verbosity) | |
| } | |
| func PrintTxInfo(tx std.Tx, res *ctypes.ResultBroadcastTxCommit, io commands.IO, verbosity Verbosity) { | |
| vp := NewGasLogger(verbosity, io.Out()) | |
| vp.Always(string(res.DeliverTx.Data)) | |
| vp.Always("OK!") | |
| vp.Always("GAS WANTED:", res.DeliverTx.GasWanted) | |
| vp.Always("GAS USED: ", res.DeliverTx.GasUsed.Total.GasConsumed) | |
| printGasDetail(res.DeliverTx.GasUsed, io, verbosity) |
Possible Methods:
func (gl *GasLogger) Category(...) // Verbosity >= 1
func (gl *GasLogger) Op(...) // Verbosity >= 2
func (gl *GasLogger) OpDetail(...) // Verbosity >= 3
func (gl *GasLogger) Always(...) // Ignores Verbosity or Verbosity == 0
Resolve issue comment #1998 (comment) . To show the gas used in `gno test` results: * For "*filetest.gno", `runFiletest` already creates a virtual machine [with a gas meter](https://github.com/gnolang/gno/blob/d4e083b4d2cd2ba8da9740ebb2dec1ba503b4108/gnovm/pkg/test/filetest.go#L77). We update `runFiletest` to add a return value for `m.GasMeter.GasConsumed()` * When `Test` calls `runFiletest`, get and display the gas used * For normal "*.gno" tests, `runTestFiles` [creates a virtual machine](https://github.com/gnolang/gno/blob/d4e083b4d2cd2ba8da9740ebb2dec1ba503b4108/gnovm/pkg/test/test.go#L401https://github.com/gnolang/gno/blob/d4e083b4d2cd2ba8da9740ebb2dec1ba503b4108/gnovm/pkg/test/test.go#L401) by calling `Machine`, where we add an optional param `gasMeter`. In `runTestFiles` we set this to `store.NewInfiniteGasMeter()` . * After `runTestFiles` uses `m.Eval` to evaluate the gno test, it prints the gas used * Update tests to expect the gas in the output like `(elapsed: 0.01s, gas: 648732)` Sample output: ``` === RUN TestTreeReverseIterateByOffset --- PASS: TestTreeReverseIterateByOffset (0.00s) --- GAS: 218576 === RUN ./z_0_filetest.gno false 2 --- PASS: ./z_0_filetest.gno (elapsed: 0.01s, gas: 648732) ``` (The CI check for codecov often fails for the filetest infrastructure. Suggest to ignore.) In the future, when #4953 is merged, then we can integrate tests with this detail. --------- Signed-off-by: Jeff Thompson <jeff@thefirst.org> Co-authored-by: ltzmaxwell <ltz.maxwell@gmail.com>
There was a problem hiding this comment.
Just a quick review, trying to look at the implementation from the macro level.
Responding from the top level comment:
- Move gas meter to a dedicated package: good
- Centralize gas operations: yes please, but I would actually keep a distinction per-module. (ie. tm2 gas, gnovm gas, gno.land gas - gno.land uses a gas cost combination of all 3). Also, as I noted, float64 should be banned almost entirely in tm2/
- Should we remove the CPU Cycles metric in favor of a more relevant one or more detailed logging?
- I would remove CPU cycles from the VM, yes, it's largely useless now
- Does it make sense to set some costs through the genesis while others are defined in a config struct somewhere in the codebase? Should we make all costs configurable via the genesis file, or remove all cost parameters from the genesis (as I did in this PR)?
- Gas costs should be configurable by the chain somehow, to allow live chain changes without having to perform a full upgrade if we get some values wrong.
| package gas | ||
|
|
||
| // Cost represents the gas cost for an operation. | ||
| type Cost float64 |
There was a problem hiding this comment.
We shouldn't calculate gas using floats.
Floats have a risk to be unpredictable (gno uses softfloat for this exact reason; so the consensus shouldn't use floating point calculation either). Instead, let's use a ratio (fraction):
type Ratio struct { Num, Denom int64 }
// Floored multiplication
func (r Ratio) Mul(n int64) int64 { return (n * r.Num) / r.Denom }
// Ceiled multiplication
func (r Ratio) MulCeil(n int64) int64 { return (n * r.Num + (r.Denom-1)) / r.Denom }
// Multiply by other ratio
func (r Ratio) MulRatio(s Ratio) Ratio {
// GCD'ing allows to keep ratio numbers small.
gcdA, gcdB := gcd(r.Num, s.Denom), gcd(s.Num, r.Denom)
return Ratio{(r.Num / gcdA) * (s.Num / gcdB), (r.Denom / gcdB) * (s.Denom / gcdA)}
}Name and implementations can be changed (maybe we should add overflow-checking), but the underlying idea is to make this work using only integer arithmetic. Gas is already expressed as a ratio anyway.
There was a problem hiding this comment.
In the spirit of what I previously mentioned, overflow shouldn't support floating points.
| ResponseBase | ||
| GasWanted int64 | ||
| GasUsed int64 | ||
| GasUsed gas.GasDetail |
There was a problem hiding this comment.
GasDetail is a big type. It is 512 * 8 bytes = 4kb in memory. These have to be listed every time on the wire, even when they're 0. Furthermore, I believe it is even stored in the block data in the database, and while I believe amino uses uvarint, it is still a lot of bytes.
I think this should not be changed. We can change sdk.Result; we can store the gas in the database, but changing ResponseDeliverTx from what I understand has too many implications.
Resolve issue comment #1998 (comment) . To show the gas used in `gno test` results: * For "*filetest.gno", `runFiletest` already creates a virtual machine [with a gas meter](https://github.com/gnolang/gno/blob/d4e083b4d2cd2ba8da9740ebb2dec1ba503b4108/gnovm/pkg/test/filetest.go#L77). We update `runFiletest` to add a return value for `m.GasMeter.GasConsumed()` * When `Test` calls `runFiletest`, get and display the gas used * For normal "*.gno" tests, `runTestFiles` [creates a virtual machine](https://github.com/gnolang/gno/blob/d4e083b4d2cd2ba8da9740ebb2dec1ba503b4108/gnovm/pkg/test/test.go#L401https://github.com/gnolang/gno/blob/d4e083b4d2cd2ba8da9740ebb2dec1ba503b4108/gnovm/pkg/test/test.go#L401) by calling `Machine`, where we add an optional param `gasMeter`. In `runTestFiles` we set this to `store.NewInfiniteGasMeter()` . * After `runTestFiles` uses `m.Eval` to evaluate the gno test, it prints the gas used * Update tests to expect the gas in the output like `(elapsed: 0.01s, gas: 648732)` Sample output: ``` === RUN TestTreeReverseIterateByOffset --- PASS: TestTreeReverseIterateByOffset (0.00s) --- GAS: 218576 === RUN ./z_0_filetest.gno false 2 --- PASS: ./z_0_filetest.gno (elapsed: 0.01s, gas: 648732) ``` (The CI check for codecov often fails for the filetest infrastructure. Suggest to ignore.) In the future, when #4953 is merged, then we can integrate tests with this detail. --------- Signed-off-by: Jeff Thompson <jeff@thefirst.org> Co-authored-by: ltzmaxwell <ltz.maxwell@gmail.com>
Resolve issue comment #1998 (comment) . To show the gas used in `gno test` results: * For "*filetest.gno", `runFiletest` already creates a virtual machine [with a gas meter](https://github.com/gnolang/gno/blob/d4e083b4d2cd2ba8da9740ebb2dec1ba503b4108/gnovm/pkg/test/filetest.go#L77). We update `runFiletest` to add a return value for `m.GasMeter.GasConsumed()` * When `Test` calls `runFiletest`, get and display the gas used * For normal "*.gno" tests, `runTestFiles` [creates a virtual machine](https://github.com/gnolang/gno/blob/d4e083b4d2cd2ba8da9740ebb2dec1ba503b4108/gnovm/pkg/test/test.go#L401https://github.com/gnolang/gno/blob/d4e083b4d2cd2ba8da9740ebb2dec1ba503b4108/gnovm/pkg/test/test.go#L401) by calling `Machine`, where we add an optional param `gasMeter`. In `runTestFiles` we set this to `store.NewInfiniteGasMeter()` . * After `runTestFiles` uses `m.Eval` to evaluate the gno test, it prints the gas used * Update tests to expect the gas in the output like `(elapsed: 0.01s, gas: 648732)` Sample output: ``` === RUN TestTreeReverseIterateByOffset --- PASS: TestTreeReverseIterateByOffset (0.00s) --- GAS: 218576 === RUN ./z_0_filetest.gno false 2 --- PASS: ./z_0_filetest.gno (elapsed: 0.01s, gas: 648732) ``` (The CI check for codecov often fails for the filetest infrastructure. Suggest to ignore.) In the future, when #4953 is merged, then we can integrate tests with this detail. --------- Signed-off-by: Jeff Thompson <jeff@thefirst.org> Co-authored-by: ltzmaxwell <ltz.maxwell@gmail.com>
Resolve issue comment gnolang#1998 (comment) . To show the gas used in `gno test` results: * For "*filetest.gno", `runFiletest` already creates a virtual machine [with a gas meter](https://github.com/gnolang/gno/blob/d4e083b4d2cd2ba8da9740ebb2dec1ba503b4108/gnovm/pkg/test/filetest.go#L77). We update `runFiletest` to add a return value for `m.GasMeter.GasConsumed()` * When `Test` calls `runFiletest`, get and display the gas used * For normal "*.gno" tests, `runTestFiles` [creates a virtual machine](https://github.com/gnolang/gno/blob/d4e083b4d2cd2ba8da9740ebb2dec1ba503b4108/gnovm/pkg/test/test.go#L401https://github.com/gnolang/gno/blob/d4e083b4d2cd2ba8da9740ebb2dec1ba503b4108/gnovm/pkg/test/test.go#L401) by calling `Machine`, where we add an optional param `gasMeter`. In `runTestFiles` we set this to `store.NewInfiniteGasMeter()` . * After `runTestFiles` uses `m.Eval` to evaluate the gno test, it prints the gas used * Update tests to expect the gas in the output like `(elapsed: 0.01s, gas: 648732)` Sample output: ``` === RUN TestTreeReverseIterateByOffset --- PASS: TestTreeReverseIterateByOffset (0.00s) --- GAS: 218576 === RUN ./z_0_filetest.gno false 2 --- PASS: ./z_0_filetest.gno (elapsed: 0.01s, gas: 648732) ``` (The CI check for codecov often fails for the filetest infrastructure. Suggest to ignore.) In the future, when gnolang#4953 is merged, then we can integrate tests with this detail. --------- Signed-off-by: Jeff Thompson <jeff@thefirst.org> Co-authored-by: ltzmaxwell <ltz.maxwell@gmail.com>
Resolve issue comment gnolang#1998 (comment) . To show the gas used in `gno test` results: * For "*filetest.gno", `runFiletest` already creates a virtual machine [with a gas meter](https://github.com/gnolang/gno/blob/d4e083b4d2cd2ba8da9740ebb2dec1ba503b4108/gnovm/pkg/test/filetest.go#L77). We update `runFiletest` to add a return value for `m.GasMeter.GasConsumed()` * When `Test` calls `runFiletest`, get and display the gas used * For normal "*.gno" tests, `runTestFiles` [creates a virtual machine](https://github.com/gnolang/gno/blob/d4e083b4d2cd2ba8da9740ebb2dec1ba503b4108/gnovm/pkg/test/test.go#L401https://github.com/gnolang/gno/blob/d4e083b4d2cd2ba8da9740ebb2dec1ba503b4108/gnovm/pkg/test/test.go#L401) by calling `Machine`, where we add an optional param `gasMeter`. In `runTestFiles` we set this to `store.NewInfiniteGasMeter()` . * After `runTestFiles` uses `m.Eval` to evaluate the gno test, it prints the gas used * Update tests to expect the gas in the output like `(elapsed: 0.01s, gas: 648732)` Sample output: ``` === RUN TestTreeReverseIterateByOffset --- PASS: TestTreeReverseIterateByOffset (0.00s) --- GAS: 218576 === RUN ./z_0_filetest.gno false 2 --- PASS: ./z_0_filetest.gno (elapsed: 0.01s, gas: 648732) ``` (The CI check for codecov often fails for the filetest infrastructure. Suggest to ignore.) In the future, when gnolang#4953 is merged, then we can integrate tests with this detail. --------- Signed-off-by: Jeff Thompson <jeff@thefirst.org> Co-authored-by: ltzmaxwell <ltz.maxwell@gmail.com>
|
See also #5289; may need to rebase. |
|
Take a look at #5289 refactor(benchops): gap-free timing model and measurement fixes and I will need to review all of these before they are merged (don't merge w/o me). Please consider splitting the PR and on top of the above three. The third one will change the way gas charging is done for opcodes that need parameterization: and there are many. |
8 participants
Summary
This PR refactors the gas metering system to make its usage more consistent across the codebase, supporting detailed metering and reporting.
Changes
1. Move gas meter to dedicated package (a9f9eeb)
This commit moves the gas meter out of the
tm2/pkg/storepackage to its own package undertm2/pkg/gas, since it's not only related to store operations anymore. It also renames some functions to be more aligned with Go best practices (e.g.gas.NewGasMeter→gas.NewMeter).2. Centralize gas operations and costs (00cb42e)
This commit makes gas metering usage more consistent. Previously, the metering was scattered across the codebase:
gnovm/pkg/gnolang/machine.go, using a multiplier ingnovm/pkg/gnolang/machine.gognovm/pkg/gnolang/alloc.gognovm/pkg/gnolang/garbage_collector.go, using a multiplier ingnovm/pkg/gnolang/machine.gognovm/pkg/gnolang/go2gno.gognovm/pkg/gnolang/store.go, costs modifiable by param of the storegnovm/pkg/gnolang/uverse.gotm2/pkg/sdk/auth/ante.go, costs can be changed by params intm2/pkg/sdk/auth/params.goAfter this commit:
tm2/pkg/gas/operation.goand the associated costs intm2/pkg/gas/config.go.gasMeter.GasConsume(OpCPUCall, 1).gasMeter.GasConsume(OpNativePrintPerByte, len(output)).float64to allow division through this simple API (multiply by 0.5 to get half, etc.), but the gas counting still usesint64.3. Add float support to overflow package (ebdf7a1)
This commit adds support for float operations to the overflow package, since the gas metering now uses
float64for cost and multiplier.4. Add detailed gas reporting (dd954ff)
This commit adds detailed gas report support. The gas meter now keeps track of how many times an operation was performed and how much gas it consumed in total. Operations are grouped into categories so we can produce reports per category.
Note: Category mapping is done on access to keep gas metering as fast as possible by relying on a simple fixed-size array to store the counters.
5. Deduplicate tx info printing (07b6cf2)
This commit deduplicates the tx info printing that was duplicated in 3 different places. Now this part only calls the
PrintTxInfofunction frominfo.go.6. Add verbose flag to gnokey (92b7b86)
This commit adds a new verbosity flag to
gnokeyto display 4 levels of gas reports.Note: The default keep the current behavior so we don't break any existing tooling relying on gnokey output.
Examples
Level 0 output
Level 1 output
Level 2 output
Level 3 output
Questions
tm2/pkg/sdk/auth/params.go. Except for the default values, these gas costs were set by the genesis file.Does it make sense to set some costs through the genesis while others are defined in a config struct somewhere in the codebase? Should we make all costs configurable via the genesis file, or remove all cost parameters from the genesis (as I did in this PR)?