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

Calling process: Query more parents, disable full scan #984

Merged
merged 2 commits into from
Mar 1, 2022

Conversation

th1000s
Copy link
Collaborator

@th1000s th1000s commented Feb 25, 2022

Also look at grandparent process when determining the caller

Disable full process scans: The env var DELTA_CALLING_PROCESS_QUERY_ALL re-enables this
last resort method. However usually this is just an expensive scan which doesn't find the caller anyhow.

@th1000s
Copy link
Collaborator Author

th1000s commented Feb 25, 2022

The full process scan could still be done on macOS and Windows, I don't think listing all processes requires thousands of syscalls there.

dandavison added a commit that referenced this pull request Feb 25, 2022
@dandavison
Copy link
Owner

I've updated @infokiller's docker script to benchmark this branch against 0.11.3 and 0.12.0 (see #977) and the results look good for this branch:

Benchmarking version: 0.11.3
delta 0.11.3
Running command "git diff --no-index a b | delta-0.11.3 >| delta-0.11.3-result" 10 times
61.40 ms per run (614 ms total)
Benchmarking version: 0.12.0
delta 0.12.0
Running command "git diff --no-index a b | delta-0.12.0 >| delta-0.12.0-result" 10 times
59.70 ms per run (597 ms total)
Benchmarking version: dev
delta 0.12.0
Running command "git diff --no-index a b | delta-dev >| delta-dev-result" 10 times
58.30 ms per run (583 ms total)
Spinning up 5000 processes
Process counts:
      1 bash
      1 ps
   5000 sleep
      1 sort
      1 uniq
Benchmarking version: 0.11.3
delta 0.11.3
Running command "git diff --no-index a b | delta-0.11.3 >| delta-0.11.3-result" 10 times
155.20 ms per run (1552 ms total)
Benchmarking version: 0.12.0
delta 0.12.0
Running command "git diff --no-index a b | delta-0.12.0 >| delta-0.12.0-result" 10 times
345.90 ms per run (3459 ms total)
Benchmarking version: dev
delta 0.12.0
Running command "git diff --no-index a b | delta-dev >| delta-dev-result" 10 times
68.80 ms per run (688 ms total)

info.find_sibling_in_refreshed_processes(my_pid, &extract_args)
// The full scan is expensive and rarely successful, so disable it by default
if std::env::vars()
.any(|(k, v)| k == "DELTA_CALLING_PROCESS_QUERY_ALL" && v != "0" && v != "false")
Copy link
Owner

Choose a reason for hiding this comment

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

Better to use std::env::var here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just never bothered to look up the equivalent for the var() call, which is .map_or(false, ...), until now :)

And I think the underlying getenv() has to walk the environ list just the same.

@dandavison
Copy link
Owner

dandavison commented Feb 25, 2022

Looks great @th1000s! One nitpick (and one clippy complaint).

@dandavison
Copy link
Owner

dandavison commented Feb 27, 2022

Sorry the clippy error was nothing to do with this PR; I've rebased on master to get rid of it.

I am curious whether you prefer std::env::vars over the "get" API std::env::var(name) (two places in process.rs)

@dandavison
Copy link
Owner

Also, this PR is in Draft status: could you let me know whether it's ready to go out?

(I'm quite keen to make a release soon with the new bat assets, and would like to include this)

The env var DELTA_CALLING_PROCESS_QUERY_ALL re-enables this
last resort method. However usually this is just an expensive
scan which doesn't find the caller anyhow.
@th1000s th1000s marked this pull request as ready for review March 1, 2022 23:03
@th1000s
Copy link
Collaborator Author

th1000s commented Mar 1, 2022

Because I can't test the behavior elsewhere I have only disabled the full scan on Linux now. Other OSes probably have faster syscalls for this query anyhow.

clippy

The 1.58 clippy wants to undo what the (I presume) 1.59 one suggested!

@dandavison dandavison merged commit d13c41c into dandavison:master Mar 1, 2022
@dandavison
Copy link
Owner

Thanks very much @th1000s!

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.

2 participants