Use nucleo instead of fuzzy-matcher
#1830
Conversation
src/components/fuzzy_find_popup.rs
Outdated
| @@ -88,17 +90,38 @@ impl FuzzyFindPopup { | |||
| self.filtered.clear(); | |||
|
|
|||
| if let Some(q) = &self.query { | |||
| let matcher = | |||
| fuzzy_matcher::skim::SkimMatcherV2::default(); | |||
| let mut line_content_buf = Vec::new(); | |||
There was a problem hiding this comment.
maybe we should also cache these buffers, right?
There was a problem hiding this comment.
maybe just wipe them as soon as one closes the window?
There was a problem hiding this comment.
Thank you for the review! I moved them to the FuzzyFindPopup alongside the matcher. Maybe I should move matcher and matcher_* to a new struct to omit this prefix?
Cargo.lock
Outdated
| @@ -649,15 +655,6 @@ dependencies = [ | |||
| "slab", | |||
| ] | |||
|
|
|||
| [[package]] | |||
| name = "fuzzy-matcher" | |||
There was a problem hiding this comment.
is this branch up to date with master? I don't see the removal of fuzzy-matcher in asyncgit which got introduced in #1791
There was a problem hiding this comment.
Sorry, this branch was really outdated, I merged latest changes from master
After a little digging in the asyncgit/src/sync/logwalker.rs, I realised that it would require more changes than in fuzzy_find_popup.rs
The main problem I'm struggling with is the fuzzy_match borrows Matcher as mutable, this will require some changes in the current implementation
The shortest way I see is this: we can wrap matcher in LogFilterSearch into RwLock, this will allow us to borrow filter as mutable only during the actual matching. Otherwise we'll be required to handle the entire filter and closures that borrow it as mutable everywhere (maybe I'm wrong)
Could you please advise how best to implement this?
Btw, the naive implementation with RwLock already shows that nucleo is faster alternative than fuzzy-matcher! I tested commit log filtering for the kubernetes repository (~117k commits):
For random query the debug build with nucleo takes ~5.5 sec, and the version with fuzzy-matcher takes ~6.5 sec!
There was a problem hiding this comment.
Mutex is fine here since we won't have multiple readers and any benefit of RwLock for this.
There was a problem hiding this comment.
I am not really aware of how your codebase works but I might mention here that fuzzy-matcher uses an implicit internal thread-local storage together with interior mutability to hide the same mutability that Nucleo has (and has similarly expensive allocations although they are lazy instead of eagerly). So if you are doing the matching in parallel (which I would recommend) the mutex will hinder nucleo and you would get better results by using one matcher instance per thread (the allocation is large but not that large either).
If you are looking to add an fzf like filter I would really recommend that you consider the high-level nucleo API (the nucleo crate). It performs the matching in parallel and can perform millions of matches with barely perceptible delay (on a beefy CPU). It's what we use in helix for the file picker (you can try it out by building this branch helix-editor/helix#7814 and running it in your root directory).
It also supports fully lock-free (and wait-free) parallel streaming so if you are iterating revs in one (or multiple) background threads that would work well too
|
btw. I would like to see nucleo mature a bit before we can merge this: helix-editor/nucleo#15 |
|
@auvred please update to current master and new nucleo release: helix-editor/nucleo#15 (comment) |
|
I'm a little busy in the next few days, I'll continue working on this PR soon, if it will still be relevant It seems that it would be better to use the high-level api as pascalkuthe advised :) |
|
@auvred its not pressing to get it in. how about reworking it to use the high level API? |
|
I'm all for it! |
|
I will close this pull request for now so as not to interfere with the others who want to implement this feature. So if anyone wants to work on this, fill free to pick it up |
This Pull Request fixes/closes #1805.
It changes the following:
fuzzy-matcherwithnucleoinFuzzyFindPopupI followed the checklist:
make checkwithout errorsAccording to the
nucleodocs, theMatcherallocates some heap memory at the creation, that's why it should be reusedI'm not sure that this change will be merged, so for now I'll open a draft PR