refactor(profiling): extract SystemProfiler

morrisonlevi · morrisonlevi · commit c72a00eea92e · 2024-08-15T10:12:10.000-06:00
The global profiler (e.g. PROFILER) and the Profiler::* functions that
worked on it were moved into a new struct SystemProfiler.
diff --git a/profiling/src/allocation.rs b/profiling/src/allocation.rs
@@ -2,7 +2,7 @@ use crate::bindings::{
     self as zend, datadog_php_install_handler, datadog_php_zif_handler,
     ddog_php_prof_copy_long_into_zval,
 };
-use crate::profiling::Profiler;
+use crate::profiling::SystemProfiler;
 use crate::{PROFILER_NAME, REQUEST_LOCALS};
 use lazy_static::lazy_static;
 use libc::{c_char, c_int, c_void, size_t};
@@ -95,7 +95,7 @@ impl AllocationProfilingStats {
 
         self.next_sampling_interval();
 
-        if let Some(profiler) = Profiler::get() {
+        if let Some(profiler) = SystemProfiler::get() {
             // Safety: execute_data was provided by the engine, and the profiler doesn't mutate it.
             unsafe {
                 profiler.collect_allocations(
diff --git a/profiling/src/exception.rs b/profiling/src/exception.rs
@@ -1,4 +1,4 @@
-use crate::profiling::Profiler;
+use crate::profiling::SystemProfiler;
 use crate::zend::{self, zend_execute_data, zend_generator, zval, InternalFunctionHandler};
 use crate::REQUEST_LOCALS;
 use log::{error, info};
@@ -83,7 +83,7 @@ impl ExceptionProfilingStats {
 
         self.next_sampling_interval();
 
-        if let Some(profiler) = Profiler::get() {
+        if let Some(profiler) = SystemProfiler::get() {
             // Safety: execute_data was provided by the engine, and the profiler doesn't mutate it.
             unsafe {
                 profiler.collect_exception(
diff --git a/profiling/src/lib.rs b/profiling/src/lib.rs
@@ -33,7 +33,7 @@ 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, SystemProfiler, VmInterrupt};
 use sapi::Sapi;
 use std::borrow::Cow;
 use std::cell::RefCell;
@@ -504,7 +504,7 @@ extern "C" fn rinit(_type: c_int, _module_number: c_int) -> ZendResult {
         return ZendResult::Success;
     }
 
-    Profiler::init(system_settings);
+    SystemProfiler::init(system_settings);
 
     if system_settings.profiling_enabled {
         REQUEST_LOCALS.with(|cell| {
@@ -541,7 +541,7 @@ extern "C" fn rinit(_type: c_int, _module_number: c_int) -> ZendResult {
                 return;
             }
 
-            if let Some(profiler) = Profiler::get() {
+            if let Some(profiler) = SystemProfiler::get() {
                 let interrupt = VmInterrupt {
                     interrupt_count_ptr: &locals.interrupt_count as *const AtomicU32,
                     engine_ptr: locals.vm_interrupt_addr,
@@ -597,7 +597,7 @@ extern "C" fn rshutdown(_type: c_int, _module_number: c_int) -> ZendResult {
         // wall-time is not expected to ever be disabled, except in testing,
         // and we don't need to optimize for that.
         if system_settings.profiling_enabled {
-            if let Some(profiler) = Profiler::get() {
+            if let Some(profiler) = SystemProfiler::get() {
                 let interrupt = VmInterrupt {
                     interrupt_count_ptr: &locals.interrupt_count,
                     engine_ptr: locals.vm_interrupt_addr,
@@ -807,7 +807,7 @@ extern "C" fn mshutdown(_type: c_int, _module_number: c_int) -> ZendResult {
     #[cfg(feature = "exception_profiling")]
     exception::exception_profiling_mshutdown();
 
-    Profiler::stop(Duration::from_secs(1));
+    SystemProfiler::stop(Duration::from_secs(1));
 
     ZendResult::Success
 }
@@ -841,7 +841,7 @@ extern "C" fn shutdown(_extension: *mut ZendExtension) {
     #[cfg(debug_assertions)]
     trace!("shutdown({:p})", _extension);
 
-    Profiler::shutdown(Duration::from_secs(2));
+    SystemProfiler::shutdown(Duration::from_secs(2));
 
     // SAFETY: calling in shutdown before zai config is shutdown, and after
     // all configuration is done being accessed. Well... in the happy-path,
@@ -870,7 +870,7 @@ fn notify_trace_finished(local_root_span_id: u64, span_type: Cow<str>, resource:
                 return;
             }
 
-            if let Some(profiler) = Profiler::get() {
+            if let Some(profiler) = SystemProfiler::get() {
                 let message = LocalRootSpanResourceMessage {
                     local_root_span_id,
                     resource: resource.into_owned(),
diff --git a/profiling/src/pcntl.rs b/profiling/src/pcntl.rs
@@ -3,7 +3,7 @@ use crate::bindings::{
     datadog_php_install_handler, datadog_php_zif_handler, zend_execute_data, zend_long, zval,
     InternalFunctionHandler,
 };
-use crate::{config, Profiler};
+use crate::{config, SystemProfiler};
 use ddcommon::cstr;
 use log::{error, warn};
 use std::ffi::CStr;
@@ -61,21 +61,21 @@ unsafe fn handle_fork(
     // Hold mutexes across the handler. If there are any spurious wakeups by
     // the threads while the fork is occurring, they cannot acquire locks
     // since this thread holds them, preventing a deadlock situation.
-    if let Some(profiler) = Profiler::get() {
+    if let Some(profiler) = SystemProfiler::get() {
         let _ = profiler.fork_prepare();
     }
 
     match dispatch(handler, execute_data, return_value) {
         Ok(ForkId::Parent) => {
-            if let Some(profiler) = Profiler::get() {
+            if let Some(profiler) = SystemProfiler::get() {
                 profiler.post_fork_parent();
             }
             return;
         }
         Ok(ForkId::Child) => { /* fallthrough */ }
         Err(ForkError::NullRetval) => {
             // Skip error message if no profiler.
-            if Profiler::get().is_some() {
+            if SystemProfiler::get().is_some() {
                 error!(
                     "Failed to read return value of forking function. A crash or deadlock may occur."
                 );
@@ -85,7 +85,7 @@ unsafe fn handle_fork(
 
         Err(ForkError::ZvalType(r#type)) => {
             // Skip error message if no profiler.
-            if Profiler::get().is_some() {
+            if SystemProfiler::get().is_some() {
                 warn!(
                     "Return type of forking function was unexpected: {type}. A crash or deadlock may occur."
                 );
@@ -99,7 +99,7 @@ unsafe fn handle_fork(
     //  2. Something went wrong, and disable it to be safe.
     // And then leak the old profiler. Its drop method is not safe to run in
     // these situations.
-    Profiler::kill();
+    SystemProfiler::kill();
 
     alloc_prof_rshutdown();
 
diff --git a/profiling/src/profiling/mod.rs b/profiling/src/profiling/mod.rs
@@ -1,12 +1,14 @@
 mod interrupts;
 mod sample_type_filter;
 pub mod stack_walking;
+mod system_profiler;
 mod thread_utils;
 mod uploader;
 
 pub use interrupts::*;
 pub use sample_type_filter::*;
 pub use stack_walking::*;
+pub use system_profiler::*;
 use uploader::*;
 
 #[cfg(all(php_has_fibers, not(test)))]
@@ -26,12 +28,10 @@ use datadog_profiling::api::{
 use datadog_profiling::exporter::Tag;
 use datadog_profiling::internal::Profile as InternalProfile;
 use log::{debug, info, trace, warn};
-use once_cell::sync::OnceCell;
 use std::borrow::Cow;
 use std::collections::HashMap;
 use std::hash::Hash;
 use std::intrinsics::transmute;
-use std::mem::forget;
 use std::num::NonZeroI64;
 use std::sync::atomic::{AtomicBool, AtomicPtr, AtomicU32, Ordering};
 use std::sync::{Arc, Barrier};
@@ -58,10 +58,6 @@ pub const NO_TIMESTAMP: i64 = 0;
 // magnitude for the capacity.
 const UPLOAD_CHANNEL_CAPACITY: usize = 8;
 
-/// The global profiler. Profiler gets made during the first rinit after an
-/// minit, and is destroyed on mshutdown.
-static mut PROFILER: OnceCell<Profiler> = OnceCell::new();
-
 /// Order this array this way:
 ///  1. Always enabled types.
 ///  2. On by default types.
@@ -520,21 +516,6 @@ const DDPROF_TIME: &str = "ddprof_time";
 const DDPROF_UPLOAD: &str = "ddprof_upload";
 
 impl Profiler {
-    /// Will initialize the `PROFILER` OnceCell and makes sure that only one thread will do so.
-    pub fn init(system_settings: &mut SystemSettings) {
-        // SAFETY: the `get_or_init` access is a thread-safe API, and the
-        // PROFILER is not being mutated outside single-threaded phases such
-        // as minit/mshutdown.
-        unsafe { PROFILER.get_or_init(|| Profiler::new(system_settings)) };
-    }
-
-    pub fn get() -> Option<&'static Profiler> {
-        // SAFETY: the `get` access is a thread-safe API, and the PROFILER is
-        // not being mutated outside single-threaded phases such as minit and
-        // mshutdown.
-        unsafe { PROFILER.get() }
-    }
-
     pub fn new(system_settings: &mut SystemSettings) -> Self {
         let fork_barrier = Arc::new(Barrier::new(3));
         let interrupt_manager = Arc::new(InterruptManager::new());
@@ -644,19 +625,6 @@ impl Profiler {
             .map_err(Box::new)
     }
 
-    /// Begins the shutdown process. To complete it, call [Profiler::shutdown].
-    /// Note that you must call [Profiler::shutdown] afterwards; it's two
-    /// parts of the same operation. It's split so you (or other extensions)
-    /// can do something while the other threads finish up.
-    pub fn stop(timeout: Duration) {
-        // SAFETY: the `get_mut` access is a thread-safe API, and the PROFILER
-        // is not being mutated outside single-threaded phases such as minit
-        // and mshutdown.
-        if let Some(profiler) = unsafe { PROFILER.get_mut() } {
-            profiler.join_and_drop_sender(timeout);
-        }
-    }
-
     pub fn join_and_drop_sender(&mut self, timeout: Duration) {
         debug!("Stopping profiler.");
 
@@ -685,20 +653,6 @@ impl Profiler {
         std::mem::swap(&mut self.upload_sender, &mut empty_sender);
     }
 
-    /// Completes the shutdown process; to start it, call [Profiler::stop]
-    /// before calling [Profiler::shutdown].
-    /// Note the timeout is per thread, and there may be multiple threads.
-    ///
-    /// Safety: only safe to be called in `SHUTDOWN`/`MSHUTDOWN` phase
-    pub fn shutdown(timeout: Duration) {
-        // SAFETY: the `take` access is a thread-safe API, and the PROFILER is
-        // not being mutated outside single-threaded phases such  as minit and
-        // mshutdown.
-        if let Some(profiler) = unsafe { PROFILER.take() } {
-            profiler.join_collector_and_uploader(timeout);
-        }
-    }
-
     pub fn join_collector_and_uploader(self, timeout: Duration) {
         if self.should_join.load(Ordering::SeqCst) {
             thread_utils::join_timeout(
@@ -718,34 +672,6 @@ impl Profiler {
         }
     }
 
-    /// Throws away the profiler and moves it to uninitialized.
-    ///
-    /// In a forking situation, the currently active profiler may not be valid
-    /// because it has join handles and other state shared by other threads,
-    /// and threads are not copied when the process is forked.
-    /// Additionally, if we've hit certain other issues like not being able to
-    /// determine the return type of the pcntl_fork function, we don't know if
-    /// we're the parent or child.
-    /// So, we throw away the current profiler and forget it, which avoids
-    /// running the destructor. Yes, this will leak some memory.
-    ///
-    /// # Safety
-    /// Must be called when no other thread is using the PROFILER object. That
-    /// includes this thread in some kind of recursive manner.
-    pub unsafe fn kill() {
-        // SAFETY: see this function's safety conditions.
-        if let Some(mut profiler) = PROFILER.take() {
-            // Drop some things to reduce memory.
-            profiler.interrupt_manager = Arc::new(InterruptManager::new());
-            profiler.message_sender = crossbeam_channel::bounded(0).0;
-            profiler.upload_sender = crossbeam_channel::bounded(0).0;
-
-            // But we're not 100% sure everything is safe to drop, notably the
-            // join handles, so we leak the rest.
-            forget(profiler)
-        }
-    }
-
     /// Collect a stack sample with elapsed wall time. Collects CPU time if
     /// it's enabled and available.
     pub fn collect_time(&self, execute_data: *mut zend_execute_data, interrupt_count: u32) {
diff --git a/profiling/src/profiling/system_profiler.rs b/profiling/src/profiling/system_profiler.rs
@@ -0,0 +1,87 @@
+use super::{InterruptManager, Profiler, SystemSettings};
+use once_cell::sync::OnceCell;
+use std::mem::forget;
+use std::sync::Arc;
+use std::time::Duration;
+
+/// The global profiler. Profiler gets made during the first rinit after an
+/// minit, and is destroyed on mshutdown.
+static mut PROFILER: SystemProfiler = SystemProfiler::new();
+
+pub struct SystemProfiler(OnceCell<Profiler>);
+
+impl SystemProfiler {
+    pub const fn new() -> Self {
+        SystemProfiler(OnceCell::new())
+    }
+
+    /// Initializes the system profiler safely by one thread.
+    pub fn init(system_settings: &mut SystemSettings) {
+        // SAFETY: the `get_or_init` access is a thread-safe API, and the
+        // PROFILER is not being mutated outside single-threaded phases such
+        // as minit and mshutdown.
+        unsafe { PROFILER.0.get_or_init(|| Profiler::new(system_settings)) };
+    }
+
+    pub fn get() -> Option<&'static Profiler> {
+        // SAFETY: the `get` access is a thread-safe API, and the PROFILER is
+        // not being mutated outside single-threaded phases such as minit and
+        // mshutdown.
+        unsafe { PROFILER.0.get() }
+    }
+
+    /// Begins the shutdown process. To complete it, call [Profiler::shutdown].
+    /// Note that you must call [Profiler::shutdown] afterward; it's two
+    /// parts of the same operation. It's split so you (or other extensions)
+    /// can do something while the other threads finish up.
+    pub fn stop(timeout: Duration) {
+        // SAFETY: the `get_mut` access is a thread-safe API, and the PROFILER
+        // is not being mutated outside single-threaded phases such as minit
+        // and mshutdown.
+        if let Some(profiler) = unsafe { PROFILER.0.get_mut() } {
+            profiler.join_and_drop_sender(timeout);
+        }
+    }
+
+    /// Completes the shutdown process; to start it, call [Profiler::stop]
+    /// before calling [Profiler::shutdown].
+    /// Note the timeout is per thread, and there may be multiple threads.
+    ///
+    /// Safety: only safe to be called in `SHUTDOWN`/`MSHUTDOWN` phase
+    pub fn shutdown(timeout: Duration) {
+        // SAFETY: the `take` access is a thread-safe API, and the PROFILER is
+        // not being mutated outside single-threaded phases such as minit and
+        // mshutdown.
+        if let Some(profiler) = unsafe { PROFILER.0.take() } {
+            profiler.join_collector_and_uploader(timeout);
+        }
+    }
+
+    /// Throws away the profiler and moves it to uninitialized.
+    ///
+    /// In a forking situation, the currently active profiler may not be valid
+    /// because it has join handles and other state shared by other threads,
+    /// and threads are not copied when the process is forked.
+    /// Additionally, if we've hit certain other issues like not being able to
+    /// determine the return type of the pcntl_fork function, we don't know if
+    /// we're the parent or child.
+    /// So, we throw away the current profiler and forget it, which avoids
+    /// running the destructor. Yes, this will leak some memory.
+    ///
+    /// # Safety
+    /// Must be called when no other thread is using the PROFILER object. That
+    /// includes this thread in some kind of recursive manner.
+    pub unsafe fn kill() {
+        // SAFETY: see this function's safety conditions.
+        if let Some(mut profiler) = PROFILER.0.take() {
+            // Drop some things to reduce memory.
+            profiler.interrupt_manager = Arc::new(InterruptManager::new());
+            profiler.message_sender = crossbeam_channel::bounded(0).0;
+            profiler.upload_sender = crossbeam_channel::bounded(0).0;
+
+            // But we're not 100% sure everything is safe to drop, notably the
+            // join handles, so we leak the rest.
+            forget(profiler)
+        }
+    }
+}
diff --git a/profiling/src/timeline.rs b/profiling/src/timeline.rs
diff --git a/profiling/src/wall_time.rs b/profiling/src/wall_time.rs
