Skip to content

Fix zsh shell hook performance issue #456

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

Closed
wants to merge 2 commits into from
Closed

Fix zsh shell hook performance issue #456

wants to merge 2 commits into from

Conversation

reegnz
Copy link

@reegnz reegnz commented Apr 11, 2023

The precmd hook is causing a significant overhead on every command executed in the shell.
The shell hook does not need to be this strict however, it can also run only on directory changes, ensuring that the shell remains snappy while executing commands.

Also contains a similar fix for fish.

Fixes #259.

The precmd hook is causing a significant overhead on every command
executed in the shell.
The shell hook does not need to be this strict however, it can also
run only on directory changes, ensuring that the shell remains snappy
while executing commands.

Also contains a similar fix for fish.

Fixes #259.
@furudean
Copy link

furudean commented Apr 11, 2023

Is checking for $PWD in fish sufficient? What if the virtualenv changes without you changing directory?

What do you think about the (second part of the) workaround outlined in #259 (comment)?

@reegnz
Copy link
Author

reegnz commented Apr 11, 2023

What if the virtualenv changes without you changing directory?

As a zsh poweruser I can safely state that the changing venv is a way too infrequent edge-case to sacrifice shell performance for. In case you'd still run into that, you can cd out from the dir and back again. People seem to be OK with not handling that edge-case but re-gaining shell performance.

TBQH I'm not a fish user, so I might remove the fish fix and let someone with fish experience fix that one.

Gonna look into the --no-rehash solution.
EDIT: nope, the --no-rehash solution does not eliminate the lag that comes from forking python process on every shell command.

@native-api
Copy link
Member

As a zsh poweruser I can safely state that the changing venv is a way too infrequent edge-case to sacrifice shell performance for.

That's actually the sole job of pyenv local.

@native-api
Copy link
Member

Currently, the shell cmd

As a zsh poweruser I can safely state that the changing venv is a way too infrequent edge-case to sacrifice shell performance for.

That's actually the sole job of pyenv local.

So by not rechecking after an invocation, you single-handedly break this entire command.

@reegnz
Copy link
Author

reegnz commented Apr 11, 2023

@native-api interesting, because when I use pyenv local 3.11.2 it immediately activates the pyenv for me using precwd, it's not broken. I wonder why that is, because I'd also expect it to break.

@native-api
Copy link
Member

@native-api interesting, because when I use pyenv local 3.11.2 it immediately activates the pyenv for me using precwd, it's not broken. I wonder why that is, because I'd also expect it to break.

I suggest tracing. I'm also very curious.

@native-api
Copy link
Member

The current hook is rather dumb -- it just activates the selected environment blindly.

AFAICS, the most productive way, instead of trying to dodge executing it altogether and hope no-one notices, would be to make it more intelligent and not do any unnecessary work.

@furudean
Copy link

furudean commented Apr 11, 2023

TBQH I'm not a fish user, so I might remove the fish fix and let someone with fish experience fix that one.

I would really dislike the behavior as implemented for fish shell. It seems very unintuitive. Maybe let's settle for just the zsh behavior for this PR. I wouldn't mind championing this as a different issue.

@furudean
Copy link

furudean commented Apr 11, 2023

See also #451 and #338

Should be separate PR to deal with fish perf issue
@reegnz
Copy link
Author

reegnz commented Apr 12, 2023

Removed the fish parts.

Gonna spend some additional time to figure out how to best deal with the zsh precmd use-case. I checked yesterday and for me the hook in general doesn't seem to set VIRTUALENV_DIR and other env vars (even without these changes). Maybe it's set up incorrectly for me. (Still have to understand a bit more what exactly the env vars bring to the table compared to the pyenv shims).

@native-api
Copy link
Member

We aren't merging anything that breaks things. Or we'll be swamped in bug reports that things no longer work and will have to fix that pronto in addition to reputational damages.

@native-api native-api closed this Apr 16, 2023
@reegnz reegnz deleted the fix_shell_hook_perf_issue branch April 20, 2023 15:32
@reegnz
Copy link
Author

reegnz commented Apr 20, 2023

Do you have any intention of fixing the highest voted issue on your project?
Closing the PR as a twitching reflex is not constructive. I would have been open to doing this in a backward-compatible manner given some feedback. You just lost a contributor.

@native-api
Copy link
Member

PRs can be reopened. And it seems to me that feedback is exactly what I gave in the closing message.

I closed it rather than just request changes because the way it is now, it looks like it has to be completely different to actually fix the issue -- so there's little point in keeping it in this topic, you'll have to start a new branch from scratch anyway.

@native-api
Copy link
Member

native-api commented Apr 20, 2023

...So I honestly don't see what the big deal is here.

Sorry if I came as haughty/condescending/annoyed, I had no intention to insult anyone.
I am annoyed because people offering flawed solutions are kinda wasting everyone's time and giving other people false hope ("Oh, there already is a fix in the works! So I don't have to look into this myself!").

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.

Slow shell performance after running pyenv virtualenv-init
3 participants