-
Notifications
You must be signed in to change notification settings - Fork 405
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
Conversation
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.
Here's my understanding of the situation: evidence has been put forward for two distinct causes of slowness:
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)? |
@th1000s see #839 (comment) |
There's some very helpful information from @ttys3 about the CPU-querying performance problems here: #839 (comment)
|
#[cfg(test)] | ||
{ | ||
info.refresh_processes(); | ||
info.find_sibling_in_refreshed_processes(my_pid, &extract_args) |
There was a problem hiding this comment.
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).
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
Meanwhile, I can see that this commit improves calling-process-finding at least in the following experiment:
|
Sure, I just wanted to make sure this got tested first, hence the draft :) |
This query now happens for more invocation types, so speed it up.
Call
refresh_process()
only on pids numerically close to the oneof 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.