diff --git a/.github/workflows/prof_correctness.yml b/.github/workflows/prof_correctness.yml index fd8e449d6d..a76a59d4fc 100644 --- a/.github/workflows/prof_correctness.yml +++ b/.github/workflows/prof_correctness.yml @@ -80,7 +80,7 @@ jobs: export DD_PROFILING_EXCEPTION_MESSAGE_ENABLED=1 php -v php -d extension=target/profiler-release/libdatadog_php_profiling.so --ri datadog-profiling - for test_case in "allocations" "time" "strange_frames" "timeline" "exceptions" "io" "allocation_time_combined" "generators"; do + for test_case in "allocations" "time" "strange_frames" "timeline" "exceptions" "io" "allocation_time_combined" "generators" "gc_survivors"; do mkdir -p profiling/tests/correctness/"$test_case"/ export DD_PROFILING_OUTPUT_PPROF=$PWD/profiling/tests/correctness/"$test_case"/test.pprof php -d extension="${PWD}/target/profiler-release/libdatadog_php_profiling.so" "profiling/tests/correctness/${test_case}.php" @@ -98,7 +98,7 @@ jobs: export DD_PROFILING_EXCEPTION_MESSAGE_ENABLED=1 php -v php -d extension=target/profiler-release/libdatadog_php_profiling.so --ri datadog-profiling - for test_case in "allocations" "time" "strange_frames" "timeline" "exceptions" "io" "allocation_time_combined" "generators"; do + for test_case in "allocations" "time" "strange_frames" "timeline" "exceptions" "io" "allocation_time_combined" "generators" "gc_survivors"; do mkdir -p profiling/tests/correctness/"$test_case"/ export DD_PROFILING_OUTPUT_PPROF=$PWD/profiling/tests/correctness/"$test_case"/test.pprof php -d extension=$PWD/target/profiler-release/libdatadog_php_profiling.so profiling/tests/correctness/"$test_case".php @@ -164,6 +164,12 @@ jobs: expected_json: profiling/tests/correctness/timeline.json pprof_path: profiling/tests/correctness/timeline/ + - name: Check profiler correctness for GC survivors + uses: Datadog/prof-correctness/analyze@main + with: + expected_json: profiling/tests/correctness/gc_survivors.json + pprof_path: profiling/tests/correctness/gc_survivors/ + - name: Check profiler correctness for allocation_time_combined uses: Datadog/prof-correctness/analyze@main with: diff --git a/profiling/src/php_ffi.c b/profiling/src/php_ffi.c index 00f684e953..9fad658ed3 100644 --- a/profiling/src/php_ffi.c +++ b/profiling/src/php_ffi.c @@ -244,6 +244,10 @@ zend_execute_data* ddog_php_prof_get_current_execute_data() { return EG(current_execute_data); } +zend_objects_store* ddog_php_prof_get_objects_store() { + return &EG(objects_store); +} + #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 f9aef1a21e..9e24e1e66e 100644 --- a/profiling/src/php_ffi.h +++ b/profiling/src/php_ffi.h @@ -153,6 +153,12 @@ void ddog_php_prof_zend_mm_set_custom_handlers(zend_mm_heap *heap, zend_execute_data* ddog_php_prof_get_current_execute_data(); +/** + * Returns the address of `EG(objects_store)`. Used by the GC survivors + * pass to walk the live-object bucket array entirely from Rust. + */ +zend_objects_store* ddog_php_prof_get_objects_store(); + #if CFG_FIBERS zend_fiber* ddog_php_prof_get_active_fiber(); zend_fiber* ddog_php_prof_get_active_fiber_test(); diff --git a/profiling/src/profiling/mod.rs b/profiling/src/profiling/mod.rs index d0cd47588f..b0fa8a899c 100644 --- a/profiling/src/profiling/mod.rs +++ b/profiling/src/profiling/mod.rs @@ -1367,8 +1367,9 @@ impl Profiler { reason: &'static str, collected: i64, #[cfg(php_gc_status)] runs: i64, + survivors: Option, ) { - let mut labels = Profiler::common_labels(4); + let mut labels = Profiler::common_labels(5); labels.push(Label { key: "event", @@ -1389,6 +1390,12 @@ impl Profiler { key: "gc collected", value: LabelValue::Num(collected, "count"), }); + if let Some(survivors) = survivors { + labels.push(Label { + key: "survivors", + value: LabelValue::Str(Cow::Owned(survivors)), + }); + } let n_labels = labels.len(); match self.prepare_and_send_message( diff --git a/profiling/src/timeline.rs b/profiling/src/timeline.rs index 3b1719c3dd..45d79ee15a 100644 --- a/profiling/src/timeline.rs +++ b/profiling/src/timeline.rs @@ -732,6 +732,8 @@ unsafe extern "C" fn ddog_php_prof_gc_collect_cycles() -> i32 { #[cfg(php_gc_status)] let status = status.assume_init(); + let survivors = gc_survivors::collect_top_n(); + trace!( "Garbage collection with reason \"{reason}\" took {} nanoseconds", duration.as_nanos() @@ -747,6 +749,7 @@ unsafe extern "C" fn ddog_php_prof_gc_collect_cycles() -> i32 { reason, collected as i64, status.runs as i64, + survivors, ); } else { profiler.collect_garbage_collection( @@ -755,6 +758,7 @@ unsafe extern "C" fn ddog_php_prof_gc_collect_cycles() -> i32 { duration.as_nanos() as i64, reason, collected as i64, + survivors, ); } } @@ -767,3 +771,152 @@ unsafe extern "C" fn ddog_php_prof_gc_collect_cycles() -> i32 { 0 } } + +/// Walks `EG(objects_store)` to produce a "top 10 live classes by instance +/// count" string, used as the `survivors` label on the GC timeline sample. +/// +/// Watching that label across consecutive GC events in the timeline view +/// surfaces object leaks: a class whose count climbs run-over-run is the +/// fingerprint. +mod gc_survivors { + use crate::zend; + use std::collections::HashMap; + + /// Don't bother emitting the label when the store is small enough that + /// the signal would be dominated by engine bookkeeping. Threshold is + /// arbitrary; the goal is just to filter out tiny CLI scripts. + const MIN_OBJECTS_THRESHOLD: u32 = 32; + + /// Number of classes to include in the formatted string. + const TOP_N: usize = 10; + + extern "C" { + fn ddog_php_prof_get_objects_store() -> *mut zend::zend_objects_store; + } + + /// Mirrors PHP's `IS_OBJ_VALID(o)`: a free-list bucket has bit 0 set + /// (the handle is shifted left and ORed with `OBJ_BUCKET_INVALID = 1`). + /// A null pointer also has bit 0 clear, so callers must null-check + /// separately. + #[inline] + fn is_obj_valid(obj: *const zend::zend_object) -> bool { + (obj as usize) & 1 == 0 + } + + pub(super) fn collect_top_n() -> Option { + // SAFETY: ddog_php_prof_get_objects_store returns a stable pointer + // to the executor's objects_store struct; the engine guarantees + // it's valid for the duration of a request. + let store = unsafe { ddog_php_prof_get_objects_store().as_ref()? }; + if store.top < MIN_OBJECTS_THRESHOLD { + return None; + } + + let mut counts: HashMap<*const zend::zend_class_entry, u64> = HashMap::new(); + for i in 0..store.top { + // SAFETY: 0..top is the engine's "in use" range of buckets. + let bucket = unsafe { *store.object_buckets.add(i as usize) }; + if bucket.is_null() || !is_obj_valid(bucket) { + continue; + } + // SAFETY: a valid bucket holds a live zend_object*; its `ce` + // is a valid zend_class_entry pointer by engine invariant. + let ce = unsafe { (*bucket).ce }; + if ce.is_null() { + continue; + } + *counts.entry(ce as *const _).or_insert(0) += 1; + } + if counts.is_empty() { + return None; + } + + let resolved: Vec<(String, u64)> = counts + .into_iter() + .filter_map(|(ce, count)| { + // SAFETY: ce came from a live object's `ce` field above. + unsafe { class_name(ce) }.map(|name| (name, count)) + }) + .collect(); + if resolved.is_empty() { + return None; + } + Some(format_top_n(resolved, TOP_N)) + } + + unsafe fn class_name(ce: *const zend::zend_class_entry) -> Option { + let name_ptr: *mut zend::zend_string = (*ce).name; + let bytes = zend::zai_str_from_zstr(name_ptr.as_mut()).into_bytes(); + if bytes.is_empty() { + None + } else { + Some(String::from_utf8_lossy(bytes).into_owned()) + } + } + + fn format_top_n(mut entries: Vec<(String, u64)>, top_n: usize) -> String { + // Sort by (count desc, name asc) for deterministic, diff-friendly + // output across consecutive GC events. + entries.sort_unstable_by(|a, b| b.1.cmp(&a.1).then_with(|| a.0.cmp(&b.0))); + entries.truncate(top_n); + + let mut out = String::new(); + for (name, count) in entries { + if !out.is_empty() { + out.push_str(", "); + } + out.push('\\'); + out.push_str(&name); + out.push(' '); + out.push_str(&count.to_string()); + } + out + } + + #[cfg(test)] + mod tests { + use super::*; + + #[test] + fn format_orders_by_count_desc() { + let input = vec![ + ("Foo".to_string(), 3), + ("Bar".to_string(), 100), + ("Baz".to_string(), 50), + ]; + assert_eq!(format_top_n(input, 10), "\\Bar 100, \\Baz 50, \\Foo 3"); + } + + #[test] + fn format_tie_break_by_name_asc() { + let input = vec![ + ("Zeta".to_string(), 5), + ("Alpha".to_string(), 5), + ("Mu".to_string(), 5), + ]; + assert_eq!(format_top_n(input, 10), "\\Alpha 5, \\Mu 5, \\Zeta 5"); + } + + #[test] + fn format_truncates_to_top_n() { + let input: Vec<(String, u64)> = (0..15) + .map(|i| (format!("Class{i:02}"), (15 - i) as u64)) + .collect(); + let s = format_top_n(input, 10); + assert!(s.starts_with("\\Class00 15"), "got: {s}"); + assert!(s.contains("\\Class09 6"), "got: {s}"); + assert!(!s.contains("Class10"), "got: {s}"); + } + + #[test] + fn format_empty_returns_empty_string() { + assert_eq!(format_top_n(vec![], 10), ""); + } + + #[test] + fn format_single_entry() { + let input = vec![("DateTime".to_string(), 24)]; + assert_eq!(format_top_n(input, 10), "\\DateTime 24"); + } + } +} diff --git a/profiling/tests/correctness/gc_survivors.json b/profiling/tests/correctness/gc_survivors.json new file mode 100644 index 0000000000..85284fe689 --- /dev/null +++ b/profiling/tests/correctness/gc_survivors.json @@ -0,0 +1,46 @@ +{ + "scale_by_duration": true, + "test_name": "php_gc_survivors", + "stacks": [ + { + "profile-type": "timeline", + "stack-content": [ + { + "regular_expression": "^\\[gc\\]$", + "percent": 100, + "error_margin": 100, + "labels": [ + { + "key": "event", + "values": [ + "gc" + ] + }, + { + "key": "gc reason", + "values": [ + "induced" + ] + }, + { + "key": "gc collected", + "values_regex": "^[0-9]+$" + }, + { + "key": "gc runs", + "values_regex": "^[0-9]+$" + }, + { + "key": "survivors", + "values_regex": "^.*\\\\Bench\\\\AlphaThing 200.*\\\\Bench\\\\BetaThing 100.*$" + }, + { + "key": "end_timestamp_ns", + "values_regex": "^[0-9]+$" + } + ] + } + ] + } + ] +} diff --git a/profiling/tests/correctness/gc_survivors.php b/profiling/tests/correctness/gc_survivors.php new file mode 100644 index 0000000000..28f056c025 --- /dev/null +++ b/profiling/tests/correctness/gc_survivors.php @@ -0,0 +1,32 @@ +