Skip to content

Commit

Permalink
Add AlreadyLocked locking strategy
Browse files Browse the repository at this point in the history
We already had code to sample with or without locking the target process,
but disabling locking also disabled some other functionality like
native tracing and looking up OS thread ids. For use as a library, its
helpful to have a 3rd option which indicates that the process is already
locked - so we shouldn't attempt to lock it, but can try to get native
traces and the OS thread id.
  • Loading branch information
benfred committed May 17, 2020
1 parent 076b975 commit bdef240
Show file tree
Hide file tree
Showing 4 changed files with 25 additions and 15 deletions.
24 changes: 17 additions & 7 deletions src/config.rs
Expand Up @@ -8,7 +8,7 @@ pub struct Config {
/// Setting this to false will reduce the performance impact on the target
/// python process, but can lead to incorrect results like partial stack
/// traces being returned or a higher sampling error rate
pub non_blocking: bool,
pub blocking: LockingStrategy,

/// Whether or not to profile native extensions. Note: this option can not be
/// used with the nonblocking option, as we have to pause the process to collect
Expand Down Expand Up @@ -60,6 +60,14 @@ arg_enum!{
}
}


#[derive(Debug, Clone, Eq, PartialEq)]
pub enum LockingStrategy {
NonBlocking,
AlreadyLocked,
Lock
}

#[derive(Debug, Clone, Eq, PartialEq)]
pub enum RecordDuration {
Unlimited,
Expand All @@ -72,7 +80,7 @@ impl Default for Config {
fn default() -> Config {
Config{pid: None, python_program: None, filename: None, format: None,
command: String::from("top"),
non_blocking: false, show_line_numbers: false, sampling_rate: 100,
blocking: LockingStrategy::Lock, show_line_numbers: false, sampling_rate: 100,
duration: RecordDuration::Unlimited, native: false,
gil_only: false, include_idle: false, include_thread_ids: false,
hide_progress: false, capture_output: true, dump_json: false, dump_locals: false, subprocesses: false}
Expand Down Expand Up @@ -259,7 +267,6 @@ impl Config {
config.gil_only = matches.occurrences_of("gil") > 0;
config.include_thread_ids = matches.occurrences_of("threads") > 0;

config.non_blocking = matches.occurrences_of("nonblocking") > 0;
config.native = matches.occurrences_of("native") > 0;
config.hide_progress = matches.occurrences_of("hideprogress") > 0;
config.dump_json = matches.occurrences_of("json") > 0;
Expand All @@ -271,10 +278,13 @@ impl Config {
config.hide_progress = true;
}

// disable native profiling if invalidly asked for
if config.native && config.non_blocking {
eprintln!("Can't get native stack traces with the --nonblocking option.");
std::process::exit(1);
if matches.occurrences_of("nonblocking") > 0 {
// disable native profiling if invalidly asked for
if config.native {
eprintln!("Can't get native stack traces with the --nonblocking option.");
std::process::exit(1);
}
config.blocking = LockingStrategy::NonBlocking;
}

#[cfg(windows)]
Expand Down
4 changes: 2 additions & 2 deletions src/lib.rs
Expand Up @@ -49,8 +49,8 @@ extern crate rand;
extern crate rand_distr;
extern crate remoteprocess;

mod config;
mod binary_parser;
pub mod config;
pub mod binary_parser;
#[cfg(unwind)]
mod cython;
#[cfg(unwind)]
Expand Down
2 changes: 1 addition & 1 deletion src/native_stack_trace.rs
Expand Up @@ -35,7 +35,7 @@ impl NativeStack {
python,
libpython,
process,
symbol_cache: LruCache::new(4096)
symbol_cache: LruCache::new(65536)
});
}

Expand Down
10 changes: 5 additions & 5 deletions src/python_spy.rs
Expand Up @@ -17,7 +17,7 @@ use proc_maps::{get_process_maps, MapRange};


use crate::binary_parser::{parse_binary, BinaryInfo};
use crate::config::Config;
use crate::config::{Config, LockingStrategy};
#[cfg(unwind)]
use crate::native_stack_trace::NativeStack;
use crate::python_bindings::{pyruntime, v2_7_15, v3_3_7, v3_5_5, v3_6_6, v3_7_0, v3_8_0};
Expand Down Expand Up @@ -186,10 +186,10 @@ impl PythonSpy {
// activity status from the OS (otherwise each thread would report being inactive always).
// This has the potential for race conditions (in that the thread activity could change
// between getting the status and locking the thread, but seems unavoidable right now
let _lock = if self.config.non_blocking {
None
} else {
let _lock = if self.config.blocking == LockingStrategy::Lock {
Some(self.process.lock().context("Failed to suspend process")?)
} else {
None
};

let gil_thread_id = self._get_gil_threadid::<I>()?;
Expand Down Expand Up @@ -329,7 +329,7 @@ impl PythonSpy {
fn _get_os_thread_id<I: InterpreterState>(&mut self, python_thread_id: u64, interp: &I) -> Result<Option<Tid>, Error> {
// in nonblocking mode, we can't get the threadid reliably (method here requires reading the RBX
// register which requires a ptrace attach). fallback to heuristic thread activity here
if self.config.non_blocking {
if self.config.blocking == LockingStrategy::NonBlocking {
return Ok(None);
}

Expand Down

0 comments on commit bdef240

Please sign in to comment.