From 32db8b9cb6636fc67831857224723843c8df6346 Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Wed, 14 Jan 2026 22:24:01 -0700 Subject: [PATCH 1/9] refactor(profiling): simplify SystemSettings --- profiling/src/config.rs | 164 +++++++++++++--------------------------- profiling/src/lib.rs | 17 +++-- 2 files changed, 64 insertions(+), 117 deletions(-) diff --git a/profiling/src/config.rs b/profiling/src/config.rs index 895d8f1412b..eab7562a7e0 100644 --- a/profiling/src/config.rs +++ b/profiling/src/config.rs @@ -8,7 +8,7 @@ use crate::bindings::{ }; use crate::zend::zai_str_from_zstr; use core::fmt::{Display, Formatter}; -use core::mem::{swap, transmute, MaybeUninit}; +use core::mem::transmute; use core::ptr; use core::str::FromStr; use libc::{c_char, c_int}; @@ -41,16 +41,25 @@ pub struct SystemSettings { } impl SystemSettings { - pub fn disable_all(&mut self) { - self.profiling_enabled = false; - self.profiling_experimental_features_enabled = false; - self.profiling_endpoint_collection_enabled = false; - self.profiling_experimental_cpu_time_enabled = false; - self.profiling_allocation_enabled = false; - self.profiling_timeline_enabled = false; - self.profiling_exception_enabled = false; - self.profiling_exception_message_enabled = false; - self.profiling_io_enabled = false; + /// Provides "initial" settings, which are all "off"-like values. + pub const fn initial() -> SystemSettings { + SystemSettings { + profiling_enabled: false, + profiling_experimental_features_enabled: false, + profiling_endpoint_collection_enabled: false, + profiling_experimental_cpu_time_enabled: false, + profiling_allocation_enabled: false, + profiling_allocation_sampling_distance: u32::MAX, + profiling_timeline_enabled: false, + profiling_exception_enabled: false, + profiling_exception_message_enabled: false, + profiling_wall_time_enabled: false, + profiling_io_enabled: false, + output_pprof: None, + profiling_exception_sampling_distance: u32::MAX, + profiling_log_level: LevelFilter::Off, + uri: AgentEndpoint::Socket(Cow::Borrowed(AgentEndpoint::DEFAULT_UNIX_SOCKET_PATH)), + } } /// # Safety @@ -83,15 +92,16 @@ impl SystemSettings { } } -static mut SYSTEM_SETTINGS: MaybeUninit = MaybeUninit::uninit(); +static mut SYSTEM_SETTINGS: SystemSettings = SystemSettings::initial(); impl SystemSettings { - /// # Safety - /// Must be called after [first_rinit] and before [shutdown]. - pub unsafe fn get() -> ptr::NonNull { - // SAFETY: required by this function's own safety requirements. - let addr = unsafe { (*ptr::addr_of_mut!(SYSTEM_SETTINGS)).assume_init_mut() }; - ptr::NonNull::from(addr) + /// Returns the "current" system settings, which are always memory-safe + /// but may point to "initial" values rather than the configured ones, + /// depending on what point in the lifecycle we're at. + pub const fn get() -> ptr::NonNull { + let addr = ptr::addr_of_mut!(SYSTEM_SETTINGS); + // SAFETY: it's derived from a static variable, it's not null. + unsafe { ptr::NonNull::new_unchecked(addr) } } /// # Safety @@ -110,10 +120,6 @@ impl SystemSettings { } } - // Initialize the lazy lock holding the env var for new origin - // detection in a safe place. - _ = std::sync::LazyLock::force(&libdd_common::entity_id::DD_EXTERNAL_ENV); - // Work around version-specific issues. #[cfg(not(php_zend_mm_set_custom_handlers_ex))] if allocation::allocation_le83::first_rinit_should_disable_due_to_jit() { @@ -123,54 +129,21 @@ impl SystemSettings { if allocation::allocation_ge84::first_rinit_should_disable_due_to_jit() { system_settings.profiling_allocation_enabled = false; } - swap( - &mut system_settings, - (*ptr::addr_of_mut!(SYSTEM_SETTINGS)).assume_init_mut(), - ); - } - /// # Safety - /// Must be called exactly once each startup in either minit or startup, - /// whether profiling is enabled or not. - unsafe fn on_startup() { - (*ptr::addr_of_mut!(SYSTEM_SETTINGS)).write(INITIAL_SYSTEM_SETTINGS.clone()); + ptr::addr_of_mut!(SYSTEM_SETTINGS).swap(&mut system_settings); } /// # Safety /// Must be called exactly once per shutdown in either mshutdown or /// shutdown, before zai config is shutdown. unsafe fn on_shutdown() { - let system_settings = (*ptr::addr_of_mut!(SYSTEM_SETTINGS)).assume_init_mut(); - *system_settings = SystemSettings { - profiling_enabled: false, - profiling_experimental_features_enabled: false, - profiling_endpoint_collection_enabled: false, - profiling_experimental_cpu_time_enabled: false, - profiling_allocation_enabled: false, - profiling_allocation_sampling_distance: 0, - profiling_timeline_enabled: false, - profiling_exception_enabled: false, - profiling_exception_message_enabled: false, - profiling_wall_time_enabled: false, - profiling_io_enabled: false, - output_pprof: None, - profiling_exception_sampling_distance: 0, - profiling_log_level: LevelFilter::Off, - uri: Default::default(), - }; + let system_settings = &mut *ptr::addr_of_mut!(SYSTEM_SETTINGS); + *system_settings = SystemSettings::initial(); } unsafe fn on_fork_in_child() { - let system_settings = (*ptr::addr_of_mut!(SYSTEM_SETTINGS)).assume_init_mut(); - system_settings.profiling_enabled = false; - system_settings.profiling_experimental_features_enabled = false; - system_settings.profiling_endpoint_collection_enabled = false; - system_settings.profiling_experimental_cpu_time_enabled = false; - system_settings.profiling_allocation_enabled = false; - system_settings.profiling_timeline_enabled = false; - system_settings.profiling_exception_enabled = false; - system_settings.profiling_exception_message_enabled = false; - system_settings.profiling_io_enabled = false; + let system_settings = &mut *ptr::addr_of_mut!(SYSTEM_SETTINGS); + *system_settings = SystemSettings::initial(); } } @@ -448,55 +421,24 @@ impl ConfigId { } } -lazy_static::lazy_static! { - /// In some SAPIs, full configuration is not known until the first request - /// is served. This is ripe for edge cases. Consider this order of events: - /// 1. Worker is created. - /// 2. No requests are served. - /// 3. As the worker shuts down, the timeline profiler will attempt to - /// add idle time to timeline. - /// What state should the configuration be in? - /// - /// Since the real configuration was never learned, assume everything is - /// disabled, which should cause fewer issues for customers than assuming - /// defaults. - pub static ref INITIAL_SYSTEM_SETTINGS: SystemSettings = SystemSettings { - profiling_enabled: false, - profiling_experimental_features_enabled: false, - profiling_endpoint_collection_enabled: false, - profiling_experimental_cpu_time_enabled: false, - profiling_allocation_enabled: false, - profiling_allocation_sampling_distance: u32::MAX, - profiling_timeline_enabled: false, - profiling_exception_enabled: false, - profiling_exception_message_enabled: false, - profiling_wall_time_enabled: false, - profiling_io_enabled: false, - output_pprof: None, - profiling_exception_sampling_distance: u32::MAX, - profiling_log_level: LevelFilter::Off, - uri: Default::default(), - }; - - /// Keep these in sync with the INI defaults. - static ref DEFAULT_SYSTEM_SETTINGS: SystemSettings = SystemSettings { - profiling_enabled: true, - profiling_experimental_features_enabled: false, - profiling_endpoint_collection_enabled: true, - profiling_experimental_cpu_time_enabled: true, - profiling_allocation_enabled: true, - profiling_allocation_sampling_distance: 1024 * 4096, - profiling_timeline_enabled: true, - profiling_exception_enabled: true, - profiling_exception_message_enabled: false, - profiling_wall_time_enabled: true, - profiling_io_enabled: false, - output_pprof: None, - profiling_exception_sampling_distance: 100, - profiling_log_level: LevelFilter::Off, - uri: Default::default(), - }; -} +/// Keep these in sync with the INI defaults. +static DEFAULT_SYSTEM_SETTINGS: SystemSettings = SystemSettings { + profiling_enabled: true, + profiling_experimental_features_enabled: false, + profiling_endpoint_collection_enabled: true, + profiling_experimental_cpu_time_enabled: true, + profiling_allocation_enabled: true, + profiling_allocation_sampling_distance: 1024 * 4096, + profiling_timeline_enabled: true, + profiling_exception_enabled: true, + profiling_exception_message_enabled: false, + profiling_wall_time_enabled: true, + profiling_io_enabled: false, + output_pprof: None, + profiling_exception_sampling_distance: 100, + profiling_log_level: LevelFilter::Off, + uri: AgentEndpoint::Socket(Cow::Borrowed(AgentEndpoint::DEFAULT_UNIX_SOCKET_PATH)), +}; /// # Safety /// This function must only be called after config has been initialized in @@ -1244,7 +1186,8 @@ pub(crate) fn minit(module_number: libc::c_int) { ); assert!(tmp); // It's literally return true in the source. - SystemSettings::on_startup(); + let system_settings = &mut *ptr::addr_of_mut!(SYSTEM_SETTINGS); + *system_settings = SystemSettings::initial(); } } @@ -1268,6 +1211,7 @@ pub(crate) unsafe fn on_fork_in_child() { #[cfg(test)] mod tests { use super::*; + use core::mem::MaybeUninit; use libc::memcmp; #[test] diff --git a/profiling/src/lib.rs b/profiling/src/lib.rs index c412b67ad1f..2fb8bfcc437 100644 --- a/profiling/src/lib.rs +++ b/profiling/src/lib.rs @@ -22,7 +22,7 @@ mod exception; mod timeline; mod vec_ext; -use crate::config::{SystemSettings, INITIAL_SYSTEM_SETTINGS}; +use crate::config::SystemSettings; use crate::zend::datadog_sapi_globals_request_info; use bindings::{ self as zend, ddog_php_prof_php_version, ddog_php_prof_php_version_id, ZendExtension, @@ -323,6 +323,9 @@ extern "C" fn minit(_type: c_int, module_number: c_int) -> ZendResult { let _connector = libdd_common::connector::Connector::default(); } + // Initialize the lazy lock holding the env var for new origin detection. + _ = std::sync::LazyLock::force(&libdd_common::entity_id::DD_EXTERNAL_ENV); + // Use a hybrid extension hack to load as a module but have the // zend_extension hooks available: // https://www.phpinternalsbook.com/php7/extensions_design/zend_extensions.html#hybrid-extensions @@ -423,7 +426,7 @@ pub struct RequestLocals { /// conditions such as in mshutdown when there were no requests served, /// then the settings are still memory safe, but they may not have the /// real configuration. Instead, they have a best-effort values such as - /// INITIAL_SYSTEM_SETTINGS, or possibly the values which were available + /// the initial settings, or possibly the values which were available /// in MINIT. pub system_settings: ptr::NonNull, @@ -434,8 +437,9 @@ pub struct RequestLocals { impl RequestLocals { #[track_caller] pub fn system_settings(&self) -> &SystemSettings { - // SAFETY: it should always be valid, either set to the - // INITIAL_SYSTEM_SETTINGS or to the SYSTEM_SETTINGS. + // SAFETY: it should always be valid, just maybe "stale", such as + // having only the initial values, or only the ones available in minit, + // rather than the fully configured values. unsafe { self.system_settings.as_ref() } } } @@ -449,7 +453,7 @@ impl Default for RequestLocals { git_commit_sha: None, git_repository_url: None, tags: vec![], - system_settings: ptr::NonNull::from(INITIAL_SYSTEM_SETTINGS.deref()), + system_settings: SystemSettings::get(), interrupt_count: AtomicU32::new(0), vm_interrupt_addr: ptr::null_mut(), } @@ -572,8 +576,7 @@ extern "C" fn rinit(_type: c_int, _module_number: c_int) -> ZendResult { unsafe { bindings::zai_config_rinit() }; - // SAFETY: We are after first rinit and before config mshutdown. - let mut system_settings = unsafe { SystemSettings::get() }; + let mut system_settings = SystemSettings::get(); // initialize the thread local storage and cache some items let result = REQUEST_LOCALS.try_with_borrow_mut(|locals| { From cfe23ddac5188874ea4212cdcfdb2d26722962d0 Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Thu, 15 Jan 2026 10:08:51 -0700 Subject: [PATCH 2/9] fix(profiling): UB with SystemSettings in rinit --- profiling/src/lib.rs | 10 +++++++--- profiling/src/profiling/mod.rs | 13 ++++++++----- 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/profiling/src/lib.rs b/profiling/src/lib.rs index 2fb8bfcc437..74b76ea7dab 100644 --- a/profiling/src/lib.rs +++ b/profiling/src/lib.rs @@ -576,7 +576,9 @@ extern "C" fn rinit(_type: c_int, _module_number: c_int) -> ZendResult { unsafe { bindings::zai_config_rinit() }; - let mut system_settings = SystemSettings::get(); + // Needs to come after config::first_rinit, because that's what sets the + // values to the ones in the configuration. + let system_settings = SystemSettings::get(); // initialize the thread local storage and cache some items let result = REQUEST_LOCALS.try_with_borrow_mut(|locals| { @@ -648,8 +650,10 @@ extern "C" fn rinit(_type: c_int, _module_number: c_int) -> ZendResult { return ZendResult::Success; } - // SAFETY: still safe to access in rinit after first_rinit. - let system_settings = unsafe { system_settings.as_mut() }; + // SAFETY: safe to dereference in rinit after first_rinit. It's important + // that this is a non-mut reference because in ZTS there's nothing which + // enforces mutual exclusion. + let system_settings = unsafe { system_settings.as_ref() }; // SAFETY: the once control is not mutable during request. let once = unsafe { &*ptr::addr_of!(RINIT_ONCE) }; diff --git a/profiling/src/profiling/mod.rs b/profiling/src/profiling/mod.rs index 8687a25e25a..b4b8a9eb20d 100644 --- a/profiling/src/profiling/mod.rs +++ b/profiling/src/profiling/mod.rs @@ -227,6 +227,9 @@ pub struct Profiler { uploader_handle: JoinHandle<()>, should_join: AtomicBool, sample_types_filter: SampleTypeFilter, + + /// Don't modify the SystemSettings through this pointer (but you can swap + /// it to a different pointer). system_settings: AtomicPtr, } @@ -654,7 +657,7 @@ const DDPROF_UPLOAD: &str = "ddprof_upload"; impl Profiler { /// Will initialize the `PROFILER` OnceCell and makes sure that only one thread will do so. - pub fn init(system_settings: &mut SystemSettings) { + pub fn init(system_settings: &SystemSettings) { // SAFETY: the `get_or_init` access is a thread-safe API, and the // PROFILER is only being mutated in single-threaded phases such as //minit/mshutdown. @@ -668,7 +671,7 @@ impl Profiler { unsafe { (*ptr::addr_of!(PROFILER)).get() } } - pub fn new(system_settings: &mut SystemSettings) -> Self { + pub fn new(system_settings: &SystemSettings) -> Self { let fork_barrier = Arc::new(Barrier::new(3)); let interrupt_manager = Arc::new(InterruptManager::new()); let (message_sender, message_receiver) = crossbeam_channel::bounded(100); @@ -705,7 +708,7 @@ impl Profiler { }), should_join: AtomicBool::new(true), sample_types_filter, - system_settings: AtomicPtr::new(system_settings), + system_settings: AtomicPtr::new(system_settings as *const _ as *mut _), } } @@ -904,7 +907,7 @@ impl Profiler { let mut timestamp = NO_TIMESTAMP; { - let system_settings = self.system_settings.load(Ordering::SeqCst); + let system_settings = self.system_settings.load(Ordering::Relaxed); // SAFETY: system settings are stable during a request. if unsafe { *ptr::addr_of!((*system_settings).profiling_timeline_enabled) } { if let Ok(now) = SystemTime::now().duration_since(UNIX_EPOCH) { @@ -1007,7 +1010,7 @@ impl Profiler { let mut timestamp = NO_TIMESTAMP; { - let system_settings = self.system_settings.load(Ordering::SeqCst); + let system_settings = self.system_settings.load(Ordering::Relaxed); // SAFETY: system settings are stable during a request. if unsafe { *ptr::addr_of!((*system_settings).profiling_timeline_enabled) } { if let Ok(now) = SystemTime::now().duration_since(UNIX_EPOCH) { From 1d51ce076cca5d50439407288f59a24ec7c28242 Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Thu, 15 Jan 2026 10:18:41 -0700 Subject: [PATCH 3/9] fix test to not use mut as well --- profiling/src/profiling/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/profiling/src/profiling/mod.rs b/profiling/src/profiling/mod.rs index b4b8a9eb20d..1d82a83415a 100644 --- a/profiling/src/profiling/mod.rs +++ b/profiling/src/profiling/mod.rs @@ -1601,7 +1601,7 @@ mod tests { settings.profiling_experimental_cpu_time_enabled = true; settings.profiling_timeline_enabled = true; - let profiler = Profiler::new(&mut settings); + let profiler = Profiler::new(&settings); let message: SampleMessage = profiler.prepare_sample_message(frames, samples, labels, 900); From f6261b9ed8aa155344f5ccd75d06d8fa1788b33c Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Thu, 15 Jan 2026 11:15:06 -0700 Subject: [PATCH 4/9] extract Profiler::is_timeline_enabled helper This also serves to document some AtomicPtr operations and orderings. --- profiling/src/profiling/mod.rs | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/profiling/src/profiling/mod.rs b/profiling/src/profiling/mod.rs index 1d82a83415a..c7e9850dcad 100644 --- a/profiling/src/profiling/mod.rs +++ b/profiling/src/profiling/mod.rs @@ -228,8 +228,9 @@ pub struct Profiler { should_join: AtomicBool, sample_types_filter: SampleTypeFilter, - /// Don't modify the SystemSettings through this pointer (but you can swap - /// it to a different pointer). + /// An atomic pointer is used to make this Send and Sync, not because we + /// need the atomicity specifically. Don't modify the SystemSettings + /// through this pointer. system_settings: AtomicPtr, } @@ -890,6 +891,17 @@ impl Profiler { } } + /// Returns true if the timeline sample type is enabled. + #[inline] + fn is_timeline_enabled(&self) -> bool { + // Relaxed is fine, atomicity used for Send/Sync, this pointer is not + // used for syncronization, no happens-before relationship is needed. + let system_settings = self.system_settings.load(Ordering::Relaxed); + + // SAFETY: system settings are valid while the Profiler is alive. + unsafe { (*system_settings).profiling_timeline_enabled } + } + /// Collect a stack sample with elapsed wall time. Collects CPU time if /// it's enabled and available. #[cfg_attr(feature = "tracing", tracing::instrument(skip_all, level = "debug"))] @@ -907,9 +919,7 @@ impl Profiler { let mut timestamp = NO_TIMESTAMP; { - let system_settings = self.system_settings.load(Ordering::Relaxed); - // SAFETY: system settings are stable during a request. - if unsafe { *ptr::addr_of!((*system_settings).profiling_timeline_enabled) } { + if self.is_timeline_enabled() { if let Ok(now) = SystemTime::now().duration_since(UNIX_EPOCH) { timestamp = now.as_nanos() as i64; } @@ -1010,9 +1020,7 @@ impl Profiler { let mut timestamp = NO_TIMESTAMP; { - let system_settings = self.system_settings.load(Ordering::Relaxed); - // SAFETY: system settings are stable during a request. - if unsafe { *ptr::addr_of!((*system_settings).profiling_timeline_enabled) } { + if self.is_timeline_enabled() { if let Ok(now) = SystemTime::now().duration_since(UNIX_EPOCH) { timestamp = now.as_nanos() as i64; } From 4352c4775e1c18529f9ba37437fb50ab22bc9e00 Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Thu, 15 Jan 2026 12:43:43 -0700 Subject: [PATCH 5/9] experiment with SystemSettingsState tracking --- profiling/src/config.rs | 53 ++++++++++++++++++++++++++++++++-- profiling/src/profiling/mod.rs | 2 ++ 2 files changed, 52 insertions(+), 3 deletions(-) diff --git a/profiling/src/config.rs b/profiling/src/config.rs index eab7562a7e0..0d077a4fcf1 100644 --- a/profiling/src/config.rs +++ b/profiling/src/config.rs @@ -14,13 +14,30 @@ use core::str::FromStr; use libc::{c_char, c_int}; use libdd_common::tag::{parse_tags, Tag}; pub use libdd_profiling::exporter::Uri; -use log::{warn, LevelFilter}; +use log::{debug, warn, LevelFilter}; use std::borrow::Cow; use std::ffi::CString; use std::path::{Path, PathBuf}; +#[derive(Copy, Clone, Debug, Default)] +pub enum SystemSettingsState { + /// Indicates the system settings are not aware of the configuration at + /// the moment. + #[default] + ConfigUnaware, + + /// Indicates the system settings _are_ aware of configuration at the + /// moment. + ConfigAware, + + /// Expressly disabled, such as a child process post-fork (forks are not + /// currently profiled, except certain forks made by SAPIs). + Disabled, +} + #[derive(Clone, Debug)] pub struct SystemSettings { + pub state: SystemSettingsState, pub profiling_enabled: bool, pub profiling_experimental_features_enabled: bool, pub profiling_endpoint_collection_enabled: bool, @@ -44,6 +61,7 @@ impl SystemSettings { /// Provides "initial" settings, which are all "off"-like values. pub const fn initial() -> SystemSettings { SystemSettings { + state: SystemSettingsState::ConfigUnaware, profiling_enabled: false, profiling_experimental_features_enabled: false, profiling_endpoint_collection_enabled: false, @@ -73,6 +91,7 @@ impl SystemSettings { let trace_agent_url = trace_agent_url(); let uri = detect_uri_from_config(trace_agent_url, agent_host, trace_agent_port); Self { + state: SystemSettingsState::ConfigAware, profiling_enabled: profiling_enabled(), profiling_experimental_features_enabled: profiling_experimental_features_enabled(), profiling_endpoint_collection_enabled: profiling_endpoint_collection_enabled(), @@ -130,20 +149,47 @@ impl SystemSettings { system_settings.profiling_allocation_enabled = false; } + SystemSettings::log_state( + (*ptr::addr_of!(SYSTEM_SETTINGS)).state, + system_settings.state, + "the first request was received", + ); ptr::addr_of_mut!(SYSTEM_SETTINGS).swap(&mut system_settings); } + fn log_state(from: SystemSettingsState, to: SystemSettingsState, reason: &str) { + debug!("SystemSettings state transitioned from {from:?} to {to:?} because {reason}."); + } + /// # Safety /// Must be called exactly once per shutdown in either mshutdown or /// shutdown, before zai config is shutdown. unsafe fn on_shutdown() { let system_settings = &mut *ptr::addr_of_mut!(SYSTEM_SETTINGS); - *system_settings = SystemSettings::initial(); + let state = SystemSettingsState::ConfigUnaware; + SystemSettings::log_state( + system_settings.state, + state, + "a shutdown command was received", + ); + *system_settings = SystemSettings { + state, + ..SystemSettings::initial() + }; } unsafe fn on_fork_in_child() { let system_settings = &mut *ptr::addr_of_mut!(SYSTEM_SETTINGS); - *system_settings = SystemSettings::initial(); + let state = SystemSettingsState::Disabled; + SystemSettings::log_state( + system_settings.state, + state, + "the processed forked, and child processes are not profiled", + ); + *system_settings = SystemSettings { + state, + ..SystemSettings::initial() + }; } } @@ -423,6 +469,7 @@ impl ConfigId { /// Keep these in sync with the INI defaults. static DEFAULT_SYSTEM_SETTINGS: SystemSettings = SystemSettings { + state: SystemSettingsState::ConfigUnaware, profiling_enabled: true, profiling_experimental_features_enabled: false, profiling_endpoint_collection_enabled: true, diff --git a/profiling/src/profiling/mod.rs b/profiling/src/profiling/mod.rs index c7e9850dcad..0e95ccb7804 100644 --- a/profiling/src/profiling/mod.rs +++ b/profiling/src/profiling/mod.rs @@ -1541,6 +1541,7 @@ mod tests { use crate::{allocation::DEFAULT_ALLOCATION_SAMPLING_INTERVAL, config::AgentEndpoint}; use libdd_profiling::exporter::Uri; use log::LevelFilter; + use crate::config::SystemSettingsState; fn get_frames() -> Vec { vec![ZendFrame { @@ -1552,6 +1553,7 @@ mod tests { pub fn get_system_settings() -> SystemSettings { SystemSettings { + state: SystemSettingsState::ConfigAware, profiling_enabled: true, profiling_experimental_features_enabled: false, profiling_endpoint_collection_enabled: false, From b5c50d0caf904dff827a1cc4ecb8b9f7d6168322 Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Thu, 15 Jan 2026 17:34:17 -0700 Subject: [PATCH 6/9] fix(profiling): SystemSettings initialization --- profiling/src/allocation/allocation_ge84.rs | 8 +-- profiling/src/allocation/allocation_le83.rs | 14 +---- profiling/src/allocation/mod.rs | 60 ++++++++++-------- profiling/src/allocation/profiling_stats.rs | 39 +++++++++--- profiling/src/config.rs | 70 ++++++++++++++------- profiling/src/lib.rs | 4 +- profiling/src/profiling/mod.rs | 4 +- 7 files changed, 121 insertions(+), 78 deletions(-) diff --git a/profiling/src/allocation/allocation_ge84.rs b/profiling/src/allocation/allocation_ge84.rs index 4554738ffcd..50e68bc94e8 100644 --- a/profiling/src/allocation/allocation_ge84.rs +++ b/profiling/src/allocation/allocation_ge84.rs @@ -146,15 +146,9 @@ lazy_static! { } pub fn first_rinit_should_disable_due_to_jit() -> bool { - if *JIT_ENABLED + *JIT_ENABLED && zend::PHP_VERSION_ID >= 80400 && (80400..80406).contains(&crate::RUNTIME_PHP_VERSION_ID.load(Relaxed)) - { - error!("Memory allocation profiling will be disabled as long as JIT is active. To enable allocation profiling disable JIT or upgrade PHP to at least version 8.4.7. See https://github.com/DataDog/dd-trace-php/pull/3199"); - true - } else { - false - } } /// This initializes the thread locale variable `ZEND_MM_STATE` with respect to the currently diff --git a/profiling/src/allocation/allocation_le83.rs b/profiling/src/allocation/allocation_le83.rs index 506c5b6c38e..693cec3946a 100644 --- a/profiling/src/allocation/allocation_le83.rs +++ b/profiling/src/allocation/allocation_le83.rs @@ -7,7 +7,7 @@ use crate::{RefCellExt, PROFILER_NAME, REQUEST_LOCALS}; use core::{cell::Cell, ptr}; use lazy_static::lazy_static; use libc::{c_char, c_int, c_void, size_t}; -use log::{debug, error, trace, warn}; +use log::{debug, trace, warn}; use std::sync::atomic::Ordering::Relaxed; #[cfg(feature = "debug_stats")] @@ -132,19 +132,9 @@ pub fn alloc_prof_ginit() { } pub fn first_rinit_should_disable_due_to_jit() -> bool { - if NEEDS_RUN_TIME_CHECK_FOR_ENABLED_JIT + NEEDS_RUN_TIME_CHECK_FOR_ENABLED_JIT && alloc_prof_needs_disabled_for_jit(crate::RUNTIME_PHP_VERSION_ID.load(Relaxed)) && *JIT_ENABLED - { - if zend::PHP_VERSION_ID >= 80400 { - error!("Memory allocation profiling will be disabled as long as JIT is active. To enable allocation profiling disable JIT or upgrade PHP to at least version 8.4.7. See https://github.com/DataDog/dd-trace-php/pull/3199"); - } else { - error!("Memory allocation profiling will be disabled as long as JIT is active. To enable allocation profiling disable JIT or upgrade PHP to at least version 8.1.21 or 8.2.8. See https://github.com/DataDog/dd-trace-php/pull/2088"); - } - true - } else { - false - } } pub fn alloc_prof_rinit() { diff --git a/profiling/src/allocation/mod.rs b/profiling/src/allocation/mod.rs index ea0f657531a..84dea83ba27 100644 --- a/profiling/src/allocation/mod.rs +++ b/profiling/src/allocation/mod.rs @@ -11,11 +11,13 @@ use crate::bindings::{self as zend}; use crate::profiling::Profiler; use crate::{RefCellExt, REQUEST_LOCALS}; use libc::size_t; -use log::{debug, error, trace}; +use log::{debug, trace}; use rand_distr::{Distribution, Poisson}; use std::ffi::c_void; +use std::num::{NonZero, NonZeroU32, NonZeroU64}; use std::sync::atomic::{AtomicU64, Ordering}; +use crate::config::SystemSettings; #[cfg(not(php_zts))] use rand::rngs::StdRng; #[cfg(php_zts)] @@ -23,12 +25,14 @@ use rand::rngs::ThreadRng; #[cfg(not(php_zts))] use rand::SeedableRng; -/// Default sampling interval in bytes (4MB) -pub const DEFAULT_ALLOCATION_SAMPLING_INTERVAL: u64 = 1024 * 4096; +/// Default sampling interval in bytes (4 MiB). +// SAFETY: value is > 0. +pub const DEFAULT_ALLOCATION_SAMPLING_INTERVAL: NonZeroU32 = + unsafe { NonZero::new_unchecked(1024 * 4096) }; -/// Sampling distance feed into poison sampling algo +/// Sampling distance feed into poison sampling algo. This must be > 0. pub static ALLOCATION_PROFILING_INTERVAL: AtomicU64 = - AtomicU64::new(DEFAULT_ALLOCATION_SAMPLING_INTERVAL); + AtomicU64::new(DEFAULT_ALLOCATION_SAMPLING_INTERVAL.get() as u64); /// This will store the count of allocations (including reallocations) during /// a profiling period. This will overflow when doing more than u64::MAX @@ -53,10 +57,9 @@ pub struct AllocationProfilingStats { } impl AllocationProfilingStats { - fn new() -> AllocationProfilingStats { - // Safety: this will only error if lambda <= 0 - let poisson = - Poisson::new(ALLOCATION_PROFILING_INTERVAL.load(Ordering::Relaxed) as f64).unwrap(); + fn new(sampling_distance: NonZeroU64) -> AllocationProfilingStats { + // SAFETY: this will only error if lambda <= 0, and it's NonZeroU64. + let poisson = unsafe { Poisson::new(sampling_distance.get() as f64).unwrap_unchecked() }; let mut stats = AllocationProfilingStats { next_sample: 0, poisson, @@ -105,30 +108,35 @@ pub fn alloc_prof_startup() { allocation_le83::alloc_prof_startup(); } -pub fn alloc_prof_first_rinit() { - let (allocation_enabled, sampling_distance) = REQUEST_LOCALS - .try_with_borrow(|locals| { - let settings = locals.system_settings(); - (settings.profiling_allocation_enabled, settings.profiling_allocation_sampling_distance) - }) - .unwrap_or_else(|err| { - error!("Allocation profiling first rinit failed because it failed to borrow the request locals. Please report this to Datadog: {err}"); - (false, DEFAULT_ALLOCATION_SAMPLING_INTERVAL as u32) - }); +pub fn first_rinit(settings: &SystemSettings) { + if !settings.profiling_allocation_enabled { + return; + } - if !allocation_enabled { + let sampling_distance = settings.profiling_allocation_sampling_distance; + ALLOCATION_PROFILING_INTERVAL.store(sampling_distance.get() as u64, Ordering::Relaxed); + + trace!("Memory allocation profiling initialized with a sampling distance of {sampling_distance} bytes."); +} + +/// # Safety +/// +/// Must be called exactly once per extension minit. +pub unsafe fn minit(settings: &SystemSettings) { + if !settings.profiling_allocation_enabled { return; } - ALLOCATION_PROFILING_INTERVAL.store(sampling_distance as u64, Ordering::Relaxed); + let sampling_distance = settings.profiling_allocation_sampling_distance; + ALLOCATION_PROFILING_INTERVAL.store(sampling_distance.get() as u64, Ordering::Relaxed); + + // SAFETY: called in minit. + unsafe { profiling_stats::minit(sampling_distance.into()) }; - trace!( - "Memory allocation profiling initialized with a sampling distance of {} bytes.", - ALLOCATION_PROFILING_INTERVAL.load(Ordering::Relaxed) - ); + trace!("Memory allocation profiling initialized with a sampling distance of {sampling_distance} bytes."); } -pub fn alloc_prof_rinit() { +pub fn rinit() { let allocation_enabled = REQUEST_LOCALS .try_with_borrow(|locals| locals.system_settings().profiling_allocation_enabled) .unwrap_or_else(|err| { diff --git a/profiling/src/allocation/profiling_stats.rs b/profiling/src/allocation/profiling_stats.rs index c2d3fa4135a..41251eb1d6e 100644 --- a/profiling/src/allocation/profiling_stats.rs +++ b/profiling/src/allocation/profiling_stats.rs @@ -3,17 +3,18 @@ //! performance sensitive. It is encapsulated so that some unsafe techniques //! can be used but expose a relatively safe API. -use super::AllocationProfilingStats; +use super::{AllocationProfilingStats, ALLOCATION_PROFILING_INTERVAL}; use libc::size_t; use std::mem::MaybeUninit; -#[cfg(php_zts)] -use std::cell::UnsafeCell; - #[cfg(php_zend_mm_set_custom_handlers_ex)] use super::allocation_ge84; #[cfg(not(php_zend_mm_set_custom_handlers_ex))] use super::allocation_le83; +#[cfg(php_zts)] +use std::cell::UnsafeCell; +use std::num::NonZeroU64; +use std::sync::atomic::Ordering; #[cfg(php_zts)] thread_local! { @@ -107,12 +108,15 @@ pub fn allocation_profiling_stats_should_collect(len: size_t) -> bool { /// Must be called once per PHP thread ginit. pub unsafe fn ginit() { // SAFETY: - // 1. During ginit, there will not be any other borrows. - // 2. This closure will not make new borrows. + // 1. During ginit, there will not be any other borrows to stats. + // 2. This closure will not make new borrows to stats. // 3. This is not during the thread-local destructor. unsafe { allocation_profiling_stats_mut(|uninit| { - uninit.write(AllocationProfilingStats::new()); + let interval = ALLOCATION_PROFILING_INTERVAL.load(Ordering::Relaxed); + // SAFETY: ALLOCATION_PROFILING_INTERVAL must always be > 0. + let nonzero = NonZeroU64::new_unchecked(interval); + uninit.write(AllocationProfilingStats::new(nonzero)); }) }; @@ -122,6 +126,27 @@ pub unsafe fn ginit() { allocation_ge84::alloc_prof_ginit(); } +/// Initializes the allocation profiler's globals with the provided sampling +/// distance. +/// +/// # Safety +/// +/// Must be called once per PHP thread minit, unless the allocation profiling +/// is disabled, in which case it can be skipped. +pub unsafe fn minit(sampling_distance: NonZeroU64) { + // SAFETY: + // 1. During minit, there will not be any other borrows. + // 2. This closure will not make new borrows. + // 3. This is not during the thread-local destructor. + unsafe { + allocation_profiling_stats_mut(|uninit| { + // SAFETY: previously initialized in ginit, we're just + // re-initializing it because we now have config + *uninit.assume_init_mut() = AllocationProfilingStats::new(sampling_distance); + }) + }; +} + /// Shuts down the allocation profiler's globals. /// /// # Safety diff --git a/profiling/src/config.rs b/profiling/src/config.rs index 0d077a4fcf1..acdc22b085c 100644 --- a/profiling/src/config.rs +++ b/profiling/src/config.rs @@ -1,4 +1,3 @@ -use crate::allocation; use crate::bindings::zai_config_type::*; use crate::bindings::{ datadog_php_profiling_copy_string_view_into_zval, ddog_php_prof_get_memoized_config, @@ -7,6 +6,7 @@ use crate::bindings::{ StringError, ZaiStr, IS_FALSE, IS_LONG, IS_TRUE, ZAI_CONFIG_NAME_BUFSIZ, ZEND_INI_DISPLAY_ORIG, }; use crate::zend::zai_str_from_zstr; +use crate::{allocation, bindings, zend}; use core::fmt::{Display, Formatter}; use core::mem::transmute; use core::ptr; @@ -14,9 +14,10 @@ use core::str::FromStr; use libc::{c_char, c_int}; use libdd_common::tag::{parse_tags, Tag}; pub use libdd_profiling::exporter::Uri; -use log::{debug, warn, LevelFilter}; +use log::{debug, error, warn, LevelFilter}; use std::borrow::Cow; use std::ffi::CString; +use std::num::NonZeroU32; use std::path::{Path, PathBuf}; #[derive(Copy, Clone, Debug, Default)] @@ -43,7 +44,7 @@ pub struct SystemSettings { pub profiling_endpoint_collection_enabled: bool, pub profiling_experimental_cpu_time_enabled: bool, pub profiling_allocation_enabled: bool, - pub profiling_allocation_sampling_distance: u32, + pub profiling_allocation_sampling_distance: NonZeroU32, pub profiling_timeline_enabled: bool, pub profiling_exception_enabled: bool, pub profiling_exception_message_enabled: bool, @@ -67,7 +68,7 @@ impl SystemSettings { profiling_endpoint_collection_enabled: false, profiling_experimental_cpu_time_enabled: false, profiling_allocation_enabled: false, - profiling_allocation_sampling_distance: u32::MAX, + profiling_allocation_sampling_distance: NonZeroU32::MAX, profiling_timeline_enabled: false, profiling_exception_enabled: false, profiling_exception_message_enabled: false, @@ -82,8 +83,7 @@ impl SystemSettings { /// # Safety /// This function must only be called after ZAI config has been - /// initialized in first rinit, and before config is uninitialized in - /// shutdown. + /// initialized, and before config is uninitialized in shutdown. unsafe fn new() -> SystemSettings { // Select agent URI/UDS. let agent_host = agent_host(); @@ -130,22 +130,19 @@ impl SystemSettings { unsafe fn on_first_request() { let mut system_settings = SystemSettings::new(); - // Initialize logging before allocation's rinit, as it logs. - cfg_if::cfg_if! { - if #[cfg(debug_assertions)] { - log::set_max_level(system_settings.profiling_log_level); - } else { - crate::logging::log_init(system_settings.profiling_log_level); - } - } - // Work around version-specific issues. #[cfg(not(php_zend_mm_set_custom_handlers_ex))] if allocation::allocation_le83::first_rinit_should_disable_due_to_jit() { + if zend::PHP_VERSION_ID >= 80400 { + error!("Memory allocation profiling will be disabled as long as JIT is active. To enable allocation profiling disable JIT or upgrade PHP to at least version 8.4.7. See https://github.com/DataDog/dd-trace-php/pull/3199"); + } else { + error!("Memory allocation profiling will be disabled as long as JIT is active. To enable allocation profiling disable JIT or upgrade PHP to at least version 8.1.21 or 8.2.8. See https://github.com/DataDog/dd-trace-php/pull/2088"); + } system_settings.profiling_allocation_enabled = false; } #[cfg(php_zend_mm_set_custom_handlers_ex)] if allocation::allocation_ge84::first_rinit_should_disable_due_to_jit() { + error!("Memory allocation profiling will be disabled as long as JIT is active. To enable allocation profiling disable JIT or upgrade PHP to at least version 8.4.7. See https://github.com/DataDog/dd-trace-php/pull/3199"); system_settings.profiling_allocation_enabled = false; } @@ -475,7 +472,8 @@ static DEFAULT_SYSTEM_SETTINGS: SystemSettings = SystemSettings { profiling_endpoint_collection_enabled: true, profiling_experimental_cpu_time_enabled: true, profiling_allocation_enabled: true, - profiling_allocation_sampling_distance: 1024 * 4096, + // SAFETY: value is > 0. + profiling_allocation_sampling_distance: unsafe { NonZeroU32::new_unchecked(1024 * 4096) }, profiling_timeline_enabled: true, profiling_exception_enabled: true, profiling_exception_message_enabled: false, @@ -542,11 +540,16 @@ unsafe fn profiling_allocation_enabled() -> bool { /// # Safety /// This function must only be called after config has been initialized in /// rinit, and before it is uninitialized in mshutdown. -unsafe fn profiling_allocation_sampling_distance() -> u32 { - get_system_uint32( +unsafe fn profiling_allocation_sampling_distance() -> NonZeroU32 { + let int = get_system_uint32( ProfilingAllocationSamplingDistance, - DEFAULT_SYSTEM_SETTINGS.profiling_allocation_sampling_distance, - ) + DEFAULT_SYSTEM_SETTINGS + .profiling_allocation_sampling_distance + .get(), + ); + // SAFETY: ProfilingAllocationSamplingDistance uses parser that ensures a + // non-zero value. + unsafe { NonZeroU32::new_unchecked(int) } } /// # Safety @@ -1233,8 +1236,31 @@ pub(crate) fn minit(module_number: libc::c_int) { ); assert!(tmp); // It's literally return true in the source. - let system_settings = &mut *ptr::addr_of_mut!(SYSTEM_SETTINGS); - *system_settings = SystemSettings::initial(); + // We set this so that we can access config for system INI settings during + // minit, for example for allocation_sampling_distance. + let in_request = false; + bindings::zai_config_first_time_rinit(in_request); + + // SAFETY: just initialized zai config. + let mut system_settings = SystemSettings::new(); + + // Initialize logging before allocation's rinit, as it logs. + cfg_if::cfg_if! { + if #[cfg(debug_assertions)] { + log::set_max_level(system_settings.profiling_log_level); + } else { + crate::logging::log_init(system_settings.profiling_log_level); + } + } + + SystemSettings::log_state( + (*ptr::addr_of!(SYSTEM_SETTINGS)).state, + system_settings.state, + "the module was initialized", + ); + ptr::addr_of_mut!(SYSTEM_SETTINGS).swap(&mut system_settings); + + allocation::minit(&*ptr::addr_of!(SYSTEM_SETTINGS)) } } diff --git a/profiling/src/lib.rs b/profiling/src/lib.rs index 74b76ea7dab..e3db4b3071c 100644 --- a/profiling/src/lib.rs +++ b/profiling/src/lib.rs @@ -692,7 +692,7 @@ extern "C" fn rinit(_type: c_int, _module_number: c_int) -> ZendResult { #[cfg(all(feature = "io_profiling", target_os = "linux"))] io::io_prof_first_rinit(); - allocation::alloc_prof_first_rinit(); + allocation::first_rinit(system_settings); }); Profiler::init(system_settings); @@ -742,7 +742,7 @@ extern "C" fn rinit(_type: c_int, _module_number: c_int) -> ZendResult { TAGS.set(Arc::default()); } - allocation::alloc_prof_rinit(); + allocation::rinit(); // SAFETY: called after config is initialized. unsafe { timeline::timeline_rinit() }; diff --git a/profiling/src/profiling/mod.rs b/profiling/src/profiling/mod.rs index 0e95ccb7804..31059ef3965 100644 --- a/profiling/src/profiling/mod.rs +++ b/profiling/src/profiling/mod.rs @@ -1538,10 +1538,10 @@ pub struct JoinError { #[cfg(test)] mod tests { use super::*; + use crate::config::SystemSettingsState; use crate::{allocation::DEFAULT_ALLOCATION_SAMPLING_INTERVAL, config::AgentEndpoint}; use libdd_profiling::exporter::Uri; use log::LevelFilter; - use crate::config::SystemSettingsState; fn get_frames() -> Vec { vec![ZendFrame { @@ -1559,7 +1559,7 @@ mod tests { profiling_endpoint_collection_enabled: false, profiling_experimental_cpu_time_enabled: false, profiling_allocation_enabled: false, - profiling_allocation_sampling_distance: DEFAULT_ALLOCATION_SAMPLING_INTERVAL as u32, + profiling_allocation_sampling_distance: DEFAULT_ALLOCATION_SAMPLING_INTERVAL, profiling_timeline_enabled: false, profiling_exception_enabled: false, profiling_exception_message_enabled: false, From f57cdf7b7c612e5642a1331127befadaf33bd003 Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Fri, 16 Jan 2026 08:32:43 -0700 Subject: [PATCH 7/9] test(profiling): allocation_sampling_distance regression --- .../phpt/allocation_sampling_distance.phpt | 49 +++++++++++++++++++ 1 file changed, 49 insertions(+) create mode 100644 profiling/tests/phpt/allocation_sampling_distance.phpt diff --git a/profiling/tests/phpt/allocation_sampling_distance.phpt b/profiling/tests/phpt/allocation_sampling_distance.phpt new file mode 100644 index 00000000000..1be47433136 --- /dev/null +++ b/profiling/tests/phpt/allocation_sampling_distance.phpt @@ -0,0 +1,49 @@ +--TEST-- +[profiling] allocation sampling distance is configurable +--DESCRIPTION-- +This code path had a regression, so it seems worth adding a test to ensure it +cannot regress again. +--SKIPIF-- + +--INI-- +datadog.profiling.enabled=1 +datadog.profiling.allocation_enabled=1 +datadog.profiling.allocation_sampling_distance=1 +datadog.profiling.log_level=trace +datadog.profiling.output_pprof=/tmp/profiling-data/pprof +zend.assertions=1 +assert.exception=1 +--FILE-- + +Done. +--EXPECTREGEX-- +.* Memory allocation profiling initialized with a sampling distance of 1 bytes.* +.* Sent stack sample of [0-9]* frames, .* labels, 11[2,9] bytes allocated, and 1 allocations to profiler.* +.*Done\..* From bb55e054e4611c18984ec0e6efd24bb4027676f0 Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Fri, 16 Jan 2026 14:08:52 -0700 Subject: [PATCH 8/9] test: remove accidental INI setting --- profiling/tests/phpt/allocation_sampling_distance.phpt | 1 - 1 file changed, 1 deletion(-) diff --git a/profiling/tests/phpt/allocation_sampling_distance.phpt b/profiling/tests/phpt/allocation_sampling_distance.phpt index 1be47433136..f1069c561cd 100644 --- a/profiling/tests/phpt/allocation_sampling_distance.phpt +++ b/profiling/tests/phpt/allocation_sampling_distance.phpt @@ -13,7 +13,6 @@ datadog.profiling.enabled=1 datadog.profiling.allocation_enabled=1 datadog.profiling.allocation_sampling_distance=1 datadog.profiling.log_level=trace -datadog.profiling.output_pprof=/tmp/profiling-data/pprof zend.assertions=1 assert.exception=1 --FILE-- From c9706f35f836fd32b814b2037f2a2b67c476038b Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Fri, 16 Jan 2026 14:29:42 -0700 Subject: [PATCH 9/9] fix bad import after merge --- profiling/src/profiling/mod.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/profiling/src/profiling/mod.rs b/profiling/src/profiling/mod.rs index c2fec29a2bb..31059ef3965 100644 --- a/profiling/src/profiling/mod.rs +++ b/profiling/src/profiling/mod.rs @@ -1542,7 +1542,6 @@ mod tests { use crate::{allocation::DEFAULT_ALLOCATION_SAMPLING_INTERVAL, config::AgentEndpoint}; use libdd_profiling::exporter::Uri; use log::LevelFilter; - use crate::config::SystemSettingsState; fn get_frames() -> Vec { vec![ZendFrame {