feat(profiling): remove execute_internal hook

This does some... uh... let's say "clever" things to avoid using the
zend_execute_internal hook. In PHP 8.4, that would prevent frameless
function call optimizations from being used. Using the hook also has
performance costs even on older versions.

Author: morrisonlevi

Cargo.toml

@@ -14,7 +14,7 @@ debug = 2 # full debug info
 
 [profile.release]
 codegen-units = 1
-debug = "line-tables-only"
+debug = 2 #todo: revert back
 incremental = false
 lto = "fat"
 

profiling/build.rs

@@ -27,6 +27,7 @@ fn main() {
     let vernum = php_config_vernum();
     let post_startup_cb = cfg_post_startup_cb(vernum);
     let preload = cfg_preload(vernum);
+    let good_closure_invoke = cfg_good_closure_invoke(vernum);
     let fibers = cfg_fibers(vernum);
     let run_time_cache = cfg_run_time_cache(vernum);
     let trigger_time_sample = cfg_trigger_time_sample();
@@ -37,6 +38,7 @@ fn main() {
         post_startup_cb,
         preload,
         run_time_cache,
+        good_closure_invoke,
         fibers,
         trigger_time_sample,
         vernum,
@@ -83,11 +85,13 @@ const ZAI_H_FILES: &[&str] = &[
     "../zend_abstract_interface/json/json.h",
 ];
 
+#[allow(clippy::too_many_arguments)]
 fn build_zend_php_ffis(
     php_config_includes: &str,
     post_startup_cb: bool,
     preload: bool,
     run_time_cache: bool,
+    good_closure_invoke: bool,
     fibers: bool,
     trigger_time_sample: bool,
     vernum: u64,
@@ -132,6 +136,7 @@ fn build_zend_php_ffis(
     let files = ["src/php_ffi.c", "../ext/handlers_api.c"];
     let post_startup_cb = if post_startup_cb { "1" } else { "0" };
     let preload = if preload { "1" } else { "0" };
+    let good_closure_invoke = if good_closure_invoke { "1" } else { "0" };
     let fibers = if fibers { "1" } else { "0" };
     let run_time_cache = if run_time_cache { "1" } else { "0" };
     let trigger_time_sample = if trigger_time_sample { "1" } else { "0" };
@@ -146,6 +151,7 @@ fn build_zend_php_ffis(
         .files(files.into_iter().chain(zai_c_files))
         .define("CFG_POST_STARTUP_CB", post_startup_cb)
         .define("CFG_PRELOAD", preload)
+        .define("CFG_GOOD_CLOSURE_INVOKE", good_closure_invoke)
         .define("CFG_FIBERS", fibers)
         .define("CFG_RUN_TIME_CACHE", run_time_cache)
         .define("CFG_STACK_WALKING_TESTS", stack_walking_tests)
@@ -308,6 +314,15 @@ fn cfg_php_major_version(vernum: u64) {
     println!("cargo:rustc-cfg=php{major_version}");
 }
 
+fn cfg_good_closure_invoke(vernum: u64) -> bool {
+    if vernum >= 80300 {
+        println!("cargo:rustc-cfg=php_good_closure_invoke");
+        true
+    } else {
+        false
+    }
+}
+
 fn cfg_fibers(vernum: u64) -> bool {
     if vernum >= 80100 {
         println!("cargo:rustc-cfg=php_has_fibers");

profiling/src/bindings/mod.rs

@@ -6,7 +6,6 @@ use libc::{c_char, c_int, c_uchar, c_uint, c_ushort, c_void, size_t};
 use std::borrow::Cow;
 use std::ffi::CStr;
 use std::marker::PhantomData;
-use std::sync::atomic::AtomicBool;
 use std::{ptr, str};
 
 extern "C" {
@@ -126,9 +125,8 @@ impl _zend_function {
 
     /// Returns the module name, if there is one. May return Some(b"\0").
     pub fn module_name(&self) -> Option<&[u8]> {
-        // Safety: the function's type field is always safe to access.
-        if unsafe { self.type_ } == ZEND_INTERNAL_FUNCTION as u8 {
-            // Safety: union access is guarded by ZEND_INTERNAL_FUNCTION, and
+        if self.is_internal() {
+            // SAFETY: union access is guarded by ZEND_INTERNAL_FUNCTION, and
             // assume its module is valid.
             unsafe { self.internal_function.module.as_ref() }
                 .filter(|module| !module.name.is_null())
@@ -138,6 +136,12 @@ impl _zend_function {
             None
         }
     }
+
+    #[inline]
+    pub fn is_internal(&self) -> bool {
+        // Safety: the function's type field is always safe to access.
+        unsafe { self.type_ == ZEND_INTERNAL_FUNCTION as u8 }
+    }
 }
 
 // In general, modify definitions which return int to return ZendResult if
@@ -283,10 +287,11 @@ impl Default for ZendExtension {
 }
 
 extern "C" {
-    /// Retrieves the VM interrupt address of the calling PHP thread.
+    /// Retrieves the addresses of various parts of executor_globals.
+    ///
     /// # Safety
     /// Must be called from a PHP thread during a request.
-    pub fn datadog_php_profiling_vm_interrupt_addr() -> *const AtomicBool;
+    pub fn ddog_php_prof_executor_global_addrs() -> ExecutorGlobalAddrs;
 
     /// Registers the extension. Note that it's kept in a zend_llist and gets
     /// pemalloc'd + memcpy'd into place. The engine says this is a mutable
@@ -342,6 +347,7 @@ extern "C" {
 }
 
 use crate::config::ConfigId;
+use crate::ExecutorGlobalAddrs;
 pub use zend_module_dep as ModuleDep;
 
 impl ModuleDep {

profiling/src/capi.rs

@@ -42,11 +42,16 @@ extern "C" fn ddog_php_prof_trigger_time_sample() {
     super::REQUEST_LOCALS.with(|cell| {
         if let Ok(locals) = cell.try_borrow() {
             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);
-                    vm_interrupt.store(true, Ordering::SeqCst);
-                }
+                // Safety: only vm interrupts are stored there.
+                let vm_interrupt = unsafe { locals.executor_global_addrs.vm_interrupt.as_ref() };
+                let current_execute_data_cache =
+                    unsafe { *locals.executor_global_addrs.current_execute_data.as_ptr() };
+
+                locals.interrupt_count.fetch_add(1, Ordering::SeqCst);
+                locals
+                    .current_execute_data_cache
+                    .store(current_execute_data_cache, Ordering::SeqCst);
+                vm_interrupt.store(true, Ordering::SeqCst);
             }
         }
     })

profiling/src/lib.rs

@@ -21,10 +21,11 @@ mod exception;
 #[cfg(feature = "timeline")]
 mod timeline;
 
+use crate::bindings::ddog_php_prof_executor_global_addrs;
 use crate::config::{SystemSettings, INITIAL_SYSTEM_SETTINGS};
 use bindings::{
-    self as zend, ddog_php_prof_php_version, ddog_php_prof_php_version_id, ZendExtension,
-    ZendResult,
+    self as zend, ddog_php_prof_php_version, ddog_php_prof_php_version_id, zend_execute_data, zval,
+    ZendExtension, ZendResult,
 };
 use clocks::*;
 use core::ptr;
@@ -33,14 +34,14 @@ use lazy_static::lazy_static;
 use libc::c_char;
 use log::{debug, error, info, trace, warn};
 use once_cell::sync::{Lazy, OnceCell};
-use profiling::{LocalRootSpanResourceMessage, Profiler, VmInterrupt};
+use profiling::{LocalRootSpanResourceMessage, Profiler};
 use sapi::Sapi;
 use std::borrow::Cow;
 use std::cell::RefCell;
 use std::ffi::CStr;
 use std::ops::Deref;
 use std::os::raw::c_int;
-use std::sync::atomic::{AtomicBool, AtomicU32, Ordering};
+use std::sync::atomic::{AtomicBool, AtomicPtr, AtomicU32, Ordering};
 use std::sync::{Arc, Mutex, Once};
 use std::time::{Duration, Instant};
 use uuid::Uuid;
@@ -333,6 +334,23 @@ extern "C" fn prshutdown() -> ZendResult {
     ZendResult::Success
 }
 
+// Keep in-sync with php_ffi.c.
+#[repr(C)]
+#[derive(Clone, Copy, Debug, Eq, PartialEq, Hash)]
+pub struct ExecutorGlobalAddrs {
+    pub vm_stack_top: ptr::NonNull<*mut zval>,
+    pub current_execute_data: ptr::NonNull<*mut zend_execute_data>,
+    pub vm_interrupt: ptr::NonNull<AtomicBool>,
+}
+
+impl ExecutorGlobalAddrs {
+    /// # Safety
+    /// Must be called on a PHP thread during a request.
+    unsafe fn new() -> ExecutorGlobalAddrs {
+        ddog_php_prof_executor_global_addrs()
+    }
+}
+
 pub struct RequestLocals {
     pub env: Option<String>,
     pub service: Option<String>,
@@ -347,29 +365,33 @@ pub struct RequestLocals {
     pub system_settings: ptr::NonNull<SystemSettings>,
 
     pub interrupt_count: AtomicU32,
-    pub vm_interrupt_addr: *const AtomicBool,
+    pub current_execute_data_cache: AtomicPtr<zend_execute_data>,
+    pub executor_global_addrs: ExecutorGlobalAddrs,
 }
 
 impl RequestLocals {
-    #[track_caller]
-    pub fn system_settings(&self) -> &SystemSettings {
-        // SAFETY: it should always be valid, either set to the
-        // INITIAL_SYSTEM_SETTINGS or to the SYSTEM_SETTINGS.
-        unsafe { self.system_settings.as_ref() }
-    }
-}
-
-impl Default for RequestLocals {
-    fn default() -> RequestLocals {
+    /// # Safety
+    /// Must be called from a PHP Thread.
+    unsafe fn new() -> RequestLocals {
         RequestLocals {
             env: None,
             service: None,
             version: None,
             system_settings: ptr::NonNull::from(INITIAL_SYSTEM_SETTINGS.deref()),
             interrupt_count: AtomicU32::new(0),
-            vm_interrupt_addr: ptr::null_mut(),
+            current_execute_data_cache: AtomicPtr::new(ptr::null_mut()),
+
+            // SAFETY: required by this function's safety conditions.
+            executor_global_addrs: ExecutorGlobalAddrs::new(),
         }
     }
+
+    #[track_caller]
+    pub fn system_settings(&self) -> &SystemSettings {
+        // SAFETY: it should always be valid, either set to the
+        // INITIAL_SYSTEM_SETTINGS or to the SYSTEM_SETTINGS.
+        unsafe { self.system_settings.as_ref() }
+    }
 }
 
 thread_local! {
@@ -378,7 +400,8 @@ thread_local! {
         wall_time: Instant::now(),
     });
 
-    static REQUEST_LOCALS: RefCell<RequestLocals> = RefCell::new(RequestLocals::default());
+    // These are only meant to be used on a PHP thread.
+    static REQUEST_LOCALS: RefCell<RequestLocals> = RefCell::new(unsafe { RequestLocals::new() });
 
     /// The tags for this thread/request. These get sent to other threads,
     /// which is why they are Arc. However, they are wrapped in a RefCell
@@ -426,9 +449,9 @@ extern "C" fn rinit(_type: c_int, _module_number: c_int) -> ZendResult {
     // initialize the thread local storage and cache some items
     REQUEST_LOCALS.with(|cell| {
         let mut locals = cell.borrow_mut();
-        // Safety: we are in rinit on a PHP thread.
-        locals.vm_interrupt_addr = unsafe { zend::datadog_php_profiling_vm_interrupt_addr() };
         locals.interrupt_count.store(0, Ordering::SeqCst);
+        // Safety: we are in rinit on a PHP thread.
+        locals.executor_global_addrs = unsafe { ddog_php_prof_executor_global_addrs() };
 
         // Safety: We are after first rinit and before mshutdown.
         unsafe {
@@ -555,11 +578,17 @@ extern "C" fn rinit(_type: c_int, _module_number: c_int) -> ZendResult {
             }
 
             if let Some(profiler) = PROFILER.lock().unwrap().as_ref() {
-                let interrupt = VmInterrupt {
-                    interrupt_count_ptr: &locals.interrupt_count as *const AtomicU32,
-                    engine_ptr: locals.vm_interrupt_addr,
+                let id = profiling::interrupts::Id {
+                    vm_interrupt_addr: locals.executor_global_addrs.vm_interrupt,
+                    current_execute_data_addr: locals.executor_global_addrs.current_execute_data,
+                };
+                let state = profiling::interrupts::State {
+                    interrupt_count_addr: ptr::NonNull::from(&locals.interrupt_count),
+                    current_execute_data_addr: ptr::NonNull::from(
+                        &locals.current_execute_data_cache,
+                    ),
                 };
-                profiler.add_interrupt(interrupt);
+                profiler.add_interrupt(id, state);
             }
         });
     } else {
@@ -611,11 +640,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.lock().unwrap().as_ref() {
-                let interrupt = VmInterrupt {
-                    interrupt_count_ptr: &locals.interrupt_count,
-                    engine_ptr: locals.vm_interrupt_addr,
+                let id = profiling::interrupts::Id {
+                    current_execute_data_addr: locals.executor_global_addrs.current_execute_data,
+                    vm_interrupt_addr: locals.executor_global_addrs.vm_interrupt,
                 };
-                profiler.remove_interrupt(interrupt);
+                profiler.remove_interrupt(id);
             }
         }
     });

profiling/src/php_ffi.c

@@ -239,7 +239,22 @@ void datadog_php_profiling_startup(zend_extension *extension) {
 #endif
 }
 
-void *datadog_php_profiling_vm_interrupt_addr(void) { return &EG(vm_interrupt); }
+// Keep in-sync with Rust ExecutorGlobalAddrs.
+struct executor_global_addrs {
+    zval **vm_stack_top;
+    zend_execute_data **current_execute_data;
+    void *vm_interrupt;
+};
+
+// Keep in-sync with Rust version.
+struct executor_global_addrs ddog_php_prof_executor_global_addrs(void) {
+    struct executor_global_addrs addrs = {
+        &EG(vm_stack_top),
+        &EG(current_execute_data),
+        (void*)&EG(vm_interrupt),
+    };
+    return addrs;
+}
 
 zend_module_entry *datadog_get_module_entry(const char *str, uintptr_t len) {
     return zend_hash_str_find_ptr(&module_registry, str, len);

profiling/src/php_ffi.h

@@ -57,11 +57,6 @@ sapi_request_info datadog_sapi_globals_request_info();
  */
 zend_module_entry *datadog_get_module_entry(const char *str, uintptr_t len);
 
-/**
- * Fetches the VM interrupt address of the calling PHP thread.
- */
-void *datadog_php_profiling_vm_interrupt_addr(void);
-
 /**
  * For Code Hotspots, we need the tracer's local root span id and the current
  * span id. This is a cross-product struct, so keep it in sync with tracer's