Configuration Trait for Store

[cemonem]

Pull Request Overview

TODO or Help Wanted

Checks

• Using Nix
  • Ran nix fmt
  • Ran nix flake check '.?submodules=1'
• Using Rust tooling
  • Ran cargo fmt
  • Ran cargo test
  • Ran cargo check
  • Ran cargo build
  • Ran cargo doc

Benchmark Results

Github Issue

[github-actions]

🗒️ WebAssembly Testsuite Report

No changes detected

Per-test details

Click here to open

File	Passed Asserts	Failed Asserts	% Passed	Notes
simd_bitwise.wast	167 / 167	0 / 167	100%	-
f32_bitwise.wast	363 / 363	0 / 363	100%	-
simd_align.wast	54 / 54	0 / 54	100%	-
simd_store8_lane.wast	51 / 51	0 / 51	100%	-
simd_i16x8_cmp.wast	463 / 463	0 / 463	100%	-
ref_null.wast	2 / 2	0 / 2	100%	-
utf8-import-field.wast	176 / 176	0 / 176	100%	-
table_grow.wast	48 / 48	0 / 48	100%	-
simd_load32_lane.wast	23 / 23	0 / 23	100%	-
address.wast	256 / 256	0 / 256	100%	-
stack.wast	5 / 5	0 / 5	100%	-
simd_i32x4_arith2.wast	147 / 147	0 / 147	100%	-
linking.wast	102 / 102	0 / 102	100%	-
simd_i32x4_extmul_i16x8.wast	116 / 116	0 / 116	100%	-
utf8-custom-section-id.wast	176 / 176	0 / 176	100%	-
simd_f64x2_arith.wast	1822 / 1822	0 / 1822	100%	-
f32.wast	2513 / 2513	0 / 2513	100%	-
memory_redundancy.wast	4 / 4	0 / 4	100%	-
simd_conversions.wast	280 / 280	0 / 280	100%	-
table_init.wast	729 / 729	0 / 729	100%	-
unreached-invalid.wast	118 / 118	0 / 118	100%	-
local_get.wast	35 / 35	0 / 35	100%	-
load.wast	96 / 96	0 / 96	100%	-
block.wast	222 / 222	0 / 222	100%	-
simd_i16x8_extmul_i8x16.wast	116 / 116	0 / 116	100%	-
simd_f64x2_pmin_pmax.wast	3886 / 3886	0 / 3886	100%	-
const.wast	376 / 376	0 / 376	100%	-
simd_i32x4_trunc_sat_f32x4.wast	106 / 106	0 / 106	100%	-
custom.wast	8 / 8	0 / 8	100%	-
data.wast	36 / 36	0 / 36	100%	-
fac.wast	7 / 7	0 / 7	100%	-
simd_lane.wast	463 / 463	0 / 463	100%	-
simd_i8x16_arith2.wast	209 / 209	0 / 209	100%	-
simd_f32x4_arith.wast	1819 / 1819	0 / 1819	100%	-
float_misc.wast	470 / 470	0 / 470	100%	-
ref_func.wast	11 / 11	0 / 11	100%	-
comments.wast	3 / 3	0 / 3	100%	-
func.wast	168 / 168	0 / 168	100%	-
binary-leb128.wast	58 / 58	0 / 58	100%	-
table.wast	10 / 10	0 / 10	100%	-
memory_size.wast	38 / 38	0 / 38	100%	-
int_literals.wast	50 / 50	0 / 50	100%	-
imports.wast	125 / 125	0 / 125	100%	-
simd_i64x2_arith.wast	198 / 198	0 / 198	100%	-
elem.wast	64 / 64	0 / 64	100%	-
simd_i16x8_arith2.wast	170 / 170	0 / 170	100%	-
skip-stack-guard-page.wast	10 / 10	0 / 10	100%	-
float_literals.wast	177 / 177	0 / 177	100%	-
utf8-import-module.wast	176 / 176	0 / 176	100%	-
unreachable.wast	63 / 63	0 / 63	100%	-
simd_store16_lane.wast	35 / 35	0 / 35	100%	-
table_copy.wast	1649 / 1649	0 / 1649	100%	-
br_if.wast	117 / 117	0 / 117	100%	-
i64.wast	415 / 415	0 / 415	100%	-
f64.wast	2513 / 2513	0 / 2513	100%	-
simd_i16x8_arith.wast	192 / 192	0 / 192	100%	-
switch.wast	27 / 27	0 / 27	100%	-
call_indirect.wast	167 / 167	0 / 167	100%	-
table_set.wast	25 / 25	0 / 25	100%	-
int_exprs.wast	89 / 89	0 / 89	100%	-
loop.wast	119 / 119	0 / 119	100%	-
local_set.wast	52 / 52	0 / 52	100%	-
forward.wast	4 / 4	0 / 4	100%	-
f64_bitwise.wast	363 / 363	0 / 363	100%	-
simd_load64_lane.wast	15 / 15	0 / 15	100%	-
i32.wast	459 / 459	0 / 459	100%	-
br_table.wast	173 / 173	0 / 173	100%	-
ref_is_null.wast	13 / 13	0 / 13	100%	-
simd_i32x4_dot_i16x8.wast	29 / 29	0 / 29	100%	-
store.wast	67 / 67	0 / 67	100%	-
float_exprs.wast	819 / 819	0 / 819	100%	-
return.wast	83 / 83	0 / 83	100%	-
exports.wast	40 / 40	0 / 40	100%	-
labels.wast	28 / 28	0 / 28	100%	-
traps.wast	32 / 32	0 / 32	100%	-
token.wast	23 / 23	0 / 23	100%	-
call.wast	90 / 90	0 / 90	100%	-
table_fill.wast	44 / 44	0 / 44	100%	-
simd_i32x4_trunc_sat_f64x2.wast	106 / 106	0 / 106	100%	-
inline-module.wast	0 / 0	0 / 0	-	-
simd_int_to_int_extend.wast	252 / 252	0 / 252	100%	-
simd_i32x4_arith.wast	192 / 192	0 / 192	100%	-
simd_f32x4.wast	788 / 788	0 / 788	100%	-
simd_i64x2_arith2.wast	23 / 23	0 / 23	100%	-
nop.wast	87 / 87	0 / 87	100%	-
unreached-valid.wast	5 / 5	0 / 5	100%	-
local_tee.wast	96 / 96	0 / 96	100%	-
start.wast	11 / 11	0 / 11	100%	-
simd_i16x8_q15mulr_sat_s.wast	29 / 29	0 / 29	100%	-
simd_i32x4_cmp.wast	473 / 473	0 / 473	100%	-
simd_i8x16_sat_arith.wast	212 / 212	0 / 212	100%	-
table-sub.wast	2 / 2	0 / 2	100%	-
simd_store64_lane.wast	15 / 15	0 / 15	100%	-
binary.wast	115 / 115	0 / 115	100%	-
simd_f64x2.wast	801 / 801	0 / 801	100%	-
left-to-right.wast	95 / 95	0 / 95	100%	-
simd_store32_lane.wast	23 / 23	0 / 23	100%	-
br.wast	96 / 96	0 / 96	100%	-
global.wast	105 / 105	0 / 105	100%	-
memory_grow.wast	94 / 94	0 / 94	100%	-
simd_load.wast	25 / 25	0 / 25	100%	-
table_size.wast	38 / 38	0 / 38	100%	-
simd_f32x4_pmin_pmax.wast	3886 / 3886	0 / 3886	100%	-
bulk.wast	66 / 66	0 / 66	100%	-
simd_i16x8_sat_arith.wast	220 / 220	0 / 220	100%	-
simd_i16x8_extadd_pairwise_i8x16.wast	20 / 20	0 / 20	100%	-
table_get.wast	14 / 14	0 / 14	100%	-
simd_bit_shift.wast	250 / 250	0 / 250	100%	-
obsolete-keywords.wast	11 / 11	0 / 11	100%	-
endianness.wast	68 / 68	0 / 68	100%	-
simd_boolean.wast	275 / 275	0 / 275	100%	-
float_memory.wast	60 / 60	0 / 60	100%	-
f64_cmp.wast	2406 / 2406	0 / 2406	100%	-
simd_store.wast	26 / 26	0 / 26	100%	-
simd_i64x2_extmul_i32x4.wast	116 / 116	0 / 116	100%	-
select.wast	146 / 146	0 / 146	100%	-
type.wast	2 / 2	0 / 2	100%	-
simd_f64x2_rounding.wast	200 / 200	0 / 200	100%	-
memory_init.wast	207 / 207	0 / 207	100%	-
simd_load_extend.wast	102 / 102	0 / 102	100%	-
simd_f32x4_cmp.wast	2605 / 2605	0 / 2605	100%	-
f32_cmp.wast	2406 / 2406	0 / 2406	100%	-
simd_load8_lane.wast	51 / 51	0 / 51	100%	-
unwind.wast	49 / 49	0 / 49	100%	-
simd_load_splat.wast	124 / 124	0 / 124	100%	-
memory.wast	77 / 77	0 / 77	100%	-
memory_copy.wast	4402 / 4402	0 / 4402	100%	-
simd_f64x2_cmp.wast	2683 / 2683	0 / 2683	100%	-
simd_load_zero.wast	37 / 37	0 / 37	100%	-
simd_splat.wast	181 / 181	0 / 181	100%	-
conversions.wast	618 / 618	0 / 618	100%	-
memory_trap.wast	180 / 180	0 / 180	100%	-
align.wast	137 / 137	0 / 137	100%	-
simd_load16_lane.wast	35 / 35	0 / 35	100%	-
simd_f32x4_rounding.wast	200 / 200	0 / 200	100%	-
simd_address.wast	46 / 46	0 / 46	100%	-
simd_i64x2_cmp.wast	112 / 112	0 / 112	100%	-
if.wast	240 / 240	0 / 240	100%	-
simd_i8x16_cmp.wast	443 / 443	0 / 443	100%	-
simd_linking.wast	0 / 0	0 / 0	-	-
func_ptrs.wast	32 / 32	0 / 32	100%	-
simd_const.wast	445 / 445	0 / 445	100%	-
utf8-invalid-encoding.wast	176 / 176	0 / 176	100%	-
simd_i32x4_extadd_pairwise_i16x8.wast	20 / 20	0 / 20	100%	-
memory_fill.wast	84 / 84	0 / 84	100%	-
simd_i8x16_arith.wast	129 / 129	0 / 129	100%	-

