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

Update datafile in background for speed #191

Closed
guoguo12 opened this issue Jul 9, 2016 · 30 comments
Closed

Update datafile in background for speed #191

guoguo12 opened this issue Jul 9, 2016 · 30 comments

Comments

@guoguo12
Copy link

guoguo12 commented Jul 9, 2016

The default $PROMPT_COMMAND is

$ echo $PROMPT_COMMAND
_z --add "$(command pwd -P 2>/dev/null)" 2>/dev/null

For me (on Cygwin, Windows 8.1), the default configuration makes it so that if I hold down the Enter key in Bash, the prompts start to lag behind. Overall, the prompt feels slow.

So I changed it to

$ echo $PROMPT_COMMAND
(_z --add "$(command pwd -P 2>/dev/null)" 2>/dev/null &)

This seems to mostly fix the issue. The z datafile is updated in the background, silently.

Is there any harm in doing this or reason why this isn't the default?

@ninrod
Copy link

ninrod commented Jul 13, 2016

interested in this too. I removed z from my setup because it seemed to lag my prompt by a large margin.

@guoguo12
Copy link
Author

Glad I'm not the only one! Did you try my fix? (I modified this line, but I believe you could also set $_Z_NO_PROMPT_COMMAND and then set $PROMPT_COMMAND.)

@ninrod
Copy link

ninrod commented Jul 13, 2016

@guoguo12 is this the right way to implement your proposed modification?

The lag remains the same with the above optimization. You can still see a signifcant hit in performance in terms of getting another prompt just by installing z.

@guoguo12
Copy link
Author

That looks about right. Does it work?

Allen
On Jul 12, 2016 7:13 PM, "Filipe Silva" notifications@github.com wrote:

@guoguo12 https://github.com/guoguo12 is this
https://github.com/ninrod/z/commit/a14affb6b90f845f9600ae0b656acd347b3b1472#diff-020ace7637fa3870bdb6af242b3fac53L244
the right way to implement your proposed modification?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#191 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AB2c-QmGD5mZuw5fA-RYHfqmYtMFmGOnks5qVEnXgaJpZM4JIh1G
.

@ninrod
Copy link

ninrod commented Jul 13, 2016

No. it remains lagging my prompt.

@guoguo12
Copy link
Author

I actually have

PROMPT_COMMAND="$PROMPT_COMMAND"$'\n''(_z --add "$(command pwd '$_Z_RESOLVE_SYMLINKS' 2>/dev/null)" 2>/dev/null &)'

Try that?

@ninrod
Copy link

ninrod commented Jul 13, 2016

then i get: _z_dirs:1: no such file or directory: /Users/ninrod/.z

but afterwards, it kind of works, but it is laggy all the same.

@guoguo12
Copy link
Author

Interesting. Well... mine definitely runs faster now. The difference seems to be about 0.1 seconds:

$ time _z --add /cygdrive/c/Users/guogu_000

real    0m0.103s
user    0m0.030s
sys     0m0.060s

$ time (_z --add /cygdrive/c/Users/guogu_000 &)

real    0m0.006s
user    0m0.000s
sys     0m0.000s

@ninrod
Copy link

ninrod commented Jul 13, 2016

hum are you under cygwin?

@guoguo12
Copy link
Author

Yeah, but that shouldn't make a difference because it's still Bash.

@ninrod
Copy link

ninrod commented Jul 14, 2016

I'm in zsh on osx.

@rupa
Copy link
Owner

rupa commented Jul 14, 2016

are you folks playing with this change seeing:

  • lots of .z. files accumulating in ~/
  • inaccurate counts in ~/.z (hard to test) (from temp files getting clobbered)

not sure how much i really care about accurate counts, but also i don't care a lot about prompt speed - the speed's never mattered to me when i'm actually working. dangling temp files are more irritating and I don't want them getting more common.

@guoguo12
Copy link
Author

@ninrod: I believe you'll want to edit this line to get it to work in zsh.

@rupa: Hey, thanks for chiming in. (BTW, I really appreciate this project!) To answer your questions:

  • I see 19 .z* files in my home directory right now, but only three have last-modified dates from after when I made the tweak. Does that count as a lot?
  • Not sure about inaccurate counts. Why would forking mess up the counts?

I agree the speed difference isn't that noticeable. (Although I imagine it's worse on Cygwin than Linux/Mac.) But if there aren't any negative side effects, I think it's a worthwhile improvement.

@ninrod
Copy link

ninrod commented Jul 14, 2016

