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

rafeca · 2019-03-28T15:40:54Z

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.

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

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 · 2019-03-28T15:46:05Z

lib/fuzzy-finder-view.js

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


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

rafeca · 2019-03-28T15:48:33Z

lib/fuzzy-finder-view.js

+    return {uri: filePath, filePath, label}
+  }
+
+  getAbsolutePath (relativePath) {


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

rafeca · 2019-03-28T15:50:01Z

spec/fuzzy-finder-spec.js

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


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 · 2019-03-28T15:58:02Z

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

rafeca · 2019-03-28T17:02:11Z

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

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).
The package is not prepared to read binaries generated for electron 2.0 (more info: https://github.com/hansonw/fuzzy-native/blob/master/lib/main.js#L10-L11).

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).

winstliu · 2019-03-28T17:17:49Z

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.

winstliu · 2019-03-28T17:32:46Z

Try adding image: Visual Studio 2015 to appveyor.yml

rafeca · 2019-03-28T17:39:18Z

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 · 2019-03-28T17:52:01Z

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 · 2019-03-28T18:12:04Z

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 · 2019-03-28T22:02:07Z

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 · 2019-03-28T22:03:43Z

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

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

maxbrunsfeld · 2019-03-28T22:07:08Z

package.json

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


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.

Sounds good, I'll do that

rafeca · 2019-03-29T09:56:54Z

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 · 2019-03-29T11:15:24Z

Ok, after a few retries all tests passing, I'm proceeding to merge this PR 🙏

rafeca added 2 commits March 28, 2019 14:46

Rename the useAlternateScoring config option

f526f7f

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

Execute fuzzy finder tests in different scoring systems

e96fb8a

rafeca self-assigned this Mar 28, 2019

rafeca requested review from jasonrudolph and maxbrunsfeld March 28, 2019 15:40

Add fuzzy-native as a scoringSystem

180d317

rafeca force-pushed the fuzzy-native branch from f06c590 to 180d317 Compare March 28, 2019 15:43

rafeca commented Mar 28, 2019

View reviewed changes

rafeca added 2 commits March 28, 2019 18:40

Update appveyor.yml to use a newer image

b0b0bcc

Fix lint errors

ca2e1f1

Use a fork of fuzzy-native with electron-v2 support

8a56b85

Use path.join() instead of hardcoding path separators

0db223e

rafeca force-pushed the fuzzy-native branch from 3bfb59b to 0db223e Compare March 28, 2019 21:51

maxbrunsfeld approved these changes Mar 28, 2019

View reviewed changes

Start using @atom/fuzzy-native fork package

8aec72c

rafeca force-pushed the fuzzy-native branch from 18b758c to 8aec72c Compare March 29, 2019 10:06

rafeca merged commit fd66901 into master Mar 29, 2019

rafeca deleted the fuzzy-native branch March 29, 2019 11:15

rafeca added the FY2019Q5 atom perf More information: https://github.com/github/pe-atom-log/issues/728 label Mar 29, 2019

This was referenced Mar 29, 2019

Add metrics for fuzzy-finder filtering time #375

Merged

⬆️ [email protected] atom/atom#19075

Merged

Show prompt to enable "fast mode" on large projects #376

Closed

jasonrudolph mentioned this pull request Apr 11, 2019

[Feedback] Experimental Fast Mode #379

Open

Use fuzzy-native as an alternate scoring system #374

Use fuzzy-native as an alternate scoring system #374

Uh oh!

Conversation

rafeca commented Mar 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of the Change

Alternate Designs

Benefits

Possible Drawbacks

Applicable Issues

Uh oh!

rafeca Mar 28, 2019

Choose a reason for hiding this comment

Uh oh!

rafeca Mar 28, 2019

Choose a reason for hiding this comment

Uh oh!

rafeca Mar 28, 2019

Choose a reason for hiding this comment

Uh oh!

rafeca commented Mar 28, 2019

Uh oh!

rafeca commented Mar 28, 2019

Uh oh!

winstliu commented Mar 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

winstliu commented Mar 28, 2019

Uh oh!

rafeca commented Mar 28, 2019

Uh oh!

rafeca commented Mar 28, 2019

Uh oh!

rafeca commented Mar 28, 2019

Uh oh!

rafeca commented Mar 28, 2019

Uh oh!

rafeca commented Mar 28, 2019

Uh oh!

maxbrunsfeld left a comment

Choose a reason for hiding this comment

Uh oh!

maxbrunsfeld Mar 28, 2019

Choose a reason for hiding this comment

Uh oh!

rafeca Mar 29, 2019

Choose a reason for hiding this comment

Uh oh!

rafeca commented Mar 29, 2019

Uh oh!

rafeca commented Mar 29, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

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

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

rafeca commented Mar 28, 2019 •

edited

Loading

winstliu commented Mar 28, 2019 •

edited

Loading