Skip to content

Commit

Permalink
Merge pull request #445 from mstange/push-yyuwnrlxzprz
Browse files Browse the repository at this point in the history
Disable JitFunctionAdd and CSwitch markers by default, and add arguments to get them back.
  • Loading branch information
mstange authored Dec 7, 2024
2 parents 8d9453f + 9165d44 commit 250c937
Show file tree
Hide file tree
Showing 9 changed files with 125 additions and 67 deletions.
48 changes: 31 additions & 17 deletions samply/src/linux_shared/converter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,12 @@ where
/// already done the adjusting, either by adjusting the call chains coming from
/// the kernel or by doing its own unwinding with an adjusting unwinder,
call_chain_return_addresses_are_preadjusted: bool,

/// Whether to emit JitFunctionAdd markers.
should_emit_jit_markers: bool,

/// Whether to emit context switch markers.
should_emit_cswitch_markers: bool,
}

const DEFAULT_OFF_CPU_SAMPLING_INTERVAL_NS: u64 = 1_000_000; // 1ms
Expand Down Expand Up @@ -240,6 +246,7 @@ where
processes: Processes::new(
profile_creation_props.reuse_threads,
profile_creation_props.unlink_aux_files,
profile_creation_props.should_emit_jit_markers,
),
timestamp_converter,
current_sample_time: first_sample_time,
Expand Down Expand Up @@ -267,6 +274,8 @@ where
.arg_count_to_include_in_process_name,
cpus,
call_chain_return_addresses_are_preadjusted,
should_emit_jit_markers: profile_creation_props.should_emit_jit_markers,
should_emit_cswitch_markers: profile_creation_props.should_emit_cswitch_markers,
}
}

Expand Down Expand Up @@ -957,14 +966,16 @@ where
Some(idle_frame_label),
);
}
cpu.notify_switch_in(
tid,
thread.thread_label(),
timestamp,
&self.timestamp_converter,
&[cpu.thread_handle, combined_thread],
&mut self.profile,
);
if self.should_emit_cswitch_markers {
cpu.notify_switch_in_for_marker(
tid,
thread.thread_label(),
timestamp,
&self.timestamp_converter,
&[cpu.thread_handle, combined_thread],
&mut self.profile,
);
}
}
}
ContextSwitchRecord::Out { preempted, .. } => {
Expand All @@ -975,15 +986,17 @@ where
let cpu = cpus.get_mut(cpu_index as usize, &mut self.profile);
self.context_switch_handler
.handle_switch_out(timestamp, &mut cpu.context_switch_data);
cpu.notify_switch_out(
tid,
timestamp,
&self.timestamp_converter,
&[cpu.thread_handle, combined_thread],
thread.profile_thread,
preempted == TaskWasPreempted::Yes,
&mut self.profile,
);
if self.should_emit_cswitch_markers {
cpu.notify_switch_out_for_marker(
tid,
timestamp,
&self.timestamp_converter,
&[cpu.thread_handle, combined_thread],
thread.profile_thread,
preempted == TaskWasPreempted::Yes,
&mut self.profile,
);
}
}
}
}
Expand Down Expand Up @@ -1511,6 +1524,7 @@ where
lib_handle,
&mut self.jit_category_manager,
&mut self.profile,
self.should_emit_jit_markers,
);
} else {
process.add_regular_lib_mapping(
Expand Down
23 changes: 13 additions & 10 deletions samply/src/linux_shared/process.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,11 +59,12 @@ where
thread_recycler: Option<ThreadRecycler>,
jit_function_recycler: Option<JitFunctionRecycler>,
unlink_aux_files: bool,
should_emit_jit_markers: bool,
) -> Self {
Self {
profile_process: process_handle,
unwinder: U::default(),
jitdump_manager: JitDumpManager::new(unlink_aux_files),
jitdump_manager: JitDumpManager::new(unlink_aux_files, should_emit_jit_markers),
lib_mapping_ops: Default::default(),
name: name.clone(),
pid,
Expand Down Expand Up @@ -207,8 +208,7 @@ where
None
};

let jitdump_manager =
std::mem::replace(&mut self.jitdump_manager, JitDumpManager::new(false));
let jitdump_manager = self.jitdump_manager;
let mut jitdump_ops = jitdump_manager.finish(
jit_category_manager,
profile,
Expand Down Expand Up @@ -298,14 +298,17 @@ where
mut lib_handle: LibraryHandle,
jit_category_manager: &mut JitCategoryManager,
profile: &mut Profile,
should_add_marker: bool,
) {
let main_thread = self.threads.main_thread.profile_thread;
let timing = MarkerTiming::Instant(profile_timestamp);
let name = match symbol_name {
Some(name) => profile.intern_string(name),
None => profile.intern_string("<unknown>"),
};
profile.add_marker(main_thread, timing, JitFunctionAddMarker(name));
if should_add_marker {
let main_thread = self.threads.main_thread.profile_thread;
let timing = MarkerTiming::Instant(profile_timestamp);
let name = match symbol_name {
Some(name) => profile.intern_string(name),
None => profile.intern_string("<unknown>"),
};
profile.add_marker(main_thread, timing, JitFunctionAddMarker(name));
}

if let (Some(name), Some(recycler)) = (symbol_name, self.jit_function_recycler.as_mut()) {
let code_size = (end_address - start_address) as u32;
Expand Down
9 changes: 8 additions & 1 deletion samply/src/linux_shared/processes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,16 @@ where

/// Whether aux files (like jitdump) should be unlinked on open
unlink_aux_data: bool,

/// Whether to emit JitFunctionAdd markers.
should_emit_jit_markers: bool,
}

impl<U> Processes<U>
where
U: Unwinder + Default,
{
pub fn new(allow_reuse: bool, unlink_aux_data: bool) -> Self {
pub fn new(allow_reuse: bool, unlink_aux_data: bool, should_emit_jit_markers: bool) -> Self {
let process_recycler = if allow_reuse {
Some(ProcessRecycler::new())
} else {
Expand All @@ -45,6 +48,7 @@ where
process_recycler,
process_sample_datas: Vec::new(),
unlink_aux_data,
should_emit_jit_markers,
}
}

Expand Down Expand Up @@ -78,6 +82,7 @@ where
Some(thread_recycler),
Some(jit_function_recycler),
self.unlink_aux_data,
self.should_emit_jit_markers,
);
return entry.insert(process);
}
Expand Down Expand Up @@ -113,6 +118,7 @@ where
thread_recycler,
jit_function_recycler,
self.unlink_aux_data,
self.should_emit_jit_markers,
);
entry.insert(process)
}
Expand Down Expand Up @@ -164,6 +170,7 @@ where
thread_recycler,
jit_function_recycler,
self.unlink_aux_data,
self.should_emit_jit_markers,
)
})
}
Expand Down
5 changes: 4 additions & 1 deletion samply/src/mac/task_profiler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,10 @@ impl TaskProfiler {
ignored_errors: Vec::new(),
unwinder: UnwinderNative::new(),
path_receiver,
jitdump_manager: JitDumpManager::new(profile_creation_props.unlink_aux_files),
jitdump_manager: JitDumpManager::new(
profile_creation_props.unlink_aux_files,
profile_creation_props.should_emit_jit_markers,
),
marker_file_paths: Vec::new(),
lib_mapping_ops: Default::default(),
unresolved_samples: Default::default(),
Expand Down
12 changes: 12 additions & 0 deletions samply/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -371,6 +371,14 @@ pub struct ProfileCreationArgs {
#[arg(long)]
per_cpu_threads: bool,

/// Emit a JitFunctionAdd markers when a JIT function is added.
#[arg(long)]
jit_markers: bool,

/// Emit context switch markers.
#[arg(long)]
cswitch_markers: bool,

/// Include up to <INCLUDE_ARGS> command line arguments in the process name.
/// This can help differentiate processes if the same executable is used
/// for different types of programs. And in --reuse-threads mode it
Expand Down Expand Up @@ -541,6 +549,8 @@ impl ImportArgs {
arg_count_to_include_in_process_name: self.profile_creation_args.include_args,
override_arch: self.override_arch.clone(),
unstable_presymbolicate: self.profile_creation_args.unstable_presymbolicate,
should_emit_jit_markers: self.profile_creation_args.jit_markers,
should_emit_cswitch_markers: self.profile_creation_args.cswitch_markers,
coreclr: to_coreclr_profile_props(&self.coreclr),
#[cfg(target_os = "windows")]
unknown_event_markers: self.profile_creation_args.unknown_event_markers,
Expand Down Expand Up @@ -664,6 +674,8 @@ impl RecordArgs {
arg_count_to_include_in_process_name: self.profile_creation_args.include_args,
override_arch: None,
unstable_presymbolicate: self.profile_creation_args.unstable_presymbolicate,
should_emit_jit_markers: self.profile_creation_args.jit_markers,
should_emit_cswitch_markers: self.profile_creation_args.cswitch_markers,
coreclr: to_coreclr_profile_props(&self.coreclr),
#[cfg(target_os = "windows")]
unknown_event_markers: self.profile_creation_args.unknown_event_markers,
Expand Down
23 changes: 15 additions & 8 deletions samply/src/shared/jitdump_manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,16 @@ pub struct JitDumpManager {
pending_jitdump_paths: Vec<(ThreadHandle, PathBuf, Vec<PathBuf>)>,
processors: Vec<SingleJitDumpProcessor>,
unlink_after_open: bool,
should_emit_jit_markers: bool,
}

impl JitDumpManager {
pub fn new(unlink_after_open: bool) -> Self {
pub fn new(unlink_after_open: bool, should_emit_jit_markers: bool) -> Self {
JitDumpManager {
pending_jitdump_paths: Vec::new(),
processors: Vec::new(),
unlink_after_open,
should_emit_jit_markers,
}
}

Expand Down Expand Up @@ -83,6 +85,7 @@ impl JitDumpManager {
profile,
recycler.as_deref_mut(),
timestamp_converter,
self.should_emit_jit_markers,
);
}
}
Expand Down Expand Up @@ -143,6 +146,7 @@ impl SingleJitDumpProcessor {
profile: &mut Profile,
mut recycler: Option<&mut JitFunctionRecycler>,
timestamp_converter: &TimestampConverter,
should_add_marker: bool,
) {
let Some(reader) = self.reader.as_mut() else {
return;
Expand Down Expand Up @@ -186,13 +190,16 @@ impl SingleJitDumpProcessor {
name: symbol_name.to_owned(),
});

let timestamp = timestamp_converter.convert_time(raw_jitdump_record.timestamp);
let symbol_name_handle = profile.intern_string(symbol_name);
profile.add_marker(
self.thread_handle,
MarkerTiming::Instant(timestamp),
JitFunctionAddMarker(symbol_name_handle),
);
if should_add_marker {
let timestamp =
timestamp_converter.convert_time(raw_jitdump_record.timestamp);
let symbol_name_handle = profile.intern_string(symbol_name);
profile.add_marker(
self.thread_handle,
MarkerTiming::Instant(timestamp),
JitFunctionAddMarker(symbol_name_handle),
);
}

let (lib_handle, relative_address_at_start) =
if let Some(recycler) = recycler.as_deref_mut() {
Expand Down
6 changes: 3 additions & 3 deletions samply/src/shared/per_cpu.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ pub struct Cpu {
pub name: StringHandle,
pub thread_handle: ThreadHandle,
pub context_switch_data: ThreadContextSwitchData,
pub current_tid: Option<(i32, StringHandle, u64)>,
current_tid: Option<(i32, StringHandle, u64)>,
}

impl Cpu {
Expand All @@ -33,7 +33,7 @@ impl Cpu {
}

#[allow(clippy::too_many_arguments)]
pub fn notify_switch_in(
pub fn notify_switch_in_for_marker(
&mut self,
tid: i32,
thread_name: StringHandle,
Expand All @@ -60,7 +60,7 @@ impl Cpu {
}

#[allow(clippy::too_many_arguments)]
pub fn notify_switch_out(
pub fn notify_switch_out_for_marker(
&mut self,
tid: i32, // tid that is being switched away from
timestamp: u64,
Expand Down
6 changes: 6 additions & 0 deletions samply/src/shared/recording_props.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,12 @@ pub struct ProfileCreationProps {
/// Time range to include, relative to start of recording.
#[allow(dead_code)]
pub time_range: Option<(std::time::Duration, std::time::Duration)>,
/// Whether to emit "JitFunctionAdd" markers.
#[allow(dead_code)]
pub should_emit_jit_markers: bool,
/// Whether to emit context switch markers.
#[allow(dead_code)]
pub should_emit_cswitch_markers: bool,
}

impl ProfileCreationProps {
Expand Down
Loading

0 comments on commit 250c937

Please sign in to comment.