Speed up fuzzy search #2639
Merged
Speed up fuzzy search #2639
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks @Bodigrim (and sorry to have dragged you into another project :) ) I have a benchmark in the Sigma codebase for this completions fuzzy search, but I will need a few days to find the time. Probably over the weekend. |
|
I've probably broken something, but it's still surprising that 9.0.1 Ubuntu and 9.2.1 Mac pass, while 9.2.1 Ubuntu fails. |
So helpful! Cannot wait for GHC 9.4 with stack traces for |
|
@pepeiborra this is finally ready for review. |
|
Benchmarked using the completions experiment of the ghcide bench suite on the Sigma codebase. The results show that the new code allocates much less and is up to 2X as fast. Nice job! BEFOREAFTER |
pepeiborra
reviewed
Jan 29, 2022
pepeiborra
requested changes
Jan 29, 2022
Comment on lines
+40
to
+67
| go !totalScore !currScore !currPOff !currSOff | ||
| -- If pattern has been matched in full | ||
| | currPOff >= pTotal | ||
| = Just totalScore | ||
| -- If there is not enough left to match the rest of the pattern, equivalent to | ||
| -- (sOff + sLen - currSOff) < (pOff + pLen - currPOff) | ||
| | currSOff > currPOff + sDelta | ||
| = Nothing | ||
| -- This is slightly broken for non-ASCII: | ||
| -- 1. If code units, consisting a single pattern code point, are found as parts | ||
| -- of different code points, it counts as a match. Unless you use a ton of emojis | ||
| -- as identifiers, such false positives should not be be a big deal, | ||
| -- and anyways HLS does not currently support such use cases, because it uses | ||
| -- code point and UTF-16 code unit positions interchangeably. | ||
| -- 2. Case conversions is not applied to non-ASCII code points, because one has | ||
| -- to call T.toLower (not T.map toLower), reallocating the string in full, which | ||
| -- is too much of performance penalty for fuzzy search. Again, anyway HLS does not | ||
| -- attempt to do justice to Unicode: proper Unicode text matching requires | ||
| -- `unicode-transforms` and friends. | ||
| -- Altogether we sacrifice correctness for the sake of performance, which | ||
| -- is a right trade-off for fuzzy search. | ||
| | pByte <- TA.unsafeIndex pArr currPOff | ||
| , sByte <- TA.unsafeIndex sArr currSOff | ||
| -- First byte (currPOff == pOff) should match exactly, otherwise - up to case. | ||
| , pByte == sByte || (currPOff /= pOff && pByte == toLowerAscii sByte) | ||
| = let curr = currScore * 2 + 1 in | ||
| go (totalScore + curr) curr (currPOff + 1) (currSOff + 1) | ||
| | otherwise |
There was a problem hiding this comment.
How do we know that the new implementation produces the same scores? I would like to see a unit test here, or a property test.
It's ok for the test suite to depend on the fuzzy package.
There was a problem hiding this comment.
Well, even the old implementation was not fully matching fuzzy package because of a different approach to case matching. What's the best place to add unit tests?
|
@pepeiborra just a gentle reminder about my questions above. |
pepeiborra
approved these changes
Feb 19, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
My intuition is that this approach should be faster, but I do not have data to back such claim. @pepeiborra any chance to test it with a huge list of completions?