From cbf86602c8bbbed95cf166788a14ec4579e4f152 Mon Sep 17 00:00:00 2001 From: TimothyBanks Date: Wed, 16 Sep 2026 14:33:03 -0400 Subject: [PATCH] fix: Use a transient wasm engine to bypass the persisted memory usage on a long lived wasm engine --- crates/xrpl-wasm-vm/src/preflight/mod.rs | 4 +- crates/xrpl-wasm-vm/src/vm.rs | 52 ++++++++++++------------ crates/xrpl-wasm-vm/tests/preflight.rs | 4 +- crates/xrpl-wasm-vm/tests/vm_limits.rs | 14 ++++--- src/benchmarks/libxrpl/wasm/README.md | 31 ++++++-------- src/benchmarks/libxrpl/wasm/Vm.cpp | 9 ++-- src/benchmarks/libxrpl/wasm/WasmBench.h | 2 +- 7 files changed, 57 insertions(+), 59 deletions(-) diff --git a/crates/xrpl-wasm-vm/src/preflight/mod.rs b/crates/xrpl-wasm-vm/src/preflight/mod.rs index 8919faf0fc..91a62561b1 100644 --- a/crates/xrpl-wasm-vm/src/preflight/mod.rs +++ b/crates/xrpl-wasm-vm/src/preflight/mod.rs @@ -27,7 +27,7 @@ use std::fmt; use wasmi::{ExternType, FuncType, Module, ValType}; use xrpl_host_functions::{HOST_MODULE, HostFunctionSpec}; -use crate::vm::{MAX_MEMORY_PAGES, MAX_TABLE_ELEMENTS, compile}; +use crate::vm::{MAX_MEMORY_PAGES, MAX_TABLE_ELEMENTS, compile, wasm_engine}; use signature::check_signature; /// Why a module cannot be run. One variant per stage, since the caller maps the @@ -76,7 +76,7 @@ impl fmt::Display for CheckError { /// the module is built on; the resource caps come last, being a request rather than a /// mistake about the ABI. pub fn check(wasm: &[u8], function_name: &str) -> Result<(), CheckError> { - let module = compile(wasm).map_err(CheckError::Compile)?; + let module = compile(&wasm_engine(), wasm).map_err(CheckError::Compile)?; check_imports(&module)?; check_entry_point(&module, function_name)?; check_exported_resources(&module) diff --git a/crates/xrpl-wasm-vm/src/vm.rs b/crates/xrpl-wasm-vm/src/vm.rs index efc468663f..3b2a5f5524 100644 --- a/crates/xrpl-wasm-vm/src/vm.rs +++ b/crates/xrpl-wasm-vm/src/vm.rs @@ -1,6 +1,5 @@ use std::cell::Cell; use std::fmt; -use std::sync::LazyLock; use wasmi::{ Config, Engine, Export, Linker, Memory, Module, Store, StoreLimits, StoreLimitsBuilder, TrapCode, @@ -165,7 +164,7 @@ impl RunFailure { /// trap and refusal all report it the same way. /// /// `Store::get_fuel` fails only on a store without fuel metering, which -/// [`build_wasm_engine`] rules out and `run`'s `set_fuel` would already have +/// [`wasm_engine`] rules out and `run`'s `set_fuel` would already have /// caught — so a failure here is a defect in this crate. It must not become a /// number: `0` forgives a run its whole cost, `gas` charges an untouched one for /// everything. [`RunError::Internal`] instead. @@ -234,19 +233,17 @@ impl From for RunError { } } -/// The process-wide wasmi engine, built once on first use. +/// A fresh wasmi engine for one caller's use: deterministic, minimal features, +/// fuel metering on. /// -/// The configuration is consensus-fixed and identical for every invocation, and an -/// [`Engine`] is an internally `Arc`ed `Send + Sync` handle, so one shared engine -/// serves concurrent [`run`] calls. -pub(crate) fn wasm_engine() -> &'static Engine { - static ENGINE: LazyLock = LazyLock::new(build_wasm_engine); - &ENGINE -} - -/// Build the wasmi engine the escrow VM requires: deterministic, minimal -/// features, fuel metering on. -fn build_wasm_engine() -> Engine { +/// The configuration is consensus-fixed and identical for every invocation, so any +/// two engines from here accept exactly the same modules. Each is nonetheless a +/// distinct engine with its own compiled-code and type registries, and wasmi ties a +/// [`Module`] to the engine that compiled it — a module cannot be instantiated in a +/// [`Store`] built on another. A caller that compiles and runs must therefore hold +/// one engine across both steps, which is why [`compile`] takes the engine rather +/// than reaching for its own. +pub(crate) fn wasm_engine() -> Engine { let mut config = Config::default(); config.consume_fuel(true); config.ignore_custom_sections(true); @@ -271,7 +268,7 @@ fn build_wasm_engine() -> Engine { /// Every resource ceiling a run is given, in one place. /// /// The two *size* caps are what a contract can reach today. The three *count* caps -/// are set to 1 although [`build_wasm_engine`] already forces each: turning +/// are set to 1 although [`wasm_engine`] already forces each: turning /// `wasm_reference_types` on would let a module declare up to /// `wasmparser::MAX_WASM_TABLES` tables, `wasm_multi_memory` likewise for memories, /// and both size caps are **per table and per memory, not aggregate** — so a feature @@ -292,13 +289,14 @@ fn store_limits() -> StoreLimits { .build() } -/// Compile `wasm` for this engine. +/// Compile `wasm` for `engine`. /// /// The one path to a [`Module`]: the configuration is what decides whether a /// contract is valid at all, so [`run`] and [`crate::check`] must not be able to -/// compile against different ones. -pub(crate) fn compile(wasm: &[u8]) -> Result { - Module::new(wasm_engine(), wasm).map_err(|e| e.to_string()) +/// compile against different ones. The engine is the caller's because the module it +/// returns may only be instantiated in a [`Store`] built on that same engine. +pub(crate) fn compile(engine: &Engine, wasm: &[u8]) -> Result { + Module::new(engine, wasm).map_err(|e| e.to_string()) } /// Run a contract: compile `wasm`, give it `gas` fuel, service its host @@ -310,11 +308,11 @@ pub fn run<'h>( function_name: &str, ) -> Result { let engine = wasm_engine(); - let module = - compile(wasm).map_err(|detail| RunFailure::owing_nothing(RunError::Compile(detail)))?; + let module = compile(&engine, wasm) + .map_err(|detail| RunFailure::owing_nothing(RunError::Compile(detail)))?; let mut store = Store::new( - engine, + &engine, VmState { host, mem_limits: store_limits(), @@ -329,7 +327,7 @@ pub fn run<'h>( .map_err(|_| RunFailure::owing_nothing(RunError::Internal))?; store.limiter(|state| &mut state.mem_limits); - let mut linker = Linker::>::new(engine); + let mut linker = Linker::>::new(&engine); register_host_functions::(&mut linker) .map_err(|_| RunFailure::owing_nothing(RunError::Internal))?; @@ -371,9 +369,13 @@ pub fn run<'h>( mod tests { use super::*; + /// Each call is its own engine, which is what makes the engine a caller's to + /// hold: a module compiled through one may not be instantiated in a store built + /// on another, so `run` must pass the engine it made to [`compile`] rather than + /// let it call here a second time. #[test] - fn the_engine_is_one_engine() { - assert!(Engine::same(wasm_engine(), wasm_engine())); + fn each_call_is_a_new_engine() { + assert!(!Engine::same(&wasm_engine(), &wasm_engine())); } /// One instance, one table, one memory — asserted here rather than through a diff --git a/crates/xrpl-wasm-vm/tests/preflight.rs b/crates/xrpl-wasm-vm/tests/preflight.rs index bd1499f8e3..e8ca77d699 100644 --- a/crates/xrpl-wasm-vm/tests/preflight.rs +++ b/crates/xrpl-wasm-vm/tests/preflight.rs @@ -80,8 +80,8 @@ fn a_text_format_module_does_not_pass() { } /// A feature the engine disables is refused here too, because both stages compile -/// against the one engine. `vm_limits.rs` walks every disabled feature; this pins -/// that screening sees the same configuration. +/// against engines built from the same configuration. `vm_limits.rs` walks every +/// disabled feature; this pins that screening sees that configuration. #[test] fn a_disabled_feature_does_not_pass() { let refusal = refusal(&module( diff --git a/crates/xrpl-wasm-vm/tests/vm_limits.rs b/crates/xrpl-wasm-vm/tests/vm_limits.rs index bcbfa7218d..bc875c3803 100644 --- a/crates/xrpl-wasm-vm/tests/vm_limits.rs +++ b/crates/xrpl-wasm-vm/tests/vm_limits.rs @@ -165,7 +165,7 @@ fn a_declared_table_maximum_past_the_cap_is_allowed_but_unreachable() { // Engine configuration // --------------------------------------------------------------------------- -/// One row per feature `build_wasm_engine` turns off: the smallest module that uses +/// One row per feature `wasm_engine` turns off: the smallest module that uses /// it, and the fragment of wasmi's refusal that names the feature. A row declaring /// its own memory omits [`ONE_PAGE`], or it is refused for having two memories /// instead. @@ -279,9 +279,9 @@ fn every_disabled_feature_is_refused_by_name() { } /// The three knobs [`every_disabled_feature_is_refused_by_name`] cannot cover. The -/// engine is a process-wide `LazyLock`, so a test observes the one configuration -/// `build_wasm_engine` makes: a knob masked by another, or with no caller-visible -/// effect, has no distinguishing module. +/// configuration is the same for every engine `wasm_engine` builds, so a test +/// observes the one `wasm_engine` makes: a knob masked by another, or with no +/// caller-visible effect, has no distinguishing module. #[test] fn the_knobs_without_a_module_of_their_own() { let host = FakeHost::new(); @@ -648,8 +648,10 @@ fn unbounded_recursion_is_stopped_by_the_call_stack_limit() { } /// A module with many functions currently compiles and runs: wasmi's only cap is its -/// 1,000,000 hard limit, so the ticket's ~24k-function module — a CodeMap-growth DoS, since -/// every validation appends to the engine's append-only CodeMap — is not refused here. +/// 1,000,000 hard limit, so the ticket's ~24k-function module is not refused here. Every +/// validation appends to the engine's append-only CodeMap, which a per-call engine now frees +/// when the run ends — so this is a peak-memory cost for the duration of one run rather than +/// the process-lifetime accumulation it was, but nothing bounds that peak. /// Enforcing a tighter bound (a function-count / average-bytes-per-function limit) belongs in /// a future preflight pass that parses the module before the engine sees it. Ignored until /// then, so this documents the gap without asserting it is acceptable. diff --git a/src/benchmarks/libxrpl/wasm/README.md b/src/benchmarks/libxrpl/wasm/README.md index 27700b937f..965cbd10dc 100644 --- a/src/benchmarks/libxrpl/wasm/README.md +++ b/src/benchmarks/libxrpl/wasm/README.md @@ -127,27 +127,22 @@ The linker rebuild and the fuel-metering overhead are **not** separable from her `runEscrowWasm` and `preflightEscrowWasm`. Both need benchmarks inside `xrpl-wasm-vm`, where `compile` and `wasm_engine` are `pub(crate)`. -### Compiling leaks — pin your iteration counts +### Pin your iteration counts -`wasm_engine()` is a process-global `LazyLock`, and what `Module::new` adds to it is never -released. Repeatedly preflighting one **60-byte** module: +`wasm_engine()` returns a fresh `Engine` per call and `run`/`check` drop theirs on return, so +compiles no longer accumulate. Compiling 64-function modules and sampling RSS from `ps`, the cost is +a one-time 0.6 MiB that arrives within the first few thousand compiles and then does not move +through 40,000 — allocator high-water mark, not growth. A shared engine grew by 5.4 KB per compile +without bound, which over the same 40,000 would have been ~216 MiB. That retires the old hazard +here, where automatic sizing reached 7.9 GB resident and every later case in the binary failed to +compile. -| `--benchmark_repetitions` | peak RSS | -| ------------------------- | -------- | -| 1 | 0.41 GB | -| 5 | 1.46 GB | -| 15 | 4.20 GB | +The measurement cycles 2,000 distinct modules, so the allocator sees a repeating size distribution; +a validator meeting varied contract sizes would settle at a somewhat higher mark, still bounded. -Linear, at roughly **800 bytes per compile**. Within the suite this is why every `Vm.cpp` case pins -`->Iterations(...)`: automatic sizing ran `preflightMinimal` ~348k times per repetition, reaching -7.9 GB at 25 repetitions, after which every later case in the binary failed to compile — 720 errored -rows, all blaming cases that were innocent. - -**Outside the suite it is worth a look.** A validator compiles once to screen an `EscrowCreate` -and again for every `EscrowFinish` that runs the contract — with no module cache between them, and -once per apply attempt rather than once per transaction — all against that same static engine. -Whether that is unbounded growth in production depends on wasmi internals not checked here (wasmi -2.0.0, wasmparser 0.228): this is the C++-visible symptom, not a diagnosis. +Pin `->Iterations(...)` anyway: automatic sizing targets a wall-clock budget rather than a compile +count, which gives the cheap cases six-figure counts and `/4096` a handful, and stops the sweep's +rows being comparable to each other or to the last run. ## Gotchas, each of which has already cost someone an afternoon diff --git a/src/benchmarks/libxrpl/wasm/Vm.cpp b/src/benchmarks/libxrpl/wasm/Vm.cpp index dd5ec2e420..353bd8a1b6 100644 --- a/src/benchmarks/libxrpl/wasm/Vm.cpp +++ b/src/benchmarks/libxrpl/wasm/Vm.cpp @@ -12,12 +12,11 @@ namespace { // What a run costs *around* the contract, rather than what its host calls cost. Only the guest's // own execution is metered, so every stage measured here is wall time no transaction pays for. -// ../README.md has what each case measures and how to read `gas_equivalent`. +// README.md has what each case measures and how to read `gas_equivalent`. // -// **Every case must pin `->Iterations(...)`.** Compiling a module allocates against the -// process-global engine and is never reclaimed — roughly 800 bytes per compile — so Google -// Benchmark's automatic sizing, which targets a wall-clock budget rather than a compile count, -// reaches gigabytes resident and the whole binary stops being able to compile anything. +// **Every case pins `->Iterations(...)`.** Automatic sizing targets a wall-clock budget rather than +// a compile count, so it gives the cheap cases six-figure counts and `/4096` a handful, leaving the +// sweep's rows incomparable. // The smallest module the engine accepts. Everything a run does to it is overhead by construction. std::string diff --git a/src/benchmarks/libxrpl/wasm/WasmBench.h b/src/benchmarks/libxrpl/wasm/WasmBench.h index 98f0921be7..3a4b00b665 100644 --- a/src/benchmarks/libxrpl/wasm/WasmBench.h +++ b/src/benchmarks/libxrpl/wasm/WasmBench.h @@ -16,7 +16,7 @@ #include #include -// The gas-calibration harness. What the numbers mean and how to read a report are in ../README.md. +// The gas-calibration harness. What the numbers mean and how to read a report are in README.md. namespace xrpl::test::bench {