Skip to content

Use new compiler symbol search helpers that are optimized for string names searches.#26331

Merged
gafter merged 24 commits intodotnet:masterfrom
CyrusNajmabadi:useNewSymbolSearch
May 31, 2018
Merged

Use new compiler symbol search helpers that are optimized for string names searches.#26331
gafter merged 24 commits intodotnet:masterfrom
CyrusNajmabadi:useNewSymbolSearch

Conversation

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor

@CyrusNajmabadi CyrusNajmabadi commented Apr 23, 2018

Followup to #26325 (merged) and #26330. in this PR i've updated the IDE to forward certain helpers to these more efficient implementations.

This helps things out by more quickly being able to determine if a type even contains a member with name, and thus whether or not it should even be hydrated into a symbol and have its members created. Previous we would have to do a linear scan on all the members in a type to determine this. Now this data is in a set which can be queried much more efficiently.

@CyrusNajmabadi CyrusNajmabadi requested review from a team as code owners April 23, 2018 01:13
@CyrusNajmabadi
Copy link
Copy Markdown
Contributor Author

Tagging @jcouv @dotnet/roslyn-ide . Note: this doesn't need to reviewed until dependent PRs go through.

@jinujoseph jinujoseph added Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Apr 24, 2018
@jcouv jcouv added the Blocked label Apr 24, 2018
@jcouv jcouv self-assigned this Apr 24, 2018
@CyrusNajmabadi
Copy link
Copy Markdown
Contributor Author

Tagging @dotnet/roslyn-compiler for new APIs exposed from the Compilation objects. Tagging @dotnet/roslyn-ide for usages of that new API.

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor Author

Tagging @jcouv for being an honorary @dotnet/roslyn-compiler and @dotnet/roslyn-ide dev :D

@jcouv jcouv removed the Blocked label Apr 24, 2018
@jcouv jcouv added this to the 15.8 milestone Apr 24, 2018
? compilation.GetSymbolsWithName(query.Name, filter, cancellationToken)
: compilation.GetSymbolsWithName(query.GetPredicate(), filter, cancellationToken);

var symbolsWithName = symbols.Select(s => new SymbolAndProjectId(s, project.Id)) .ToImmutableArray();
Copy link
Copy Markdown
Contributor

@Neme12 Neme12 Apr 24, 2018

Choose a reason for hiding this comment

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

SelectAsArray? #Resolved

Copy link
Copy Markdown
Contributor Author

@CyrusNajmabadi CyrusNajmabadi Apr 24, 2018

Choose a reason for hiding this comment

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

Will do. #Resolved

{
return this.SupportsCompilation &&
await _solution.State.ContainsSymbolsWithNameAsync(Id, name, filter, cancellationToken).ConfigureAwait(false);
}
Copy link
Copy Markdown
Contributor Author

@CyrusNajmabadi CyrusNajmabadi Apr 24, 2018

Choose a reason for hiding this comment

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

Near copy of the function directly below

}

return compilation.ContainsSymbolsWithName(name, filter, cancellationToken);
}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Near copy of the function below

return new PredicateSymbolSearcher(this, filter, predicate, cancellationToken).GetSymbolsWithName();
}

#pragma warning disable RS0026 // Do not add multiple public overloads with optional parameters
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

these overloads shouldnot be a problem. There is no conflict/ambiguity between them and the existing overloads that take delegates.

@jcouv jcouv added the Area-IDE label Apr 27, 2018
Copy link
Copy Markdown
Member

@tmat tmat left a comment

Choose a reason for hiding this comment

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

:shipit:

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor Author

@gafter @jaredpar Can you look at this small public API addition? Thanks!

@jcouv
Copy link
Copy Markdown
Member

jcouv commented May 22, 2018

@gafter for review of compiler change. Thanks

@gafter
Copy link
Copy Markdown
Member

gafter commented May 22, 2018

@gafter for review of compiler change. Thanks

@jcouv The principal compiler change is a public API addition. I've scheduled a review of that API for tomorrow at 2pm. I'll comment here on the result of that review.

@jcouv jcouv added the Blocked label May 22, 2018
@gafter
Copy link
Copy Markdown
Member

gafter commented May 23, 2018

We didn't get to this API change in today's review. Rescheduled for Tuesday.

@gafter
Copy link
Copy Markdown
Member

gafter commented May 30, 2018

@CyrusNajmabadi
Reviewed this just now. One small change is requested: please document the language-specific behavior of name in Compilation.ContainsSymbolsWithName and Compilation.GetSymbolsWithName in the new APIs, and in the existing related APIs. e.g. mention that it is case-sensitive for C# and case-insensitive for VB if that is the case.

@gafter gafter removed the Blocked label May 30, 2018
gafter
gafter previously requested changes May 30, 2018
Copy link
Copy Markdown
Member

@gafter gafter left a comment

Choose a reason for hiding this comment

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

Please make small doc change requested by API review team.

@gafter
Copy link
Copy Markdown
Member

gafter commented May 30, 2018

@CyrusNajmabadi
Also please bring up-to-date with master.
Please confirm if it would be OK with you if we "Squash and merge" to complete this PR.

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor Author

Reviewed this just now. One small change is requested: please document the language-specific behavior of name in Compilation.ContainsSymbolsWithName and Compilation.GetSymbolsWithName in the new APIs, and in the existing related APIs. e.g. mention that it is case-sensitive for C# and case-insensitive for VB if that is the case.

Absolutely.

Also please bring up-to-date with master.

Will do!

Please confirm if it would be OK with you if we "Squash and merge" to complete this PR.

I'm 100% ok with that.

@gafter gafter unassigned tmat and jcouv May 30, 2018
@gafter gafter dismissed their stale review May 30, 2018 21:31

Requested changes have been made.

@gafter
Copy link
Copy Markdown
Member

gafter commented May 30, 2018

Pending merge approval.

@jaredpar
Copy link
Copy Markdown
Member

approved

@jcouv
Copy link
Copy Markdown
Member

jcouv commented May 30, 2018

@jinujoseph Can you also approve for 15.8 ask-mode since there is an IDE change? Thanks

@jinujoseph
Copy link
Copy Markdown
Contributor

Approved to merge for 15.8.Preview3

@jcouv
Copy link
Copy Markdown
Member

jcouv commented May 31, 2018

@gafter This PR is ready (approved by Jared and Jinu). But I don't know what "Squash and merge" is safe on a PR that includes merge commits from master...

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor Author

@jcouv I'm happy to rebase and submit for you to merge. Would you prefer that?

@gafter gafter merged commit 3a6f582 into dotnet:master May 31, 2018
@gafter
Copy link
Copy Markdown
Member

gafter commented May 31, 2018

@CyrusNajmabadi THANK YOU!!!!!

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor Author

Thank you!

@CyrusNajmabadi CyrusNajmabadi deleted the useNewSymbolSearch branch May 31, 2018 22:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Approved to merge Area-Compilers Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants