From 0a9ede643a105f39bf7e1ffe2eccb490331012fb Mon Sep 17 00:00:00 2001 From: Paul Osborne Date: Wed, 1 Apr 2026 15:14:56 -0500 Subject: [PATCH] Refactor profiling configuration to use unified ProfilingConfig enum Replace the confusing dual profiling configuration system (ProfilingStrategy + GuestProfileConfig) with a single, unified ProfilingConfig enum that cleanly distinguishes between native profiling (PerfMap/JitDump/VTune) and guest profiling (Firefox Profiler format). --- CHANGELOG.md | 1 + cli/src/execute_ctx.rs | 6 +-- cli/src/opts.rs | 49 ++++++++---------- cli/src/subcommands/run.rs | 7 +-- cli/src/subcommands/serve.rs | 14 ++--- cli/tests/integration/common.rs | 20 +++---- cli/tests/integration/profiling.rs | 4 +- src/execute.rs | 83 +++++++++++++++++++++++------- src/lib.rs | 6 ++- src/service.rs | 5 +- 10 files changed, 110 insertions(+), 85 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d3168d35..12b3b663 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ - Add no-op implementations for stale-if-error hostcalls ([#591](https://github.com/fastly/Viceroy/pull/591)) - Guest profiling support added for components. ([#593](https://github.com/fastly/Viceroy/pull/593)) - Add `manifest_version` validation to fastly.toml parsing. ([#590](https://github.com/fastly/Viceroy/pull/590)) +- Internal unification of native/guest profiling config. (#[597](https://github.com/fastly/Viceroy/pull/597)) ## 0.16.4 (2026-01-26) diff --git a/cli/src/execute_ctx.rs b/cli/src/execute_ctx.rs index ae827854..0751c6f3 100644 --- a/cli/src/execute_ctx.rs +++ b/cli/src/execute_ctx.rs @@ -6,7 +6,7 @@ use std::time::Duration; use tokio::time::timeout; use tracing::{Level, Metadata, event}; use tracing_subscriber::fmt::writer::MakeWriter; -use viceroy_lib::{BackendConnector, ExecuteCtx, GuestProfileConfig, config::FastlyConfig}; +use viceroy_lib::{BackendConnector, ExecuteCtx, config::FastlyConfig}; pub(crate) enum Stdio { Stdout(Stdout), @@ -67,14 +67,12 @@ impl<'a> MakeWriter<'a> for StdWriter { pub(crate) async fn create_execution_context( args: &SharedArgs, check_backends: bool, - guest_profile_config: Option, ) -> Result, anyhow::Error> { let input = args.input(); let ctx = ExecuteCtx::build( input, - args.profiling_strategy(), + args.profiling_config(), args.wasi_modules(), - guest_profile_config, args.unknown_import_behavior(), args.adapt(), )? diff --git a/cli/src/opts.rs b/cli/src/opts.rs index c44e170f..238edc7a 100644 --- a/cli/src/opts.rs +++ b/cli/src/opts.rs @@ -2,8 +2,6 @@ use std::time::Duration; -use viceroy_lib::{GuestProfileConfig, config::UnknownImportBehavior}; - use { clap::{Args, Parser, Subcommand, ValueEnum}, std::net::{IpAddr, Ipv4Addr}, @@ -12,7 +10,11 @@ use { net::SocketAddr, path::{Path, PathBuf}, }, - viceroy_lib::{Error, ProfilingStrategy, config::ExperimentalModule}, + viceroy_lib::{ + Error, ProfilingConfig, + config::{ExperimentalModule, UnknownImportBehavior}, + }, + wasmtime::ProfilingStrategy, }; // Command-line arguments for the Viceroy CLI. @@ -179,39 +181,30 @@ impl SharedArgs { self.log_stderr } - /// Whether to enable wasmtime's builtin profiler. - pub fn profiling_strategy(&self) -> ProfilingStrategy { - match self.profile { - Some(Profile::Native(s)) => s, - _ => ProfilingStrategy::None, - } - } - - /// Port running local Pushpin proxy. - pub fn local_pushpin_proxy_port(&self) -> Option { - self.local_pushpin_proxy_port - } - - /// Configuration for guest profiling if enabled - pub fn guest_profile_config(&self) -> Option { - if let Some(Profile::Guest { - path, - sample_period, - }) = &self.profile - { - Some(GuestProfileConfig { + /// Get the unified profiling configuration + pub fn profiling_config(&self) -> ProfilingConfig { + match &self.profile { + Some(Profile::Native(strategy)) => ProfilingConfig::Native(*strategy), + Some(Profile::Guest { + path, + sample_period, + }) => ProfilingConfig::Guest { path: PathBuf::from( path.as_ref() .map(|p| p.as_str()) .unwrap_or("guest-profiles"), ), - sample_period: sample_period.unwrap_or_else(|| Duration::from_micros(50)), - }) - } else { - None + sample_period: sample_period.unwrap_or(Duration::from_micros(50)), + }, + None => ProfilingConfig::None, } } + /// Port running local Pushpin proxy. + pub fn local_pushpin_proxy_port(&self) -> Option { + self.local_pushpin_proxy_port + } + /// Set of experimental wasi modules to link against. pub fn wasi_modules(&self) -> HashSet { self.experimental_modules.iter().map(|x| x.into()).collect() diff --git a/cli/src/subcommands/run.rs b/cli/src/subcommands/run.rs index d29272e4..86ba9cb3 100644 --- a/cli/src/subcommands/run.rs +++ b/cli/src/subcommands/run.rs @@ -24,12 +24,7 @@ pub(crate) async fn exec(run_args: RunArgs) -> ExitCode { /// Execute a Wasm program in the Viceroy environment. 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.shared().guest_profile_config(), - ) - .await?; + let ctx = create_execution_context(run_args.shared(), false).await?; let input = run_args.shared().input(); let program_name = match input.file_stem() { Some(stem) => stem.to_string_lossy(), diff --git a/cli/src/subcommands/serve.rs b/cli/src/subcommands/serve.rs index 521c1e07..59b9ec09 100644 --- a/cli/src/subcommands/serve.rs +++ b/cli/src/subcommands/serve.rs @@ -2,7 +2,7 @@ use crate::opts::ServeArgs; use crate::{create_execution_context, install_tracing_subscriber}; use std::process::ExitCode; use tracing::{Level, event}; -use viceroy_lib::{Error, ViceroyService}; +use viceroy_lib::{Error, ProfilingConfig, ViceroyService}; pub(crate) async fn exec(serve_args: ServeArgs) -> ExitCode { install_tracing_subscriber(serve_args.shared().verbosity()); @@ -29,15 +29,11 @@ pub(crate) async fn exec(serve_args: ServeArgs) -> ExitCode { /// Create a new server, bind it to an address, and serve responses until an error occurs. 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.shared().guest_profile_config(), - ) - .await?; + let ctx = create_execution_context(serve_args.shared(), true).await?; - if let Some(guest_profile_config) = serve_args.shared().guest_profile_config() { - std::fs::create_dir_all(guest_profile_config.path)?; + // Create profile directory if guest profiling is enabled + if let ProfilingConfig::Guest { path, .. } = serve_args.shared().profiling_config() { + std::fs::create_dir_all(path)?; } let addr = serve_args.addr(); diff --git a/cli/tests/integration/common.rs b/cli/tests/integration/common.rs index 9a52b692..56953c8f 100644 --- a/cli/tests/integration/common.rs +++ b/cli/tests/integration/common.rs @@ -14,7 +14,7 @@ use std::{ use tracing_subscriber::filter::EnvFilter; use viceroy_lib::config::UnknownImportBehavior; use viceroy_lib::{ - ExecuteCtx, ProfilingStrategy, ViceroyService, + ExecuteCtx, ProfilingConfig, ViceroyService, body::Body, config::{ Acls, DeviceDetection, Dictionaries, FastlyConfig, Geolocation, ObjectStores, SecretStores, @@ -92,8 +92,7 @@ pub struct Test { via_hyper: bool, unknown_import_behavior: UnknownImportBehavior, adapt_component: bool, - profiling_strategy: ProfilingStrategy, - guest_profile_config: Option, + profiling: ProfilingConfig, } impl Test { @@ -118,8 +117,7 @@ impl Test { via_hyper: false, unknown_import_behavior: Default::default(), adapt_component: false, - profiling_strategy: ProfilingStrategy::None, - guest_profile_config: None, + profiling: ProfilingConfig::None, } } @@ -144,8 +142,7 @@ impl Test { via_hyper: false, unknown_import_behavior: Default::default(), adapt_component: false, - profiling_strategy: ProfilingStrategy::None, - guest_profile_config: None, + profiling: ProfilingConfig::None, } } @@ -293,9 +290,9 @@ impl Test { self } - /// Enable guest profiling with the specified configuration. - pub fn with_guest_profiling(mut self, config: viceroy_lib::GuestProfileConfig) -> Self { - self.guest_profile_config = Some(config); + /// Set the profiling configuration for this test. + pub fn with_profiling(mut self, profiling: ProfilingConfig) -> Self { + self.profiling = profiling; self } @@ -343,9 +340,8 @@ impl Test { let ctx = ExecuteCtx::build( &self.module_path, - self.profiling_strategy.clone(), + self.profiling.clone(), HashSet::new(), - self.guest_profile_config.clone(), self.unknown_import_behavior, self.adapt_component, )? diff --git a/cli/tests/integration/profiling.rs b/cli/tests/integration/profiling.rs index c4fdf436..c4f17173 100644 --- a/cli/tests/integration/profiling.rs +++ b/cli/tests/integration/profiling.rs @@ -7,7 +7,7 @@ use { }, hyper::StatusCode, std::time::Duration, - viceroy_lib::GuestProfileConfig, + viceroy_lib::ProfilingConfig, }; viceroy_test!(guest_profiling_works, |is_component| { @@ -18,7 +18,7 @@ viceroy_test!(guest_profiling_works, |is_component| { let resp = Test::using_fixture("noop.wasm") .adapt_component(is_component) - .with_guest_profiling(GuestProfileConfig { + .with_profiling(ProfilingConfig::Guest { path: profile_dir.clone(), sample_period: Duration::from_micros(50), }) diff --git a/src/execute.rs b/src/execute.rs index d239d9d4..f4c40b68 100644 --- a/src/execute.rs +++ b/src/execute.rs @@ -76,14 +76,56 @@ impl Instance { } } +/// Configuration for profiling Wasm execution. +/// +/// Viceroy supports two types of profiling: +/// - **Guest profiling**: Profiles the WebAssembly guest code itself, producing Firefox profiler compatible JSON. +/// - **Native profiling**: Profiles wasmtime's JIT compiler for debugging across guest, viceroy and wasmtime. #[derive(Clone)] -pub struct GuestProfileConfig { - /// Path to write profiling results from the guest. In serve mode, - /// this must refer to a directory, while in run mode it names - /// a file. - pub path: PathBuf, - /// Period at which the guest should be profiled. - pub sample_period: Duration, +pub enum ProfilingConfig { + /// No profiling enabled + None, + /// Profile the WebAssembly guest code + Guest { + /// Path to write profiling results. In serve mode, this should be a directory + /// where per-request profiles are written. In run mode, this is a single file. + path: PathBuf, + /// Period at which the guest should be sampled (default: 50μs) + sample_period: Duration, + }, + /// Profile wasmtime's JIT compiler using native profiling tools + Native(wasmtime::ProfilingStrategy), +} + +impl ProfilingConfig { + /// Get the native profiling strategy for wasmtime + pub fn native_strategy(&self) -> wasmtime::ProfilingStrategy { + match self { + ProfilingConfig::Native(strategy) => *strategy, + _ => wasmtime::ProfilingStrategy::None, + } + } + + /// Get the guest profile configuration if guest profiling is enabled + fn guest_config(&self) -> Option { + match self { + ProfilingConfig::Guest { + path, + sample_period, + } => Some(GuestProfileConfig { + path: path.clone(), + sample_period: *sample_period, + }), + _ => None, + } + } +} + +// Keep GuestProfileConfig internal for now to maintain backwards compatibility +#[derive(Clone)] +struct GuestProfileConfig { + path: PathBuf, + sample_period: Duration, } pub struct NextRequest(Option<(DownstreamRequest, Arc)>); @@ -165,12 +207,11 @@ pub struct ExecuteCtx { } impl ExecuteCtx { - /// Build a new execution context, given the path to a module and a set of experimental wasi modules. + /// Build a new execution context with unified profiling configuration. pub fn build( module_path: impl AsRef, - profiling_strategy: ProfilingStrategy, + profiling: ProfilingConfig, wasi_modules: HashSet, - guest_profile_config: Option, unknown_import_behavior: UnknownImportBehavior, adapt_components: bool, ) -> Result { @@ -200,7 +241,7 @@ impl ExecuteCtx { (is_wat, is_component, input) }; - let config = &configure_wasmtime(is_component, profiling_strategy); + let config = &configure_wasmtime(is_component, profiling.native_strategy()); let engine = Engine::new(config)?; let instance_pre = if is_component { warn!( @@ -278,7 +319,8 @@ impl ExecuteCtx { let epoch_increment_stop = Arc::new(AtomicBool::new(false)); let engine_clone = engine.clone(); let epoch_increment_stop_clone = epoch_increment_stop.clone(); - let sample_period = guest_profile_config + let sample_period = profiling + .guest_config() .as_ref() .map(|c| c.sample_period) .unwrap_or(DEFAULT_EPOCH_INTERRUPTION_PERIOD); @@ -309,7 +351,7 @@ impl ExecuteCtx { shielding_sites: ShieldingSites::new(), epoch_increment_thread, epoch_increment_stop, - guest_profile_config: guest_profile_config.map(|c| Arc::new(c)), + guest_profile_config: profiling.guest_config().map(Arc::new), cache: Arc::new(Cache::default()), pending_reuse: Arc::new(AsyncMutex::new(vec![])), }; @@ -317,20 +359,20 @@ impl ExecuteCtx { Ok(ExecuteCtxBuilder { inner }) } - /// Create a new execution context, given the path to a module and a set of experimental wasi modules. + /// Create a new execution context with unified profiling configuration. + /// + /// This is a convenience wrapper around `build().finish()`. pub fn new( module_path: impl AsRef, - profiling_strategy: ProfilingStrategy, + profiling: ProfilingConfig, wasi_modules: HashSet, - guest_profile_config: Option, unknown_import_behavior: UnknownImportBehavior, adapt_components: bool, ) -> Result, Error> { ExecuteCtx::build( module_path, - profiling_strategy, + profiling, wasi_modules, - guest_profile_config, unknown_import_behavior, adapt_components, )? @@ -415,11 +457,12 @@ impl ExecuteCtx { /// ```no_run /// # use std::collections::HashSet; /// use hyper::{Body, http::Request}; - /// # use viceroy_lib::{Error, ExecuteCtx, ProfilingStrategy, ViceroyService}; + /// # use viceroy_lib::{Error, ExecuteCtx, ProfilingConfig, ViceroyService}; + /// # use viceroy_lib::config::UnknownImportBehavior; /// # 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", ProfilingConfig::None, HashSet::new(), UnknownImportBehavior::LinkError, 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/src/lib.rs b/src/lib.rs index 1c987b4d..4e3cd651 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -42,6 +42,8 @@ mod upstream; pub mod wiggle_abi; pub use { - error::Error, execute::ExecuteCtx, execute::GuestProfileConfig, service::ViceroyService, - upstream::BackendConnector, wasmtime::ProfilingStrategy, + error::Error, + execute::{ExecuteCtx, ProfilingConfig}, + service::ViceroyService, + upstream::BackendConnector, }; diff --git a/src/service.rs b/src/service.rs index c2c8af01..f46b5d6e 100644 --- a/src/service.rs +++ b/src/service.rs @@ -42,10 +42,11 @@ impl ViceroyService { /// /// ```no_run /// # use std::collections::HashSet; - /// use viceroy_lib::{Error, ExecuteCtx, ProfilingStrategy, ViceroyService}; + /// use viceroy_lib::{Error, ExecuteCtx, ProfilingConfig, ViceroyService}; + /// # use viceroy_lib::config::UnknownImportBehavior; /// # 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", ProfilingConfig::None, HashSet::new(), UnknownImportBehavior::LinkError, adapt_core_wasm)?; /// let svc = ViceroyService::new(ctx); /// # Ok(()) /// # }