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..47cc015 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,31 @@ 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) { + 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, &mut nodes); + detect_frame_in_call_tree(root, options, nodes); } - } 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); + }; + + 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() { + for root in call_trees { + detect_frame_in_call_tree(root, options, &mut nodes); + } } } } @@ -596,7 +620,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 +1471,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([ @@ -1529,4 +1553,66 @@ 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" + ); + } } 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 415fbae..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; } @@ -537,6 +540,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 } @@ -2130,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" + ); + } } 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>;