-
Notifications
You must be signed in to change notification settings - Fork 4
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
spec: Runtime spec for gas and stack accounting #4
Conversation
I’m… no longer finding a way to set PR as a draft... |
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.
Just carrying one leftover comment from the previous PR, and I also noticed that the wording was not exactly right yet for stack height metering. It was written as though the check was supposed to validate the whole call tree.
spec/runtime.mkd
Outdated
|
||
**TODO**: | ||
|
||
## `gas(will_use: u64)` |
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.
Note that today it is gas(u32)
where u32
measures opcodes rather than gas cost directly. Ie, it's conceptually the host who multiplies ops by gas-cost.
Spec-wise it seems easier to count instructions, but that might close the door to differently-weighting instructions. Or at least make that doorframe narrower -- even with ops, we can say that sqrt is 2x usual op.
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 don’t believe the spec should specify a specific scheme in that regard. I adjusted the wording to use a fee(insn)
mechanism where fee
is defined by the embedder – this way they can specify whatever they want – counting instructions (fee = 1), or something else entirely.
However, in practice it is impossible to continue counting each instruction as 1. Some instructions, even today, scale linearly depending on the operands they pop (memory.grow
) and they will only continue increasing (e.g. memory.fill
).
And either way, we’ve had problems with fitting some things into u32, so making the gas limit an u64 seems like an obvious improvement in flexibility.
I went ahead and reworded the specification to achieve the following:
This new wording however has some interesting implications. For example, as written, we ought to charge some gas when evaluating the (data (offset (i32.const 0)) ...) which clearly requires involvement of the VM. Implementation wise, we can also charge this after the fact too in e.g. the |
and is out of scope for this specification. | ||
|
||
**Note:** Instructions may decrease gas based on the operands the instruction would consume. This | ||
is particularly useful for bulk instructions such as `memory.grow`. |
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.
This I believe effectively requires special handling for these kinds of instructions in the instrumentation or the VM. That is, you can’t just
(call $gas (i32.const 1))
(memory.grow (i32.const 123))
You actually need either a variadic $gas
that is able to take all the operands as an argument (before returning them) or maybe some other nasty hack…
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.
Yeah, I think our current impl rewrites memory.grow
into a funciton call.
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.
Overal, LGTM! Fees surprisingly simple even
@@ -0,0 +1,106 @@ | |||
# [Execution](https://www.w3.org/TR/wasm-core-1/#execution%E2%91%A1) | |||
|
|||
The specific mechanism to instrument a WebAssembly module is implementation-specific. This section |
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.
Might be worth to explicitly mention that
the specification aims to support at least two general implementation strategies: desugaring in the terms of core WASM, or built-in accounting in the VM itself.
and is out of scope for this specification. | ||
|
||
**Note:** Instructions may decrease gas based on the operands the instruction would consume. This | ||
is particularly useful for bulk instructions such as `memory.grow`. |
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.
Yeah, I think our current impl rewrites memory.grow
into a funciton call.
spec/runtime.mkd
Outdated
instantiation process. | ||
|
||
Before the `module.start` start function is invoked, charge gas for each local of the invoked | ||
`funcaddr` and . |
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.
incomplete sentence
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.
Aha, because this is no longer necessary due to this logic having moved to the “Invocation of function address a” portion of the spec. Nice catch, removed.
Split out from #2