Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Query fewer processes when searching for the parent #841

Merged
merged 1 commit into from
Dec 11, 2021

Conversation

th1000s
Copy link
Collaborator

@th1000s th1000s commented Dec 8, 2021

This query now happens for more invocation types, so speed it up.
Call refresh_process() only on pids numerically close to the one
of delta itself.


Follow-up to #824 (comment):

This now checks the pid range of $delta_pid - 10 / + 20 when looking at the parent and predicted sibling finds nothing.

@infokiller @jebaum @ttys3

Could you apply the patch (3) from #824 (comment) and run git log -p | time delta on top of this and see if the slowdown is gone and the calling git process is still found?

Also, is this even required on macOS or is querying the entire process structure less expensive there?

And was the slowdown ever observed when delta was configured as the pager in the git config and git show (no pipe to delta) was called directly? It really should not, because the parent process should always be found.

This query now happens for more invocation types, so speed it up.
Call `refresh_process()` only on pids numerically close to the one
of delta itself.
@dandavison
Copy link
Owner

dandavison commented Dec 9, 2021

Here's my understanding of the situation: evidence has been put forward for two distinct causes of slowness:

  1. Querying many processes
    A mitigation for this has been released (Disable last-resort process tree inspection #838) which is drastic: it entirely removed the call to find_sibling_process. @infokiller found that this solved the slowness. This PR should help with this cause of slowness in that, while it will not make the released version faster, it will make it better at finding processes.

  2. Querying CPU data (linux only?)
    @ttys3 has found that delta is still unacceptably slow, even with Disable last-resort process tree inspection #838, because of the way that sysinfo is querying for CPU data. This PR will not improve that cause of slowness. It may make it worse because of more calls to the sysinfo library? I haven't looked into details there.

Is that summary in line with your understanding @th1000s and others?

If that summary is accurate, should we avoid releasing this change until we have solved cause (2)?

@ttys3
Copy link

ttys3 commented Dec 9, 2021

@th1000s
I've made a simple demo to re-produce the Linux slowdown problem

see #839 (comment)

@dandavison
Copy link
Owner

There's some very helpful information from @ttys3 about the CPU-querying performance problems here: #839 (comment)

so for each core, get_cpu_frequency() call cost about 15ms, if you CPU core is less than 10, this problem is not easy to found, because you are not very obvious to feel the delay gap.

basically, I think this affects all Linux users.

#[cfg(test)]
{
info.refresh_processes();
info.find_sibling_in_refreshed_processes(my_pid, &extract_args)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of interest can you explain why we make this recursive call in the tests? (Not blocking merge as this is a test-only code path).

@dandavison dandavison marked this pull request as ready for review December 11, 2021 19:47
@dandavison
Copy link
Owner

Thanks @th1000s! I'm merging this now (I took it out of Draft state -- hope that was ok) since both @ttys3 and @unphased have reported good performance with a branch containing this commit, in addition to the no-cpu-querying sysinfo commit. To confirm that this branch improves performance when there are many processes I've run it with @infokiller's script:

Benchmarking version: 0.10.2
Running command "git diff --no-index a b | delta-0.10.2 >| delta-0.10.2-result" 10 times
101.60 ms per run (1016 ms total)
Benchmarking version: 0.11.0
Running command "git diff --no-index a b | delta-0.11.0 >| delta-0.11.0-result" 10 times
509.40 ms per run (5094 ms total)
Benchmarking version: 0.11.2
Running command "git diff --no-index a b | delta-0.11.2 >| delta-0.11.2-result" 10 times
108.00 ms per run (1080 ms total)
Benchmarking version: procs
Running command "git diff --no-index a b | delta-procs >| delta-procs-result" 10 times
100.70 ms per run (1007 ms total)

Meanwhile, I can see that this commit improves calling-process-finding at least in the following experiment:

  1. Set core.pager=delta.sh where delta.sh contains bash -c delta
  2. Observe that git grep output is handled correctly with this commit, and incorrectly with this commit reverted.

@dandavison dandavison merged commit 5c1612e into dandavison:master Dec 11, 2021
@th1000s
Copy link
Collaborator Author

th1000s commented Dec 12, 2021

Sure, I just wanted to make sure this got tested first, hence the draft :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants