pallet-revive: Fix the contract size related benchmarks

prdoc/pr_7568.prdoc

@@ -0,0 +1,24 @@
+title: 'pallet-revive: Fix the contract size related benchmarks'
+doc:
+- audience: Runtime Dev
+  description: |-
+    Partly addresses https://github.com/paritytech/polkadot-sdk/issues/6157
+
+    The benchmarks measuring the impact of contract sizes on calling or instantiating a contract were bogus because they needed to be written in assembly in order to tightly control the basic block size.
+
+    This fixes the benchmarks for:
+    - call_with_code_per_byte
+    - upload_code
+    - instantiate_with_code
+
+    And adds a new benchmark that accounts for the fact that the interpreter will always compile whole basic blocks:
+    - basic_block_compilation
+
+    After this PR only the weight we assign to instructions need to be addressed.
+crates:
+- name: pallet-revive
+  bump: major
+- name: pallet-revive-fixtures
+  bump: major
+- name: pallet-revive-uapi
+  bump: major

substrate/frame/revive/Cargo.toml

@@ -27,7 +27,8 @@ hex = { workspace = true }
 impl-trait-for-tuples = { workspace = true }
 log = { workspace = true }
 paste = { workspace = true }
-polkavm = { version = "0.19.0", default-features = false }
+polkavm = { version = "0.21.0", default-features = false }
+polkavm-common = { version = "0.21.0", default-features = false, optional = true }
 rlp = { workspace = true }
 scale-info = { features = ["derive"], workspace = true }
 serde = { features = [
@@ -93,6 +94,7 @@ std = [
 	"pallet-timestamp/std",
 	"pallet-transaction-payment/std",
 	"pallet-utility/std",
+	"polkavm-common?/std",
 	"polkavm/std",
 	"rlp/std",
 	"scale-info/std",
@@ -122,6 +124,7 @@ runtime-benchmarks = [
 	"pallet-timestamp/runtime-benchmarks",
 	"pallet-transaction-payment/runtime-benchmarks",
 	"pallet-utility/runtime-benchmarks",
+	"polkavm-common/alloc",
 	"sp-consensus-aura",
 	"sp-consensus-babe",
 	"sp-consensus-slots",

substrate/frame/revive/fixtures/Cargo.toml

@@ -21,7 +21,7 @@ sp-io = { workspace = true, default-features = true, optional = true }
 
 [build-dependencies]
 anyhow = { workspace = true, default-features = true }
-polkavm-linker = { version = "0.19.0" }
+polkavm-linker = { version = "0.21.0" }
 toml = { workspace = true }
 
 [features]

substrate/frame/revive/fixtures/build/_Cargo.toml

@@ -15,7 +15,7 @@ edition = "2021"
 uapi = { package = 'pallet-revive-uapi', path = "", features = ["unstable-hostfn"], default-features = false }
 common = { package = 'pallet-revive-fixtures-common', path = "" }
 hex-literal = { version = "0.4.1", default-features = false }
-polkavm-derive = { version = "0.19.0" }
+polkavm-derive = { version = "0.21.0" }
 
 [profile.release]
 opt-level = 3

substrate/frame/revive/fixtures/contracts/caller_contract.rs

@@ -31,7 +31,7 @@ pub extern "C" fn deploy() {}
 #[no_mangle]
 #[polkavm_derive::polkavm_export]
 pub extern "C" fn call() {
-	input!(code_hash: &[u8; 32], load_code_ref_time: u64,);
+	input!(code_hash: &[u8; 32], load_code_ref_time: u64, load_code_proof_size: u64,);
 
 	// The value to transfer on instantiation and calls. Chosen to be greater than existential
 	// deposit.
@@ -122,9 +122,8 @@ pub extern "C" fn call() {
 	let res = api::call(
 		uapi::CallFlags::empty(),
 		&callee,
-		load_code_ref_time, // Too little ref_time weight.
-		u64::MAX,           /* How much proof_size weight to devote for the execution. u64::MAX
-		                     * = use all. */
+		load_code_ref_time, // just enough to load the contract
+		load_code_proof_size, // just enough to load the contract
 		&[u8::MAX; 32], // No deposit limit.
 		&value,
 		&INPUT,
@@ -137,7 +136,7 @@ pub extern "C" fn call() {
 		uapi::CallFlags::empty(),
 		&callee,
 		u64::MAX, // How much ref_time weight to devote for the execution. u64::MAX = use all.
-		1u64,     // too little proof_size weight
+		load_code_proof_size, //just enough to load the contract
 		&[u8::MAX; 32], // No deposit limit.
 		&value,
 		&INPUT,

substrate/frame/revive/src/benchmarking/code.rs

@@ -24,7 +24,8 @@
 //! we define this simple definition of a contract that can be passed to `create_code` that
 //! compiles it down into a `WasmModule` that can be used as a contract's code.
 
-use alloc::vec::Vec;
+use crate::limits;
+use alloc::{fmt::Write, string::ToString, vec::Vec};
 use pallet_revive_fixtures::bench as bench_fixtures;
 use sp_core::H256;
 use sp_io::hashing::keccak_256;
@@ -47,9 +48,46 @@ impl WasmModule {
 		Self::new(bench_fixtures::dummy_unique(replace_with))
 	}
 
-	/// A contract code of specified sizte that does nothing.
-	pub fn sized(_size: u32) -> Self {
-		Self::dummy()
+	/// Same as as `with_num_instructions` but based on the blob size.
+	///
+	/// This is needed when we weigh a blob without knowing how much instructions it
+	/// contains.
+	pub fn sized(size: u32) -> Self {
+		// Due to variable length encoding of instructions this is not precise. But we only
+		// need rough numbers for our benchmarks.
+		Self::with_num_instructions(size / 3)
+	}
+
+	/// A contract code of specified number of instructions that uses all its bytes for instructions
+	/// but will return immediately.
+	///
+	/// All the basic blocks are maximum sized (only the first is important though). This is to
+	/// account for the fact that the interpreter will compile one basic block at a time even
+	/// when no code is executed. Hence this contract will trigger the compilation of a maximum
+	/// sized basic block and then return with its first instruction.
+	///
+	/// All the code will be put into the "call" export. Hence this code can be safely used for the
+	/// `instantiate_with_code` benchmark where no compilation of any block should be measured.
+	pub fn with_num_instructions(num_instructions: u32) -> Self {
+		let mut text = "
+		pub @deploy:
+		ret
+		pub @call:
+		"
+		.to_string();
+		for i in 0..num_instructions {
+			match i {
+				// return execution right away without breaking up basic block
+				// SENTINEL is a hard coded syscall that terminates execution
+				0 => writeln!(text, "ecalli {}", crate::SENTINEL).unwrap(),
+				i if i % (limits::code::BASIC_BLOCK_SIZE - 1) == 0 =>
+					text.push_str("fallthrough\n"),
+				_ => text.push_str("a0 = a1 + a2\n"),
+			}
+		}
+		text.push_str("ret\n");
+		let code = polkavm_common::assembler::assemble(&text).unwrap();
+		Self::new(code)
 	}
 
 	/// A contract code that calls the "noop" host function in a loop depending in the input.

substrate/frame/revive/src/benchmarking/mod.rs

@@ -259,14 +259,19 @@ mod benchmarks {
 	}
 
 	// This benchmarks the overhead of loading a code of size `c` byte from storage and into
-	// the execution engine. This does **not** include the actual execution for which the gas meter
-	// is responsible. This is achieved by generating all code to the `deploy` function
-	// which is in the wasm module but not executed on `call`.
-	// The results are supposed to be used as `call_with_code_per_byte(c) -
-	// call_with_code_per_byte(0)`.
+	// the execution engine.
+	//
+	// `call_with_code_per_byte(c) - call_with_code_per_byte(0)`
+	//
+	// This does **not** include the actual execution for which the gas meter
+	// is responsible. The code used here will just return on call.
+	//
+	// We expect the influence of `c` to be none in this benchmark because every instruction that
+	// is not in the first basic block is never read. We are primarily interested in the
+	// `proof_size` result of this benchmark.
 	#[benchmark(pov_mode = Measured)]
 	fn call_with_code_per_byte(
-		c: Linear<0, { limits::code::BLOB_BYTES }>,
+		c: Linear<0, { limits::code::STATIC_MEMORY_BYTES / limits::code::BYTES_PER_INSTRUCTION }>,
 	) -> Result<(), BenchmarkError> {
 		let instance =
 			Contract::<T>::with_caller(whitelisted_caller(), WasmModule::sized(c), vec![])?;
@@ -286,11 +291,46 @@ mod benchmarks {
 		Ok(())
 	}
 
+	// Measure the amount of time it takes to compile a single basic block.
+	//
+	// (basic_block_compilation(1) - basic_block_compilation(0)).ref_time()
+	//
+	// This is needed because the interpreter will always compile a whole basic block at
+	// a time. To prevent a contract from triggering compilation without doing any execution
+	// we will always charge one max sized block per contract call.
+	//
+	// We ignore the proof size component when using this benchmark as this is already accounted
+	// for in `call_with_code_per_byte`.
+	#[benchmark(pov_mode = Measured)]
+	fn basic_block_compilation(b: Linear<0, 1>) -> Result<(), BenchmarkError> {
+		let instance = Contract::<T>::with_caller(
+			whitelisted_caller(),
+			WasmModule::with_num_instructions(limits::code::BASIC_BLOCK_SIZE),
+			vec![],
+		)?;
+		let value = Pallet::<T>::min_balance();
+		let storage_deposit = default_deposit_limit::<T>();
+
+		#[block]
+		{
+			Pallet::<T>::call(
+				RawOrigin::Signed(instance.caller.clone()).into(),
+				instance.address,
+				value,
+				Weight::MAX,
+				storage_deposit,
+				vec![],
+			)?;
+		}
+
+		Ok(())
+	}
+
 	// `c`: Size of the code in bytes.
 	// `i`: Size of the input in bytes.
 	#[benchmark(pov_mode = Measured)]
 	fn instantiate_with_code(
-		c: Linear<0, { limits::code::BLOB_BYTES }>,
+		c: Linear<0, { limits::code::STATIC_MEMORY_BYTES / limits::code::BYTES_PER_INSTRUCTION }>,
 		i: Linear<0, { limits::code::BLOB_BYTES }>,
 	) {
 		let input = vec![42u8; i as usize];
@@ -416,7 +456,9 @@ mod benchmarks {
 	// It creates a maximum number of metering blocks per byte.
 	// `c`: Size of the code in bytes.
 	#[benchmark(pov_mode = Measured)]
-	fn upload_code(c: Linear<0, { limits::code::BLOB_BYTES }>) {
+	fn upload_code(
+		c: Linear<0, { limits::code::STATIC_MEMORY_BYTES / limits::code::BYTES_PER_INSTRUCTION }>,
+	) {
 		let caller = whitelisted_caller();
 		T::Currency::set_balance(&caller, caller_funding::<T>());
 		let WasmModule { code, hash, .. } = WasmModule::sized(c);