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).

Author: morrisonlevi

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 {

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<VmInterruptFn> = 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);
         }
     }
 

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,

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

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");