-
-
Notifications
You must be signed in to change notification settings - Fork 610
Use nucleo
instead of fuzzy-matcher
#1830
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 nucleo
instead of fuzzy-matcher
#1830
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please bring up to date with master and remove all usages of fuzzy-matcher
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we should also cache these buffers, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe just wipe them as soon as one closes the window?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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-matcher
withnucleo
inFuzzyFindPopup
I followed the checklist:
make check
without errorsAccording to the
nucleo
docs, theMatcher
allocates 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