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

kill autocompletion is very slow on macOS #5541

Closed
nhooyr opened this Issue Jan 17, 2019 · 14 comments

Comments

Projects
None yet
3 participants
@nhooyr
Copy link
Contributor

nhooyr commented Jan 17, 2019

I'm on

fish, version 2.7.1-1936-gf5893ba4

and macOS 10.14

(for some reason fish thinks its 2.7.1 but the commit its built from is ahead of fish 3)

If I type

kill |

And then press tab, it takes 3 full seconds before the pager shows up. I think thats way too long. Not sure whats causing this.

@nhooyr

This comment has been minimized.

Copy link
Contributor Author

nhooyr commented Jan 17, 2019

Interestingly completion for pkill is pretty much instant.

@nhooyr

This comment has been minimized.

Copy link
Contributor Author

nhooyr commented Jan 17, 2019

kill autocompletion script: https://github.com/fish-shell/fish-shell/blob/72d80c3d91bbd35bed0aafb5514c9834bb48e256/share/completions/kill.fish

The script relies on https://github.com/fish-shell/fish-shell/blob/72d80c3d91bbd35bed0aafb5514c9834bb48e256/share/functions/__fish_complete_pids.fish for the PIDs.

When I run the second script on its own, its pretty much instant so its not clear to me why the autocomplete for kill is so slow.

@nhooyr

This comment has been minimized.

Copy link
Contributor Author

nhooyr commented Jan 17, 2019

The number of PIDs returned by that second script is 600. Could the large number of options in the pager be the cause?

@nhooyr

This comment has been minimized.

Copy link
Contributor Author

nhooyr commented Jan 17, 2019

Hmm doesn't seem likely given when I autocomplete an arg to pkill, it has 400 options but is pretty much instant. I did notice that kill has special highlighting on the autocomplete. Maybe thats related.

@nhooyr

This comment has been minimized.

Copy link
Contributor Author

nhooyr commented Jan 17, 2019

kill: screen shot 2019-01-16 at 9 35 55 pm

pkill: screen shot 2019-01-16 at 9 35 35 pm

@faho faho closed this in 84339d5 Jan 17, 2019

faho added a commit that referenced this issue Jan 17, 2019

complete: Don't allow wrapping a command with itself
Double-fixes #5541, by not allowing it to happen.
@faho

This comment has been minimized.

Copy link
Member

faho commented Jan 17, 2019

So, this turned out to be an interesting little problem! The completions weren't nearly as slow for me as they apparently are for you (~200ms as opposed to 3s), but fish --profile /tmp/kill -c 'complete -C"kill "' showed weird output. In particular, it showed that __fish_complete_pids was executed 25 times. Which is weird, since it's only ever used once in the completions.

As it turns out, we define wrapper functions for kill to enable %-expansion, and the wrapper looked like:

function kill --wraps kill
    command kill (__fish_expand_pid_args $argv)
end

See that "--wraps kill" there? That caused fish to set up a wrap-chain, so that each kill would be completed, and then also completed like "kill", which in turn would be completed like itself and then also like "kill", up to 25 times, because that's apparently the maximum depth of a wrap-chain.

I've fixed that in 84339d5 by removing the "--wraps" declaration, and in 58b696b by not allowing a command to wrap itself.

(Also, the kill completions could use some love - I strongly suspect that machinery to generate the signals is complete over_kill_ (hehe), and that we might be able to just hardcode the known signals)

@faho faho added the bug label Jan 17, 2019

@faho faho added this to the fish 3.0.1 milestone Jan 17, 2019

@faho

This comment has been minimized.

Copy link
Member

faho commented Jan 17, 2019

(@zanchey: This one would be nice for 3.0.1. Taking just 84339d5 is sufficient)

@nhooyr

This comment has been minimized.

Copy link
Contributor Author

nhooyr commented Jan 17, 2019

@faho nice debugging.

(~200ms as opposed to 3s)

Why would there be such a big difference for us? I'm not on a slow machine, I've got a 4.2 GHz Intel Core i7-7700K, an NVMe SSD and 40 GB of ram.

@faho

This comment has been minimized.

Copy link
Member

faho commented Jan 17, 2019

Why would there be such a big difference for us? I'm not on a slow machine

I'm on linux.

faho added a commit that referenced this issue Jan 17, 2019

Test that things can't wrap themselves
This is a test belonging to the previous commit, 58b696b.

See #5541.
@nhooyr

This comment has been minimized.

Copy link
Contributor Author

nhooyr commented Jan 17, 2019

I'm on linux.

That is true but a 15x difference is huge. It'd be interesting to see what is causing the huge difference.

@nhooyr

This comment has been minimized.

Copy link
Contributor Author

nhooyr commented Jan 17, 2019

(btw I just installed and can confirm this fixed it for me)

@faho

This comment has been minimized.

Copy link
Member

faho commented Jan 17, 2019

That is true but a 15x difference is huge. It'd be interesting to see what is causing the huge difference.

The main thing that __fish_complete_pids does is execute ps axc -o pid,ucomm,tty. So you'd want to profile that.

One run of it gives me ~180 processes, and takes ~8ms (echo $CMD_DURATION).

It's not all that surprising. Linux is really fricking fast at starting processes! I've had to use a windows (7) machine recently, and git is dog slow there, because it relies heavily on starting processes.

Plus ps integrates rather closely with the system (its main thing to do is to ask the kernel for information), and that's something that can be faster or slower depending on choices made in kernel design.

Either that, or you have an older __fish_complete_pids lying around somewhere. Check type __fish_complete_pids.

@nhooyr

This comment has been minimized.

Copy link
Contributor Author

nhooyr commented Jan 17, 2019

It takes 35 ms for me, which means about 875ms total. I guess maybe I exaggerated the time and it felt like it took longer cause autocomplete is usually so fast.

edit: __fish_complete_pids is the latest version.

@ridiculousfish

This comment has been minimized.

Copy link
Member

ridiculousfish commented Jan 22, 2019

Picked into 3.0.1 as 97f0cc9

ridiculousfish added a commit that referenced this issue Jan 22, 2019

Don't wrap functions with themselves
Our weird %-expanding function wrappers around kill et all defined
"--wraps" for the same name.

As it turns out, fish follows that one, and executes the completion
multiple times.

I didn't notice because these tend to be rather quick on linux, but on
macOS that's apparently a real issue.

Fixes #5541.

[ci skip]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.