Skip to content

Commit

Permalink
When computing the product name, don't just pick the first item in th…
Browse files Browse the repository at this point in the history
…e command array, because it might be an environment variable.

This fixes what's displayed in the top left corner in the profiler UI when
doing 'samply record MYENV=val my-command' .
  • Loading branch information
mstange committed Apr 16, 2024
1 parent fa07849 commit f7a0398
Show file tree
Hide file tree
Showing 4 changed files with 79 additions and 56 deletions.
25 changes: 13 additions & 12 deletions samply/src/linux/profiler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ use crate::linux_shared::{
ConvertRegs, Converter, EventInterpretation, MmapRangeOrVec, OffCpuIndicator,
};
use crate::server::{start_server_main, ServerProps};
use crate::shared::recording_props::{ProfileCreationProps, RecordingProps};
use crate::shared::recording_props::{ProcessLaunchProps, ProfileCreationProps, RecordingProps};

#[cfg(target_arch = "x86_64")]
pub type ConvertRegsNative = crate::linux_shared::ConvertRegsX86_64;
Expand All @@ -37,16 +37,20 @@ pub type ConvertRegsNative = crate::linux_shared::ConvertRegsAarch64;

#[allow(clippy::too_many_arguments)]
pub fn start_recording(
command_name: OsString,
command_args: &[OsString],
env_vars: &[(OsString, OsString)],
iteration_count: u32,
process_launch_props: ProcessLaunchProps,
recording_props: RecordingProps,
profile_creation_props: ProfileCreationProps,
server_props: Option<ServerProps>,
) -> Result<ExitStatus, ()> {
// We want to profile a child process which we are about to launch.

let ProcessLaunchProps {
env_vars,
command_name,
args,
iteration_count,
} = process_launch_props;

// Ignore SIGINT in our process while the child process is running. The
// signal will still reach the child process, because Ctrl+C sends the
// SIGINT signal to all processes in the foreground process group.
Expand All @@ -61,7 +65,7 @@ pub fn start_recording(
// Start a new process for the launched command and get its pid.
// The command will not start running until we tell it to.
let process =
SuspendedLaunchedProcess::launch_in_suspended_state(&command_name, command_args, env_vars)
SuspendedLaunchedProcess::launch_in_suspended_state(&command_name, &args, &env_vars)
.unwrap();
let pid = process.pid();

Expand Down Expand Up @@ -158,12 +162,9 @@ pub fn start_recording(
break;
}
eprintln!("Running iteration {i} of {iteration_count}...");
let process = SuspendedLaunchedProcess::launch_in_suspended_state(
&command_name,
command_args,
env_vars,
)
.unwrap();
let process =
SuspendedLaunchedProcess::launch_in_suspended_state(&command_name, &args, &env_vars)
.unwrap();
let pid = process.pid();

// Tell the sampler to start profiling another pid, and wait for it to signal us to go ahead.
Expand Down
21 changes: 12 additions & 9 deletions samply/src/mac/profiler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ use serde_json::to_writer;

use std::collections::hash_map::Entry;
use std::collections::HashMap;
use std::ffi::OsString;
use std::fs::File;
use std::io::BufWriter;
use std::process::ExitStatus;
Expand All @@ -17,7 +16,7 @@ use super::process_launcher::{MachError, ReceivedStuff, TaskAccepter};
use super::sampler::{JitdumpOrMarkerPath, Sampler, TaskInit};
use super::time::get_monotonic_timestamp;
use crate::server::{start_server_main, ServerProps};
use crate::shared::recording_props::{ProfileCreationProps, RecordingProps};
use crate::shared::recording_props::{ProcessLaunchProps, ProfileCreationProps, RecordingProps};

pub fn start_profiling_pid(
_pid: u32,
Expand All @@ -31,17 +30,22 @@ pub fn start_profiling_pid(
}

pub fn start_recording(
command_name: OsString,
command_args: &[OsString],
env_vars: &[(OsString, OsString)],
iteration_count: u32,
process_launch_props: ProcessLaunchProps,
recording_props: RecordingProps,
profile_creation_props: ProfileCreationProps,
server_props: Option<ServerProps>,
) -> Result<ExitStatus, MachError> {
let (task_sender, task_receiver) = unbounded();
let ProcessLaunchProps {
env_vars,
command_name,
args,
iteration_count,
} = process_launch_props;
let command_name_copy = command_name.to_string_lossy().to_string();
let output_file = recording_props.output_file.clone();

let (task_sender, task_receiver) = unbounded();

let sampler_thread = thread::spawn(move || {
let sampler = Sampler::new(
command_name_copy,
Expand All @@ -63,8 +67,7 @@ pub fn start_recording(
)
.expect("cannot register signal handler");

let (mut task_accepter, task_launcher) =
TaskAccepter::new(&command_name, command_args, env_vars)?;
let (mut task_accepter, task_launcher) = TaskAccepter::new(&command_name, &args, &env_vars)?;

let (accepter_sender, accepter_receiver) = unbounded();
let accepter_thread = thread::spawn(move || {
Expand Down
75 changes: 41 additions & 34 deletions samply/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,9 @@ mod shared;

use clap::{Args, Parser, Subcommand};
use profile_json_preparse::parse_libinfo_map_from_profile_file;
use shared::recording_props::{ProfileCreationProps, RecordingProps};
use shared::recording_props::{ProcessLaunchProps, ProfileCreationProps, RecordingProps};

use std::ffi::{OsStr, OsString};
use std::ffi::OsStr;
use std::fs::File;
use std::io::{BufReader, BufWriter};
use std::path::{Path, PathBuf};
Expand Down Expand Up @@ -237,6 +237,7 @@ fn main() {

#[cfg(any(target_os = "android", target_os = "macos", target_os = "linux"))]
Action::Record(record_args) => {
let process_launch_props = record_args.process_launch_props();
let recording_props = record_args.recording_props();
let profile_creation_props = record_args.profile_creation_props();
let server_props = record_args.server_props();
Expand All @@ -249,12 +250,8 @@ fn main() {
server_props,
);
} else {
let (env_vars, command_name, args) = parse_command(&record_args.command);
let exit_status = match profiler::start_recording(
command_name,
args,
&env_vars,
record_args.iteration_count,
process_launch_props,
recording_props,
profile_creation_props,
server_props,
Expand Down Expand Up @@ -330,13 +327,45 @@ impl RecordArgs {
}
}

pub fn process_launch_props(&self) -> ProcessLaunchProps {
let command = &self.command;
let iteration_count = self.iteration_count;
assert!(
!command.is_empty(),
"CLI parsing should have ensured that we have at least one command name"
);

let mut env_vars = Vec::new();
let mut i = 0;
while let Some((var_name, var_val)) = command.get(i).and_then(|s| split_at_first_equals(s))
{
env_vars.push((var_name.to_owned(), var_val.to_owned()));
i += 1;
}
if i == command.len() {
eprintln!("Error: No command name found. Every item looks like an environment variable (contains '='): {command:?}");
std::process::exit(1);
}
let command_name = command[i].clone();
let args = command[(i + 1)..].to_owned();
ProcessLaunchProps {
env_vars,
command_name,
args,
iteration_count,
}
}

#[allow(unused)]
pub fn profile_creation_props(&self) -> ProfileCreationProps {
let profile_name = match (self.profile_creation_args.profile_name.clone(), self.pid, self.command.first()) {
(Some(profile_name), _, _) => profile_name,
(None, Some(pid), _) => format!("PID {pid}"),
(None, None, Some(command)) => command.to_string_lossy().to_string(),
(None, None, None) => panic!("Either pid or command is guaranteed to be present (clap should have done the validation)"),
let profile_name = match (self.profile_creation_args.profile_name.clone(), self.pid) {
(Some(profile_name), _) => profile_name,
(None, Some(pid)) => format!("PID {pid}"),
_ => self
.process_launch_props()
.command_name
.to_string_lossy()
.to_string(),
};
ProfileCreationProps {
profile_name,
Expand Down Expand Up @@ -384,28 +413,6 @@ fn split_at_first_equals(s: &OsStr) -> Option<(&OsStr, &OsStr)> {
Some((name, val))
}

#[allow(unused)]
fn parse_command(command: &[OsString]) -> (Vec<(OsString, OsString)>, OsString, &[OsString]) {
assert!(
!command.is_empty(),
"CLI parsing should have ensured that we have at least one command name"
);

let mut env_vars = Vec::new();
let mut i = 0;
while let Some((var_name, var_val)) = command.get(i).and_then(|s| split_at_first_equals(s)) {
env_vars.push((var_name.to_owned(), var_val.to_owned()));
i += 1;
}
if i == command.len() {
eprintln!("Error: No command name found. Every item looks like an environment variable (contains '='): {command:?}");
std::process::exit(1);
}
let command_name = command[i].clone();
let args = &command[(i + 1)..];
(env_vars, command_name, args)
}

fn convert_file_to_profile(
filename: &Path,
input_file: &File,
Expand Down
14 changes: 13 additions & 1 deletion samply/src/shared/recording_props.rs
Original file line number Diff line number Diff line change
@@ -1,16 +1,28 @@
use std::{path::PathBuf, time::Duration};
use std::{ffi::OsString, path::PathBuf, time::Duration};

/// Properties which are meaningful both for recording a fresh process
/// as well as for recording an existing process.
pub struct RecordingProps {
pub output_file: PathBuf,
pub time_limit: Option<Duration>,
pub interval: Duration,
pub main_thread_only: bool,
}

/// Properties which are meaningful both for recording a profile and
/// for converting a perf.data file to a profile.
pub struct ProfileCreationProps {
pub profile_name: String,
/// Merge non-overlapping threads of the same name.
pub reuse_threads: bool,
/// Fold repeated frames at the base of the stack.
pub fold_recursive_prefix: bool,
}

/// Properties which are meaningful for launching and recording a fresh process.
pub struct ProcessLaunchProps {
pub env_vars: Vec<(OsString, OsString)>,
pub command_name: OsString,
pub args: Vec<OsString>,
pub iteration_count: u32,
}

0 comments on commit f7a0398

Please sign in to comment.