Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

contracts: switch to wasmi gas metering #14084

Merged
merged 66 commits into from Jul 3, 2023
Merged

Conversation

agryaznov
Copy link
Contributor

@agryaznov agryaznov commented May 5, 2023

Closes #13639.

Summary

In order to switch to wasmi built-in fuel metering, the following is done:

  1. Removed uploaded Wasm blob instrumentation.
  2. We don't store instrumented code anymore.
    (See Migration section for details.)
  3. We don't need Wasm instructions weights in the Schedule. They have been removed, along with their benchmarks.
    Except the i64.const instruction which is used as the base (see Fuel Syncs section).
  4. Removed code caching & re-instrumentation logic.
    It whole was in place to guarantee the contract being executed is instrumented with the latest Schedule.
  5. Removed gas host function, as well as the restriction to import a function with this name inside the Wasm module of a contract.
  6. Added fuel synchronizations between host and execution engine.
  7. Added migration.

Fuel Syncs

We have now two separate fuel meters, each having its own units of measurement:

  1. Gas Meter built into the pallet, measures 2-dimension Weight burn during runtime host function execution.
  2. Fuel Meter built into the wasmi, measures 1-dimension Fuel burn in engine for Wasm instructions execution.

Units of fuel consumption in wasmi are normalized, so that basic operations like i64.const cost 1 Fuel. For conversion between the two UoM, the ref_time component of i64.const instruction Weight is used. This is why we keep its benchmark and its weight in the Schedule

We do fuel consumption level synchronizations at the times of execution context switching, namely:

  1. Right before starting Wasm module execution in the engine.
  2. At very beginning and very end of each host function.
  3. Right after completed module execution in the engine.

Migration

Storage migration incurred by this change does the following:

  1. Removes CodeStorage with all instrumented contract codes.
    The only one we need is PristineCode with the original uploaded (and validated) codes.
  2. Renames OwnerInfo into CodeInfo, which incurs moving it to another StorageMap.
    We store code related "meta" data in a separate storage item, which now contains not only owner-related fields. Hence the rename.
  3. As we are freeing up a decent (see Effect section for numbers) amount of storage, deposits attached to it are refunded.
    The amount due to refund is calculated pro rata to the currently held deposit.

Effect

