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

Provide a faster implementation of Fuzzy Searching for 'Add Using'. #7390

Merged
merged 62 commits into from
Dec 16, 2015

Conversation

CyrusNajmabadi
Copy link
Member

This change refactors things to move fuzzy searching to a BK-tree implementatoin. The BK tree helps reduce the number of of dictionary checks we have to do by about 90%.

@CyrusNajmabadi
Copy link
Member Author

Tagging @dpoeschl

var currentNode = _builderNodes[currentNodeIndex];

var editDistance = EditDistance.GetEditDistance(currentNode.LowerCaseCharacters, lowerCaseCharacters);
// This shoudl never happen. We dedupe all items before proceeding to the 'Add' step.
Copy link
Contributor

Choose a reason for hiding this comment

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

should

@CyrusNajmabadi CyrusNajmabadi force-pushed the fuzzyMatchingIndex branch 3 times, most recently from 83757ce to aad4ebd Compare December 11, 2015 01:46
// of children along with each node. However, this would be very inefficient and would
// put an enormous amount of memory pressure on the system.
//
// Imperical data for a nice large assembly like mscorlib gives us the following
Copy link
Contributor

Choose a reason for hiding this comment

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

Empirical

Copy link
Contributor

Choose a reason for hiding this comment

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

This is in Utilities, so you should probably add more context here, that you added all names in mscorlib and got this distribution.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup yup.

@davkean
Copy link
Member

davkean commented Dec 11, 2015

Linux failure due to #7411.
retest prtest/lin/dbg/unit32 please

switch (kind)
{
case SearchKind.Exact:
_predicate = s => StringComparer.Ordinal.Equals(name, s);
Copy link
Contributor

Choose a reason for hiding this comment

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

Cache this statically?

Copy link
Member Author

Choose a reason for hiding this comment

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

Can't. It captures the parameter to the outer method.

@CyrusNajmabadi
Copy link
Member Author

retest this please

@CyrusNajmabadi
Copy link
Member Author

test vsi please

@CyrusNajmabadi CyrusNajmabadi force-pushed the fuzzyMatchingIndex branch 2 times, most recently from 1ccf2b3 to da98648 Compare December 13, 2015 22:50
@CyrusNajmabadi
Copy link
Member Author

test vsi please

1 similar comment
@CyrusNajmabadi
Copy link
Member Author

test vsi please

// y |∞ 8 *
//
// And then consider a point above that diagonal (indicated by x). In the example
// above, the edit distance to * from + will be (x+4). If, for example, threshold
Copy link
Contributor

Choose a reason for hiding this comment

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

From +?

CyrusNajmabadi added a commit that referenced this pull request Dec 16, 2015
Provide a faster implementation of Fuzzy Searching for 'Add Using'.
@CyrusNajmabadi CyrusNajmabadi merged commit b91400c into dotnet:master Dec 16, 2015
@CyrusNajmabadi CyrusNajmabadi deleted the fuzzyMatchingIndex branch December 16, 2015 00:04
@dpoeschl
Copy link
Contributor

@CyrusNajmabadi Does the BKTree creation only happen when you first ctrl+dot in a way that triggers add using? Or is it done earlier than that, on idle or something? I think we said the other day that it won't be done earlier due to some bug -- is something tracking that?

@CyrusNajmabadi
Copy link
Member Author

The BKTrees will currently happen when asked for for ctrl-dot (or if we're trying to decide if we should show the lightbulb or not). There is nothing tracking any work to make the index construction happen any sooner than it already did before the addition of these trees.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants