Conversation
|
/benchmark runtime pallet pallet_contracts |
|
Finished benchmark for branch: at-rework-exec Benchmark: Benchmark Runtime Pallet 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 ResultsPallet: "pallet_contracts", Extrinsic: "on_initialize", Lowest values: [], Highest values: [], Steps: [50], Repeat: 20
|
This removes the need for a runtime check of the specified `MaxDepth`. We can now garantuee that we don't need to allocate when a new call frame is pushed.
Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>
|
/benchmark runtime pallet pallet_contracts |
|
Finished benchmark for branch: at-rework-exec Benchmark: Benchmark Runtime Pallet 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 ResultsPallet: "pallet_contracts", Extrinsic: "on_initialize", Lowest values: [], Highest values: [], Steps: [50], Repeat: 20
|
…/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
pepyakin
left a comment
There was a problem hiding this comment.
Initial review. Good approach and mostly looks good. There are some defects discussed in DMs. I was not able to fully review the rent part: I am too much out of the loop there.
|
I fixed the nits first. Now I will look into the defect we discussed. Starting with a test that reproduces the bug. |
|
The defect is fixed and I added a regression test for it. |
|
bot merge |
|
Waiting for commit status. |
* contracts: Add default implementation for Executable::occupied_storage() * contracts: Refactor the exec module * Let runtime specify the backing type of the call stack This removes the need for a runtime check of the specified `MaxDepth`. We can now garantuee that we don't need to allocate when a new call frame is pushed. * Fix doc typo Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com> * 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 * Review nits * Fix defect in contract info caching behaviour * Add more docs * Fix wording and typos Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com> Co-authored-by: Parity Benchmarking Bot <admin@parity.io>
Fixes #6439
Fixes #6448
Background
The contracts pallet consists of three main modules where the exec module sits in the middle as an intermediator between the other two:
wasmmodule is responsible for handling instrumentation, sandboxing and execution of wasm code completely agnostic of the semantics of a contract. I gets called into from theexecmodule.runtimemodule defines the interface available to contracts. It calls into theexecmodule.execmodule constitutes the business logic of contract execution and is at the heart of the contracts pallet.The
execmodule manages the call stack of contracts and all the logic attached to it. This is the module we refactor in this PR. Currently, this module is roughly structured like this:It contains a
ExecutionContextstruct which represents one contract in the call stack. In order to form a stack it contains a shared reference to its parentExecutionContext. The rootExecutionContext(which does not have a parent) is special in that it does not represent a contract execution but the signed origin which initiated the call stack. To tackle this difference uses of the same type theExecutionContextis made up ofOptionswhich need to be unwrapped at runtime when it is known that the current execution must come from a contract (e.Gtrie_id).The module also contains a trait named
Extwhich describes the functions theexecmodule exposes to theruntimemodule. This trait is not directly implemented by theExecutionContextbut a on each subsequent call temporarily created struct namedCallContextin order to satisfy the borrow checker.Motivation
The current design while somewhat elegant in that it works solely with references has some drawbacks which we like to address with this refactor:
ExecutionContexts is only possible with shared references. It is therefore impossible to have shared mutable state between all contract calls. Something like this is needed in order to improve debug capabilities (a shared debug buffer).ExecutionContextand its children is not expressed in the type but byOptionalswhich introduce panic locations (with necessary proofs) when unwrapped all over the module.ContractInfoto be applied to storage eagerly which means a read-modify-write cycle on each access of this this stucture.Implementation
This PR fundamentally changes how the call stack is represented: The
ExecutionContextis split up into a type namedStackand another type namedFrame. TheCallContexthas no replacement because the borrow checker workarounds are no longer necessary. By that we immediately got ridThe
Stackrepresents the whole call stack and is made up ofFrames(stored as aSmallVec) which represent the individual call frames. TheStackalso contains a non optionalfirst_framewhich statically codifies our assumption of always having an active contract (the one that is being called from the signed origin) while theStackexists.The
Extis now directly implemented by theStackand the currentFramecan easily be accessed by accessing the last element of theVecor falling back to thefirst_framein order to prevent another panic location.Each
Framecontains (among other things) theContractInfodata structure of the contract that belongs to theFrameand operates on this while executing the contract instead of eagerly accessing storage. When theFramefinishes execution the modified structure is written back to storage and other references to the same data on the call stack are invalidated forcing a reload from storage on next access. This reload is the only panic location which remains in the code: It is impossible for the contract to be deleted so that the reload panics but this is not enforced statically.This new design allows mutably sharing data between call frames in the
Stackdata structure addressing our need for a call stack global buffer.Weight Impact
The benefits of caching the
ContractInfocan be seen when looking at the few functions which modify theContractInfodata structure:seal_terminate: Saved one writeseal_restore_to: Saved one writeseal_set_storage: 4.7x cheaperseal_set_rent_allowance: 4.2x cheaperseal_clear_storage: 1.8x cheaperFuture Work
StackandFramein different modules)ContractInfoinside theStackrather than relying on the client in order to remove the last panic location.