Ability to disable cache after it's been configured on Config (#5542)

* Wasmtime: Add `Config::disable_cache`

* bench-api: Always disable the cache

* bench-api: Always get a `Config` from CLI flags

This commit fixes an issue that I ran into just now where benchmarking
one `*.so` with `--engine-flags` was giving wildly unexpected results
comparing to something without `--engine-flags`. The root cause here
appears to that when specifying `--engine-flags` the CLI parsing code is
used to create a `Config` and when omitted a `Config::new` instance is
created. The main difference between these is that for the CLI caching
is enabled by default and for `Config::new` it is not. Coupled with the
fact that caching doesn't really work for the `main` branch this ended
up giving wild results.

The fix here is to first always use the CLI parsing code to create a
`Config` to ensure that a config is consistently created. Next the
`--disable-cache` flag is unconditionally passed to the CLI parsing to
ensure that compilation actually happens.

Once applied this enables comparing an engine without flags and an
engine with flags which provides consistent results.

* Fix compile error

Co-authored-by: Alex Crichton <alex@alexcrichton.com>

Cargo.lock

@@ -3405,6 +3405,7 @@ version = "6.0.0"
 dependencies = [
  "anyhow",
  "cap-std",
+ "clap 3.2.8",
  "shuffling-allocator",
  "target-lexicon",
  "wasi-cap-std-sync",

crates/bench-api/Cargo.toml

@@ -26,6 +26,7 @@ wasmtime-wasi-crypto = { workspace = true, optional = true }
 wasmtime-wasi-nn = { workspace = true, optional = true }
 wasi-cap-std-sync = { workspace = true }
 cap-std = { workspace = true }
+clap = { workspace = true }
 
 [dev-dependencies]
 wat = { workspace = true }

crates/bench-api/src/lib.rs

@@ -137,11 +137,12 @@ mod unsafe_send_sync;
 
 use crate::unsafe_send_sync::UnsafeSendSync;
 use anyhow::{Context, Result};
+use clap::Parser;
 use std::os::raw::{c_int, c_void};
 use std::slice;
 use std::{env, path::PathBuf};
 use target_lexicon::Triple;
-use wasmtime::{Config, Engine, Instance, Linker, Module, Store};
+use wasmtime::{Engine, Instance, Linker, Module, Store};
 use wasmtime_cli_flags::{CommonOptions, WasiModules};
 use wasmtime_wasi::{sync::WasiCtxBuilder, I32Exit, WasiCtx};
 
@@ -238,19 +239,23 @@ impl WasmBenchConfig {
         Ok(Some(stdin_path.into()))
     }
 
-    fn execution_flags(&self) -> Result<Option<CommonOptions>> {
-        if self.execution_flags_ptr.is_null() {
-            return Ok(None);
-        }
-
-        let execution_flags = unsafe {
-            std::slice::from_raw_parts(self.execution_flags_ptr, self.execution_flags_len)
+    fn execution_flags(&self) -> Result<CommonOptions> {
+        let flags = if self.execution_flags_ptr.is_null() {
+            ""
+        } else {
+            let execution_flags = unsafe {
+                std::slice::from_raw_parts(self.execution_flags_ptr, self.execution_flags_len)
+            };
+            std::str::from_utf8(execution_flags)
+                .context("given execution flags string is not valid UTF-8")?
         };
-        let execution_flags = std::str::from_utf8(execution_flags)
-            .context("given execution flags string is not valid UTF-8")?;
-
-        let options = CommonOptions::parse_from_str(execution_flags)?;
-        Ok(Some(options))
+        let options = CommonOptions::try_parse_from(
+            ["wasmtime"]
+                .into_iter()
+                .chain(flags.split(' ').filter(|s| !s.is_empty())),
+        )
+        .context("failed to parse options")?;
+        Ok(options)
     }
 }
 
@@ -420,7 +425,7 @@ struct HostState {
 
 impl BenchState {
     fn new(
-        options: Option<CommonOptions>,
+        options: CommonOptions,
         compilation_timer: *mut u8,
         compilation_start: extern "C" fn(*mut u8),
         compilation_end: extern "C" fn(*mut u8),
@@ -432,12 +437,9 @@ impl BenchState {
         execution_end: extern "C" fn(*mut u8),
         make_wasi_cx: impl FnMut() -> Result<WasiCtx> + 'static,
     ) -> Result<Self> {
-        let config = if let Some(o) = &options {
-            o.config(Some(&Triple::host().to_string()))?
-        } else {
-            Config::new()
-        };
-        // NB: do not configure a code cache.
+        let mut config = options.config(Some(&Triple::host().to_string()))?;
+        // NB: always disable the compilation cache.
+        config.disable_cache();
         let engine = Engine::new(&config)?;
         let mut linker = Linker::<HostState>::new(&engine);
 
@@ -456,17 +458,10 @@ impl BenchState {
             Ok(())
         })?;
 
-        let mut epoch_interruption = false;
-        let mut fuel = None;
-        if let Some(opts) = &options {
-            epoch_interruption = opts.epoch_interruption;
-            fuel = opts.fuel;
-        }
+        let epoch_interruption = options.epoch_interruption;
+        let fuel = options.fuel;
 
-        let wasi_modules = options
-            .map(|o| o.wasi_modules)
-            .flatten()
-            .unwrap_or(WasiModules::default());
+        let wasi_modules = options.wasi_modules.unwrap_or(WasiModules::default());
 
         if wasi_modules.wasi_common {
             wasmtime_wasi::add_to_linker(&mut linker, |cx| &mut cx.wasi)?;

crates/cli-flags/src/lib.rs

@@ -16,7 +16,7 @@
     )
 )]
 
-use anyhow::{bail, Context, Result};
+use anyhow::{bail, Result};
 use clap::Parser;
 use std::collections::HashMap;
 use std::path::PathBuf;
@@ -234,17 +234,6 @@ pub struct CommonOptions {
 }
 
 impl CommonOptions {
-    pub fn parse_from_str(s: &str) -> Result<Self> {
-        let parts = s.split(" ").filter(|s| !s.is_empty());
-        // The first argument is the name of the executable, which we don't use
-        // here, but have to provide because `clap` skips over it, and otherwise
-        // our first CLI flag will be ignored.
-        let parts = Some("wasmtime").into_iter().chain(parts);
-        let options =
-            Self::try_parse_from(parts).context("unable to parse options from passed flags")?;
-        Ok(options)
-    }
-
     pub fn init_logging(&self) {
         if self.disable_logging {
             return;
@@ -723,24 +712,4 @@ mod test {
             }
         );
     }
-
-    #[test]
-    fn test_parse_from_str() {
-        fn use_func(flags: &str) -> CommonOptions {
-            CommonOptions::parse_from_str(flags).unwrap()
-        }
-        fn use_clap_parser(flags: &[&str]) -> CommonOptions {
-            CommonOptions::try_parse_from(flags).unwrap()
-        }
-
-        assert_eq!(use_func(""), use_clap_parser(&[]));
-        assert_eq!(
-            use_func("--wasm-features=threads"),
-            use_clap_parser(&["foo", "--wasm-features=threads"])
-        );
-        assert_eq!(
-            use_func("--cranelift-set enable_simd=true"),
-            use_clap_parser(&["foo", "--cranelift-set", "enable_simd=true"])
-        );
-    }
 }

crates/wasmtime/src/config.rs

@@ -946,6 +946,23 @@ impl Config {
         Ok(self)
     }
 
+    /// Disable caching.
+    ///
+    /// Every call to [`Module::new(my_wasm)`][crate::Module::new] will
+    /// recompile `my_wasm`, even when it is unchanged.
+    ///
+    /// By default, new configs do not have caching enabled. This method is only
+    /// useful for disabling a previous cache configuration.
+    ///
+    /// This method is only available when the `cache` feature of this crate is
+    /// enabled.
+    #[cfg(feature = "cache")]
+    #[cfg_attr(nightlydoc, doc(cfg(feature = "cache")))]
+    pub fn disable_cache(&mut self) -> &mut Self {
+        self.cache_config = CacheConfig::new_cache_disabled();
+        self
+    }
+
     /// Loads cache configuration from the system default path.
     ///
     /// This commit is the same as [`Config::cache_config_load`] except that it