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

Use `fuzzy-native` as an alternate scoring system #374

Merged
merged 8 commits into from Mar 29, 2019

Conversation

Projects
None yet
3 participants
@rafeca
Copy link
Contributor

commented Mar 28, 2019

Description of the Change

This PR does two things:

  • Renames the useAlternateScoring config option to convert it to an enum called scoringSystems (using the same default value as before).
  • Adds a new scoringSystem called fast (πŸ€·β€β™‚οΈ) which uses fuzzy-native to dramatically improve filtering times on large projects.

Alternate Designs

A few other options have been considered:

  • Adding a debouncer without changing the current filtering logic.
  • Keep the current filtering logic but move it to an async Task.
  • Use the nuclide-prebuilt-libs package instead of fuzzy-native.
  • Create a new package with the fuzzy-native filtering logic executed in a background thread.

At the end, this solution is what IMHO gives more benefits and less tradeoffs per unit of effort πŸ˜…

For more information about benefits/tradeoffs, check out the discussion on the applicable issue.

Benefits

Drastically faster filtering for large projects, which are now usable.

Type of project master native-fuzzy Perf improvements
Small 10-24ms 1ms πŸ‘ 17X faster
Medium 25-160ms 1.5-9ms πŸ‘ 17X faster
Large 400-1800ms 20-73ms πŸ‘ 22X faster

For the benchmark I've configured native-fuzzy to use a single thread. When using 4 threads the time to filter improves by ~2-3X on large projects but stays almost the same for medium/small.

demo

Possible Drawbacks

Yet a new scoring system added. As a follow-up we may decided to remove the standard scoring system, which was superseeded by the alternate one 3 years ago.

Applicable Issues

#370

rafeca added some commits Mar 28, 2019

Rename the useAlternateScoring config option
The old useAlternateScoring config option has been renamed to
scoringSystem and has been converted to an enum, to accommodate the
future native-fuzzy system.

@rafeca rafeca self-assigned this Mar 28, 2019

@rafeca rafeca requested review from jasonrudolph and maxbrunsfeld Mar 28, 2019

@rafeca rafeca force-pushed the fuzzy-native branch from f06c590 to 180d317 Mar 28, 2019

return items
}

return this.nativeFuzzy.match(query, {maxResults: MAX_RESULTS}).map(

This comment has been minimized.

Copy link
@rafeca

rafeca Mar 28, 2019

Author Contributor

For now, we're executing native-fuzzy on a single thread, but we can easily change this in the future

return {uri: filePath, filePath, label}
}

getAbsolutePath (relativePath) {

This comment has been minimized.

Copy link
@rafeca

rafeca Mar 28, 2019

Author Contributor

This method is not great because it'll do a few fs accesses via fs.existSync, but it should not be very expensive because it does it against the list of filtered items (which is maximum 10 items).

This means that it'll do 10 * number of opened projects file system accesses. Since I don't expect users having many projects opened in the same window, this should be fine

[false, 'standard'],
[true, 'standard'],
[false, 'alternate']
[false, 'fast']

This comment has been minimized.

Copy link
@rafeca

rafeca Mar 28, 2019

Author Contributor

I don't think it's worth to test on every single permutation of useRipGrep:scoringSystem, since these two params are orthogonal, so I've just selected a few permutations that ensure that both variables get fully tested.

@rafeca

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2019

uhm, appveyor shows failures due to python2 not being available on they vms... Investigating

@rafeca

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2019

Ok, I've been able to build and use fuzzy-native locally on a Windows machine, but I found two things:

I think that both issues are related with the fact that the package is not prepared for electron v2.0.

I'm not sure if this is related to the issue that's happening on AppVeyor (I don't think so).

@50Wliu

This comment has been minimized.

Copy link
Member

commented Mar 28, 2019

  • npm install on fuzzy-finder does not generate the correct binary on windows. I had to run apm install to be able to generate the correct one (for electron).

This is expected; apm install should always be used for installing Atom packages.

EDIT: Oh, and the python2 error is a red herring; the fallback is at C:\Python27\python.exe, which works.

@50Wliu

This comment has been minimized.

Copy link
Member

commented Mar 28, 2019

Try adding image: Visual Studio 2015 to appveyor.yml

@rafeca

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2019

This is expected; apm install should always be used for installing Atom packages.

Oh interesting, I still have to learn a lot about Atom πŸ˜…

Try adding image: Visual Studio 2015 to appveyor.yml

I'm gonna try, thanks!

rafeca added some commits Mar 28, 2019

@rafeca

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2019

Ok, that fixed the issue πŸ˜„

Now the remaining issue is the one caused by these lines: https://github.com/hansonw/fuzzy-native/blob/master/lib/main.js#L10-L11

@rafeca

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2019

Ok, using a fork that adds support for electron v2 we're able to run the tests (only that 2 of them fail πŸ˜…), I'm gonna try to reproduce the failures on my windows machine (on OSX all of them pass)

We're making progress!

@rafeca rafeca force-pushed the fuzzy-native branch from 3bfb59b to 0db223e Mar 28, 2019

@rafeca

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2019

Yay! AppVeyor builds are finally passing πŸ˜„ There was a test that didn't contain correct Windows paths and it didn't work well with the path transformations that the logic for fuzzy-native does πŸ˜…

@rafeca

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2019

For some reason Travis builds get queued forever: https://travis-ci.org/atom/fuzzy-finder/builds/512728958?utm_source=github_status&utm_medium=notification ... I'll check back tomorrow

@maxbrunsfeld
Copy link
Contributor

left a comment

This looks so great! Can't wait to try it out on a large project.

@@ -11,6 +11,7 @@
"fs-plus": "^3.0.0",
"fuzzaldrin": "^2.0",
"fuzzaldrin-plus": "^0.6.0",
"fuzzy-native": "git://github.com/rafeca/fuzzy-native.git#electron-v2",

This comment has been minimized.

Copy link
@maxbrunsfeld

maxbrunsfeld Mar 28, 2019

Contributor

Maybe you should publish this to NPM as @atom/fuzzy-native or something. That's what we've done for a few forks that we've had in the past, like nsfw and react.

This comment has been minimized.

Copy link
@rafeca

rafeca Mar 29, 2019

Author Contributor

Sounds good, I'll do that

@rafeca

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

I've forked fuzzy-native to https://github.com/atom/fuzzy-native and created a @atom/fuzzy-native package with v0.7.0 that just contains the fix for electron v2.

It seems that npm caching logic is not super smart and the package cannot be found yet from CI systems... I'll wait some time and retrigger the build.

I'll also set up a similar CI system that the original fuzzy-native package has (again, I cannot do it now since Travis cannot find the new GitHub project yet)

@rafeca rafeca force-pushed the fuzzy-native branch from 18b758c to 8aec72c Mar 29, 2019

@rafeca

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

Ok, after a few retries all tests passing, I'm proceeding to merge this PR πŸ™

@rafeca rafeca merged commit fd66901 into master Mar 29, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rafeca rafeca deleted the fuzzy-native branch Mar 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.