[codecov]

Codecov Report

❌ Patch coverage is 85.00000% with 3 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/execution/function_ref.rs	60.00%	2 Missing ⚠️
src/execution/store.rs	66.66%	0 Missing and 1 partial ⚠️

Files with missing lines	Coverage Δ
src/execution/config.rs	100.00% <100.00%> (ø)
src/execution/const_interpreter_loop.rs	76.62% <100.00%> (ø)
src/execution/interpreter_loop.rs	87.31% <100.00%> (+<0.01%)	⬆️
src/execution/mod.rs	90.97% <100.00%> (-0.25%)	⬇️
src/execution/store.rs	90.24% <66.66%> (ø)
src/execution/function_ref.rs	70.58% <60.00%> (+7.43%)	⬆️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[florianhartung]

I've made a couple small comments, but the rest looks good to me!

[wucke13]

Please remind me, why do put the T not in the RuntimeInstance but into the Store?

Other than that: I'd vow to directly implement the two limits that we already have (and the third one which we should have) in the config trait already well:

wasm-interpreter/src/execution/value_stack.rs

Lines 10 to 11 in 65af9bd

	const MAX_VALUE_STACK_SIZE: usize = 0xf0000; // 64 Kibi-Values
	const MAX_CALL_STACK_SIZE: usize = 0x1000; // 4 Kibi-Functions

