Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/android/profile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,10 @@ impl ProfileInterface for AndroidProfile {
})
}

fn get_main_thread_id(&self) -> Option<u64> {
None
}

fn get_transaction_tags(&self) -> &HashMap<String, String> {
&self.transaction_tags
}
Expand Down
138 changes: 112 additions & 26 deletions src/occurrence/detect_frame.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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<NodeInfo> {
Expand Down Expand Up @@ -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<NodeInfo> {
Expand Down Expand Up @@ -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([
Expand Down Expand Up @@ -240,7 +252,7 @@ pub static DETECT_FRAME_JOBS: Lazy<
]),
}) as Box<dyn DetectFrameOptions + Send + Sync>,
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([
Expand All @@ -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([
Expand Down Expand Up @@ -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([
Expand Down Expand Up @@ -564,19 +576,31 @@ pub fn detect_frame(
// List nodes matching criteria
let mut nodes: HashMap<NodeKey, NodeInfo> = 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<NodeKey, NodeInfo>| {
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;
Comment thread
cursor[bot] marked this conversation as resolved.
};
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);
}
}
}
}
Comment thread
sentry[bot] marked this conversation as resolved.
Expand All @@ -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,
},
};

Expand Down Expand Up @@ -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([
Expand Down Expand Up @@ -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"
);
}
}
2 changes: 1 addition & 1 deletion src/profile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion src/profile_chunk.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
95 changes: 93 additions & 2 deletions src/sample/v1.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<u64, Vec<Rc<RefCell<Node>>>> = HashMap::new();
let mut samples_by_thread_id: HashMap<u64, Vec<&Sample>> = HashMap::new();

Expand All @@ -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;
}

Expand Down Expand Up @@ -537,6 +540,25 @@ impl ProfileInterface for SampleProfile {
Cow::Borrowed(&self.transaction)
}

fn get_main_thread_id(&self) -> Option<u64> {
match self.platform.as_str() {
"cocoa" => self
.profile
.thread_metadata
.as_ref()?
.iter()
.find_map(|(id, meta)| {
let thread_id = id.parse::<u64>().ok()?;
if meta.name.as_deref() == Some("main") {
Some(thread_id)
} else {
None
}
}),
_ => None,
}
}

fn get_transaction_tags(&self) -> &HashMap<String, String> {
&self.transaction_tags
}
Expand Down Expand Up @@ -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"
);
}
}
2 changes: 2 additions & 0 deletions src/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<u64>;
fn get_transaction_tags(&self) -> &HashMap<String, String>;
fn get_debug_meta(&self) -> &DebugMeta;
fn get_measurements(&self) -> Option<&HashMap<String, Measurement>>;
Expand Down
Loading