Hi @rupa. Well I do care for prompt speed. Without your plugin, my prompt displays as fast as if I had invoked zsh with zsh -f. With your plugin loaded, I can actually see my computer trying to draw the prompt letter by letter. It's fast, but those 120ms are enough to bother me.

Now, why must z do something every time that a prompt is redrawn? couldn't z only do it's thing when a user cd into a directory? What z does every time that a prompt is redrawn? Does z use some kind of zsh hook for that?

@balta2ar
Copy link

Sorry, @rupa, but the difference with and without z in my configuration is very noticeable. I reported this issue a while ago in another source of slowness of my configuration, zsh-autosuggestions: zsh-users/zsh-autosuggestions#136 (comment)

Unfortunately, both autosuggestions and z are very important in my daily workflow (especially z, can't live without it). If you just cd across your directories, you may not even notice the delay. However, if you start using MidnightCommander, things get horribly slow. This is maybe due to the fact that MC sends cd command to the underlying shell character by character. I created an issue about that: https://github.com/MidnightCommander/mc/issues/101. I have little hopes this will ever be fixed on their side, though.

I also tried hacking MC and surrounded the cd command with zsh's bracketed paste magic to avoid triggering zsh hooks: balta2ar/mc@f17ebb8. It didn't really help. Maybe I'm missing something.

Besides of the suggested fix (which I haven't tried yet, but I will), would you be willing to consider asyncronous extension similar to this pull request, @rupa? zsh-users/zsh-autosuggestions#134

@rupa
Copy link
Owner

rupa commented Jul 20, 2016

I've pushed an async branch containing this change. I'll run it for a few days and if everything seems basically fine, I'll merge to master. Would appreciate any reports from others:

  • faster?
  • increase in ~/.z.XXXX garbage
  • counts off
  • anything else

@balta2ar
Copy link

balta2ar commented Jul 20, 2016

I've just noticed that suggested change is for bash only. I did the same change in the corresponding zsh if branch above (two places). I've finally managed to conduct an experiment and obtain actual numbers.

Maybe this is not really strict and rigorous, but it visualizes the difference. First, I created a nested directory structure (110 dirs) with this code:

$ for n in $(seq 110); do DN="${DN}/notsolongdirnametotestnested_$n"; done; mkdir -p "./${DN}"

Next I wrote an automation script that runs mc with time command, traverses 30 of these directories and exits. time prints seconds it took to enter these 30 dirs. Here is the script:

#!/bin/bash

# To find out the window id use this command:
# $ xdotool selectwindow
WINDOW_ID=$1

echo "Starting test"

xdotool type --window $WINDOW_ID "time mc"
xdotool key --window $WINDOW_ID Return
# Without this keys that follow are not processed correctly
sleep 1

COMMAND=""
for i in $(seq 30); do
    COMMAND="$COMMAND Down Return"
done

xdotool key --window $WINDOW_ID $COMMAND ctrl+o
xdotool type --window $WINDOW_ID "exit"
xdotool key --window $WINDOW_ID Return

echo "Test has been finished"

Next I ran 5 different set ups 5 times each. Each setup was run in a new terminal tab. In setups I tried different combinations of zsh-autosuggestions and z. Setups are:

no_z_no_zus — z and zsh-autosuggestions are disabled
with_z_with_zus — z async and zsh-autosuggestions are enabled
only_z_async — only z async is enabled
only_z_sync — only z sync is enabled
only_zus — only zsh-autosuggestions is enabled

No no_z_no_zus with_z_with_zus only_z_async only_z_sync only_zus
0 4.446000 8.720000 4.39200 4.862000 8.67900
1 4.447000 8.646000 4.37500 4.959000 8.58700
2 4.455000 8.867000 4.39300 4.993000 8.76600
3 4.415000 8.882000 4.38200 4.829000 8.54600
4 4.536000 8.697000 4.48900 4.892000 8.41800
mean 4.459800 8.762400 4.40620 4.907000 8.59920
std 0.045252 0.105912 0.04688 0.067886 0.13229

As you can see from only_z_async vs only_z_sync it's 4.4 vs 4.9 (I don't know why only_z_async is faster than no_z_no_zus). Unfortunately, in my configuration zus shadows z to a very big extent, but I still think it's worth it.

No new ~/.z.XXXX garbage appeared after these tests.

@rupa
Copy link
Owner

rupa commented Jul 20, 2016

@balta2ar woops - can you post the changes you made for zsh support? I'm not great at zsh stuff.

@ninrod
Copy link

ninrod commented Jul 20, 2016

So almost half a second difference from zsync to noz_nozus. seems about what I get from my end.

@balta2ar
Copy link

@rupa The idea of changes is exactly the same: offload --add to background process:

diff --git a/z.sh b/z.sh
index 197c536..b9638b5 100644
--- a/z.sh
+++ b/z.sh
@@ -216,11 +216,11 @@ if type compctl >/dev/null 2>&1; then
         # populate directory list, avoid clobbering any other precmds.
         if [ "$_Z_NO_RESOLVE_SYMLINKS" ]; then
             _z_precmd() {
-                _z --add "${PWD:a}"
+                (_z --add "${PWD:a}" &)
             }
         else
             _z_precmd() {
-                _z --add "${PWD:A}"
+                (_z --add "${PWD:A}" &)
             }
         fi
         [[ -n "${precmd_functions[(r)_z_precmd]}" ]] || {

@ninrod Are you sure? The difference that I have demonstrated is a difference of cumulative traversal of 30 directories in a row conducted specifically for the experiment to amplify the effect. That's absolutely unrealistic case. What's your set up? Please don't get me wrong, I'm not denying your delays, — I have them too, — I just I don't want to mislead anyone comparing apples to oranges.

@ninrod
Copy link

ninrod commented Jul 20, 2016

Yeah: 500/30 = 16ms.

i get something like 30-50ms delta difference just listing directories with rupa/z turned on. but my setup is heavier. I'll probably have to make a more correct benchmark like yours though.

@ninrod
Copy link

ninrod commented Jul 20, 2016

@rupa. please protect cd against user alias shenanigans.

I've actually defined a function called cd here that uses your plugin which in turn uses cd alias, which in turn uses z, which in turn uses... well, I've got a segmentation fault.

So instead of using plain cd please use builtin cd instead to protect against alias and name shadowing shenanigans.

@rupa
Copy link
Owner

rupa commented Jul 21, 2016

good point. done.

@ninrod
Copy link

ninrod commented Jul 21, 2016

Thanks @rupa. but maybe that is not as simple as that?

I'm in zsh and I'm not getting core dumped anymore, but I'm getting this after your last commit:

~cd code
_z:175: command not found: builtin cd
~builtin cd code
code ➜

here's the full function I'm using:

cd () {
  if [[ -z $1 ]]; then
    # $1 is empty. go home
    builtin cd ~
  elif [[ $1 == '-' ]]; then
    # $1 == '-': switch to last visited dir
    builtin cd - > /dev/null
  elif [[ $1 == '..' ]]; then
    builtin cd ..
  elif [[ $1 =~ '\+[0-9]{1,2}' ]]; then
    # $1: `cd +8`, `cd +10`, cherry pick auto_pushd stack
    builtin cd $1 > /dev/null
  elif command -v z > /dev/null; then
    # careful now. z should use a `builtin cd`. If not we're screwed with an infinite loop.
    z $1
  else
    # rupa/z is not available, use normal cd
    builtin cd $1
  fi
}

@ninrod
Copy link

ninrod commented Jul 21, 2016

I think you're missing an eval in those lines. especially here, I think

something like [ $? -eq 0 ] && [ "$cd" ] && eval "${echo:-builtin cd} $cd"

@rupa
Copy link
Owner

rupa commented Jul 21, 2016

It's this BS http://zsh.sourceforge.net/FAQ/zshfaq03.html (first question)

not gonna use eval, luckily I can fix it by removing some uselessly clever nonsense.

@ninrod
Copy link

ninrod commented Jul 21, 2016

nice one @rupa. so what happens when user has never ever had a .z file in his home directory? I'm getting screwed by that. I suppose we should check if a ~/.z exists and if not, create one?

steps to reproduce: I've deleted ~/.z to get a fresh start. after z some/valid/dir I get shell return code 1.

touch ~/.z does not seem to satisfy z. Maybe ~/.z has to be well formed?

@molovo
Copy link

molovo commented Aug 24, 2016

Checked out the async branch, haven't run any benchmarks but it looks like a great improvement. Would love to see it merged

@rupa
Copy link
Owner

rupa commented Sep 30, 2016

merged into master. sorry I waited so long, slow and steady :)

@rupa rupa closed this as completed Sep 30, 2016
@guoguo12
Copy link
Author

Cool, thanks!

Flowm added a commit to Flowm/dotfiles that referenced this issue Feb 5, 2017
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

No branches or pull requests

5 participants