Skip to content

Do not scan through all PIDs to find parent PID command #817

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Do not scan through all PIDs to find parent PID command #817

wants to merge 1 commit into from

Conversation

@Fil0sOFF
Copy link

@Fil0sOFF Fil0sOFF commented Dec 19, 2023 •
edited
Loading

There's one command that takes a long time after profiling bash startup.
We currently scan though all processes just to find out $PPID command.
The time to do it grows linearly with a number of processes running.
Let's fix that.

Compare:

strace -e openat ps -o comm= -p $PPID

vs

strace -e openat ps -o comm= -q $PPID

Technical checklist:

  • code follows our shell standards:
    • correct use of IFS
    • careful quoting
    • cautious array access
    • portable array indexing with _LP_FIRST_INDEX
    • printing a "%" character is done with _LP_PERCENT
    • functions/variable naming conventions
    • functions have local variables
    • data functions have optimization guards (early exits)
    • subshells are avoided as much as possible
    • string substitutions may be done differently in Bash and Zsh (use _LP_SHELL_*)
  • tests and checks have been added, ran, and their warnings fixed:
    • unit tests have been updated (see tests/test_*.sh files)
    • ran tests.sh
    • ran shellcheck.sh (requires shellcheck).
  • documentation have been updated accordingly:
    • functions and attributes are documented in alphabetical order
    • new sections are documented in the default theme
    • tag .. versionadded:: X.Y or .. versionchanged:: Y.Z
    • functions signatures have arguments, returned code, and set value(s)
    • attributes have types and defaults
    • ran docs/docs-lint.sh (requires Python 3 and requirements.txt
      installed (cd docs/; python3 -m venv venv; . venv/bin/activate; pip install -r requirements.txt))

Rycieos
Rycieos requested changes Dec 19, 2023
View reviewed changes
Copy link
Collaborator

@Rycieos Rycieos left a comment

Choose a reason for hiding this comment

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

ps does not support the -q flag on MacOS. This can't be merged as is.

Can you post some timing info (time ps -o comm= -p "$PPID") from the situations that you think this is slow?

If this is really a problem, we could try to have two different paths for those that support the -q flag vs those that don't.

@Fil0sOFF
Copy link
Author

Fil0sOFF commented Dec 19, 2023

Oops, -q seems to be available on only on linux, since 2014

@Rycieos
Copy link
Collaborator

Rycieos commented Dec 19, 2023

I'm probably missing something, but why was a separate option added? If -q only is allowed when no other filters are specified, why not just use the optimized search when -p is specified but nothing else is.

@Fil0sOFF
Copy link
Author

Fil0sOFF commented Dec 19, 2023

I'm not sure why ps works that way. I only found about -q from a manpage, which i opened after strace ps -o comm= -p $PPID showed a long list of /proc/xx files.

-p is not that slow most of the time, but it can be in some cases.

$ time ps -o comm= -p "$PPID"
wezterm-gui

real    0m0.054s
user    0m0.026s
sys     0m0.031s


$ time ps -o comm= -q "$PPID"
wezterm-gui

real    0m0.012s
user    0m0.012s
sys     0m0.002s

after running 1000 sleeps:

$ for i in `seq 1000`; do sleep 30 & done

$ time ps -o comm= -p "$PPID"
wezterm-gui

real    0m0.104s
user    0m0.058s
sys     0m0.048s

$ time ps -o comm= -q "$PPID"
wezterm-gui

real    0m0.012s
user    0m0.011s
sys     0m0.003s

If different code paths for different systems are possible, then var=$(<file) might be the fastest option for linux. No forking, no external command execution. Bash- and zsh-compatible

@Rycieos
Copy link
Collaborator

Rycieos commented Dec 19, 2023

If different code paths for different systems are possible, then var=$(<file) might be the fastest option for linux. No forking, no external command execution. Bash- and zsh-compatible

What file are you proposing to read? /proc/$PPID/comm? That should work, and I agree, it would be faster.

If you want to try to implement it, here are a few tips:

  • Read with a command like IFS="" read -r sess_parent <"$_LP_LINUX_COMM_FILE". Make sure to set local sess_parent first.
  • Use the var for the file path to allow for mocking it in tests. See

    liquidprompt/liquidprompt

    Lines 608 to 609 in 046d830

    # For mocking tests.
    _LP_LINUX_POWERSUPPLY_PATH="/sys/class/power_supply"
    for where those are defined.
  • Likely split line 1644 into a function, like __lp_sess_parent() { sess_parent="$(ps -o comm= -p "$PPID" 2> /dev/null)"; }. Then you can put the above in a different function.
  • Don't define both functions, define one based on the OS at load time. See

    liquidprompt/liquidprompt

    Lines 3589 to 3591 in 046d830

    case "$LP_OS" in
    Linux)
    __lp_wifi_signal_strength_raw() {
    for an example. Set the local before you call that function.
  • Make sure to add reading from this file to the external tool tester, even though it likely won't be very useful.

jollaitbot pushed a commit to sailfishos-mirror/procps that referenced this pull request Nov 27, 2024
@cgzones
ps: use quick mode if possible
7f80362 
Since the quick mode option -q is not portable, portable scripts cannot
use it.  Tweak the behavior of -p to use the quick mode if no
conflicting options are specified.

Inspired by liquidprompt/liquidprompt#817
@cgzones
Copy link

cgzones commented Nov 29, 2024

This approach might be obsolete with the next version of ps, see https://gitlab.com/procps-ng/procps/-/commit/7f80362b9ff8cb2b699a01e06a70972a4ae9399f

@Rycieos
Copy link
Collaborator

Rycieos commented Dec 1, 2024

Nice! Thanks for making that PR, and then bringing it to our attention. Hopefully it will propagate to distros soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Rycieos Rycieos Rycieos requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Fil0sOFF @Rycieos @cgzones