diff --git a/profiling/src/allocation/allocation_ge84.rs b/profiling/src/allocation/allocation_ge84.rs index 4554738ffc..50e68bc94e 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 ab9472812b..96acdab5e4 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 ea0f657531..84dea83ba2 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 c2d3fa4135..41251eb1d6 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 0d077a4fcf..acdc22b085 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 74b76ea7da..e3db4b3071 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 0e95ccb780..31059ef396 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, diff --git a/profiling/tests/phpt/allocation_sampling_distance.phpt b/profiling/tests/phpt/allocation_sampling_distance.phpt new file mode 100644 index 0000000000..f1069c561c --- /dev/null +++ b/profiling/tests/phpt/allocation_sampling_distance.phpt @@ -0,0 +1,48 @@ +--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 +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\..*