From 878fd7b7ea93ab9316efa56fe302cee87f901cd2 Mon Sep 17 00:00:00 2001 From: Pascal Kuthe Date: Sun, 30 Jul 2023 05:02:12 +0200 Subject: [PATCH] fix typos --- README.md | 10 +++++----- src/items.rs | 2 +- src/lib.rs | 4 ++-- src/worker.rs | 6 +++--- 4 files changed, 11 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index 19e892c..18bcca8 100644 --- a/README.md +++ b/README.md @@ -13,16 +13,16 @@ An optimized rust port of the fzf fuzzy matching algorithm * space complexity: skim needs at least 8x more memory => much worse case locality, fzf always allocates a full `O(mn)` matrix even during matching => byebye cache (atleast they use smaller integers tough) * nucleos' matrix only was width `n-m+1` instead of width `n`. This comes from the observation that the `p.` char requires `p-1` chars before it and `m-p` chars after it, so there are always `p-1 + m-p = m+1` chars that can never match the current char. This works especially well with only using a single row because the first relevant char is always at the same position even tough its technically further to the right. This is particularly nice because we precalculate the m -matrix which is computed from diagonal elements, so the precalculated values stay in the same matrix cell. * a couple simpler (but arguably even more impactful) optimizations: - * we presegment unicode, unicode segmentation is somewhat slow and matcher will filter the same elements quite often so only doing it once is nice. It also prevents a very common soruce fo bugs (mixing of char indices which we use here and utf8 indices) and makes the code a lot simpler as a result. - * we special case ASCII since 90% of practical text is ASCII. Ascii can be stored as bytes instead of `chars` => much better cache locality => we can use memchar (SIMD!). + * we presegment unicode, unicode segmentation is somewhat slow and matcher will filter the same elements quite often so only doing it once is nice. It also prevents a very common source of bugs (mixing of char indices which we use here and utf8 indices) and makes the code a lot simpler as a result. + * we special case ASCII since 90% of practical text is ASCII. ASCII can be stored as bytes instead of `chars` => much better cache locality => we can use memchar (SIMD!). * we aggressively prefilter (especially ASCII but also unicode to a lesser extent) to ensure we reject non-matching haystacks as fast as possible. Usually most haystacks will not match when fuzzy matching large lists so having very quick reject past is good * for very long matches we fallback to a greedy matcher which runs in `O(N)` (and `O(1)` space complexity) to avoid the `O(mn)` blowup. This is fzfs old algorithm and yields decent (but not great) results. * There is a misunderstanding in both skim and fzf. Basically what they do is give a bonus to each character (like word boundaries). That makes senes and is reasonable, but the problem is that they use the **maximum bonus** when multiple chars match in sequence. That means that the bonus of a character depends on which characters exactly matched around it. But the fundamental assumption of this algorithm (and why it doesn't require backtracking) is that the score of each character is independent of what other chars matched (this is the difference between the affine gap and the generic gap case shown in the paper too). During fuzzing I found many cases where this mechanism leads to a non-optimal match being reported (so the sort order and fuzzy indices would be wrong). In my testing removing this mechanism and slightly tweaking the bonus calculation results in similar match quality but made sure the algorithm always worked correctly (and removed a bunch of weird edges cases). * [x] substring/prefix/postfix/exact matcher -* [ ] case mismatch penalty. This doens't seem like a good idea to me. Fzf doens't do this (only skin), smartcase should cover most cases. .would be nice for fully case insensitive matching without smartcase like in autocompletions tough. Realistically there won't be more than 3 items that are identical with different casing tough, so I don't think it matters too much. It is a bit annoying to implement since you can no longer prenormalize queries(or need two queries) :/ +* [ ] case mismatch penalty. This doesn't seem like a good idea to me. FZF doesn't do this (only skin), smart case should cover most cases. .would be nice for fully case-insensitive matching without smart case like in autocompletion tough. Realistically there won't be more than 3 items that are identical with different casing tough, so I don't think it matters too much. It is a bit annoying to implement since you can no longer pre-normalize queries(or need two queries) :/ * [ ] high level API (worker thread, query parsing, sorting), in progress - * apparently sorting is superfast (at most 5% of match time for nucleo matcher with a highly selective query, otherwise its completely negligible compared to fuzzy matching). All the bending over backwards fzf does (and skim copied but way worse) seems a little silly. I think fzf does it because go doens't have a good parallel sort. Fzf divides the matches into a couple fairly large chunks and sorts those on each worker thread and then lazily merges the result. That makes the sorting without the merging `Nlog(N/M)` which is basically equivalent for large `N` and small `M` as is the case here. Atleast its parallel tough. In rust we have a great pattern defeating parallel quicksort tough (rayon) which is way easier. - * [x] basic implemenation (workers, streaming, invalidation) + * apparently sorting is superfast (at most 5% of match time for nucleo matcher with a highly selective query, otherwise its completely negligible compared to fuzzy matching). All the bending over backwards fzf does (and skim copied but way worse) seems a little silly. I think fzf does it because go doesn't have a good parallel sort. Fzf divides the matches into a couple fairly large chunks and sorts those on each worker thread and then lazily merges the result. That makes the sorting without the merging `Nlog(N/M)` which is basically equivalent for large `N` and small `M` as is the case here. Atleast its parallel tough. In rust we have a great pattern defeating parallel quicksort tough (rayon) which is way easier. + * [x] basic implementation (workers, streaming, invalidation) * [ ] verify it actually works * [ ] query paring * [ ] hook up to helix diff --git a/src/items.rs b/src/items.rs index 7b819d9..2f50289 100644 --- a/src/items.rs +++ b/src/items.rs @@ -67,7 +67,7 @@ impl Item { impl Drop for Item { fn drop(&mut self) { // safety: cols is basically a box and treated the same as a box, - // however there can be other references (that won't be accesed + // however there can be other references (that won't be accessed // anymore at this point) so using a box (unique ptr) would be an alias // violation unsafe { drop(Box::from_raw(self.cols.as_ptr())) } diff --git a/src/lib.rs b/src/lib.rs index f0f0d72..ff9ee33 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -66,7 +66,7 @@ impl Items { } pub struct Nucleo { - // the way the API is build we totally don't actually neeed these to be Arcs + // the way the API is build we totally don't actually need these to be Arcs // but this lets us avoid some unsafe worker: Arc>, canceled: Arc, @@ -182,7 +182,7 @@ impl Drop for Nucleo { } } /// convenicne function to easily fuzzy match -/// on a (relatievly small list of inputs). This is not recommended for building a full tui +/// on a (relatively small list of inputs). This is not recommended for building a full tui /// application that can match large numbers of matches as all matching is done on the current /// thread, effectively blocking the UI pub fn fuzzy_match>( diff --git a/src/worker.rs b/src/worker.rs index 9165080..af0c3ab 100644 --- a/src/worker.rs +++ b/src/worker.rs @@ -135,12 +135,12 @@ impl Worker { } if !self.canceled.load(atomic::Ordering::Relaxed) { - // TODO: cancel sort in progess? + // TODO: cancel sort in progress? self.matches.par_sort_unstable_by(|match1, match2| { match2.score.cmp(&match1.score).then_with(|| { // the tie breaker is comparitevly rarely needed so we keep it - // in a branch especially beacuse we need to acceess the items - // array here which invovles some pointer chasing + // in a branch especially because we need to access the items + // array here which involves some pointer chasing let item1 = &items[match1.idx as usize]; let item2 = &items[match2.idx as usize]; (item1.len, match1.idx).cmp(&(item2.len, match2.idx))