From 8d05a71fd2910262caf4cf2a39a8e0ab4144a8e7 Mon Sep 17 00:00:00 2001 From: Tony Xiao Date: Thu, 4 Jun 2026 17:20:33 -0400 Subject: [PATCH 1/4] feat(occurrence): Add per-thread frame detection selection Replace the boolean active_thread_only on the frame detection options with a DetectionThread enum (ActiveThread, MainThread, AllThreads) so detectors can express thread selection explicitly. Switch the cocoa detectors to run on the main thread, resolving it via a new ProfileInterface::get_main_thread_id and falling back to the active thread when no main thread is found. Co-Authored-By: Claude --- src/android/profile.rs | 4 ++ src/occurrence/detect_frame.rs | 74 +++++++++++++++++++++------------- src/sample/v1.rs | 19 +++++++++ src/types.rs | 2 + 4 files changed, 72 insertions(+), 27 deletions(-) diff --git a/src/android/profile.rs b/src/android/profile.rs index 4664b06..f4a40ca 100644 --- a/src/android/profile.rs +++ b/src/android/profile.rs @@ -257,6 +257,10 @@ impl ProfileInterface for AndroidProfile { }) } + fn get_main_thread_id(&self) -> Option { + None + } + fn get_transaction_tags(&self) -> &HashMap { &self.transaction_tags } diff --git a/src/occurrence/detect_frame.rs b/src/occurrence/detect_frame.rs index 34249c3..ae7800c 100644 --- a/src/occurrence/detect_frame.rs +++ b/src/occurrence/detect_frame.rs @@ -35,10 +35,22 @@ pub(crate) const VIEW_RENDER: &str = "view_render"; pub(crate) const VIEW_UPDATE: &str = "view_update"; pub(crate) const XPC: &str = "xpc"; +/// Selects which threads frame detection runs over. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] +pub enum DetectionThread { + /// Only consider the profile's active thread. + ActiveThread, + /// Only consider the profile's main thread. + MainThread, + /// Consider all threads. + #[default] + AllThreads, +} + /// Trait for frame detection options with configurable behavior. pub trait DetectFrameOptions { - /// Returns whether to only check the active thread. - fn only_check_active_thread(&self) -> bool; + /// Returns which threads frame detection should run over. + fn detection_thread(&self) -> DetectionThread; /// Checks a node and returns information about it if it matches detection criteria. /// Returns None if the node doesn't match the criteria. @@ -48,8 +60,8 @@ pub trait DetectFrameOptions { /// Options for detecting exact frames in profiling data. #[derive(Debug, Clone, Default)] pub struct DetectExactFrameOptions { - /// Whether to only consider the active thread - pub active_thread_only: bool, + /// Which threads to consider for detection + pub detection_thread: DetectionThread, /// Minimum duration threshold for frame detection pub duration_threshold: Duration, @@ -65,8 +77,8 @@ pub struct DetectExactFrameOptions { /// Options for detecting Android frames in profiling data. #[derive(Debug, Clone)] pub struct DetectAndroidFrameOptions { - /// Whether to only consider the active thread - pub active_thread_only: bool, + /// Which threads to consider for detection + pub detection_thread: DetectionThread, /// Minimum duration threshold for frame detection pub duration_threshold: Duration, @@ -103,8 +115,8 @@ pub struct NodeInfo { } impl DetectFrameOptions for DetectExactFrameOptions { - fn only_check_active_thread(&self) -> bool { - self.active_thread_only + fn detection_thread(&self) -> DetectionThread { + self.detection_thread } fn check_node(&self, node: &Node) -> Option { @@ -138,8 +150,8 @@ impl DetectFrameOptions for DetectExactFrameOptions { } impl DetectFrameOptions for DetectAndroidFrameOptions { - fn only_check_active_thread(&self) -> bool { - self.active_thread_only + fn detection_thread(&self) -> DetectionThread { + self.detection_thread } fn check_node(&self, node: &Node) -> Option { @@ -189,7 +201,7 @@ pub static DETECT_FRAME_JOBS: Lazy< // Node.js platform ("node".to_string(), vec![ Box::new(DetectExactFrameOptions { - active_thread_only: true, + detection_thread: DetectionThread::ActiveThread, duration_threshold: Duration::from_millis(0), sample_threshold: 1, functions_by_package: HashMap::from([ @@ -240,7 +252,7 @@ pub static DETECT_FRAME_JOBS: Lazy< ]), }) as Box, Box::new(DetectExactFrameOptions { - active_thread_only: false, + detection_thread: DetectionThread::AllThreads, duration_threshold: Duration::from_millis(100), sample_threshold: 1, functions_by_package: HashMap::from([ @@ -254,7 +266,7 @@ pub static DETECT_FRAME_JOBS: Lazy< // Cocoa platform ("cocoa".to_string(), vec![ Box::new(DetectExactFrameOptions { - active_thread_only: true, + detection_thread: DetectionThread::MainThread, duration_threshold: Duration::from_millis(16), sample_threshold: 4, functions_by_package: HashMap::from([ @@ -398,7 +410,7 @@ pub static DETECT_FRAME_JOBS: Lazy< // Android platform ("android".to_string(), vec![ Box::new(DetectAndroidFrameOptions { - active_thread_only: true, + detection_thread: DetectionThread::ActiveThread, duration_threshold: Duration::from_millis(40), sample_threshold: 1, functions_by_package: HashMap::from([ @@ -564,19 +576,27 @@ pub fn detect_frame( // List nodes matching criteria let mut nodes: HashMap = HashMap::new(); - if options.only_check_active_thread() { - let active_thread_id = profile.get_transaction().active_thread_id; - if let Some(call_trees) = call_trees_per_thread_id.get(&active_thread_id) { - for root in call_trees { - detect_frame_in_call_tree(root, options, &mut nodes); + match options.detection_thread() { + DetectionThread::ActiveThread | DetectionThread::MainThread => { + // For MainThread, fall back to the active thread if we can't find a main thread. + let thread_id = match options.detection_thread() { + DetectionThread::MainThread => profile.get_main_thread_id(), + _ => None, + } + .unwrap_or_else(|| profile.get_transaction().active_thread_id); + if let Some(call_trees) = call_trees_per_thread_id.get(&thread_id) { + for root in call_trees { + detect_frame_in_call_tree(root, options, &mut nodes); + } + } else { + return; } - } else { - return; } - } else { - for call_trees in call_trees_per_thread_id.values() { - for root in call_trees { - detect_frame_in_call_tree(root, options, &mut nodes); + DetectionThread::AllThreads => { + for call_trees in call_trees_per_thread_id.values() { + for root in call_trees { + detect_frame_in_call_tree(root, options, &mut nodes); + } } } } @@ -596,7 +616,7 @@ mod tests { nodetree::Node, occurrence::detect_frame::{ detect_frame_in_call_tree, DetectAndroidFrameOptions, DetectExactFrameOptions, - DetectFrameOptions, NodeInfo, NodeKey, FILE_READ, IMAGE_DECODE, + DetectFrameOptions, DetectionThread, NodeInfo, NodeKey, FILE_READ, IMAGE_DECODE, }, }; @@ -1447,7 +1467,7 @@ mod tests { TestStruct { name: "Detect android frame".to_string(), job: Box::new(DetectAndroidFrameOptions { - active_thread_only: false, + detection_thread: DetectionThread::AllThreads, duration_threshold: Duration::from_millis(16), sample_threshold: 1, functions_by_package: HashMap::from([ diff --git a/src/sample/v1.rs b/src/sample/v1.rs index 415fbae..6cfd08a 100644 --- a/src/sample/v1.rs +++ b/src/sample/v1.rs @@ -537,6 +537,25 @@ impl ProfileInterface for SampleProfile { Cow::Borrowed(&self.transaction) } + fn get_main_thread_id(&self) -> Option { + match self.platform.as_str() { + "cocoa" => self + .profile + .thread_metadata + .as_ref()? + .iter() + .find_map(|(id, meta)| { + let thread_id = id.parse::().ok()?; + if meta.name.as_deref() == Some("main") { + Some(thread_id) + } else { + None + } + }), + _ => None, + } + } + fn get_transaction_tags(&self) -> &HashMap { &self.transaction_tags } diff --git a/src/types.rs b/src/types.rs index 08e03df..3cbdd25 100644 --- a/src/types.rs +++ b/src/types.rs @@ -227,6 +227,8 @@ pub trait ProfileInterface { fn sdk_version(&self) -> Option<&str>; fn duration_ns(&self) -> u64; fn get_transaction(&self) -> Cow<'_, Transaction>; + /// Returns the thread ID of the main thread, if it can be determined. + fn get_main_thread_id(&self) -> Option; fn get_transaction_tags(&self) -> &HashMap; fn get_debug_meta(&self) -> &DebugMeta; fn get_measurements(&self) -> Option<&HashMap>; From 375bce6a3e848aa81c7ca6b7879c3f3d6539350e Mon Sep 17 00:00:00 2001 From: Tony Xiao Date: Thu, 4 Jun 2026 17:41:18 -0400 Subject: [PATCH 2/4] fix(occurrence): build call trees for cocoa main thread; fix clippy Cocoa frame detection resolves the main thread id, but SampleProfile::call_trees only built trees for the active thread. When the main thread differs from the active thread, detect_frame found no trees and detected nothing. Build trees for the main thread in addition to the active thread. Also fix clippy::unnecessary_sort_by by switching to sort_by_key. Co-Authored-By: Claude Opus 4.8 --- src/profile.rs | 2 +- src/profile_chunk.rs | 2 +- src/sample/v1.rs | 76 ++++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 76 insertions(+), 4 deletions(-) diff --git a/src/profile.rs b/src/profile.rs index f6a0fce..83d4981 100644 --- a/src/profile.rs +++ b/src/profile.rs @@ -332,7 +332,7 @@ impl Profile { } // sort the list in descending order, and take the top N results - functions_list.sort_by(|a, b| b.sum_self_time_ns.cmp(&a.sum_self_time_ns)); + functions_list.sort_by_key(|f| std::cmp::Reverse(f.sum_self_time_ns)); functions_list.truncate(max_unique_functions.unwrap_or(functions_list.len())); Ok(functions_list) diff --git a/src/profile_chunk.rs b/src/profile_chunk.rs index 901b965..e38f9fd 100644 --- a/src/profile_chunk.rs +++ b/src/profile_chunk.rs @@ -314,7 +314,7 @@ impl ProfileChunk { } // sort the list in descending order, and take the top N results - functions_list.sort_by(|a, b| b.sum_self_time_ns.cmp(&a.sum_self_time_ns)); + functions_list.sort_by_key(|f| std::cmp::Reverse(f.sum_self_time_ns)); functions_list.truncate(max_unique_functions.unwrap_or(functions_list.len())); Ok(functions_list) diff --git a/src/sample/v1.rs b/src/sample/v1.rs index 6cfd08a..ea850cc 100644 --- a/src/sample/v1.rs +++ b/src/sample/v1.rs @@ -412,9 +412,12 @@ impl ProfileInterface for SampleProfile { // Sort samples by timestamp self.profile .samples - .sort_by(|a, b| a.elapsed_since_start_ns.cmp(&b.elapsed_since_start_ns)); + .sort_by_key(|s| s.elapsed_since_start_ns); let active_thread_id = self.transaction.active_thread_id; + // Cocoa frame detection targets the main thread, which may differ from the + // active thread, so build call trees for it as well when one is resolved. + let main_thread_id = self.get_main_thread_id(); let mut trees_by_thread_id: HashMap>>> = HashMap::new(); let mut samples_by_thread_id: HashMap> = HashMap::new(); @@ -428,7 +431,7 @@ impl ProfileInterface for SampleProfile { let mut hasher = Fnv64::default(); for (thread_id, samples) in samples_by_thread_id { - if thread_id != active_thread_id { + if thread_id != active_thread_id && Some(thread_id) != main_thread_id { continue; } @@ -2149,4 +2152,73 @@ mod tests { ); } } + + #[test] + fn test_call_trees_builds_main_thread() { + use crate::sample::ThreadMetadata; + use std::collections::HashMap; + + // Cocoa profile whose main thread (id 2) differs from the active thread (id 1). + let mut profile = SampleProfile { + platform: "cocoa".to_string(), + transaction: Transaction { + active_thread_id: 1, + ..Default::default() + }, + profile: Profile { + samples: vec![ + Sample { + stack_id: 0, + thread_id: 1, + elapsed_since_start_ns: 10, + ..Default::default() + }, + Sample { + stack_id: 0, + thread_id: 1, + elapsed_since_start_ns: 20, + ..Default::default() + }, + Sample { + stack_id: 0, + thread_id: 2, + elapsed_since_start_ns: 10, + ..Default::default() + }, + Sample { + stack_id: 0, + thread_id: 2, + elapsed_since_start_ns: 20, + ..Default::default() + }, + ], + stacks: vec![vec![0]], + frames: vec![Frame { + function: Some("function0".to_string()), + ..Default::default() + }], + thread_metadata: Some(HashMap::from([( + "2".to_string(), + ThreadMetadata { + name: Some("main".to_string()), + priority: None, + }, + )])), + ..Default::default() + }, + ..Default::default() + }; + + let call_trees = profile.call_trees().unwrap(); + // Trees must be built for both the active thread and the resolved main thread, + // otherwise MainThread frame detection finds no trees for the main thread. + assert!( + call_trees.contains_key(&1), + "missing call trees for the active thread" + ); + assert!( + call_trees.contains_key(&2), + "missing call trees for the main thread" + ); + } } From ac6ba88a9ad015296f600de7a07bf66b488ad594 Mon Sep 17 00:00:00 2001 From: Tony Xiao Date: Fri, 5 Jun 2026 09:51:01 -0400 Subject: [PATCH 3/4] fix(occurrence): Don't fall back to active thread for main thread detection DetectionThread::MainThread previously fell back to the active thread when no main thread could be resolved, causing detection to run on the wrong thread. Make MainThread strict: only run when a main thread is resolvable, otherwise produce no occurrences. Co-Authored-By: Claude --- src/occurrence/detect_frame.rs | 86 +++++++++++++++++++++++++++++----- 1 file changed, 73 insertions(+), 13 deletions(-) diff --git a/src/occurrence/detect_frame.rs b/src/occurrence/detect_frame.rs index ae7800c..d2ee3d5 100644 --- a/src/occurrence/detect_frame.rs +++ b/src/occurrence/detect_frame.rs @@ -576,21 +576,25 @@ pub fn detect_frame( // List nodes matching criteria let mut nodes: HashMap = HashMap::new(); - match options.detection_thread() { - DetectionThread::ActiveThread | DetectionThread::MainThread => { - // For MainThread, fall back to the active thread if we can't find a main thread. - let thread_id = match options.detection_thread() { - DetectionThread::MainThread => profile.get_main_thread_id(), - _ => None, + let detect_in_thread = |thread_id: u64, nodes: &mut HashMap| { + if let Some(call_trees) = call_trees_per_thread_id.get(&thread_id) { + for root in call_trees { + detect_frame_in_call_tree(root, options, nodes); } - .unwrap_or_else(|| profile.get_transaction().active_thread_id); - if let Some(call_trees) = call_trees_per_thread_id.get(&thread_id) { - for root in call_trees { - detect_frame_in_call_tree(root, options, &mut nodes); - } - } else { + } + }; + + match options.detection_thread() { + DetectionThread::ActiveThread => { + let thread_id = profile.get_transaction().active_thread_id; + detect_in_thread(thread_id, &mut nodes); + } + DetectionThread::MainThread => { + // Only consider the main thread; do not fall back to the active thread. + let Some(thread_id) = profile.get_main_thread_id() else { return; - } + }; + detect_in_thread(thread_id, &mut nodes); } DetectionThread::AllThreads => { for call_trees in call_trees_per_thread_id.values() { @@ -1549,4 +1553,60 @@ mod tests { assert_eq!(nodes, test.want, "test '{}' failed", test.name); } } + + #[test] + fn test_detect_frame_main_thread_no_fallback() { + use crate::{occurrence::detect_frame::detect_frame, sample::v1::SampleProfile, types::Transaction}; + + // A matching node living on the active thread (id 1). There is no main thread. + let matching_node = Rc::new(RefCell::new(Node { + duration_ns: 20_000_000, + sample_count: 4, + name: "func".to_string(), + package: "pkg".to_string(), + ..Default::default() + })); + let call_trees: crate::types::CallTreesU64 = + HashMap::from([(1u64, vec![matching_node])]); + + let options = DetectExactFrameOptions { + detection_thread: DetectionThread::MainThread, + duration_threshold: Duration::from_millis(16), + sample_threshold: 1, + functions_by_package: HashMap::from([("pkg", HashMap::from([("func", FILE_READ)]))]), + }; + + // Cocoa profile whose active thread (id 1) has the matching node but which has + // no resolvable main thread. + let profile = SampleProfile { + platform: "cocoa".to_string(), + transaction: Transaction { + active_thread_id: 1, + ..Default::default() + }, + ..Default::default() + }; + + // MainThread detection must not fall back to the active thread, so no + // occurrences should be produced. + let mut occurrences = Vec::new(); + detect_frame(&profile, &call_trees, &options, &mut occurrences); + assert!( + occurrences.is_empty(), + "MainThread detection should not fall back to the active thread" + ); + + // Sanity check: the node is matchable, so ActiveThread detection finds it. + let active_options = DetectExactFrameOptions { + detection_thread: DetectionThread::ActiveThread, + ..options + }; + let mut active_occurrences = Vec::new(); + detect_frame(&profile, &call_trees, &active_options, &mut active_occurrences); + assert_eq!( + active_occurrences.len(), + 1, + "ActiveThread detection should find the matching node" + ); + } } From 7091fe573c1a5b1523ef94975a68d1da22ee32da Mon Sep 17 00:00:00 2001 From: Tony Xiao Date: Fri, 5 Jun 2026 10:04:13 -0400 Subject: [PATCH 4/4] style: run cargo fmt on detect_frame test Co-Authored-By: Claude Opus 4.8 --- src/occurrence/detect_frame.rs | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/occurrence/detect_frame.rs b/src/occurrence/detect_frame.rs index d2ee3d5..47cc015 100644 --- a/src/occurrence/detect_frame.rs +++ b/src/occurrence/detect_frame.rs @@ -1556,7 +1556,9 @@ mod tests { #[test] fn test_detect_frame_main_thread_no_fallback() { - use crate::{occurrence::detect_frame::detect_frame, sample::v1::SampleProfile, types::Transaction}; + use crate::{ + occurrence::detect_frame::detect_frame, sample::v1::SampleProfile, types::Transaction, + }; // A matching node living on the active thread (id 1). There is no main thread. let matching_node = Rc::new(RefCell::new(Node { @@ -1566,8 +1568,7 @@ mod tests { package: "pkg".to_string(), ..Default::default() })); - let call_trees: crate::types::CallTreesU64 = - HashMap::from([(1u64, vec![matching_node])]); + let call_trees: crate::types::CallTreesU64 = HashMap::from([(1u64, vec![matching_node])]); let options = DetectExactFrameOptions { detection_thread: DetectionThread::MainThread, @@ -1602,7 +1603,12 @@ mod tests { ..options }; let mut active_occurrences = Vec::new(); - detect_frame(&profile, &call_trees, &active_options, &mut active_occurrences); + detect_frame( + &profile, + &call_trees, + &active_options, + &mut active_occurrences, + ); assert_eq!( active_occurrences.len(), 1,