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

Performance improvements #44

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Performance improvements #44

wants to merge 2 commits into from

Conversation

0xabu
Copy link

@0xabu 0xabu commented Jul 28, 2017

This PR improves performance, especially when we're not in a git repo, by cutting down the number of forks/execs needed just to render the prompt. It's a particularly noticeable speedup on Cygwin.

This saves multiple fork/execs (for the subshell and wc) each
time the prompt is rendered
Rather than forking a subshell to print the prompt, construct it
incrementally by appending to $PROMPT. To enable this, expand the
unicode escapes ahead of time.
@belak
Copy link
Contributor

belak commented Nov 24, 2017

While this may improve performance, I don't think this will drastically reduce the number of forks. print is a zsh builtin and shouldn't cause a fork. Additionally, any call to a zsh function shouldn't cause a fork.

There's also a large chance this adds back the exploit (https://github.com/njhartwell/pw3nage) which was fixed in #33.

The jobstates fix does seem like it might help though.

@0xabu
Copy link
Author

0xabu commented Nov 24, 2017

@belak Hmm, I think you're right about the exploit being re-introduced; I hadn't noticed that when I did this work. But what about the other change (897b31f) to use $jobstates rather than $(jobs | wc) -- that's two forks avoided (one for the subshell, one for wc), and you could take it independently of the other commit.

@belak
Copy link
Contributor

belak commented Nov 24, 2017

Unfortunately I don't have merge access to this repo... I'm just fairly familiar with the code.

The first commit looks fine to me though. :)

@@ -117,7 +119,7 @@ prompt_status() {
symbols=()
[[ $RETVAL -ne 0 ]] && symbols+="%{%F{red}%}$CROSS"
[[ $UID -eq 0 ]] && symbols+="%{%F{yellow}%}$LIGHTNING"
[[ $(jobs -l | wc -l) -gt 0 ]] && symbols+="%{%F{cyan}%}$GEAR"
[[ ${#jobstates} -ne 0 ]] && symbols+="%{%F{cyan}%}$GEAR"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note: this conflicts with the nohup filtering proposed in #61.

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

Successfully merging this pull request may close these issues.

3 participants