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

Directories with lots of inodes grind completion to a crawl in prompt #3604

Closed
nrdxp opened this issue Aug 30, 2022 · 5 comments
Closed

Directories with lots of inodes grind completion to a crawl in prompt #3604

nrdxp opened this issue Aug 30, 2022 · 5 comments
Labels
C-bug Category: This is a bug

Comments

@nrdxp
Copy link
Contributor

nrdxp commented Aug 30, 2022

Summary

When completing a directory with a large quanity of files and folders in the prompt, say with :o, if the directory in question has thousands of inodes, as does the /nix/store when using Nix, for example, typing becomes extremely laggy and difficult. Even pasting text takes a long time, and worse, makes helix unresponsive until finished.

I would guess that the entire directory is being crawled on each keystroke and that's why it takes so long.

Reproduction Steps

The following asciinema shows a text being pasted into the prompt directly from the system clipboard:
https://asciinema.org/a/kV7WxOfdvmFxNwEUsPzQPDogT

Helix log

No relevant log segment

Platform

Linux

Terminal Emulator

kitty 0.25.2

Helix Version

22.05

@nrdxp nrdxp added the C-bug Category: This is a bug label Aug 30, 2022
@archseer
Copy link
Member

Yep, there's no caching of any sorts, this runs every time completion is fetched:

// TODO: we could return an iter/lazy thing so it can fetch as many as it needs.
fn filename_impl<F>(editor: &Editor, input: &str, filter_fn: F) -> Vec<Completion>
where
F: Fn(&ignore::DirEntry) -> FileMatch,
{
// Rust's filename handling is really annoying.
use ignore::WalkBuilder;
use std::path::Path;
let is_tilde = input.starts_with('~') && input.len() == 1;
let path = helix_core::path::expand_tilde(Path::new(input));
let (dir, file_name) = if input.ends_with(std::path::MAIN_SEPARATOR) {
(path, None)
} else {
let file_name = path
.file_name()
.and_then(|file| file.to_str().map(|path| path.to_owned()));
let path = match path.parent() {
Some(path) if !path.as_os_str().is_empty() => path.to_path_buf(),
// Path::new("h")'s parent is Some("")...
_ => std::env::current_dir().expect("couldn't determine current directory"),
};
(path, file_name)
};
let end = input.len()..;
let mut files: Vec<_> = WalkBuilder::new(&dir)
.hidden(false)
.follow_links(editor.config().file_picker.follow_symlinks)
.max_depth(Some(1))
.build()
.filter_map(|file| {
file.ok().and_then(|entry| {
let fmatch = filter_fn(&entry);
if fmatch == FileMatch::Reject {
return None;
}
//let is_dir = entry.file_type().map_or(false, |entry| entry.is_dir());
let path = entry.path();
let mut path = if is_tilde {
// if it's a single tilde an absolute path is displayed so that when `TAB` is pressed on
// one of the directories the tilde will be replaced with a valid path not with a relative
// home directory name.
// ~ -> <TAB> -> /home/user
// ~/ -> <TAB> -> ~/first_entry
path.to_path_buf()
} else {
path.strip_prefix(&dir).unwrap_or(path).to_path_buf()
};
if fmatch == FileMatch::AcceptIncomplete {
path.push("");
}
let path = path.to_str()?.to_owned();
Some((end.clone(), Cow::from(path)))
})
}) // TODO: unwrap or skip
.filter(|(_, path)| !path.is_empty()) // TODO
.collect();
// if empty, return a list of dirs and files in current dir
if let Some(file_name) = file_name {
let matcher = Matcher::default();
// inefficient, but we need to calculate the scores, filter out None, then sort.
let mut matches: Vec<_> = files
.into_iter()
.filter_map(|(_range, file)| {
matcher
.fuzzy_match(&file, &file_name)
.map(|score| (file, score))
})
.collect();
let range = (input.len().saturating_sub(file_name.len()))..;
matches.sort_unstable_by(|(file1, score1), (file2, score2)| {
(Reverse(*score1), file1).cmp(&(Reverse(*score2), file2))
});
files = matches
.into_iter()
.map(|(file, _)| (range.clone(), file))
.collect();
// TODO: complete to longest common match
} else {
files.sort_unstable_by(|(_, path1), (_, path2)| path1.cmp(path2));
}
files
}

If would be good to limit to a subset of entries, but we'd need a way to start scanning from a certain "page" onwards

@archseer
Copy link
Member

I think

.follow_links(editor.config().file_picker.follow_symlinks)
should be removed since we're not recursively resolving. I also get decent performance after :o /nix/store/ since the kernel will cache these types of lookups.

@archseer
Copy link
Member

I think the problem here is that you're actually pasting on 22.05 where bracketed paste didn't exist yet. (#3233) Can you try latest master?

@archseer
Copy link
Member

8a4fbf6 e77b7d1

@nrdxp
Copy link
Contributor Author

nrdxp commented Sep 7, 2022

Thanks, it works as expected 🙏

@nrdxp nrdxp closed this as completed Sep 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug
Projects
None yet
Development

No branches or pull requests

2 participants