From 02acd656e963354265362eeb6184cdd3ac6e7219 Mon Sep 17 00:00:00 2001 From: Florian Engelhardt Date: Tue, 28 Jul 2026 12:39:21 +0200 Subject: [PATCH 1/9] perf(profiling): skip idle internal interrupt handling On PHP 8.3 and older, the execute_internal hook called the full profiler interrupt handler after every internal function. Check EG(vm_interrupt) first so the TLS, request-state, and atomic work only runs when PHP has a pending VM interrupt. Across six balanced 60-second runs per binary on PHP 8.3 ZTS, mean allocation-loop throughput increased from 34,160,664/s to 36,061,991/s (+5.57%) and median throughput increased by 5.92%. Native samples reduced the manual interrupt path from 14.04% to 8.62% of main-thread samples. Wall-time correctness remained unchanged: a four-second CPU/sleep workload attributed 50.23% to sleep before and 50.31% after. Validation: PHP 7.3 ZTS cargo check; PHP 8.3 ZTS and PHP 8.5 NTS cargo test (22 passed each). https://datadoghq.atlassian.net/browse/PROF-15506 --- profiling/src/php_ffi.c | 8 ++++++++ profiling/src/php_ffi.h | 1 + profiling/src/wall_time.rs | 6 ++++-- 3 files changed, 13 insertions(+), 2 deletions(-) diff --git a/profiling/src/php_ffi.c b/profiling/src/php_ffi.c index 1e906f3cbd3..5799822ef0a 100644 --- a/profiling/src/php_ffi.c +++ b/profiling/src/php_ffi.c @@ -248,6 +248,14 @@ zend_execute_data* ddog_php_prof_get_current_execute_data() { return EG(current_execute_data); } +bool ddog_php_prof_vm_interrupt_pending() { +#if PHP_VERSION_ID >= 80000 + return zend_atomic_bool_load_ex(&EG(vm_interrupt)); +#else + return EG(vm_interrupt); +#endif +} + #if CFG_FIBERS // defined by build.rs zend_fiber* ddog_php_prof_get_active_fiber() { diff --git a/profiling/src/php_ffi.h b/profiling/src/php_ffi.h index 558c3de4413..1f402732ecb 100644 --- a/profiling/src/php_ffi.h +++ b/profiling/src/php_ffi.h @@ -162,6 +162,7 @@ void ddog_php_prof_zend_mm_set_custom_handlers(zend_mm_heap *heap, ddog_php_prof_zend_mm_realloc _realloc); zend_execute_data* ddog_php_prof_get_current_execute_data(); +bool ddog_php_prof_vm_interrupt_pending(); #if CFG_FRAMELESS void ddog_php_prof_post_startup(); diff --git a/profiling/src/wall_time.rs b/profiling/src/wall_time.rs index 3f0a948127a..7227fbcbdd5 100644 --- a/profiling/src/wall_time.rs +++ b/profiling/src/wall_time.rs @@ -79,8 +79,10 @@ mod execute_internal { unsafe { prev_execute_internal(execute_data, return_value) }; // See safety section of `execute_data_func_is_trampoline` docs for why - // the leaf frame is used instead of the execute_data ptr. - ddog_php_prof_interrupt_function(leaf_frame); + // the leaf frame is used instead of the execute_data ptr. + if unsafe { zend::ddog_php_prof_vm_interrupt_pending() } { + ddog_php_prof_interrupt_function(leaf_frame); + } } /// # Safety From be256447ac02963392231381428c2b2500b7b2c0 Mon Sep 17 00:00:00 2001 From: Florian Engelhardt Date: Tue, 28 Jul 2026 13:22:13 +0200 Subject: [PATCH 2/9] fix(profiling): guard atomic interrupt load on PHP 8.2 --- profiling/src/php_ffi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/profiling/src/php_ffi.c b/profiling/src/php_ffi.c index 5799822ef0a..be674ed2167 100644 --- a/profiling/src/php_ffi.c +++ b/profiling/src/php_ffi.c @@ -249,7 +249,7 @@ zend_execute_data* ddog_php_prof_get_current_execute_data() { } bool ddog_php_prof_vm_interrupt_pending() { -#if PHP_VERSION_ID >= 80000 +#if PHP_VERSION_ID >= 80200 return zend_atomic_bool_load_ex(&EG(vm_interrupt)); #else return EG(vm_interrupt); From 9cefb02219a3b37836480dffe5fe2167784d6758 Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Tue, 28 Jul 2026 18:05:59 -0600 Subject: [PATCH 3/9] Revert "fix(profiling): guard atomic interrupt load on PHP 8.2" This reverts commit be256447ac02963392231381428c2b2500b7b2c0. --- profiling/src/php_ffi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/profiling/src/php_ffi.c b/profiling/src/php_ffi.c index be674ed2167..5799822ef0a 100644 --- a/profiling/src/php_ffi.c +++ b/profiling/src/php_ffi.c @@ -249,7 +249,7 @@ zend_execute_data* ddog_php_prof_get_current_execute_data() { } bool ddog_php_prof_vm_interrupt_pending() { -#if PHP_VERSION_ID >= 80200 +#if PHP_VERSION_ID >= 80000 return zend_atomic_bool_load_ex(&EG(vm_interrupt)); #else return EG(vm_interrupt); From 9ac8a332b4dff04ed1586522b1dda36b253aed55 Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Tue, 28 Jul 2026 18:06:02 -0600 Subject: [PATCH 4/9] Revert "perf(profiling): skip idle internal interrupt handling" This reverts commit 02acd656e963354265362eeb6184cdd3ac6e7219. --- profiling/src/php_ffi.c | 8 -------- profiling/src/php_ffi.h | 1 - profiling/src/wall_time.rs | 6 ++---- 3 files changed, 2 insertions(+), 13 deletions(-) diff --git a/profiling/src/php_ffi.c b/profiling/src/php_ffi.c index 5799822ef0a..1e906f3cbd3 100644 --- a/profiling/src/php_ffi.c +++ b/profiling/src/php_ffi.c @@ -248,14 +248,6 @@ zend_execute_data* ddog_php_prof_get_current_execute_data() { return EG(current_execute_data); } -bool ddog_php_prof_vm_interrupt_pending() { -#if PHP_VERSION_ID >= 80000 - return zend_atomic_bool_load_ex(&EG(vm_interrupt)); -#else - return EG(vm_interrupt); -#endif -} - #if CFG_FIBERS // defined by build.rs zend_fiber* ddog_php_prof_get_active_fiber() { diff --git a/profiling/src/php_ffi.h b/profiling/src/php_ffi.h index 1f402732ecb..558c3de4413 100644 --- a/profiling/src/php_ffi.h +++ b/profiling/src/php_ffi.h @@ -162,7 +162,6 @@ void ddog_php_prof_zend_mm_set_custom_handlers(zend_mm_heap *heap, ddog_php_prof_zend_mm_realloc _realloc); zend_execute_data* ddog_php_prof_get_current_execute_data(); -bool ddog_php_prof_vm_interrupt_pending(); #if CFG_FRAMELESS void ddog_php_prof_post_startup(); diff --git a/profiling/src/wall_time.rs b/profiling/src/wall_time.rs index 7227fbcbdd5..3f0a948127a 100644 --- a/profiling/src/wall_time.rs +++ b/profiling/src/wall_time.rs @@ -79,10 +79,8 @@ mod execute_internal { unsafe { prev_execute_internal(execute_data, return_value) }; // See safety section of `execute_data_func_is_trampoline` docs for why - // the leaf frame is used instead of the execute_data ptr. - if unsafe { zend::ddog_php_prof_vm_interrupt_pending() } { - ddog_php_prof_interrupt_function(leaf_frame); - } + // the leaf frame is used instead of the execute_data ptr. + ddog_php_prof_interrupt_function(leaf_frame); } /// # Safety From 8cb3cde012b27d0ea07cfd3d7b3cac23d19b9faa Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Tue, 28 Jul 2026 18:07:04 -0600 Subject: [PATCH 5/9] perf(profiling): use relaxed read to skip idle interrupts This is very similar to the approach taken by Florian, which did a check for EG(vm_interrupt). Although I was able to reproduce a small speedup there, I realized that it's more nuanced than that: 1. This doesn't apply to actual VM interrupts, only to things which need to check for a pending interrupt. 2. EG(vm_interrupt) technically isn't related to the thing we care about, which is the interrupt count. So this adds ddog_php_prof_interrupt_function_unlikely which is the same as ddog_php_prof_interrupt_function at a high level, but it is optimized to assume that there isn't a pending interrupt (opposite of ddog_php_prof_interrupt_function). --- profiling/src/capi.rs | 4 +- profiling/src/wall_time.rs | 95 +++++++++++++++++++++----------- tests/tea/profiling/profiling.cc | 6 +- tests/tea/profiling/profiling.h | 1 + tracer/engine_hooks.c | 10 +++- 5 files changed, 78 insertions(+), 38 deletions(-) diff --git a/profiling/src/capi.rs b/profiling/src/capi.rs index 0ad6588fb3b..a96ddf24fc2 100644 --- a/profiling/src/capi.rs +++ b/profiling/src/capi.rs @@ -57,7 +57,9 @@ extern "C" fn ddog_php_prof_trigger_time_sample() { } } -pub use crate::wall_time::ddog_php_prof_interrupt_function; +pub use crate::wall_time::{ + ddog_php_prof_interrupt_function, ddog_php_prof_interrupt_function_unlikely, +}; #[cfg(test)] mod tests { diff --git a/profiling/src/wall_time.rs b/profiling/src/wall_time.rs index 3f0a948127a..f1e3160fb64 100644 --- a/profiling/src/wall_time.rs +++ b/profiling/src/wall_time.rs @@ -2,7 +2,7 @@ //! implementation reasons, it has cpu-time code as well. use crate::bindings::{zend_execute_data, zend_interrupt_function, VmInterruptFn}; -use crate::{profiling::Profiler, RefCellExt, REQUEST_LOCALS}; +use crate::{profiling::Profiler, RefCellExt, RequestLocals, REQUEST_LOCALS}; use core::ptr; use log::debug; use std::sync::atomic::Ordering; @@ -79,8 +79,8 @@ mod execute_internal { unsafe { prev_execute_internal(execute_data, return_value) }; // See safety section of `execute_data_func_is_trampoline` docs for why - // the leaf frame is used instead of the execute_data ptr. - ddog_php_prof_interrupt_function(leaf_frame); + // the leaf frame is used instead of the execute_data ptr. + ddog_php_prof_interrupt_function_unlikely(leaf_frame); } /// # Safety @@ -109,31 +109,65 @@ static mut PREV_INTERRUPT_FUNCTION: Option = None; #[no_mangle] #[inline(never)] pub extern "C" fn ddog_php_prof_interrupt_function(execute_data: *mut zend_execute_data) { - let result = REQUEST_LOCALS.try_with_borrow(|locals| { - if !locals.system_settings().profiling_enabled { - return; - } + if let Err(err) = + REQUEST_LOCALS.try_with_borrow(|locals| interrupt_function(locals, execute_data)) + { + debug!("ddog_php_prof_interrupt_function failed to borrow request locals: {err}"); + } +} + +fn interrupt_function(locals: &RequestLocals, execute_data: *mut zend_execute_data) { + let profiling_disabled = !locals.system_settings().profiling_enabled; + + /* Other extensions/modules or the engine itself may trigger an + * interrupt, but given how expensive it is to gather a stack trace, + * it should only be done if we triggered it ourselves. So + * interrupt_count serves dual purposes: + * 1. Track how many interrupts there were. + * 2. Ensure we don't collect on someone else's interrupt. + */ + let interrupt_count = locals.interrupt_count.swap(0, Ordering::SeqCst); + if profiling_disabled | (interrupt_count == 0) { + return; + } + + if let Some(profiler) = Profiler::get() { + // Safety: execute_data was provided by the engine, and the profiler doesn't mutate it. + profiler.collect_time(execute_data, interrupt_count); + } +} - /* Other extensions/modules or the engine itself may trigger an - * interrupt, but given how expensive it is to gather a stack trace, - * it should only be done if we triggered it ourselves. So - * interrupt_count serves dual purposes: - * 1. Track how many interrupts there were. - * 2. Ensure we don't collect on someone else's interrupt. - */ - let interrupt_count = locals.interrupt_count.swap(0, Ordering::SeqCst); - if interrupt_count == 0 { +/// This function is like [`ddog_php_prof_interrupt_function`] except it's +/// optimized for the expectation that it's unlikely that there's actually an +/// interrupt actively needing to be handled. This is because we have to insert +/// this check in a variety of places, and in many of them (execute_internal, +/// frameless functions) there won't be an interrupt at all. +/// +/// # Safety +/// The zend_execute_data pointer should come from the engine to ensure it and +/// its sub-objects are valid. +#[no_mangle] +#[inline(never)] +pub extern "C" fn ddog_php_prof_interrupt_function_unlikely(execute_data: *mut zend_execute_data) { + let result = REQUEST_LOCALS.try_with_borrow(|locals| { + // Optimize here with the expectation there isn't an interrupt, because + // overwhelmingly, there will not be one. The relaxed load avoids the + // more expensive atomic read-modify-write in `interrupt_function` on + // the idle path. It is only a fast-path hint: `interrupt_function` does + // the authoritative swap. If another consumer clears the count between + // the load and swap, the swap simply observes zero; if a producer adds + // an interrupt after this load observes zero, the count remains pending + // and the corresponding VM interrupt will provide another opportunity + // to handle it. + if locals.interrupt_count.load(Ordering::Relaxed) == 0 { return; } - if let Some(profiler) = Profiler::get() { - // Safety: execute_data was provided by the engine, and the profiler doesn't mutate it. - profiler.collect_time(execute_data, interrupt_count); - } + interrupt_function(locals, execute_data); }); if let Err(err) = result { - debug!("ddog_php_prof_interrupt_function failed to borrow request locals: {err}"); + debug!("ddog_php_prof_interrupt_function_unlikely failed to borrow request locals: {err}"); } } @@ -144,7 +178,8 @@ mod frameless { use crate::bindings::{ zend_flf_functions, zend_flf_handlers, zend_frameless_function_info, }; - use crate::{profiling::Profiler, zend, RefCellExt, REQUEST_LOCALS}; + use crate::wall_time::ddog_php_prof_interrupt_function; + use crate::{zend, RefCellExt, REQUEST_LOCALS}; use dynasmrt::{dynasm, DynasmApi, ExecutableBuffer}; use log::error; use std::ffi::c_void; @@ -271,23 +306,17 @@ mod frameless { #[inline(never)] pub extern "C" fn ddog_php_prof_icall_trampoline_target() { let interrupt_count = REQUEST_LOCALS - .try_with_borrow(|locals| { - if !locals.system_settings().profiling_enabled { - return 0; - } - locals.interrupt_count.swap(0, Ordering::SeqCst) - }) + .try_with_borrow(|locals| locals.interrupt_count.load(Ordering::Relaxed)) .unwrap_or(0); if interrupt_count == 0 { return; } - if let Some(profiler) = Profiler::get() { - // SAFETY: profiler doesn't mutate execute_data - let execute_data = unsafe { zend::ddog_php_prof_get_current_execute_data() }; - profiler.collect_time(execute_data, interrupt_count); - } + // Fetching the execute data is intentionally delayed until we know + // that the interrupt count is greater than 0 for perf. + let execute_data = unsafe { zend::ddog_php_prof_get_current_execute_data() }; + ddog_php_prof_interrupt_function(execute_data); } } diff --git a/tests/tea/profiling/profiling.cc b/tests/tea/profiling/profiling.cc index 8c6e361427f..7de77795305 100644 --- a/tests/tea/profiling/profiling.cc +++ b/tests/tea/profiling/profiling.cc @@ -9,7 +9,7 @@ ZEND_TLS datadog_php_stack_sample last_stack_sample; ZEND_API datadog_php_stack_sample tea_get_last_stack_sample(void) { return last_stack_sample; } -ZEND_API void ddog_php_prof_interrupt_function(zend_execute_data *execute_data) { +ZEND_API void ddog_php_prof_interrupt_function_unlikely(zend_execute_data *execute_data) { datadog_php_stack_sample_ctor(&last_stack_sample); /* Don't try to re-implement everything. Remember, the tracer is being @@ -34,6 +34,10 @@ ZEND_API void ddog_php_prof_interrupt_function(zend_execute_data *execute_data) } } +ZEND_API void ddog_php_prof_interrupt_function(zend_execute_data *execute_data) { + ddog_php_prof_interrupt_function_unlikely(execute_data); +} + ZEND_API zend_extension_version_info extension_version_info = { ZEND_EXTENSION_API_NO, ZEND_EXTENSION_BUILD_ID, diff --git a/tests/tea/profiling/profiling.h b/tests/tea/profiling/profiling.h index 75f8aa6aa83..452476071f8 100644 --- a/tests/tea/profiling/profiling.h +++ b/tests/tea/profiling/profiling.h @@ -10,6 +10,7 @@ BEGIN_EXTERN_C() ZEND_API datadog_php_stack_sample tea_get_last_stack_sample(void); ZEND_API void ddog_php_prof_interrupt_function(zend_execute_data *execute_data); +ZEND_API void ddog_php_prof_interrupt_function_unlikely(zend_execute_data *execute_data); END_EXTERN_C() #endif diff --git a/tracer/engine_hooks.c b/tracer/engine_hooks.c index e2b2eaea976..bbf9e16cb93 100644 --- a/tracer/engine_hooks.c +++ b/tracer/engine_hooks.c @@ -41,10 +41,14 @@ void dd_search_for_profiling_symbols(void *arg) { if (extension->name && strcmp(extension->name, "datadog-profiling") == 0) { DL_HANDLE handle = extension->handle; - profiling_interrupt_function = (void(*)(zend_execute_data *))DL_FETCH_SYMBOL(handle, "ddog_php_prof_interrupt_function"); + profiling_interrupt_function = (void(*)(zend_execute_data *))DL_FETCH_SYMBOL(handle, "ddog_php_prof_interrupt_function_unlikely"); + if (!profiling_interrupt_function) { + // Fall back for compatibility with profiler versions from before the + // unlikely-pending fast path was exported. + profiling_interrupt_function = (void(*)(zend_execute_data *))DL_FETCH_SYMBOL(handle, "ddog_php_prof_interrupt_function"); + } if (UNEXPECTED(!profiling_interrupt_function)) { - LOG(WARN, "[Datadog Trace] Profiling was detected, but locating symbol %s failed: %s\n", "ddog_php_prof_interrupt_function", - GET_DL_ERROR()); + LOG(WARN, "[Datadog Trace] Profiling was detected, but locating an interrupt function failed: %s\n", GET_DL_ERROR()); } profiling_notify_trace_finished = (profiling_notify_trace_finished_t)DL_FETCH_SYMBOL(handle, "datadog_profiling_notify_trace_finished"); From 32abe928af2f1a804426a975fc77c589ecc92955 Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Tue, 28 Jul 2026 19:41:36 -0600 Subject: [PATCH 6/9] perf(profiling): move interrupt_count to module globals And use system settings from the global, rather than through the REQUEST_LOCALS. --- profiling/src/allocation/mod.rs | 7 +-- profiling/src/capi.rs | 5 +- profiling/src/lib.rs | 12 +++-- profiling/src/module_globals.rs | 56 ++++++++++++++++++---- profiling/src/wall_time.rs | 82 ++++++++++++++++----------------- 5 files changed, 105 insertions(+), 57 deletions(-) diff --git a/profiling/src/allocation/mod.rs b/profiling/src/allocation/mod.rs index f366d6a1cbe..51ecac48dbf 100644 --- a/profiling/src/allocation/mod.rs +++ b/profiling/src/allocation/mod.rs @@ -199,9 +199,10 @@ pub fn collect_allocation(ptr: *mut c_void, len: size_t) { // Check if there's a pending time interrupt that we can handle now // instead of waiting for an interrupt handler. This is slightly more // accurate and efficient, win-win. - let interrupt_count = REQUEST_LOCALS - .try_with_borrow(|locals| locals.interrupt_count.swap(0, Ordering::SeqCst)) - .unwrap_or(0); + // SAFETY: allocation samples are collected on an initialized PHP request thread. + let globals = unsafe { module_globals::get_profiler_globals() }; + // SAFETY: the current thread's module globals are valid through GSHUTDOWN. + let interrupt_count = unsafe { (*globals).interrupt_count.swap(0, Ordering::SeqCst) }; // SAFETY: execute_data was provided by the engine, and the profiler // doesn't mutate it. diff --git a/profiling/src/capi.rs b/profiling/src/capi.rs index a96ddf24fc2..932b6d530d2 100644 --- a/profiling/src/capi.rs +++ b/profiling/src/capi.rs @@ -46,7 +46,10 @@ extern "C" fn ddog_php_prof_trigger_time_sample() { if locals.system_settings().profiling_enabled { // Safety: only vm interrupts are stored there, or possibly null (edges only). if let Some(vm_interrupt) = unsafe { locals.vm_interrupt_addr.as_ref() } { - locals.interrupt_count.fetch_add(1, Ordering::SeqCst); + // SAFETY: this callback runs on an initialized PHP request thread. + let globals = unsafe { crate::module_globals::get_profiler_globals() }; + // SAFETY: the current thread's module globals are valid through GSHUTDOWN. + unsafe { (*globals).interrupt_count.fetch_add(1, Ordering::SeqCst) }; vm_interrupt.store(true, Ordering::SeqCst); } } diff --git a/profiling/src/lib.rs b/profiling/src/lib.rs index e327b25ff5c..f815f1514c4 100644 --- a/profiling/src/lib.rs +++ b/profiling/src/lib.rs @@ -425,7 +425,6 @@ pub struct RequestLocals { pub system_settings: ptr::NonNull, pub profiling_experimental_heap_live_enabled: bool, - pub interrupt_count: AtomicU32, pub vm_interrupt_addr: *const AtomicBool, } @@ -450,7 +449,6 @@ impl Default for RequestLocals { tags: vec![], system_settings: SystemSettings::get(), profiling_experimental_heap_live_enabled: false, - interrupt_count: AtomicU32::new(0), vm_interrupt_addr: ptr::null_mut(), } } @@ -738,8 +736,11 @@ extern "C" fn rinit(_type: c_int, _module_number: c_int) -> ZendResult { } if let Some(profiler) = Profiler::get() { + // SAFETY: PHP module globals are initialized for this request thread. + let globals = unsafe { module_globals::get_profiler_globals() }; let interrupt = VmInterrupt { - interrupt_count_ptr: &locals.interrupt_count as *const AtomicU32, + // SAFETY: `globals` is valid until this thread's GSHUTDOWN. + interrupt_count_ptr: unsafe { ptr::addr_of!((*globals).interrupt_count) }, engine_ptr: locals.vm_interrupt_addr, }; profiler.add_interrupt(interrupt); @@ -795,8 +796,11 @@ extern "C" fn rshutdown(_type: c_int, _module_number: c_int) -> ZendResult { // and we don't need to optimize for that. if system_settings.profiling_enabled { if let Some(profiler) = Profiler::get() { + // SAFETY: PHP module globals remain initialized through RSHUTDOWN. + let globals = unsafe { module_globals::get_profiler_globals() }; let interrupt = VmInterrupt { - interrupt_count_ptr: &locals.interrupt_count, + // SAFETY: `globals` remains valid until this thread's GSHUTDOWN. + interrupt_count_ptr: unsafe { ptr::addr_of!((*globals).interrupt_count) }, engine_ptr: locals.vm_interrupt_addr, }; profiler.remove_interrupt(interrupt); diff --git a/profiling/src/module_globals.rs b/profiling/src/module_globals.rs index 85c938e5355..470b6a08f49 100644 --- a/profiling/src/module_globals.rs +++ b/profiling/src/module_globals.rs @@ -2,6 +2,7 @@ use crate::allocation; use core::cell::Cell; use core::ffi::c_void; use core::ptr; +use core::sync::atomic::AtomicU32; #[cfg(php_zend_mm_set_custom_handlers_ex)] use crate::allocation::allocation_ge84::ZendMMState; @@ -13,6 +14,12 @@ pub struct ProfilerGlobals { /// Wrapped in `Cell` to prevent torn reads/writes when allocation hooks /// are called re-entrantly during `rinit()`/`rshutdown()`. pub zend_mm_state: Cell, + /// Number of profiler time interrupts pending for this PHP thread. + /// + /// The profiler timer thread updates this through a pointer registered by + /// the PHP thread, so the value must remain atomic despite living in + /// thread-local PHP module globals. + pub interrupt_count: AtomicU32, } /// We need TSRM to call into GINIT and GSHUTDOWN to observe spawning and @@ -29,8 +36,17 @@ pub static mut GLOBALS_ID: i32 = 0; #[cfg(not(php_zts))] pub static mut GLOBALS: ProfilerGlobals = ProfilerGlobals { zend_mm_state: Cell::new(ZendMMState::new()), + interrupt_count: AtomicU32::new(0), }; +// Unit tests are not loaded by PHP, so provide the TSRM symbol needed to link +// tests that retain the ZTS module-global accessors. +#[cfg(all(test, php_zts))] +#[no_mangle] +unsafe extern "C" fn tsrm_get_ls_cache() -> *mut c_void { + ptr::null_mut() +} + #[cfg(php_zts)] mod zts { use core::ffi::c_void; @@ -40,9 +56,13 @@ mod zts { } #[inline] - pub unsafe fn tsrmg_bulk(id: i32) -> *mut c_void { - let tls = tsrm_get_ls_cache() as *mut *mut *mut c_void; - let storage = *tls; // void** storage + pub unsafe fn get_ls_cache() -> *mut c_void { + tsrm_get_ls_cache() + } + + #[inline] + pub unsafe fn tsrmg_bulk(ls_cache: *mut c_void, id: i32) -> *mut c_void { + let storage = *(ls_cache as *mut *mut *mut c_void); // void** storage // TSRM_UNSHUFFLE_RSRC_ID(id) is just `id - 1`. let idx = (id - 1) as usize; @@ -51,6 +71,27 @@ mod zts { } } +#[cfg(php_zts)] +#[inline] +pub unsafe fn get_tsrm_ls_cache() -> *mut c_void { + zts::get_ls_cache() +} + +#[cfg(php_zts)] +#[inline] +pub unsafe fn get_tsrm_resource_from_cache(ls_cache: *mut c_void, id: i32) -> *mut c_void { + zts::tsrmg_bulk(ls_cache, id) +} + +#[cfg(php_zts)] +#[inline] +pub unsafe fn get_profiler_globals_from_cache(ls_cache: *mut c_void) -> *mut ProfilerGlobals { + // SAFETY: As long as this is called during the times documented by + // get_profiler_globals(), GLOBALS_ID will be set by PHP. + let id = ptr::addr_of!(GLOBALS_ID).read(); + get_tsrm_resource_from_cache(ls_cache, id).cast() +} + /// Returns a pointer to the profiler globals for the current thread. /// /// # Safety @@ -64,10 +105,7 @@ mod zts { pub unsafe fn get_profiler_globals() -> *mut ProfilerGlobals { #[cfg(php_zts)] { - // SAFETY: As long as this is called during the times documented by - // our own safety requirements, GLOBALS_ID will be set by PHP. - let id = ptr::addr_of!(GLOBALS_ID).read(); - zts::tsrmg_bulk(id).cast() + get_profiler_globals_from_cache(get_tsrm_ls_cache()) } #[cfg(not(php_zts))] @@ -85,11 +123,13 @@ pub unsafe extern "C" fn ginit(_globals_ptr: *mut c_void) { #[cfg(php_zts)] crate::timeline::timeline_ginit(); + let globals = _globals_ptr.cast::(); + (*globals).interrupt_count = AtomicU32::new(0); + // Initialize ZendMMState in PHP globals for ZTS builds. For NTS builds, // this was already done in its const initializer. #[cfg(php_zts)] { - let globals = _globals_ptr.cast::(); (*globals).zend_mm_state = Cell::new(ZendMMState::new()); } diff --git a/profiling/src/wall_time.rs b/profiling/src/wall_time.rs index f1e3160fb64..a698b7ab9ac 100644 --- a/profiling/src/wall_time.rs +++ b/profiling/src/wall_time.rs @@ -2,9 +2,10 @@ //! implementation reasons, it has cpu-time code as well. use crate::bindings::{zend_execute_data, zend_interrupt_function, VmInterruptFn}; -use crate::{profiling::Profiler, RefCellExt, RequestLocals, REQUEST_LOCALS}; +use crate::config::SystemSettings; +use crate::module_globals::{self, ProfilerGlobals}; +use crate::profiling::Profiler; use core::ptr; -use log::debug; use std::sync::atomic::Ordering; #[cfg(not(php_frameless))] @@ -109,16 +110,12 @@ static mut PREV_INTERRUPT_FUNCTION: Option = None; #[no_mangle] #[inline(never)] pub extern "C" fn ddog_php_prof_interrupt_function(execute_data: *mut zend_execute_data) { - if let Err(err) = - REQUEST_LOCALS.try_with_borrow(|locals| interrupt_function(locals, execute_data)) - { - debug!("ddog_php_prof_interrupt_function failed to borrow request locals: {err}"); - } + // SAFETY: interrupt callbacks run while the current PHP thread's module globals are valid. + let globals = unsafe { &*module_globals::get_profiler_globals() }; + interrupt_function(globals, execute_data); } -fn interrupt_function(locals: &RequestLocals, execute_data: *mut zend_execute_data) { - let profiling_disabled = !locals.system_settings().profiling_enabled; - +fn interrupt_function(globals: &ProfilerGlobals, execute_data: *mut zend_execute_data) { /* Other extensions/modules or the engine itself may trigger an * interrupt, but given how expensive it is to gather a stack trace, * it should only be done if we triggered it ourselves. So @@ -126,8 +123,15 @@ fn interrupt_function(locals: &RequestLocals, execute_data: *mut zend_execute_da * 1. Track how many interrupts there were. * 2. Ensure we don't collect on someone else's interrupt. */ - let interrupt_count = locals.interrupt_count.swap(0, Ordering::SeqCst); - if profiling_disabled | (interrupt_count == 0) { + let interrupt_count = globals.interrupt_count.swap(0, Ordering::SeqCst); + if interrupt_count == 0 { + return; + } + + // SAFETY: `SystemSettings::get()` points to an initialized process + // lifetime static. Lifecycle updates happen in synchronized startup, fork, + // or shutdown phases, and this reference is short-lived. + if !unsafe { SystemSettings::get().as_ref() }.profiling_enabled { return; } @@ -149,26 +153,23 @@ fn interrupt_function(locals: &RequestLocals, execute_data: *mut zend_execute_da #[no_mangle] #[inline(never)] pub extern "C" fn ddog_php_prof_interrupt_function_unlikely(execute_data: *mut zend_execute_data) { - let result = REQUEST_LOCALS.try_with_borrow(|locals| { - // Optimize here with the expectation there isn't an interrupt, because - // overwhelmingly, there will not be one. The relaxed load avoids the - // more expensive atomic read-modify-write in `interrupt_function` on - // the idle path. It is only a fast-path hint: `interrupt_function` does - // the authoritative swap. If another consumer clears the count between - // the load and swap, the swap simply observes zero; if a producer adds - // an interrupt after this load observes zero, the count remains pending - // and the corresponding VM interrupt will provide another opportunity - // to handle it. - if locals.interrupt_count.load(Ordering::Relaxed) == 0 { - return; - } - - interrupt_function(locals, execute_data); - }); - - if let Err(err) = result { - debug!("ddog_php_prof_interrupt_function_unlikely failed to borrow request locals: {err}"); + // SAFETY: interrupt checks run while the current PHP thread's module globals are valid. + let globals = unsafe { &*module_globals::get_profiler_globals() }; + + // Optimize here with the expectation there isn't an interrupt, because + // overwhelmingly, there will not be one. The relaxed load avoids the + // more expensive atomic read-modify-write in `interrupt_function` on + // the idle path. It is only a fast-path hint: `interrupt_function` does + // the authoritative swap. If another consumer clears the count between + // the load and swap, the swap simply observes zero; if a producer adds + // an interrupt after this load observes zero, the count remains pending + // and the corresponding VM interrupt will provide another opportunity + // to handle it. + if globals.interrupt_count.load(Ordering::Relaxed) == 0 { + return; } + + interrupt_function(globals, execute_data); } #[cfg(php_frameless)] @@ -178,8 +179,9 @@ mod frameless { use crate::bindings::{ zend_flf_functions, zend_flf_handlers, zend_frameless_function_info, }; - use crate::wall_time::ddog_php_prof_interrupt_function; - use crate::{zend, RefCellExt, REQUEST_LOCALS}; + use crate::module_globals; + use crate::wall_time::interrupt_function; + use crate::zend; use dynasmrt::{dynasm, DynasmApi, ExecutableBuffer}; use log::error; use std::ffi::c_void; @@ -305,18 +307,16 @@ mod frameless { #[no_mangle] #[inline(never)] pub extern "C" fn ddog_php_prof_icall_trampoline_target() { - let interrupt_count = REQUEST_LOCALS - .try_with_borrow(|locals| locals.interrupt_count.load(Ordering::Relaxed)) - .unwrap_or(0); - - if interrupt_count == 0 { + // SAFETY: frameless handlers run while the current PHP thread's module globals are + // valid. Retain the pointer so the authoritative swap reuses the same TSRM lookup. + let globals = unsafe { &*module_globals::get_profiler_globals() }; + if globals.interrupt_count.load(Ordering::Relaxed) == 0 { return; } - // Fetching the execute data is intentionally delayed until we know - // that the interrupt count is greater than 0 for perf. + // Fetching execute data is intentionally delayed until a profiler interrupt is pending. let execute_data = unsafe { zend::ddog_php_prof_get_current_execute_data() }; - ddog_php_prof_interrupt_function(execute_data); + interrupt_function(globals, execute_data); } } From d9ea1f85f49b0f47056701e17cde409b6aa2e3cd Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Wed, 29 Jul 2026 09:26:50 -0600 Subject: [PATCH 7/9] Revert "perf(profiling): move interrupt_count to module globals" This reverts commit 32abe928af2f1a804426a975fc77c589ecc92955. --- profiling/src/allocation/mod.rs | 7 ++- profiling/src/capi.rs | 5 +- profiling/src/lib.rs | 12 ++--- profiling/src/module_globals.rs | 56 ++++------------------ profiling/src/wall_time.rs | 82 ++++++++++++++++----------------- 5 files changed, 57 insertions(+), 105 deletions(-) diff --git a/profiling/src/allocation/mod.rs b/profiling/src/allocation/mod.rs index 51ecac48dbf..f366d6a1cbe 100644 --- a/profiling/src/allocation/mod.rs +++ b/profiling/src/allocation/mod.rs @@ -199,10 +199,9 @@ pub fn collect_allocation(ptr: *mut c_void, len: size_t) { // Check if there's a pending time interrupt that we can handle now // instead of waiting for an interrupt handler. This is slightly more // accurate and efficient, win-win. - // SAFETY: allocation samples are collected on an initialized PHP request thread. - let globals = unsafe { module_globals::get_profiler_globals() }; - // SAFETY: the current thread's module globals are valid through GSHUTDOWN. - let interrupt_count = unsafe { (*globals).interrupt_count.swap(0, Ordering::SeqCst) }; + let interrupt_count = REQUEST_LOCALS + .try_with_borrow(|locals| locals.interrupt_count.swap(0, Ordering::SeqCst)) + .unwrap_or(0); // SAFETY: execute_data was provided by the engine, and the profiler // doesn't mutate it. diff --git a/profiling/src/capi.rs b/profiling/src/capi.rs index 932b6d530d2..a96ddf24fc2 100644 --- a/profiling/src/capi.rs +++ b/profiling/src/capi.rs @@ -46,10 +46,7 @@ extern "C" fn ddog_php_prof_trigger_time_sample() { if locals.system_settings().profiling_enabled { // Safety: only vm interrupts are stored there, or possibly null (edges only). if let Some(vm_interrupt) = unsafe { locals.vm_interrupt_addr.as_ref() } { - // SAFETY: this callback runs on an initialized PHP request thread. - let globals = unsafe { crate::module_globals::get_profiler_globals() }; - // SAFETY: the current thread's module globals are valid through GSHUTDOWN. - unsafe { (*globals).interrupt_count.fetch_add(1, Ordering::SeqCst) }; + locals.interrupt_count.fetch_add(1, Ordering::SeqCst); vm_interrupt.store(true, Ordering::SeqCst); } } diff --git a/profiling/src/lib.rs b/profiling/src/lib.rs index f815f1514c4..e327b25ff5c 100644 --- a/profiling/src/lib.rs +++ b/profiling/src/lib.rs @@ -425,6 +425,7 @@ pub struct RequestLocals { pub system_settings: ptr::NonNull, pub profiling_experimental_heap_live_enabled: bool, + pub interrupt_count: AtomicU32, pub vm_interrupt_addr: *const AtomicBool, } @@ -449,6 +450,7 @@ impl Default for RequestLocals { tags: vec![], system_settings: SystemSettings::get(), profiling_experimental_heap_live_enabled: false, + interrupt_count: AtomicU32::new(0), vm_interrupt_addr: ptr::null_mut(), } } @@ -736,11 +738,8 @@ extern "C" fn rinit(_type: c_int, _module_number: c_int) -> ZendResult { } if let Some(profiler) = Profiler::get() { - // SAFETY: PHP module globals are initialized for this request thread. - let globals = unsafe { module_globals::get_profiler_globals() }; let interrupt = VmInterrupt { - // SAFETY: `globals` is valid until this thread's GSHUTDOWN. - interrupt_count_ptr: unsafe { ptr::addr_of!((*globals).interrupt_count) }, + interrupt_count_ptr: &locals.interrupt_count as *const AtomicU32, engine_ptr: locals.vm_interrupt_addr, }; profiler.add_interrupt(interrupt); @@ -796,11 +795,8 @@ extern "C" fn rshutdown(_type: c_int, _module_number: c_int) -> ZendResult { // and we don't need to optimize for that. if system_settings.profiling_enabled { if let Some(profiler) = Profiler::get() { - // SAFETY: PHP module globals remain initialized through RSHUTDOWN. - let globals = unsafe { module_globals::get_profiler_globals() }; let interrupt = VmInterrupt { - // SAFETY: `globals` remains valid until this thread's GSHUTDOWN. - interrupt_count_ptr: unsafe { ptr::addr_of!((*globals).interrupt_count) }, + interrupt_count_ptr: &locals.interrupt_count, engine_ptr: locals.vm_interrupt_addr, }; profiler.remove_interrupt(interrupt); diff --git a/profiling/src/module_globals.rs b/profiling/src/module_globals.rs index 470b6a08f49..85c938e5355 100644 --- a/profiling/src/module_globals.rs +++ b/profiling/src/module_globals.rs @@ -2,7 +2,6 @@ use crate::allocation; use core::cell::Cell; use core::ffi::c_void; use core::ptr; -use core::sync::atomic::AtomicU32; #[cfg(php_zend_mm_set_custom_handlers_ex)] use crate::allocation::allocation_ge84::ZendMMState; @@ -14,12 +13,6 @@ pub struct ProfilerGlobals { /// Wrapped in `Cell` to prevent torn reads/writes when allocation hooks /// are called re-entrantly during `rinit()`/`rshutdown()`. pub zend_mm_state: Cell, - /// Number of profiler time interrupts pending for this PHP thread. - /// - /// The profiler timer thread updates this through a pointer registered by - /// the PHP thread, so the value must remain atomic despite living in - /// thread-local PHP module globals. - pub interrupt_count: AtomicU32, } /// We need TSRM to call into GINIT and GSHUTDOWN to observe spawning and @@ -36,17 +29,8 @@ pub static mut GLOBALS_ID: i32 = 0; #[cfg(not(php_zts))] pub static mut GLOBALS: ProfilerGlobals = ProfilerGlobals { zend_mm_state: Cell::new(ZendMMState::new()), - interrupt_count: AtomicU32::new(0), }; -// Unit tests are not loaded by PHP, so provide the TSRM symbol needed to link -// tests that retain the ZTS module-global accessors. -#[cfg(all(test, php_zts))] -#[no_mangle] -unsafe extern "C" fn tsrm_get_ls_cache() -> *mut c_void { - ptr::null_mut() -} - #[cfg(php_zts)] mod zts { use core::ffi::c_void; @@ -56,13 +40,9 @@ mod zts { } #[inline] - pub unsafe fn get_ls_cache() -> *mut c_void { - tsrm_get_ls_cache() - } - - #[inline] - pub unsafe fn tsrmg_bulk(ls_cache: *mut c_void, id: i32) -> *mut c_void { - let storage = *(ls_cache as *mut *mut *mut c_void); // void** storage + pub unsafe fn tsrmg_bulk(id: i32) -> *mut c_void { + let tls = tsrm_get_ls_cache() as *mut *mut *mut c_void; + let storage = *tls; // void** storage // TSRM_UNSHUFFLE_RSRC_ID(id) is just `id - 1`. let idx = (id - 1) as usize; @@ -71,27 +51,6 @@ mod zts { } } -#[cfg(php_zts)] -#[inline] -pub unsafe fn get_tsrm_ls_cache() -> *mut c_void { - zts::get_ls_cache() -} - -#[cfg(php_zts)] -#[inline] -pub unsafe fn get_tsrm_resource_from_cache(ls_cache: *mut c_void, id: i32) -> *mut c_void { - zts::tsrmg_bulk(ls_cache, id) -} - -#[cfg(php_zts)] -#[inline] -pub unsafe fn get_profiler_globals_from_cache(ls_cache: *mut c_void) -> *mut ProfilerGlobals { - // SAFETY: As long as this is called during the times documented by - // get_profiler_globals(), GLOBALS_ID will be set by PHP. - let id = ptr::addr_of!(GLOBALS_ID).read(); - get_tsrm_resource_from_cache(ls_cache, id).cast() -} - /// Returns a pointer to the profiler globals for the current thread. /// /// # Safety @@ -105,7 +64,10 @@ pub unsafe fn get_profiler_globals_from_cache(ls_cache: *mut c_void) -> *mut Pro pub unsafe fn get_profiler_globals() -> *mut ProfilerGlobals { #[cfg(php_zts)] { - get_profiler_globals_from_cache(get_tsrm_ls_cache()) + // SAFETY: As long as this is called during the times documented by + // our own safety requirements, GLOBALS_ID will be set by PHP. + let id = ptr::addr_of!(GLOBALS_ID).read(); + zts::tsrmg_bulk(id).cast() } #[cfg(not(php_zts))] @@ -123,13 +85,11 @@ pub unsafe extern "C" fn ginit(_globals_ptr: *mut c_void) { #[cfg(php_zts)] crate::timeline::timeline_ginit(); - let globals = _globals_ptr.cast::(); - (*globals).interrupt_count = AtomicU32::new(0); - // Initialize ZendMMState in PHP globals for ZTS builds. For NTS builds, // this was already done in its const initializer. #[cfg(php_zts)] { + let globals = _globals_ptr.cast::(); (*globals).zend_mm_state = Cell::new(ZendMMState::new()); } diff --git a/profiling/src/wall_time.rs b/profiling/src/wall_time.rs index a698b7ab9ac..f1e3160fb64 100644 --- a/profiling/src/wall_time.rs +++ b/profiling/src/wall_time.rs @@ -2,10 +2,9 @@ //! implementation reasons, it has cpu-time code as well. use crate::bindings::{zend_execute_data, zend_interrupt_function, VmInterruptFn}; -use crate::config::SystemSettings; -use crate::module_globals::{self, ProfilerGlobals}; -use crate::profiling::Profiler; +use crate::{profiling::Profiler, RefCellExt, RequestLocals, REQUEST_LOCALS}; use core::ptr; +use log::debug; use std::sync::atomic::Ordering; #[cfg(not(php_frameless))] @@ -110,12 +109,16 @@ static mut PREV_INTERRUPT_FUNCTION: Option = None; #[no_mangle] #[inline(never)] pub extern "C" fn ddog_php_prof_interrupt_function(execute_data: *mut zend_execute_data) { - // SAFETY: interrupt callbacks run while the current PHP thread's module globals are valid. - let globals = unsafe { &*module_globals::get_profiler_globals() }; - interrupt_function(globals, execute_data); + if let Err(err) = + REQUEST_LOCALS.try_with_borrow(|locals| interrupt_function(locals, execute_data)) + { + debug!("ddog_php_prof_interrupt_function failed to borrow request locals: {err}"); + } } -fn interrupt_function(globals: &ProfilerGlobals, execute_data: *mut zend_execute_data) { +fn interrupt_function(locals: &RequestLocals, execute_data: *mut zend_execute_data) { + let profiling_disabled = !locals.system_settings().profiling_enabled; + /* Other extensions/modules or the engine itself may trigger an * interrupt, but given how expensive it is to gather a stack trace, * it should only be done if we triggered it ourselves. So @@ -123,15 +126,8 @@ fn interrupt_function(globals: &ProfilerGlobals, execute_data: *mut zend_execute * 1. Track how many interrupts there were. * 2. Ensure we don't collect on someone else's interrupt. */ - let interrupt_count = globals.interrupt_count.swap(0, Ordering::SeqCst); - if interrupt_count == 0 { - return; - } - - // SAFETY: `SystemSettings::get()` points to an initialized process - // lifetime static. Lifecycle updates happen in synchronized startup, fork, - // or shutdown phases, and this reference is short-lived. - if !unsafe { SystemSettings::get().as_ref() }.profiling_enabled { + let interrupt_count = locals.interrupt_count.swap(0, Ordering::SeqCst); + if profiling_disabled | (interrupt_count == 0) { return; } @@ -153,23 +149,26 @@ fn interrupt_function(globals: &ProfilerGlobals, execute_data: *mut zend_execute #[no_mangle] #[inline(never)] pub extern "C" fn ddog_php_prof_interrupt_function_unlikely(execute_data: *mut zend_execute_data) { - // SAFETY: interrupt checks run while the current PHP thread's module globals are valid. - let globals = unsafe { &*module_globals::get_profiler_globals() }; - - // Optimize here with the expectation there isn't an interrupt, because - // overwhelmingly, there will not be one. The relaxed load avoids the - // more expensive atomic read-modify-write in `interrupt_function` on - // the idle path. It is only a fast-path hint: `interrupt_function` does - // the authoritative swap. If another consumer clears the count between - // the load and swap, the swap simply observes zero; if a producer adds - // an interrupt after this load observes zero, the count remains pending - // and the corresponding VM interrupt will provide another opportunity - // to handle it. - if globals.interrupt_count.load(Ordering::Relaxed) == 0 { - return; - } + let result = REQUEST_LOCALS.try_with_borrow(|locals| { + // Optimize here with the expectation there isn't an interrupt, because + // overwhelmingly, there will not be one. The relaxed load avoids the + // more expensive atomic read-modify-write in `interrupt_function` on + // the idle path. It is only a fast-path hint: `interrupt_function` does + // the authoritative swap. If another consumer clears the count between + // the load and swap, the swap simply observes zero; if a producer adds + // an interrupt after this load observes zero, the count remains pending + // and the corresponding VM interrupt will provide another opportunity + // to handle it. + if locals.interrupt_count.load(Ordering::Relaxed) == 0 { + return; + } - interrupt_function(globals, execute_data); + interrupt_function(locals, execute_data); + }); + + if let Err(err) = result { + debug!("ddog_php_prof_interrupt_function_unlikely failed to borrow request locals: {err}"); + } } #[cfg(php_frameless)] @@ -179,9 +178,8 @@ mod frameless { use crate::bindings::{ zend_flf_functions, zend_flf_handlers, zend_frameless_function_info, }; - use crate::module_globals; - use crate::wall_time::interrupt_function; - use crate::zend; + use crate::wall_time::ddog_php_prof_interrupt_function; + use crate::{zend, RefCellExt, REQUEST_LOCALS}; use dynasmrt::{dynasm, DynasmApi, ExecutableBuffer}; use log::error; use std::ffi::c_void; @@ -307,16 +305,18 @@ mod frameless { #[no_mangle] #[inline(never)] pub extern "C" fn ddog_php_prof_icall_trampoline_target() { - // SAFETY: frameless handlers run while the current PHP thread's module globals are - // valid. Retain the pointer so the authoritative swap reuses the same TSRM lookup. - let globals = unsafe { &*module_globals::get_profiler_globals() }; - if globals.interrupt_count.load(Ordering::Relaxed) == 0 { + let interrupt_count = REQUEST_LOCALS + .try_with_borrow(|locals| locals.interrupt_count.load(Ordering::Relaxed)) + .unwrap_or(0); + + if interrupt_count == 0 { return; } - // Fetching execute data is intentionally delayed until a profiler interrupt is pending. + // Fetching the execute data is intentionally delayed until we know + // that the interrupt count is greater than 0 for perf. let execute_data = unsafe { zend::ddog_php_prof_get_current_execute_data() }; - interrupt_function(globals, execute_data); + ddog_php_prof_interrupt_function(execute_data); } } From f0dde0818c356fde0a77ac4e1bda187c36f0b3d8 Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Wed, 29 Jul 2026 09:27:32 -0600 Subject: [PATCH 8/9] Revert "perf(profiling): use relaxed read to skip idle interrupts" This reverts commit 8cb3cde012b27d0ea07cfd3d7b3cac23d19b9faa. --- profiling/src/capi.rs | 4 +- profiling/src/wall_time.rs | 95 +++++++++++--------------------- tests/tea/profiling/profiling.cc | 6 +- tests/tea/profiling/profiling.h | 1 - tracer/engine_hooks.c | 10 +--- 5 files changed, 38 insertions(+), 78 deletions(-) diff --git a/profiling/src/capi.rs b/profiling/src/capi.rs index a96ddf24fc2..0ad6588fb3b 100644 --- a/profiling/src/capi.rs +++ b/profiling/src/capi.rs @@ -57,9 +57,7 @@ extern "C" fn ddog_php_prof_trigger_time_sample() { } } -pub use crate::wall_time::{ - ddog_php_prof_interrupt_function, ddog_php_prof_interrupt_function_unlikely, -}; +pub use crate::wall_time::ddog_php_prof_interrupt_function; #[cfg(test)] mod tests { diff --git a/profiling/src/wall_time.rs b/profiling/src/wall_time.rs index f1e3160fb64..3f0a948127a 100644 --- a/profiling/src/wall_time.rs +++ b/profiling/src/wall_time.rs @@ -2,7 +2,7 @@ //! implementation reasons, it has cpu-time code as well. use crate::bindings::{zend_execute_data, zend_interrupt_function, VmInterruptFn}; -use crate::{profiling::Profiler, RefCellExt, RequestLocals, REQUEST_LOCALS}; +use crate::{profiling::Profiler, RefCellExt, REQUEST_LOCALS}; use core::ptr; use log::debug; use std::sync::atomic::Ordering; @@ -79,8 +79,8 @@ mod execute_internal { unsafe { prev_execute_internal(execute_data, return_value) }; // See safety section of `execute_data_func_is_trampoline` docs for why - // the leaf frame is used instead of the execute_data ptr. - ddog_php_prof_interrupt_function_unlikely(leaf_frame); + // the leaf frame is used instead of the execute_data ptr. + ddog_php_prof_interrupt_function(leaf_frame); } /// # Safety @@ -109,65 +109,31 @@ static mut PREV_INTERRUPT_FUNCTION: Option = None; #[no_mangle] #[inline(never)] pub extern "C" fn ddog_php_prof_interrupt_function(execute_data: *mut zend_execute_data) { - if let Err(err) = - REQUEST_LOCALS.try_with_borrow(|locals| interrupt_function(locals, execute_data)) - { - debug!("ddog_php_prof_interrupt_function failed to borrow request locals: {err}"); - } -} - -fn interrupt_function(locals: &RequestLocals, execute_data: *mut zend_execute_data) { - let profiling_disabled = !locals.system_settings().profiling_enabled; - - /* Other extensions/modules or the engine itself may trigger an - * interrupt, but given how expensive it is to gather a stack trace, - * it should only be done if we triggered it ourselves. So - * interrupt_count serves dual purposes: - * 1. Track how many interrupts there were. - * 2. Ensure we don't collect on someone else's interrupt. - */ - let interrupt_count = locals.interrupt_count.swap(0, Ordering::SeqCst); - if profiling_disabled | (interrupt_count == 0) { - return; - } - - if let Some(profiler) = Profiler::get() { - // Safety: execute_data was provided by the engine, and the profiler doesn't mutate it. - profiler.collect_time(execute_data, interrupt_count); - } -} - -/// This function is like [`ddog_php_prof_interrupt_function`] except it's -/// optimized for the expectation that it's unlikely that there's actually an -/// interrupt actively needing to be handled. This is because we have to insert -/// this check in a variety of places, and in many of them (execute_internal, -/// frameless functions) there won't be an interrupt at all. -/// -/// # Safety -/// The zend_execute_data pointer should come from the engine to ensure it and -/// its sub-objects are valid. -#[no_mangle] -#[inline(never)] -pub extern "C" fn ddog_php_prof_interrupt_function_unlikely(execute_data: *mut zend_execute_data) { let result = REQUEST_LOCALS.try_with_borrow(|locals| { - // Optimize here with the expectation there isn't an interrupt, because - // overwhelmingly, there will not be one. The relaxed load avoids the - // more expensive atomic read-modify-write in `interrupt_function` on - // the idle path. It is only a fast-path hint: `interrupt_function` does - // the authoritative swap. If another consumer clears the count between - // the load and swap, the swap simply observes zero; if a producer adds - // an interrupt after this load observes zero, the count remains pending - // and the corresponding VM interrupt will provide another opportunity - // to handle it. - if locals.interrupt_count.load(Ordering::Relaxed) == 0 { + if !locals.system_settings().profiling_enabled { return; } - interrupt_function(locals, execute_data); + /* Other extensions/modules or the engine itself may trigger an + * interrupt, but given how expensive it is to gather a stack trace, + * it should only be done if we triggered it ourselves. So + * interrupt_count serves dual purposes: + * 1. Track how many interrupts there were. + * 2. Ensure we don't collect on someone else's interrupt. + */ + let interrupt_count = locals.interrupt_count.swap(0, Ordering::SeqCst); + if interrupt_count == 0 { + return; + } + + if let Some(profiler) = Profiler::get() { + // Safety: execute_data was provided by the engine, and the profiler doesn't mutate it. + profiler.collect_time(execute_data, interrupt_count); + } }); if let Err(err) = result { - debug!("ddog_php_prof_interrupt_function_unlikely failed to borrow request locals: {err}"); + debug!("ddog_php_prof_interrupt_function failed to borrow request locals: {err}"); } } @@ -178,8 +144,7 @@ mod frameless { use crate::bindings::{ zend_flf_functions, zend_flf_handlers, zend_frameless_function_info, }; - use crate::wall_time::ddog_php_prof_interrupt_function; - use crate::{zend, RefCellExt, REQUEST_LOCALS}; + use crate::{profiling::Profiler, zend, RefCellExt, REQUEST_LOCALS}; use dynasmrt::{dynasm, DynasmApi, ExecutableBuffer}; use log::error; use std::ffi::c_void; @@ -306,17 +271,23 @@ mod frameless { #[inline(never)] pub extern "C" fn ddog_php_prof_icall_trampoline_target() { let interrupt_count = REQUEST_LOCALS - .try_with_borrow(|locals| locals.interrupt_count.load(Ordering::Relaxed)) + .try_with_borrow(|locals| { + if !locals.system_settings().profiling_enabled { + return 0; + } + locals.interrupt_count.swap(0, Ordering::SeqCst) + }) .unwrap_or(0); if interrupt_count == 0 { return; } - // Fetching the execute data is intentionally delayed until we know - // that the interrupt count is greater than 0 for perf. - let execute_data = unsafe { zend::ddog_php_prof_get_current_execute_data() }; - ddog_php_prof_interrupt_function(execute_data); + if let Some(profiler) = Profiler::get() { + // SAFETY: profiler doesn't mutate execute_data + let execute_data = unsafe { zend::ddog_php_prof_get_current_execute_data() }; + profiler.collect_time(execute_data, interrupt_count); + } } } diff --git a/tests/tea/profiling/profiling.cc b/tests/tea/profiling/profiling.cc index 7de77795305..8c6e361427f 100644 --- a/tests/tea/profiling/profiling.cc +++ b/tests/tea/profiling/profiling.cc @@ -9,7 +9,7 @@ ZEND_TLS datadog_php_stack_sample last_stack_sample; ZEND_API datadog_php_stack_sample tea_get_last_stack_sample(void) { return last_stack_sample; } -ZEND_API void ddog_php_prof_interrupt_function_unlikely(zend_execute_data *execute_data) { +ZEND_API void ddog_php_prof_interrupt_function(zend_execute_data *execute_data) { datadog_php_stack_sample_ctor(&last_stack_sample); /* Don't try to re-implement everything. Remember, the tracer is being @@ -34,10 +34,6 @@ ZEND_API void ddog_php_prof_interrupt_function_unlikely(zend_execute_data *execu } } -ZEND_API void ddog_php_prof_interrupt_function(zend_execute_data *execute_data) { - ddog_php_prof_interrupt_function_unlikely(execute_data); -} - ZEND_API zend_extension_version_info extension_version_info = { ZEND_EXTENSION_API_NO, ZEND_EXTENSION_BUILD_ID, diff --git a/tests/tea/profiling/profiling.h b/tests/tea/profiling/profiling.h index 452476071f8..75f8aa6aa83 100644 --- a/tests/tea/profiling/profiling.h +++ b/tests/tea/profiling/profiling.h @@ -10,7 +10,6 @@ BEGIN_EXTERN_C() ZEND_API datadog_php_stack_sample tea_get_last_stack_sample(void); ZEND_API void ddog_php_prof_interrupt_function(zend_execute_data *execute_data); -ZEND_API void ddog_php_prof_interrupt_function_unlikely(zend_execute_data *execute_data); END_EXTERN_C() #endif diff --git a/tracer/engine_hooks.c b/tracer/engine_hooks.c index bbf9e16cb93..e2b2eaea976 100644 --- a/tracer/engine_hooks.c +++ b/tracer/engine_hooks.c @@ -41,14 +41,10 @@ void dd_search_for_profiling_symbols(void *arg) { if (extension->name && strcmp(extension->name, "datadog-profiling") == 0) { DL_HANDLE handle = extension->handle; - profiling_interrupt_function = (void(*)(zend_execute_data *))DL_FETCH_SYMBOL(handle, "ddog_php_prof_interrupt_function_unlikely"); - if (!profiling_interrupt_function) { - // Fall back for compatibility with profiler versions from before the - // unlikely-pending fast path was exported. - profiling_interrupt_function = (void(*)(zend_execute_data *))DL_FETCH_SYMBOL(handle, "ddog_php_prof_interrupt_function"); - } + profiling_interrupt_function = (void(*)(zend_execute_data *))DL_FETCH_SYMBOL(handle, "ddog_php_prof_interrupt_function"); if (UNEXPECTED(!profiling_interrupt_function)) { - LOG(WARN, "[Datadog Trace] Profiling was detected, but locating an interrupt function failed: %s\n", GET_DL_ERROR()); + LOG(WARN, "[Datadog Trace] Profiling was detected, but locating symbol %s failed: %s\n", "ddog_php_prof_interrupt_function", + GET_DL_ERROR()); } profiling_notify_trace_finished = (profiling_notify_trace_finished_t)DL_FETCH_SYMBOL(handle, "datadog_profiling_notify_trace_finished"); From d329cf623da6c2e29d17150ecd3646677b4b6e1e Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Wed, 29 Jul 2026 11:26:09 -0600 Subject: [PATCH 9/9] perf(prof): use TSRM globals for interrupt_count, Ordering::Relaxed These changes specifically speed up the hot paths of the interrupt function. Notably, this can be a bit hot on 8.3 and below because of execute_internal, and it's used on frameless functions on 8.4+. The biggest change comes from avoiding REQUEST_LOCALS. In this case, this is totally safe because interrupt_count is atomic. This also removes the profiling_enabled check of the interrupt function. This is unnecessary because: 1. We do not set interrupts when disabled to begin with, generally. trigger_time_sample can still trigger but this is test only. 2. For edge cases having a pending interrupt going into a fork, the child will call `Profiler::kill()` which will cause `Profiler::get()` to return None. --- profiling/src/allocation/mod.rs | 7 ++- profiling/src/capi.rs | 5 +- profiling/src/lib.rs | 12 ++-- profiling/src/module_globals.rs | 65 +++++++++++++++++++--- profiling/src/profiling/interrupts.rs | 4 +- profiling/src/profiling/mod.rs | 1 + profiling/src/wall_time.rs | 79 +++++++++++++-------------- 7 files changed, 112 insertions(+), 61 deletions(-) diff --git a/profiling/src/allocation/mod.rs b/profiling/src/allocation/mod.rs index f366d6a1cbe..bb6a762f260 100644 --- a/profiling/src/allocation/mod.rs +++ b/profiling/src/allocation/mod.rs @@ -199,9 +199,10 @@ pub fn collect_allocation(ptr: *mut c_void, len: size_t) { // Check if there's a pending time interrupt that we can handle now // instead of waiting for an interrupt handler. This is slightly more // accurate and efficient, win-win. - let interrupt_count = REQUEST_LOCALS - .try_with_borrow(|locals| locals.interrupt_count.swap(0, Ordering::SeqCst)) - .unwrap_or(0); + // SAFETY: allocation samples are collected on an initialized PHP request thread. + let globals = unsafe { module_globals::get_profiler_globals() }; + // SAFETY: the current thread's module globals are valid through GSHUTDOWN. + let interrupt_count = unsafe { (*globals).interrupt_count.swap(0, Ordering::Relaxed) }; // SAFETY: execute_data was provided by the engine, and the profiler // doesn't mutate it. diff --git a/profiling/src/capi.rs b/profiling/src/capi.rs index 0ad6588fb3b..0fe39a7b388 100644 --- a/profiling/src/capi.rs +++ b/profiling/src/capi.rs @@ -46,7 +46,10 @@ extern "C" fn ddog_php_prof_trigger_time_sample() { if locals.system_settings().profiling_enabled { // Safety: only vm interrupts are stored there, or possibly null (edges only). if let Some(vm_interrupt) = unsafe { locals.vm_interrupt_addr.as_ref() } { - locals.interrupt_count.fetch_add(1, Ordering::SeqCst); + // SAFETY: this callback runs on an initialized PHP request thread. + let globals = unsafe { crate::module_globals::get_profiler_globals() }; + // SAFETY: the current thread's module globals are valid through GSHUTDOWN. + unsafe { (*globals).interrupt_count.fetch_add(1, Ordering::Relaxed) }; vm_interrupt.store(true, Ordering::SeqCst); } } diff --git a/profiling/src/lib.rs b/profiling/src/lib.rs index e327b25ff5c..f815f1514c4 100644 --- a/profiling/src/lib.rs +++ b/profiling/src/lib.rs @@ -425,7 +425,6 @@ pub struct RequestLocals { pub system_settings: ptr::NonNull, pub profiling_experimental_heap_live_enabled: bool, - pub interrupt_count: AtomicU32, pub vm_interrupt_addr: *const AtomicBool, } @@ -450,7 +449,6 @@ impl Default for RequestLocals { tags: vec![], system_settings: SystemSettings::get(), profiling_experimental_heap_live_enabled: false, - interrupt_count: AtomicU32::new(0), vm_interrupt_addr: ptr::null_mut(), } } @@ -738,8 +736,11 @@ extern "C" fn rinit(_type: c_int, _module_number: c_int) -> ZendResult { } if let Some(profiler) = Profiler::get() { + // SAFETY: PHP module globals are initialized for this request thread. + let globals = unsafe { module_globals::get_profiler_globals() }; let interrupt = VmInterrupt { - interrupt_count_ptr: &locals.interrupt_count as *const AtomicU32, + // SAFETY: `globals` is valid until this thread's GSHUTDOWN. + interrupt_count_ptr: unsafe { ptr::addr_of!((*globals).interrupt_count) }, engine_ptr: locals.vm_interrupt_addr, }; profiler.add_interrupt(interrupt); @@ -795,8 +796,11 @@ extern "C" fn rshutdown(_type: c_int, _module_number: c_int) -> ZendResult { // and we don't need to optimize for that. if system_settings.profiling_enabled { if let Some(profiler) = Profiler::get() { + // SAFETY: PHP module globals remain initialized through RSHUTDOWN. + let globals = unsafe { module_globals::get_profiler_globals() }; let interrupt = VmInterrupt { - interrupt_count_ptr: &locals.interrupt_count, + // SAFETY: `globals` remains valid until this thread's GSHUTDOWN. + interrupt_count_ptr: unsafe { ptr::addr_of!((*globals).interrupt_count) }, engine_ptr: locals.vm_interrupt_addr, }; profiler.remove_interrupt(interrupt); diff --git a/profiling/src/module_globals.rs b/profiling/src/module_globals.rs index 85c938e5355..c91559656e4 100644 --- a/profiling/src/module_globals.rs +++ b/profiling/src/module_globals.rs @@ -2,6 +2,7 @@ use crate::allocation; use core::cell::Cell; use core::ffi::c_void; use core::ptr; +use core::sync::atomic::AtomicU32; #[cfg(php_zend_mm_set_custom_handlers_ex)] use crate::allocation::allocation_ge84::ZendMMState; @@ -13,6 +14,12 @@ pub struct ProfilerGlobals { /// Wrapped in `Cell` to prevent torn reads/writes when allocation hooks /// are called re-entrantly during `rinit()`/`rshutdown()`. pub zend_mm_state: Cell, + /// Number of profiler time interrupts pending for this PHP thread. + /// + /// The profiler timer thread updates this through a pointer registered by + /// the PHP thread, so the value must remain atomic despite living in + /// thread-local PHP module globals. + pub interrupt_count: AtomicU32, } /// We need TSRM to call into GINIT and GSHUTDOWN to observe spawning and @@ -29,6 +36,7 @@ pub static mut GLOBALS_ID: i32 = 0; #[cfg(not(php_zts))] pub static mut GLOBALS: ProfilerGlobals = ProfilerGlobals { zend_mm_state: Cell::new(ZendMMState::new()), + interrupt_count: AtomicU32::new(0), }; #[cfg(php_zts)] @@ -40,9 +48,13 @@ mod zts { } #[inline] - pub unsafe fn tsrmg_bulk(id: i32) -> *mut c_void { - let tls = tsrm_get_ls_cache() as *mut *mut *mut c_void; - let storage = *tls; // void** storage + pub unsafe fn get_ls_cache() -> *mut c_void { + tsrm_get_ls_cache() + } + + #[inline] + pub unsafe fn tsrmg_bulk(ls_cache: *mut c_void, id: i32) -> *mut c_void { + let storage = *(ls_cache as *mut *mut *mut c_void); // void** storage // TSRM_UNSHUFFLE_RSRC_ID(id) is just `id - 1`. let idx = (id - 1) as usize; @@ -51,6 +63,27 @@ mod zts { } } +#[cfg(php_zts)] +#[inline] +pub unsafe fn get_tsrm_ls_cache() -> *mut c_void { + zts::get_ls_cache() +} + +#[cfg(php_zts)] +#[inline] +pub unsafe fn get_tsrm_resource_from_cache(ls_cache: *mut c_void, id: i32) -> *mut c_void { + zts::tsrmg_bulk(ls_cache, id) +} + +#[cfg(php_zts)] +#[inline] +pub unsafe fn get_profiler_globals_from_cache(ls_cache: *mut c_void) -> *mut ProfilerGlobals { + // SAFETY: As long as this is called during the times documented by + // get_profiler_globals(), GLOBALS_ID will be set by PHP. + let id = ptr::addr_of!(GLOBALS_ID).read(); + get_tsrm_resource_from_cache(ls_cache, id).cast() +} + /// Returns a pointer to the profiler globals for the current thread. /// /// # Safety @@ -64,10 +97,7 @@ mod zts { pub unsafe fn get_profiler_globals() -> *mut ProfilerGlobals { #[cfg(php_zts)] { - // SAFETY: As long as this is called during the times documented by - // our own safety requirements, GLOBALS_ID will be set by PHP. - let id = ptr::addr_of!(GLOBALS_ID).read(); - zts::tsrmg_bulk(id).cast() + get_profiler_globals_from_cache(get_tsrm_ls_cache()) } #[cfg(not(php_zts))] @@ -85,12 +115,13 @@ pub unsafe extern "C" fn ginit(_globals_ptr: *mut c_void) { #[cfg(php_zts)] crate::timeline::timeline_ginit(); - // Initialize ZendMMState in PHP globals for ZTS builds. For NTS builds, - // this was already done in its const initializer. + // Initialize PHP globals for ZTS builds. For NTS builds, this was already + // done in its const initializer. #[cfg(php_zts)] { let globals = _globals_ptr.cast::(); (*globals).zend_mm_state = Cell::new(ZendMMState::new()); + (*globals).interrupt_count = AtomicU32::new(0); } // SAFETY: this is called in thread ginit as expected, and no other places. @@ -113,3 +144,19 @@ pub unsafe extern "C" fn gshutdown(_globals_ptr: *mut c_void) { // SAFETY: this is called in thread gshutdown as expected, no other places. allocation::gshutdown(); } + +// Unit tests are not loaded by PHP, so provide the PHP globals and TSRM symbol +// needed to link code retained in the test executable. +#[cfg(test)] +mod test_symbols { + #[cfg(not(php_zts))] + #[export_name = "compiler_globals"] + static mut TEST_COMPILER_GLOBALS: core::mem::MaybeUninit = + core::mem::MaybeUninit::zeroed(); + + #[cfg(php_zts)] + #[no_mangle] + unsafe extern "C" fn tsrm_get_ls_cache() -> *mut core::ffi::c_void { + core::ptr::null_mut() + } +} diff --git a/profiling/src/profiling/interrupts.rs b/profiling/src/profiling/interrupts.rs index bd1e32107eb..7cd9ab2830d 100644 --- a/profiling/src/profiling/interrupts.rs +++ b/profiling/src/profiling/interrupts.rs @@ -50,7 +50,7 @@ impl InterruptManager { // Reset interrupt counter to prevent sampling during `mshutdown` (PHP 8.0 bug with // userland destructors), but leave the interrupt flag unchanged as other extensions // may have raised it. - (*interrupt.interrupt_count_ptr).store(0, Ordering::SeqCst); + (*interrupt.interrupt_count_ptr).store(0, Ordering::Relaxed); } } @@ -62,7 +62,7 @@ impl InterruptManager { pub(super) fn trigger_interrupts(&self) { let vm_interrupts = self.vm_interrupts.lock().unwrap(); vm_interrupts.iter().for_each(|obj| unsafe { - (*obj.interrupt_count_ptr).fetch_add(1, Ordering::SeqCst); + (*obj.interrupt_count_ptr).fetch_add(1, Ordering::Relaxed); (*obj.engine_ptr).store(true, Ordering::SeqCst); }); } diff --git a/profiling/src/profiling/mod.rs b/profiling/src/profiling/mod.rs index 89c686bd189..759066c9379 100644 --- a/profiling/src/profiling/mod.rs +++ b/profiling/src/profiling/mod.rs @@ -1130,6 +1130,7 @@ impl Profiler { /// Collect a stack sample with elapsed wall time. Collects CPU time if /// it's enabled and available. + #[export_name = "ddog_php_prof_collect_time"] #[cfg_attr(feature = "tracing", tracing::instrument(skip_all, level = "debug"))] pub fn collect_time(&self, execute_data: *mut zend_execute_data, interrupt_count: u32) { // todo: should probably exclude the wall and CPU time used by collecting the sample. diff --git a/profiling/src/wall_time.rs b/profiling/src/wall_time.rs index 3f0a948127a..4acca712c8a 100644 --- a/profiling/src/wall_time.rs +++ b/profiling/src/wall_time.rs @@ -2,10 +2,10 @@ //! implementation reasons, it has cpu-time code as well. use crate::bindings::{zend_execute_data, zend_interrupt_function, VmInterruptFn}; -use crate::{profiling::Profiler, RefCellExt, REQUEST_LOCALS}; +use crate::module_globals; +use crate::profiling::Profiler; use core::ptr; -use log::debug; -use std::sync::atomic::Ordering; +use core::sync::atomic::Ordering; #[cfg(not(php_frameless))] mod execute_internal { @@ -79,7 +79,7 @@ mod execute_internal { unsafe { prev_execute_internal(execute_data, return_value) }; // See safety section of `execute_data_func_is_trampoline` docs for why - // the leaf frame is used instead of the execute_data ptr. + // the leaf frame is used instead of the execute_data ptr. ddog_php_prof_interrupt_function(leaf_frame); } @@ -109,31 +109,29 @@ static mut PREV_INTERRUPT_FUNCTION: Option = None; #[no_mangle] #[inline(never)] pub extern "C" fn ddog_php_prof_interrupt_function(execute_data: *mut zend_execute_data) { - let result = REQUEST_LOCALS.try_with_borrow(|locals| { - if !locals.system_settings().profiling_enabled { - return; - } - - /* Other extensions/modules or the engine itself may trigger an - * interrupt, but given how expensive it is to gather a stack trace, - * it should only be done if we triggered it ourselves. So - * interrupt_count serves dual purposes: - * 1. Track how many interrupts there were. - * 2. Ensure we don't collect on someone else's interrupt. - */ - let interrupt_count = locals.interrupt_count.swap(0, Ordering::SeqCst); - if interrupt_count == 0 { - return; - } - - if let Some(profiler) = Profiler::get() { - // Safety: execute_data was provided by the engine, and the profiler doesn't mutate it. - profiler.collect_time(execute_data, interrupt_count); - } - }); + // SAFETY: interrupt callbacks run while the current PHP thread's module globals are valid. + let atomic_count = unsafe { &(*module_globals::get_profiler_globals()).interrupt_count }; + + /* Other extensions/modules or the engine itself may trigger an + * interrupt, but given how expensive it is to gather a stack trace, + * it should only be done if we triggered it ourselves. So + * interrupt_count serves dual purposes: + * 1. Track how many interrupts there were. + * 2. Ensure we don't collect on someone else's interrupt. + */ + let interrupt_count = atomic_count.swap(0, Ordering::Relaxed); + if interrupt_count == 0 { + return; + } + collect_time_if_enabled(execute_data, interrupt_count); +} - if let Err(err) = result { - debug!("ddog_php_prof_interrupt_function failed to borrow request locals: {err}"); +#[inline(never)] +#[export_name = "ddog_php_prof_collect_time_if_enabled"] +extern "C" fn collect_time_if_enabled(execute_data: *mut zend_execute_data, interrupt_count: u32) { + if let Some(profiler) = Profiler::get() { + // Safety: execute_data was provided by the engine, and the profiler doesn't mutate it. + profiler.collect_time(execute_data, interrupt_count); } } @@ -144,7 +142,9 @@ mod frameless { use crate::bindings::{ zend_flf_functions, zend_flf_handlers, zend_frameless_function_info, }; - use crate::{profiling::Profiler, zend, RefCellExt, REQUEST_LOCALS}; + use crate::module_globals; + use crate::wall_time::collect_time_if_enabled; + use crate::zend; use dynasmrt::{dynasm, DynasmApi, ExecutableBuffer}; use log::error; use std::ffi::c_void; @@ -270,24 +270,19 @@ mod frameless { #[no_mangle] #[inline(never)] pub extern "C" fn ddog_php_prof_icall_trampoline_target() { - let interrupt_count = REQUEST_LOCALS - .try_with_borrow(|locals| { - if !locals.system_settings().profiling_enabled { - return 0; - } - locals.interrupt_count.swap(0, Ordering::SeqCst) - }) - .unwrap_or(0); + // SAFETY: frameless handlers run while the current PHP thread's module globals are + // valid. Retain the pointer so the authoritative swap reuses the same TSRM lookup. + let atomic_count = + unsafe { &(*module_globals::get_profiler_globals()).interrupt_count }; + let interrupt_count = atomic_count.swap(0, Ordering::Relaxed); if interrupt_count == 0 { return; } - if let Some(profiler) = Profiler::get() { - // SAFETY: profiler doesn't mutate execute_data - let execute_data = unsafe { zend::ddog_php_prof_get_current_execute_data() }; - profiler.collect_time(execute_data, interrupt_count); - } + // Fetching execute data is intentionally delayed until a profiler interrupt is pending. + let execute_data = unsafe { zend::ddog_php_prof_get_current_execute_data() }; + collect_time_if_enabled(execute_data, interrupt_count); } }