-
Notifications
You must be signed in to change notification settings - Fork 2.6k
contracts: Add automated weights for wasm instructions #7361
Conversation
f1218c5
to
9cbfda4
Compare
006a5d3
to
0a1aa84
Compare
ee738b5
to
d54b230
Compare
/bench pallet pallet_contracts |
Error running benchmark: at-seal-instr-benchmark stdoutTypeError: Cannot read property 'title' of undefined |
/bench runtime pallet pallet_contracts |
Starting benchmark for branch: at-seal-instr-benchmark (vs master) Comment will be updated. |
ff034a3
to
6ad1d02
Compare
Co-authored-by: Andrew Jones <ascjones@gmail.com>
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.
All in all LGTM, pending a review from someone with more wasm expertise.
One observation: where the instruction costs are essentially equivalent (within a certain error threshold) could we equalize them e.g. all the instr_i64*
are virtually identical ~24_650_000 + 7_400_000
, the minor differences being from the benchmarking run.
This is expected and most other contract platforms define manually curated categories of instructions in order to coalesce them. However, my goal here is to not make unnecessary assumptions about the execution engine. We just take the results at face value because changing the execution engine could break those category assumptions. We want that to be a highly automated process. So this category stuff won't fly here. What you are proposing is a post process step to equalize weights that are closely together, right? I don't see the benefit of that. And frankly I don't want to introduce yet another tuning knob (what should the threshold be?). |
Sorry for being too late Looks great, other than few nits |
Co-authored-by: Nikolay Volf <nikvolf@gmail.com>
bot merge |
Trying merge. |
* pallet_contracts: Inline benchmark helper that is only used once * Move all max_* Schedule items into a new struct * Limit the number of globals a module can declare * The current limits are too high for wasmi to even execute * Limit the amount of parameters any wasm function is allowed to have * Limit the size the BrTable's immediate value * Add instruction benchmarks * Add new benchmarks to the schedule and make use of it * Add Benchmark Results generated by the bench bot * Add proc macro that implements `Debug` for `Schedule` * Add missing imports necessary for no_std build * Make the WeightDebug macro available for no_std In this case a dummy implementation is derived in order to not blow up the code size akin to the RuntimeDebug macro. * Rework instr_memory_grow benchmark to use only the maximum amount of pages allowed * Add maximum amount of memory when benching (seal_)call/instantiate * cargo run --release --features runtime-benchmarks --manifest-path bin/node/cli/Cargo.toml -- benchmark --chain dev --steps 50 --repeat 20 --extrinsic * --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output ./bin/node/runtime/src/weights --header ./HEADER --pallet pallet_contracts * Added utility benchmark that allows pretty printing of the real schedule * review: Add missing header to the proc-macro lib.rs * review: Clarify why #[allow(dead_code)] attribute is there * review: Fix pwasm-utils line * review: Fixup rand usage * review: Fix typo * review: Imported -> Exported * cargo run --release --features=runtime-benchmarks --manifest-path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_contracts --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/contracts/src/weights.rs --template=./.maintain/frame-weight-template.hbs * contracts: Adapt to new weight structure * contracts: Fixup runtime WeightInfo * contracts: Remove unneeded fullpath of WeightInfo type * Apply suggestions from code review Co-authored-by: Andrew Jones <ascjones@gmail.com> * Fix typo in schedule.rs Co-authored-by: Andrew Jones <ascjones@gmail.com> * Fix docs in schedule.rs * Apply suggestions from code review Co-authored-by: Nikolay Volf <nikvolf@gmail.com> * Don't publish proc-macro crate until 3.0.0 is ready * Optimize imports for less repetition * Break overlong line Co-authored-by: Parity Benchmarking Bot <admin@parity.io> Co-authored-by: Andrew Jones <ascjones@gmail.com> Co-authored-by: Nikolay Volf <nikvolf@gmail.com>
This PR is the last piece of having a completely automatic weight determination for contracts that is able to generate secure weights. After adding weights for extrinsic and contract callable host functions this adds the weights for each supported instruction.
It also adds some additional static checks for contracts that are necessary to price all instructions securely:
The resulting
Schedule
as benchmarked by the bot for the substrate runtime looks as follows:Schedule (click to open)
The strategy here is to include every wasm instruction on its own and not manually curate them into categories. Otherwise a manual intervention would be necessary whenever those categories don't fit the current wasm execution engine we are benchmarking those on. Our strategy minimizes assumptions and therefore maximizes the amount of automation. Creating a schedule for compiled execution (this schedule is for wasmi executed contracts) is possible with a simple re-benchmark without writing any new code.
However, there are some assumptions that we needed to make. They are explained here:
substrate/frame/contracts/src/schedule.rs
Lines 112 to 134 in b3b6099
As always: The easiest way to review this might be commit by commit.
@jacogr This will also add changes to the
Schedule
data structure that therefore needs an update in its js mirror.