Major effects are on storage usage efficiency (as we don't store instrumented code anymore), and performance improvements (as we get rid of (re-)instrumentation during on-chain execution, and fuel metering on the engine side works faster). Both things imply cost savings for contract authors and contract users.

Storage Usage & Deposits

The migration has been tested on Rococo with try-runtime, showing the following outcomes:

  • Freed up 22 071 Kb (53.3%) of storage occupied with contract codes.
  • Refunded 2.345 ROC (42.3%) of storage deposits held from contract uploaders.

Performance


Follow-up tasks:

  1. remove wasm-instrument/parity-wasm dependency (contracts: switch from parity-wasm-based to wasmi-based module validation #14449)

Copy link
Member

@athei athei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the right direction just a few important points:

  • We need a more elegant way to pass around the consumed fuel. Don't add it to an exposed interface type (see my comment).
  • We really should not cache any values anymore here. I am talking about memory min and max values. We really only want the PristineCode and OwnerInfo in-storage. The PrefabWasmModule should only be constructed in-memory.

frame/contracts/proc-macro/src/lib.rs Outdated Show resolved Hide resolved
frame/contracts/src/lib.rs Outdated Show resolved Hide resolved
frame/contracts/src/lib.rs Outdated Show resolved Hide resolved
frame/contracts/primitives/src/lib.rs Outdated Show resolved Hide resolved
frame/contracts/proc-macro/src/lib.rs Outdated Show resolved Hide resolved
frame/contracts/src/gas.rs Outdated Show resolved Hide resolved
tests::run_out_of_gas_engine, need 2 more

save: 2 bugs with gas syncs: 1 of 2 tests done

gas_syncs_no_overcharge bug fixed, test passes!

cleaned out debug prints

second bug is not a bug

disabled_chain_extension test fix (err msg)

tests run_out_of_fuel_host, chain_extension pass

all tests pass
@agryaznov
Copy link
Contributor Author

@athei @pgherveou addressed all but the migration change which is in progress...

frame/contracts/src/benchmarking/code.rs Outdated Show resolved Hide resolved
frame/contracts/proc-macro/src/lib.rs Show resolved Hide resolved
frame/contracts/src/benchmarking/code.rs Outdated Show resolved Hide resolved
frame/contracts/src/benchmarking/code.rs Outdated Show resolved Hide resolved
frame/contracts/src/benchmarking/mod.rs Show resolved Hide resolved
frame/contracts/src/lib.rs Show resolved Hide resolved
frame/contracts/src/migration/v12.rs Outdated Show resolved Hide resolved
frame/contracts/src/wasm/runtime.rs Outdated Show resolved Hide resolved
frame/contracts/src/wasm/runtime.rs Show resolved Hide resolved
frame/contracts/src/wasm/mod.rs Show resolved Hide resolved
Copy link
Contributor

@pgherveou pgherveou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good for me once all the comments are addressed and I think you missed some from Alex too

frame/contracts/src/tests.rs Show resolved Hide resolved
frame/contracts/src/wasm/mod.rs Outdated Show resolved Hide resolved
frame/contracts/src/wasm/prepare.rs Outdated Show resolved Hide resolved
@agryaznov
Copy link
Contributor Author

bot bench $ pallet dev pallet_contracts

@command-bot

This comment was marked as outdated.

@command-bot

This comment was marked as outdated.

frame/contracts/src/migration/v12.rs Outdated Show resolved Hide resolved
frame/contracts/src/migration/v12.rs Show resolved Hide resolved
@agryaznov
Copy link
Contributor Author

bot merge

@paritytech-processbot paritytech-processbot bot merged commit ffbc02f into master Jul 3, 2023
58 checks passed
@paritytech-processbot paritytech-processbot bot deleted the ag-wasmeter branch July 3, 2023 11:04
@agryaznov
Copy link
Contributor Author

agryaznov commented Jul 3, 2023

Note that every host function benchmark should be re-worked, as in current implementation it leads to double charging the weight spent and counted in the executor. It is safe to over-charge, and the weights changes are not that big, therefore it should be fine to tune up these benchmarks in a follow-up PR.

What I mean by this is that benchmarks for all host functions should not now account for weight spent on the engine side, as it is being accounted for with wasmi built-in fuel meter. Otherwise we charge for the engine spent part twice.

tl;dr: but this overhead seems to be negligible anyway, see below for the details

Example

Let's consider for example the seal_call benchmark. It produces the wasm module which is represented in wat as follows:

wat code
(module
  (type (;0;) (func))
  (type (;1;) (func (param i32 i32 i64 i64 i32 i32 i32 i32 i32 i32) (result i32)))
  (import "env" "memory" (memory (;0;) 16 16))
  (import "seal2" "call" (func (;0;) (type 1)))
  (func (;1;) (type 0))
  (func (;2;) (type 0)
    i32.const 0
    i32.const 24
    i64.const 0
    i64.const 0
    i32.const 8
    i32.const 0
    i32.const 0
    i32.const 0
    i32.const -1
    i32.const 0
    call 0
    drop
    i32.const 0
    i32.const 56
    i64.const 0
    i64.const 0
    i32.const 16
    i32.const 0
    i32.const 0
    i32.const 0
    i32.const -1
    i32.const 0
    call 0
    drop)
  (export "deploy" (func 1))
  (export "call" (func 2))
  (data (;0;) (i32.const 0) "\00\00\00\00\00\00\00\00")
  (data (;1;) (i32.const 8) "\00\00\00\00\00\00\00\00\9b\ff\ff\ff\00\00\00\00")
  (data (;2;) (i32.const 24) "\92\e2\bf\1c\db\9a\b6\8d\bf|\92\8f\8dl\03!\ee\adCL!\a47'.\0f\93\07s\087\eb\fb\a3@\9c\16dX+\e4\e7E\8e\b3\d0\ba>\051\a9\d5\c33\d9\b3\dcd\c7!\86\d6\db\92"))

Here for simplicity I gave the example for r=2, where r is benchmark's complexity parameter, simply put it's how many times we call our host function per benchmark run.

Every time we call our host function, we add this auxiliary set of 12 instructions:

    i32.const 0
    i32.const 24
    i64.const 0
    i64.const 0
    i32.const 8
    i32.const 0
    i32.const 0
    i32.const 0
    i32.const -1
    i32.const 0
    call 0
    drop

Each of these instructions cost 1 fuel in wasmi, so we basically have an overhead of 12 fuel added to the resulting weight of calling a host function. This corresponds to some execution time spent by the engine. This time is measured together with the time spent by runtime for executing the host function itself. We get it included in the resulting weight our benchmark produces. Then we charge it for the host function execution, while still charging the wasmi part as well. Therefore those 12 fuel (or 12 base_weight speaking in our Schedule terms) are being charged twice for every seal_call invocation in every contract.

That being said though, this overhead is negligible compared to the host function weight:
12*base_weight / seal_call_weight = 63k / 309M = 0.02%, which is several orders of magnitude less than the error of the benchmarks itselves.

nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
* upgrade to wasmi 0.29

* prepare cleanup

* sync ref_time w engine from the stack frame

* proc_macro: sync gas in host funcs

save: compiles, only gas pushing left to macro

WIP proc macro

proc macro: done

* clean benchmarks & schedule: w_base = w_i64const

* scale gas values btw engine and gas meter

* (re)instrumentation & code_cache removed

* remove gas() host fn, continue clean-up

save

* address review comments

* move from CodeStorage&PrefabWasmModule to PristineCode&WasmBlob

* refactor: no reftime_limit&schedule passes, no CodeStorage

* bugs fixing

* fix tests: expected deposit amount

* fix prepare::tests

* update tests and fix bugs

tests::run_out_of_gas_engine, need 2 more

save: 2 bugs with gas syncs: 1 of 2 tests done

gas_syncs_no_overcharge bug fixed, test passes!

cleaned out debug prints

second bug is not a bug

disabled_chain_extension test fix (err msg)

tests run_out_of_fuel_host, chain_extension pass

all tests pass

* update docs

* bump wasmi 0.30.0

* benchmarks updated, tests pass

* refactoring

* s/OwnerInfo/CodeInfo/g;

* migration: draft, compiles

* migration: draft, runs

* migration: draft, runs (fixing)

* deposits repaid non pro rata

* deposits repaid pro rata

* better try-runtime output

* even better try-runtime output

* benchmark migration

* fix merge leftover

* add forgotten fixtures, fix docs

* address review comments

* ci fixes

* cleanup

* benchmarks::prepare to return DispatchError

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_contracts

* store memory limits to CodeInfo

* ci: roll back weights

* ".git/.scripts/commands/bench-vm/bench-vm.sh" pallet dev pallet_contracts

* drive-by: update Readme and pallet rustdoc

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_contracts

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_contracts

* use wasmi 0.29

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_contracts

* use wasmi 0.30 again

* query memory limits from wasmi

* better migration types

* ci: pull weights from master

* refactoring

* ".git/.scripts/commands/bench-vm/bench-vm.sh" pallet dev pallet_contracts

* addressing review comments

* refactor

* address review comments

* optimize migration

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_contracts

* another review round comments addressed

* ci fix one

* clippy fix

* ci fix two

---------

Co-authored-by: command-bot <>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B1-note_worthy Changes should be noted in the release notes C5-high PR touches the given topic and has a high impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. E0-runtime_migration PR introduces code that might require downstream chains to run a runtime upgrade. T1-runtime This PR/Issue is related to the topic “runtime”. Z4-involved Can be fixed by an expert coder with good knowledge of the codebase.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

contracts: Switch to wasmi's builtin metering
5 participants