wasm-interpreter/src/execution/linear_memory.rs

Lines 109 to 114 in 65af9bd

	pub fn grow(&self, pages_to_add: PageCountTy) {
	let mut lock_guard = self.inner_data.write();
	let prior_length_bytes = lock_guard.len();
	let new_length_bytes = prior_length_bytes + Self::PAGE_SIZE * pages_to_add as usize;
	lock_guard.resize_with(new_length_bytes, || UnsafeCell::new(0));
	}

Further: shall we add a configuration function pre_allocate()->bool that determines whether the stacks and the lin memory are pre-allocated with their maximum size from the start?

[florianhartung]

@wucke13 We probably want to get rid of the RuntimeInstance type anyway, so I don't see a point in moving the T there right now.

The RuntimeInstance is only a light wrapper around the Store and it generally only causes trouble - at least this is the conclusion Cem and I came to.

[cemonem]

Please remind me, why do put the T not in the RuntimeInstance but into the Store?

When a host function is called, we need to supply config to the host function, I think this is the main reason we store the configuration within the store. Another alternative is supplying config to Store::resume and interpreter_loop::run. I am less comfortable with the latter since config might hold other items pertaining to other stuff like the following you suggested:

Other than that: I'd vow to directly implement the two limits that we already have (and the third one which we should have) in the config trait already well:

wasm-interpreter/src/execution/value_stack.rs

Lines 10 to 11 in 65af9bd

	const MAX_VALUE_STACK_SIZE: usize = 0xf0000; // 64 Kibi-Values
	const MAX_CALL_STACK_SIZE: usize = 0x1000; // 4 Kibi-Functions

wasm-interpreter/src/execution/linear_memory.rs

Lines 109 to 114 in 65af9bd

	pub fn grow(&self, pages_to_add: PageCountTy) {
	let mut lock_guard = self.inner_data.write();
	let prior_length_bytes = lock_guard.len();
	let new_length_bytes = prior_length_bytes + Self::PAGE_SIZE * pages_to_add as usize;
	lock_guard.resize_with(new_length_bytes, || UnsafeCell::new(0));
	}

Yes this is a good idea. I can also add these as type variables to execution::Stack, there seems to be one for linear memory already. I can do this in a subsequent PR (I want to have this part merged in first so that i can work on configuration items seperately in seperate PRs)

Further: shall we add a configuration function pre_allocate()->bool that determines whether the stacks and the lin memory are pre-allocated with their maximum size from the start?

We surely can, I can do this in a subsequent PR (I want to have this part merged in first so that i can work on configuration items seperately in seperate PRs)