From 158116b2e234b80abef25853f2b485398ee9a9ff Mon Sep 17 00:00:00 2001 From: Omar Jatoi Date: Fri, 11 Oct 2024 20:19:31 -0400 Subject: [PATCH 1/9] Add guest profile interval --- cli/src/opts.rs | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/cli/src/opts.rs b/cli/src/opts.rs index 4f34ad78..7d25118c 100644 --- a/cli/src/opts.rs +++ b/cli/src/opts.rs @@ -1,5 +1,7 @@ //! Command line arguments. +use std::time::Duration; + use viceroy_lib::config::UnknownImportBehavior; use { @@ -92,9 +94,10 @@ pub struct SharedArgs { /// /// The `guest` option can be additionally configured as: /// - /// --profile=guest[,path] + /// --profile=guest[,path[,interval]] /// - /// where `path` is the directory or filename to write the profile(s) to. + /// where `path` is the directory or filename to write the profile(s) to + /// and `interval` is the duration between samples. #[arg(long = "profile", value_name = "STRATEGY", value_parser = check_wasmtime_profiler_mode)] profile: Option, /// Set of experimental WASI modules to link against. @@ -120,7 +123,10 @@ pub struct SharedArgs { #[derive(Debug, Clone)] enum Profile { Native(ProfilingStrategy), - Guest { path: Option }, + Guest { + path: Option, + interval: Option, + }, } impl ServeArgs { @@ -132,7 +138,7 @@ impl ServeArgs { /// The path to write guest profiles to pub fn profile_guest(&self) -> Option { - if let Some(Profile::Guest { path }) = &self.shared.profile { + if let Some(Profile::Guest { path, .. }) = &self.shared.profile { Some( path.clone() .unwrap_or_else(|| "guest-profiles".to_string()) @@ -143,6 +149,15 @@ impl ServeArgs { } } + /// The interval for guest profiling + pub fn profile_guest_interval(&self) -> Option { + if let Some(Profile::Guest { interval, .. }) = &self.shared.profile { + *interval + } else { + None + } + } + pub fn shared(&self) -> &SharedArgs { &self.shared } From 882a9ffe66474aa08eff5523f04e171c055be84b Mon Sep 17 00:00:00 2001 From: Omar Jatoi Date: Fri, 11 Oct 2024 20:21:38 -0400 Subject: [PATCH 2/9] Update the profiler mode checking function Add match case for when interval is None, and when one exists --- cli/src/opts.rs | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/cli/src/opts.rs b/cli/src/opts.rs index 7d25118c..e07921bb 100644 --- a/cli/src/opts.rs +++ b/cli/src/opts.rs @@ -338,10 +338,21 @@ fn check_wasmtime_profiler_mode(s: &str) -> Result { ["jitdump"] => Ok(Profile::Native(ProfilingStrategy::JitDump)), ["perfmap"] => Ok(Profile::Native(ProfilingStrategy::PerfMap)), ["vtune"] => Ok(Profile::Native(ProfilingStrategy::VTune)), - ["guest"] => Ok(Profile::Guest { path: None }), + ["guest"] => Ok(Profile::Guest { + path: None, + interval: None, + }), ["guest", path] => Ok(Profile::Guest { path: Some(path.to_string()), + interval: None, }), + ["guest", path, interval] => { + let interval_ms = interval.parse().map_err(|_| Error::ProfilingStrategy)?; + Ok(Profile::Guest { + path: Some(path.to_string()), + interval: Some(Duration::from_millis(interval_ms)), + }) + } _ => Err(Error::ProfilingStrategy), } } From 8b65baaa46a25a50fe42309a088a625fbcbc3e2a Mon Sep 17 00:00:00 2001 From: Omar Jatoi Date: Fri, 11 Oct 2024 20:20:50 -0400 Subject: [PATCH 3/9] Add `profile_guest_interval()` to RunArgs aswell also update `profile_guest()`, ignore interval in that function --- cli/src/opts.rs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/cli/src/opts.rs b/cli/src/opts.rs index e07921bb..55a630bb 100644 --- a/cli/src/opts.rs +++ b/cli/src/opts.rs @@ -175,7 +175,7 @@ impl RunArgs { /// The path to write a guest profile to pub fn profile_guest(&self) -> Option { - if let Some(Profile::Guest { path }) = &self.shared.profile { + if let Some(Profile::Guest { path, .. }) = &self.shared.profile { Some( path.clone() .unwrap_or_else(|| "guest-profile.json".to_string()) @@ -185,6 +185,15 @@ impl RunArgs { None } } + + /// The interval for guest profiling + pub fn profile_guest_interval(&self) -> Option { + if let Some(Profile::Guest { interval, .. }) = &self.shared.profile { + *interval + } else { + None + } + } } impl SharedArgs { From f2e07d02bdbb13c4d0e58baa22217de8081343a2 Mon Sep 17 00:00:00 2001 From: Omar Jatoi Date: Fri, 11 Oct 2024 20:23:03 -0400 Subject: [PATCH 4/9] Add profile interval in ExecuteCtx --- lib/src/execute.rs | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/lib/src/execute.rs b/lib/src/execute.rs index a8aaee0c..6655776b 100644 --- a/lib/src/execute.rs +++ b/lib/src/execute.rs @@ -103,6 +103,8 @@ pub struct ExecuteCtx { /// this must refer to a directory, while in run mode it names /// a file. guest_profile_path: Arc>, + /// Duration for the interval between samples. + guest_profile_interval: Arc>, } impl ExecuteCtx { @@ -112,6 +114,7 @@ impl ExecuteCtx { profiling_strategy: ProfilingStrategy, wasi_modules: HashSet, guest_profile_path: Option, + guest_profile_interval: Option, unknown_import_behavior: UnknownImportBehavior, adapt_components: bool, ) -> Result { @@ -223,6 +226,7 @@ impl ExecuteCtx { epoch_increment_thread, epoch_increment_stop, guest_profile_path: Arc::new(guest_profile_path), + guest_profile_interval: Arc::new(guest_profile_interval), }) } @@ -536,9 +540,13 @@ impl ExecuteCtx { Instance::Module(module, instance_pre) => { let profiler = self.guest_profile_path.is_some().then(|| { let program_name = "main"; + let interval = self + .guest_profile_interval + .as_ref() + .unwrap_or(EPOCH_INTERRUPTION_PERIOD); GuestProfiler::new( program_name, - EPOCH_INTERRUPTION_PERIOD, + interval, vec![(program_name.to_string(), module.clone())], ) }); @@ -636,9 +644,13 @@ impl ExecuteCtx { let (module, instance_pre) = self.instance_pre.unwrap_module(); let profiler = self.guest_profile_path.is_some().then(|| { + let interval = self + .guest_profile_interval + .as_ref() + .unwrap_or(EPOCH_INTERRUPTION_PERIOD); GuestProfiler::new( program_name, - EPOCH_INTERRUPTION_PERIOD, + interval, vec![(program_name.to_string(), module.clone())], ) }); From 927c59c1e1fd294fab025525dd147cb2c37b0fa1 Mon Sep 17 00:00:00 2001 From: Omar Jatoi Date: Fri, 11 Oct 2024 20:22:27 -0400 Subject: [PATCH 5/9] Pass profiler interval when starting Viceroy --- cli/src/main.rs | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/cli/src/main.rs b/cli/src/main.rs index 7df98cae..7bb8ff84 100644 --- a/cli/src/main.rs +++ b/cli/src/main.rs @@ -40,8 +40,13 @@ use { /// Create a new server, bind it to an address, and serve responses until an error occurs. pub async fn serve(serve_args: ServeArgs) -> Result<(), Error> { // Load the wasm module into an execution context - let ctx = - create_execution_context(serve_args.shared(), true, serve_args.profile_guest()).await?; + let ctx = create_execution_context( + serve_args.shared(), + true, + serve_args.profile_guest(), + serve_args.profile_guest_interval(), + ) + .await?; if let Some(guest_profile_path) = serve_args.profile_guest() { std::fs::create_dir_all(guest_profile_path)?; @@ -161,7 +166,13 @@ pub async fn main() -> ExitCode { /// Execute a Wasm program in the Viceroy environment. pub async fn run_wasm_main(run_args: RunArgs) -> Result<(), anyhow::Error> { // Load the wasm module into an execution context - let ctx = create_execution_context(run_args.shared(), false, run_args.profile_guest()).await?; + let ctx = create_execution_context( + run_args.shared(), + false, + run_args.profile_guest(), + run_args.profile_guest_interval(), + ) + .await?; let input = run_args.shared().input(); let program_name = match input.file_stem() { Some(stem) => stem.to_string_lossy(), @@ -297,6 +308,7 @@ async fn create_execution_context( args: &SharedArgs, check_backends: bool, guest_profile_path: Option, + guest_profile_interval: Option, ) -> Result { let input = args.input(); let mut ctx = ExecuteCtx::new( @@ -304,6 +316,7 @@ async fn create_execution_context( args.profiling_strategy(), args.wasi_modules(), guest_profile_path, + guest_profile_interval, args.unknown_import_behavior(), args.adapt(), )? From 13f2cd22a1099e9bd96d8288bcda1386f208c826 Mon Sep 17 00:00:00 2001 From: Omar Jatoi Date: Fri, 11 Oct 2024 20:27:14 -0400 Subject: [PATCH 6/9] Update documentation to specify milliseconds --- cli/src/opts.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/src/opts.rs b/cli/src/opts.rs index 55a630bb..dc0c0bd9 100644 --- a/cli/src/opts.rs +++ b/cli/src/opts.rs @@ -97,7 +97,7 @@ pub struct SharedArgs { /// --profile=guest[,path[,interval]] /// /// where `path` is the directory or filename to write the profile(s) to - /// and `interval` is the duration between samples. + /// and `interval` is the duration in milliseconds between samples. #[arg(long = "profile", value_name = "STRATEGY", value_parser = check_wasmtime_profiler_mode)] profile: Option, /// Set of experimental WASI modules to link against. From 1a88bd0cace0c23ee7cfe28827994272d18256f9 Mon Sep 17 00:00:00 2001 From: Omar Jatoi Date: Fri, 11 Oct 2024 20:46:33 -0400 Subject: [PATCH 7/9] Update ExecuteCtx arguments in tests Pass `None` for profiling interval in tests --- cli/tests/integration/common.rs | 1 + cli/tests/trap-test/src/main.rs | 1 + 2 files changed, 2 insertions(+) diff --git a/cli/tests/integration/common.rs b/cli/tests/integration/common.rs index f2c5239f..03e91564 100644 --- a/cli/tests/integration/common.rs +++ b/cli/tests/integration/common.rs @@ -325,6 +325,7 @@ impl Test { ProfilingStrategy::None, HashSet::new(), None, + None, self.unknown_import_behavior, self.adapt_component, )? diff --git a/cli/tests/trap-test/src/main.rs b/cli/tests/trap-test/src/main.rs index 4cdc0318..46046437 100644 --- a/cli/tests/trap-test/src/main.rs +++ b/cli/tests/trap-test/src/main.rs @@ -20,6 +20,7 @@ async fn fatal_error_traps_impl(adapt_core_wasm: bool) -> TestResult { ProfilingStrategy::None, HashSet::new(), None, + None, viceroy_lib::config::UnknownImportBehavior::LinkError, adapt_core_wasm, )?; From 86f7c58dc6f1669c278533de728d1e936d85844c Mon Sep 17 00:00:00 2001 From: Omar Jatoi Date: Fri, 11 Oct 2024 20:47:30 -0400 Subject: [PATCH 8/9] Update example code in comments ExecuteCtx::new takes an additional argument now, namely the profiling interval. --- lib/src/execute.rs | 2 +- lib/src/service.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/src/execute.rs b/lib/src/execute.rs index 6655776b..2ab65281 100644 --- a/lib/src/execute.rs +++ b/lib/src/execute.rs @@ -356,7 +356,7 @@ impl ExecuteCtx { /// # async fn f() -> Result<(), Error> { /// # let req = Request::new(Body::from("")); /// let adapt_core_wasm = false; - /// let ctx = ExecuteCtx::new("path/to/a/file.wasm", ProfilingStrategy::None, HashSet::new(), None, Default::default(), adapt_core_wasm)?; + /// let ctx = ExecuteCtx::new("path/to/a/file.wasm", ProfilingStrategy::None, HashSet::new(), None, None, Default::default(), adapt_core_wasm)?; /// let local = "127.0.0.1:80".parse().unwrap(); /// let remote = "127.0.0.1:0".parse().unwrap(); /// let resp = ctx.handle_request(req, local, remote).await?; diff --git a/lib/src/service.rs b/lib/src/service.rs index 1f33a4e4..5fd8f06c 100644 --- a/lib/src/service.rs +++ b/lib/src/service.rs @@ -44,7 +44,7 @@ impl ViceroyService { /// use viceroy_lib::{Error, ExecuteCtx, ProfilingStrategy, ViceroyService}; /// # fn f() -> Result<(), Error> { /// let adapt_core_wasm = false; - /// let ctx = ExecuteCtx::new("path/to/a/file.wasm", ProfilingStrategy::None, HashSet::new(), None, Default::default(), adapt_core_wasm)?; + /// let ctx = ExecuteCtx::new("path/to/a/file.wasm", ProfilingStrategy::None, HashSet::new(), None, None, Default::default(), adapt_core_wasm)?; /// let svc = ViceroyService::new(ctx); /// # Ok(()) /// # } From 6042fe98c9895680dcbaf1c01d74306ccfe77494 Mon Sep 17 00:00:00 2001 From: Omar Jatoi Date: Fri, 25 Oct 2024 16:05:28 -0400 Subject: [PATCH 9/9] Support humantime compatible profile interval - Should be allowed to pass interval as `100ms` or `100` and default to milliseconds --- Cargo.lock | 1 + cli/Cargo.toml | 1 + cli/src/opts.rs | 18 +++++++++++++++--- 3 files changed, 17 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 48b8fbfb..fe03cd42 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2417,6 +2417,7 @@ dependencies = [ "base64", "clap", "futures", + "humantime", "hyper", "itertools 0.10.5", "libc", diff --git a/cli/Cargo.toml b/cli/Cargo.toml index 1756d15d..6354b208 100644 --- a/cli/Cargo.toml +++ b/cli/Cargo.toml @@ -50,6 +50,7 @@ wat = "^1.0.38" wasmtime = { workspace = true } wasmtime-wasi = { workspace = true } libc = "^0.2.139" +humantime = "2.1.0" [dev-dependencies] anyhow = { workspace = true } diff --git a/cli/src/opts.rs b/cli/src/opts.rs index dc0c0bd9..32e1e883 100644 --- a/cli/src/opts.rs +++ b/cli/src/opts.rs @@ -1,6 +1,6 @@ //! Command line arguments. -use std::time::Duration; +use std::{str::FromStr, time::Duration}; use viceroy_lib::config::UnknownImportBehavior; @@ -356,16 +356,28 @@ fn check_wasmtime_profiler_mode(s: &str) -> Result { interval: None, }), ["guest", path, interval] => { - let interval_ms = interval.parse().map_err(|_| Error::ProfilingStrategy)?; + let interval_duration = parse_duration(interval)?; Ok(Profile::Guest { path: Some(path.to_string()), - interval: Some(Duration::from_millis(interval_ms)), + interval: Some(interval_duration), }) } _ => Err(Error::ProfilingStrategy), } } +/// Parse duration from either a string slice as with a duration unit (e.g. 100ms) +/// or without (e.g. 100) and default to milliseconds. +fn parse_duration(dur: &str) -> Result { + if let Ok(duration) = humantime::Duration::from_str(dur) { + return Ok(duration.into()); + } + if let Ok(millis) = dur.parse::() { + return Ok(std::time::Duration::from_millis(millis)); + } + Err(Error::ProfilingStrategy) +} + /// A collection of unit tests for our CLI argument parsing. /// /// Note: When using [`Clap::try_parse_from`][from] to test how command line arguments are