Speed up the calling process query #869

th1000s · 2021-12-22T22:57:07Z

Always determining the parent process is faster now that this information is needed most of the time anyhow.

Also fixes the Linux process query slowdown for good.

src/utils/process.rs

dandavison · 2021-12-23T13:29:10Z

src/utils/process.rs

-    static ref CACHED_CALLING_PROCESS: Option<CallingProcess> = determine_calling_process();
+pub fn start_determining_calling_process_in_thread() {
+    // Do not keep the join handle so the main thread can exit early if
+    // it does not need to know its parent process.


This comment is referring to the fact that there is no return value of this function (and that we use the spawn method on the builder as opposed to the thread::spawn function), right?

The builder is used to set the thread name - in case someone without access to the sources wonders why delta looks at all the processes. It could also return a JoinHandle, but this is ignored. I updated the comments to reflect this.

dandavison · 2021-12-23T13:30:40Z

src/utils/process.rs

+        // /proc file descriptors (fds) 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.


Out of interest, have you identified this bug in the linux kernel issue tracker?

So this is what allows us to fearlessly query many processes on linux again? Very interesting to see that you've got to the bottom of this problem.

As the Linux bug tracker suggests, for now I have reported it to the Debian kernel team, see #1001818 for a minimal C and C++ example.

And yes, on Linux this makes process queries as fast as e.g. ps auxf.

dandavison · 2021-12-23T13:35:32Z

src/utils/process.rs

+    let (caller_mutex, determine_done) = &**CALLER;
+    determine_done
+        .wait_while(caller_mutex.lock().unwrap(), |pending| {
+            *pending == CallingProcess::Pending


Beautiful. And very useful to me to have an example of how to do this.

th1000s · 2021-12-23T23:34:12Z

panic at 'attempt to subtract with overflow', [..] sysinfo-0.22.3/src/lib.rs:132:28 -- (let diff = max - **x;)

Looks like on 32bit sysinfo does not like (concurrent?) set_open_files_limit(0), @GuillaumeGomez?

Also, #[cfg(test)] pub fn calling_process() -> Box<CallingProcess> is currently called 4412 times during cargo test.

GuillaumeGomez · 2021-12-24T11:08:08Z

Also, #[cfg(test)] pub fn calling_process() -> Box<CallingProcess> is currently called 4412 times during cargo test.

You mean in sysinfo? As far as I can see, there is no calling_process function.

GuillaumeGomez · 2021-12-25T18:42:16Z

Forgot to mention but I opened GuillaumeGomez/sysinfo#650. Please tell me if it fixes your issue.

Ref #869 Ref GuillaumeGomez/sysinfo#650

dandavison · 2021-12-28T16:07:46Z

@GuillaumeGomez thanks for helping with this. I've created a version of this PR with your sysinfo fix: #872

It appears that it is still failing, due to the subtraction on the line before the line that you fixed:

thread 'tests::test_example_diffs::tests::test_single_character_line_is_not_whitespace_error'
panicked at 'attempt to subtract with overflow',
/cargo/git/checkouts/sysinfo-bc2293244598544b/62374ac/src/lib.rs:132:28

https://github.com/GuillaumeGomez/sysinfo/blob/62374ac97393dec978d44a888f6b0907b211938d/src/lib.rs#L132-L133

GuillaumeGomez · 2021-12-28T16:20:28Z

I updated the PR, both subtractions are checked now.

dandavison · 2021-12-28T21:46:31Z

Great, looks like it works now. I'll leave it up to @th1000s to finalize things here.

GuillaumeGomez · 2021-12-28T21:51:51Z

Ok, I'll release a new sysinfo version then.

This query only happens once, so caching is not needed Also update sysinfo version to fix a crash related to this.

Start the query even before determining if information about the parent process is required (which are most invocations anyhow).

Ensures that no unrelated process is found when selectively refreshing a pid range.

th1000s · 2022-01-03T21:56:25Z

Thanks, version 0.22.4 fixes the crash.

And the calling_process() function is from delta, this patchset also reduces the number of times it is called during testing.

dandavison · 2022-01-03T22:28:50Z

Thanks @th1000s and @GuillaumeGomez!

dandavison · 2022-02-20T17:22:56Z

The docker script provided by @infokiller in #973 seems to show that having if there are many processes then delta is still significantly slowed down.

dandavison reviewed Dec 23, 2021

View reviewed changes

src/utils/process.rs Outdated Show resolved Hide resolved

dandavison reviewed Dec 23, 2021

View reviewed changes

src/utils/process.rs Outdated Show resolved Hide resolved

dandavison reviewed Dec 23, 2021

View reviewed changes

th1000s force-pushed the open_files branch from 3f1b027 to 4076ec4 Compare December 23, 2021 23:10

GuillaumeGomez mentioned this pull request Dec 24, 2021

Fix panic when changing the limit of open files GuillaumeGomez/sysinfo#650

Merged

dandavison added a commit that referenced this pull request Dec 28, 2021

Update sysinfo dependency to include fix

9c18977

Ref #869 Ref GuillaumeGomez/sysinfo#650

dandavison mentioned this pull request Dec 28, 2021

Speed up the calling process query -- with sysinfo fix #872

Closed

th1000s added 5 commits January 3, 2022 21:36

Disable /proc fd caching on Linux when querying processes

767446a

This query only happens once, so caching is not needed Also update sysinfo version to fix a crash related to this.

Re-enable full process scans on Linux

ad88282

Move parent process query into a thread

cb3fb95

Start the query even before determining if information about the parent process is required (which are most invocations anyhow).

The calling process must have a common parent with delta

225af06

Ensures that no unrelated process is found when selectively refreshing a pid range.

Cache parent process when testing unless FakeParentArgs are used

91da24d

th1000s force-pushed the open_files branch from 4076ec4 to 91da24d Compare January 3, 2022 20:53

dandavison merged commit 59604a3 into dandavison:master Jan 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Speed up the calling process query #869

Speed up the calling process query #869

th1000s commented Dec 22, 2021

dandavison Dec 23, 2021

th1000s Dec 23, 2021

dandavison Dec 23, 2021 •

edited

dandavison Dec 23, 2021

th1000s Dec 23, 2021

dandavison Dec 23, 2021

th1000s commented Dec 23, 2021

GuillaumeGomez commented Dec 24, 2021

GuillaumeGomez commented Dec 25, 2021

dandavison commented Dec 28, 2021

GuillaumeGomez commented Dec 28, 2021

dandavison commented Dec 28, 2021

GuillaumeGomez commented Dec 28, 2021

th1000s commented Jan 3, 2022

dandavison commented Jan 3, 2022

dandavison commented Feb 20, 2022

Speed up the calling process query #869

Speed up the calling process query #869

Conversation

th1000s commented Dec 22, 2021

dandavison Dec 23, 2021

Choose a reason for hiding this comment

th1000s Dec 23, 2021

Choose a reason for hiding this comment

dandavison Dec 23, 2021 • edited

Choose a reason for hiding this comment

dandavison Dec 23, 2021

Choose a reason for hiding this comment

th1000s Dec 23, 2021

Choose a reason for hiding this comment

dandavison Dec 23, 2021

Choose a reason for hiding this comment

th1000s commented Dec 23, 2021

GuillaumeGomez commented Dec 24, 2021

GuillaumeGomez commented Dec 25, 2021

dandavison commented Dec 28, 2021

GuillaumeGomez commented Dec 28, 2021

dandavison commented Dec 28, 2021

GuillaumeGomez commented Dec 28, 2021

th1000s commented Jan 3, 2022

dandavison commented Jan 3, 2022

dandavison commented Feb 20, 2022

dandavison Dec 23, 2021 •

edited