From 948e99335ce3385cf532e0392b15543a8e232275 Mon Sep 17 00:00:00 2001 From: Thomas Otto Date: Wed, 22 Dec 2021 23:07:16 +0100 Subject: [PATCH 1/6] Disable /proc fd caching on Linux when querying processes --- src/utils/process.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/utils/process.rs b/src/utils/process.rs index 68b57a6a4..eac564e02 100644 --- a/src/utils/process.rs +++ b/src/utils/process.rs @@ -226,6 +226,13 @@ struct ProcInfo { } impl ProcInfo { fn new() -> Self { + // On Linux sysinfo optimizes for repeated process queries and keeps per-process + // /proc file descriptors open. This caching is not needed here, so + // set this to zero (this does nothing on other platforms). + // Also, there is currently a kernel bug which slows down syscalls when threads are + // involved (here: the ctrlc handler) and a lot of files are kept open. + sysinfo::set_open_files_limit(0); + ProcInfo { info: sysinfo::System::new(), } From 85a66cfbd21b10fe98f83ceb92d3cfade25c258a Mon Sep 17 00:00:00 2001 From: Thomas Otto Date: Wed, 22 Dec 2021 23:35:47 +0100 Subject: [PATCH 2/6] Re-enable full process scans on Linux --- src/utils/process.rs | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/src/utils/process.rs b/src/utils/process.rs index eac564e02..ed281d8d2 100644 --- a/src/utils/process.rs +++ b/src/utils/process.rs @@ -444,13 +444,8 @@ where match info.find_sibling_in_refreshed_processes(my_pid, &extract_args) { None => { - #[cfg(test)] - { - info.refresh_processes(); - info.find_sibling_in_refreshed_processes(my_pid, &extract_args) - } - #[cfg(not(test))] - None + info.refresh_processes(); + info.find_sibling_in_refreshed_processes(my_pid, &extract_args) } some => some, } From f5e7a31b4f1ffbbc71d555bc079ba34c39b621d0 Mon Sep 17 00:00:00 2001 From: Thomas Otto Date: Wed, 22 Dec 2021 23:19:04 +0100 Subject: [PATCH 3/6] Move parent process query into a thread --- src/handlers/git_show_file.rs | 4 +-- src/handlers/grep.rs | 11 ++++--- src/handlers/hunk.rs | 12 ++++---- src/main.rs | 6 ++++ src/utils/process.rs | 55 ++++++++++++++++++++++++++--------- 5 files changed, 58 insertions(+), 30 deletions(-) diff --git a/src/handlers/git_show_file.rs b/src/handlers/git_show_file.rs index d5eeb5127..eb261203a 100644 --- a/src/handlers/git_show_file.rs +++ b/src/handlers/git_show_file.rs @@ -9,9 +9,7 @@ impl<'a> StateMachine<'a> { self.painter.emit()?; let mut handled_line = false; if matches!(self.state, State::Unknown) { - if let Some(process::CallingProcess::GitShow(_, extension)) = - process::calling_process().as_deref() - { + if let process::CallingProcess::GitShow(_, extension) = &*process::calling_process() { self.state = State::GitShowFile; self.painter.set_syntax(extension.as_deref()); self.painter.set_highlighter(); diff --git a/src/handlers/grep.rs b/src/handlers/grep.rs index 32e1463c5..c3d6da6cd 100644 --- a/src/handlers/grep.rs +++ b/src/handlers/grep.rs @@ -246,8 +246,8 @@ fn get_code_style_sections<'b>( } fn make_output_config() -> GrepOutputConfig { - match process::calling_process().as_deref() { - Some(process::CallingProcess::GitGrep(command_line)) + match &*process::calling_process() { + process::CallingProcess::GitGrep(command_line) if command_line.short_options.contains("-W") || command_line.long_options.contains("--function-context") => { @@ -265,7 +265,7 @@ fn make_output_config() -> GrepOutputConfig { pad_line_number: true, } } - Some(process::CallingProcess::GitGrep(command_line)) + process::CallingProcess::GitGrep(command_line) if command_line.short_options.contains("-p") || command_line.long_options.contains("--show-function") => { @@ -380,9 +380,8 @@ pub fn parse_grep_line(line: &str) -> Option { if line.starts_with('{') { ripgrep_json::parse_line(line) } else { - match process::calling_process().as_deref() { - Some(process::CallingProcess::GitGrep(_)) - | Some(process::CallingProcess::OtherGrep) => [ + match &*process::calling_process() { + process::CallingProcess::GitGrep(_) | process::CallingProcess::OtherGrep => [ &*GREP_LINE_REGEX_ASSUMING_FILE_EXTENSION, &*GREP_LINE_REGEX_ASSUMING_NO_INTERNAL_SEPARATOR_CHARS, ] diff --git a/src/handlers/hunk.rs b/src/handlers/hunk.rs index 2acf2eae9..b541d3268 100644 --- a/src/handlers/hunk.rs +++ b/src/handlers/hunk.rs @@ -27,13 +27,11 @@ lazy_static! { } fn compute_is_word_diff() -> bool { - match process::calling_process().as_deref() { - Some( - CallingProcess::GitDiff(cmd_line) - | CallingProcess::GitShow(cmd_line, _) - | CallingProcess::GitLog(cmd_line) - | CallingProcess::GitReflog(cmd_line), - ) => { + match &*process::calling_process() { + CallingProcess::GitDiff(cmd_line) + | CallingProcess::GitShow(cmd_line, _) + | CallingProcess::GitLog(cmd_line) + | CallingProcess::GitReflog(cmd_line) => { cmd_line.long_options.contains("--word-diff") || cmd_line.long_options.contains("--word-diff-regex") || cmd_line.long_options.contains("--color-words") diff --git a/src/main.rs b/src/main.rs index 5e6eab5e6..1de9375b4 100644 --- a/src/main.rs +++ b/src/main.rs @@ -65,6 +65,12 @@ pub mod errors { #[cfg(not(tarpaulin_include))] fn main() -> std::io::Result<()> { + // Do this first because both parsing all the input in `run_app()` and + // listing all processes takes about 50ms on Linux. + // It also improves the chance that the calling process is still around when + // input is piped into delta (e.g. `git show --word-diff=color | delta`). + utils::process::start_determining_calling_process_in_thread(); + // Ignore ctrl-c (SIGINT) to avoid leaving an orphaned pager process. // See https://github.com/dandavison/delta/issues/681 ctrlc::set_handler(|| {}) diff --git a/src/utils/process.rs b/src/utils/process.rs index ed281d8d2..a3c7f6875 100644 --- a/src/utils/process.rs +++ b/src/utils/process.rs @@ -1,9 +1,9 @@ -use std::borrow::Cow; use std::collections::{HashMap, HashSet}; use std::path::Path; -use sysinfo::{Pid, Process, ProcessExt, ProcessRefreshKind, SystemExt}; +use std::sync::{Arc, Condvar, Mutex, MutexGuard}; use lazy_static::lazy_static; +use sysinfo::{Pid, Process, ProcessExt, ProcessRefreshKind, SystemExt}; #[derive(Clone, Debug, PartialEq)] pub enum CallingProcess { @@ -13,6 +13,8 @@ pub enum CallingProcess { GitReflog(CommandLine), GitGrep(CommandLine), OtherGrep, // rg, grep, ag, ack, etc + None, // no matching process could be found + Pending, // calling process is currently being determined } // TODO: Git blame is currently handled differently @@ -23,23 +25,48 @@ pub struct CommandLine { last_arg: Option, } -pub fn calling_process() -> Option> { - #[cfg(not(test))] - { - CACHED_CALLING_PROCESS.as_ref().map(Cow::Borrowed) - } - #[cfg(test)] - { - determine_calling_process().map(Cow::Owned) - } +lazy_static! { + static ref CALLER: Arc<(Mutex, Condvar)> = + Arc::new((Mutex::new(CallingProcess::Pending), Condvar::new())); } -lazy_static! { - static ref CACHED_CALLING_PROCESS: Option = determine_calling_process(); +pub fn start_determining_calling_process_in_thread() { + // The handle is neither kept nor returned nor joined but dropped, so the main + // thread can exit early if it does not need to know its parent process. + std::thread::Builder::new() + .name("find_calling_process".into()) + .spawn(move || { + let calling_process = determine_calling_process(); + + let (caller_mutex, determine_done) = &**CALLER; + + let mut caller = caller_mutex.lock().unwrap(); + *caller = calling_process; + determine_done.notify_all(); + }) + .unwrap(); +} + +#[cfg(not(test))] +pub fn calling_process() -> MutexGuard<'static, CallingProcess> { + let (caller_mutex, determine_done) = &**CALLER; + determine_done + .wait_while(caller_mutex.lock().unwrap(), |caller| { + *caller == CallingProcess::Pending + }) + .unwrap() +} + +// The return value is duck-typed to work in place of a MutexGuard when testing. +#[cfg(test)] +pub fn calling_process() -> Box { + type _UnusedImport = MutexGuard<'static, i8>; + Box::new(determine_calling_process()) } -fn determine_calling_process() -> Option { +fn determine_calling_process() -> CallingProcess { calling_process_cmdline(ProcInfo::new(), describe_calling_process) + .unwrap_or(CallingProcess::None) } // Return value of `extract_args(args: &[String]) -> ProcessArgs` function which is From 4076ec46923d5ad08a0d530acd53725b1b1cd7c2 Mon Sep 17 00:00:00 2001 From: Thomas Otto Date: Wed, 22 Dec 2021 23:30:30 +0100 Subject: [PATCH 4/6] The calling process must have a common parent with delta Ensures that no unrelated process is found when selectively refreshing a pid range. --- src/utils/process.rs | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/src/utils/process.rs b/src/utils/process.rs index a3c7f6875..fd5f1fe89 100644 --- a/src/utils/process.rs +++ b/src/utils/process.rs @@ -363,7 +363,11 @@ trait ProcessInterface { }; iter_parents(self, pid, &mut sum_distance); - Some((length_of_process_chain, args)) + if length_of_process_chain == usize::MAX { + None + } else { + Some((length_of_process_chain, args)) + } } _ => None, }) @@ -847,6 +851,20 @@ pub mod tests { calling_process_cmdline(ProcInfo::new(), guess_git_blame_filename_extension); } + #[test] + fn test_process_blame_no_parent_found() { + let two_trees = MockProcInfo::with(&[ + (2, 100, "-shell", None), + (3, 100, "git blame src/main.rs", Some(2)), + (4, 100, "call_delta.sh", None), + (5, 100, "delta", Some(4)), + ]); + assert_eq!( + calling_process_cmdline(two_trees, guess_git_blame_filename_extension), + None + ); + } + #[test] fn test_process_blame_info_with_parent() { let no_processes = MockProcInfo::with(&[]); From 9c1897717449893be6752c56b86e95c9da4f9f46 Mon Sep 17 00:00:00 2001 From: Dan Davison Date: Tue, 28 Dec 2021 15:49:33 +0000 Subject: [PATCH 5/6] Update sysinfo dependency to include fix Ref #869 Ref https://github.com/GuillaumeGomez/sysinfo/pull/650 --- Cargo.lock | 3 +-- Cargo.toml | 3 ++- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 51135de18..cf336a6d6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1054,8 +1054,7 @@ dependencies = [ [[package]] name = "sysinfo" version = "0.22.3" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b645b59c59114c25d3d554f781b0a1f1f01545d1d02f271bfb1c897bdfdfdcf3" +source = "git+https://github.com/GuillaumeGomez/sysinfo?rev=62374ac97393dec978d44a888f6b0907b211938d#62374ac97393dec978d44a888f6b0907b211938d" dependencies = [ "cfg-if 1.0.0", "core-foundation-sys", diff --git a/Cargo.toml b/Cargo.toml index cdcf455e1..4f92ec229 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -53,7 +53,8 @@ default-features = false features = ["parsing", "assets", "yaml-load", "dump-load", "regex-onig"] [dependencies.sysinfo] -version = "0.22.3" +git = "https://github.com/GuillaumeGomez/sysinfo" +rev = "62374ac97393dec978d44a888f6b0907b211938d" # no default features to disable the use of threads default-features = false features = [] From ba761d8c69c60400593c9a3146da2236ce64314b Mon Sep 17 00:00:00 2001 From: Dan Davison Date: Tue, 28 Dec 2021 16:29:10 +0000 Subject: [PATCH 6/6] Update sysinfo --- Cargo.lock | 2 +- Cargo.toml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index cf336a6d6..3c3f3daba 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1054,7 +1054,7 @@ dependencies = [ [[package]] name = "sysinfo" version = "0.22.3" -source = "git+https://github.com/GuillaumeGomez/sysinfo?rev=62374ac97393dec978d44a888f6b0907b211938d#62374ac97393dec978d44a888f6b0907b211938d" +source = "git+https://github.com/GuillaumeGomez/sysinfo?rev=04406372b284ebde04c26862decc811f50bfcb07#04406372b284ebde04c26862decc811f50bfcb07" dependencies = [ "cfg-if 1.0.0", "core-foundation-sys", diff --git a/Cargo.toml b/Cargo.toml index 4f92ec229..d155e1ddc 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -54,7 +54,7 @@ features = ["parsing", "assets", "yaml-load", "dump-load", "regex-onig"] [dependencies.sysinfo] git = "https://github.com/GuillaumeGomez/sysinfo" -rev = "62374ac97393dec978d44a888f6b0907b211938d" +rev = "04406372b284ebde04c26862decc811f50bfcb07" # no default features to disable the use of threads default-features = false features = []