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

[file-search] improve results obtained when performing file search #6642

Merged
merged 1 commit into from
Jan 14, 2020

Conversation

vince-fugnitto
Copy link
Member

What it does

Fixes #5636

Description:

The current logic of the file-search meant that fuzzy results were being returned over exact results due to the limit. This PR quickly addresses that issue by ensuring that exact matches are prioritized, and thus returns better results. The PR has been tested with large workspaces (and even multi-root workspaces) and the performance stayed largely identical. There is likely to be a tradeoff of performance for better results which in the end I believe users would appreciate.

Implementation

The idea is to cancel the search if the exact matches hits the allowed limit.
Only the limit of results is sent over the wire.

How to test

  • perform file-search on a large workspace (or multi-root workspace)
  • verify the performance and if the results are better than previously (ex: searching for os.ts)

Review checklist

Reminder for reviewers

Signed-off-by: vince-fugnitto [email protected]

@vince-fugnitto vince-fugnitto added the file search issues related to the file search label Nov 27, 2019
@vince-fugnitto vince-fugnitto self-assigned this Nov 27, 2019
Copy link
Contributor

@elaihau elaihau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the idea is "not stop the search until the number of exact matches exceeds the limit".

The code lowers the "worst-case performance", and should not have major impacts to the "average case performance". I am OK with this change.

@vince-fugnitto
Copy link
Member Author

So the idea is "not stop the search until the number of exact matches exceeds the limit".

The code lowers the "worst-case performance", and should not have major impacts to the "average case performance". I am OK with this change.

Yes, the general idea is to use the limit only for the exact matches (attempt to get the most best possible matches), and some fuzzy results. In the end only the limit is passed over the wire.

It improves the results obtained while also not being too much of a radical change.
I'll get other's opinions on the change before merging :) Thank you for the review!

@vince-fugnitto vince-fugnitto marked this pull request as ready for review December 10, 2019 16:35
@vince-fugnitto
Copy link
Member Author

@lmcbout do you mind trying out this pull request for me when you get the chance?

@lmcbout
Copy link
Contributor

lmcbout commented Dec 20, 2019

I will test it

Copy link
Contributor

@lmcbout lmcbout left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
Code wise: ok
Testing on Ubuntu and Chrome: works fine, performance is good
Matching result: Great

@vince-fugnitto
Copy link
Member Author

If possible, I'd really like to get more opinions/reviews on this pull request before merging.
I attempted to improve the ordering of search results while keeping the changes/impact to a minimum including performance and complexity.

@akosyakov
Copy link
Member

I'm fine to try.

Fixes #5636

- improve the results obtained when performing the `file-search`.
- exact results are now prioritized over fuzzy results, leading to a more consistent experience.
- only the limit of results is sent over the wire.

Signed-off-by: vince-fugnitto <[email protected]>
@vince-fugnitto
Copy link
Member Author

I'm fine to try.

Sounds good, we can try to see if the behavior is better, and if anything it is always possible to rollback/revert the changes as they are in history.

@vince-fugnitto vince-fugnitto merged commit 3acbec2 into master Jan 14, 2020
@vince-fugnitto vince-fugnitto deleted the vf/GH-5636 branch January 14, 2020 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
file search issues related to the file search
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[file-search] As a user I want exact matches to show up in file search
4 participants