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

Use native walker since output of DOS command is not UTF-8 encoded (Makes fzf 300x faster on Windows) #1847

Merged
merged 5 commits into from Feb 4, 2020

Conversation

mattn
Copy link
Contributor

@mattn mattn commented Feb 3, 2020

Current implementation of walker on Windows use MSDOS command. But the output of the command is not UTF-8 encoded. Also heavily slow.

image

This pull-request delete it and add implementation of native walker. This works 300 times faster on Windows on my environment.

@mattn
Copy link
Contributor Author

mattn commented Feb 3, 2020

old
terminal7

new
terminal7

@mattn
Copy link
Contributor Author

mattn commented Feb 3, 2020

This implementation ignore hidden files. @junegunn If you don't like, I fix this to show all.

src/reader.go Show resolved Hide resolved
@junegunn
Copy link
Owner

junegunn commented Feb 3, 2020

Would it be possible to make the walker loop terminate early on reload action? Will it make it much slower?

FZF_DEFAULT_COMMAND= fzf --bind 'space:reload:echo foo'

@mattn
Copy link
Contributor Author

mattn commented Feb 4, 2020

Added canceling

@junegunn
Copy link
Owner

junegunn commented Feb 4, 2020

Thanks, do you think it's okay to read r.killed without locking the mutex? Can we ignore complaints from GORACE in this context?

@mattn
Copy link
Contributor Author

mattn commented Feb 4, 2020

Done

@junegunn
Copy link
Owner

junegunn commented Feb 4, 2020

Do you notice any slowdown because of the locking?

@mattn
Copy link
Contributor Author

mattn commented Feb 4, 2020

Should I modify title of this issue to "250x times faster" ?

(FYI, I can't notice which version is faster)

@junegunn
Copy link
Owner

junegunn commented Feb 4, 2020

😲

So not much difference? I was just wondering how much performance impact it would make, though I was pretty sure that it's not going to dwarf the initial performance improvement. Anyway, thanks, I'll merge this now.

@junegunn junegunn merged commit 311b78a into junegunn:master Feb 4, 2020
@mattn
Copy link
Contributor Author

mattn commented Feb 4, 2020

Thank you.

@janlazo
Copy link
Contributor

janlazo commented May 13, 2020

Does the native file walker work on directory symlinks and junctions?

@mattn
Copy link
Contributor Author

mattn commented May 15, 2020

janlazo added a commit to janlazo/dotvim8 that referenced this pull request May 15, 2020
fzf uses https://github.com/saracen/walker on Windows since v0.21.0
but it does not work on directory symlinks and junctions.
I use directory junctions a lot.
I prefer a list of files by default over nothing.

junegunn/fzf#1847 (comment)
https://github.com/saracen/walker/blob/324a081bae7e580aa0bf3afe8164acb16634afca/walker.go#L85
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.

None yet

3 participants