[go: up one dir, main page]

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

Very slow IntelliSense on relatively small code base #67926

Open
Youssef1313 opened this issue Apr 22, 2023 · 33 comments
Open

Very slow IntelliSense on relatively small code base #67926

Youssef1313 opened this issue Apr 22, 2023 · 33 comments
Assignees
Milestone

Comments

@Youssef1313
Copy link
Member

image

image

I'm not sure which process should be looked into for this case, devenv or ServiceHub.RoslynCodeAnalysisService.

GetSymbolInfo is commonly showing up as expensive in both:

  • ServiceHub.RoslynCodeAnalysisService: GetSymbolInfo is called by NameSyntaxClassifier.AddClassification
  • devenv: GetSymbolInfo is called by CSharpSyntaxContext.CreateContext and CSharpTypeInferenceService+TypeInferrer.InferTypesWorker_DoNotCallDirectly.

If I clear GroupPats and FoldPats (which I read somewhere, but I don't understand what they actually do), I see GetUseSiteInfo starts showing up.

image

image

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Apr 22, 2023
@Youssef1313
Copy link
Member Author

Trace shared with @CyrusNajmabadi and @sharwell.

@Youssef1313
Copy link
Member Author

I'm suspecting this is related to:

// NOTE: We use a quadratic algorithm to determine which members override/hide
// each other (i.e. we compare them pairwise). We could move to a linear
// algorithm that builds the closure set of overridden/hidden members and then
// uses that set to filter the candidate, but that would still involve realizing
// a lot of PE symbols. Instead, we partition the candidates by containing type.
// With this information, we can efficiently skip checks where the (potentially)
// overriding or hiding member is not in a subtype of the type containing the
// (potentially) overridden or hidden member.

Tagging @AlekseyTs as well since this seems a compiler issue.

@Youssef1313
Copy link
Member Author
Youssef1313 commented Apr 23, 2023

On the IDE side, it was a simple call to LookupSymbols
image

protected ImmutableArray<ISymbol> LookupSymbolsInContainer(

Random thought, are use site diagnostics (or diagnostics in general) relevant in the IDE scenario above? Could they somehow be eliminated?

@AlekseyTs
Copy link
Contributor

How many requests IDE performs for this scenario?

@AlekseyTs
Copy link
Contributor

Also, how much in terms of elanced time we are talking about

@Youssef1313
Copy link
Member Author

How many requests IDE performs for this scenario?
Also, how much in terms of elanced time we are talking about

@sharwell Do you know the answer for these?

@Youssef1313
Copy link
Member Author

@sharwell @AlekseyTs How can we move forward with this?

@jaredpar
Copy link
Member
jaredpar commented May 9, 2023

@Youssef1313

Do you have a code sample that illustrates this problem? Can you share out the ETL traces here? It's hard to tell what is going on from the screen shots alone.

@cston PTAL once the information is available.

@jaredpar jaredpar added Bug Area-Compilers and removed Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels May 9, 2023
@jaredpar jaredpar added this to the 17.7 milestone May 9, 2023
@Youssef1313
Copy link
Member Author

@jaredpar @cston I have invited you to a private repo with a repro.

@jaredpar
Copy link
Member
jaredpar commented May 9, 2023

@Youssef1313 that's a fairly sprawling bit of code. Can you help narrow down what file is having the perf issue?

@Youssef1313
Copy link
Member Author

@jaredpar I added more info the repo readme.

@cston
Copy link
Member
cston commented May 10, 2023

@Youssef1313, I didn't see a notification for the repo. Please resend if possible, thanks.

@Youssef1313
Copy link
Member Author

@cston Re-sent.

@cston
Copy link
Member
cston commented May 24, 2023

Thanks @Youssef1313 for the initial investigation. It looks like the perf issue may be determining which overloads are hidden, as you indicated above. I'll investigate a possible fix.

@jeromelaban
Copy link

@cston Thanks for the update! When you're mentioning hidden overloads, are you referring to the browsable attribute, or hidden by accessibility?

@Youssef1313
Copy link
Member Author

@jeromelaban I think it's about shadowing, even if the code doesn't have any methods that shadow others from base. It's an algorithm that runs as part of the overload resolution:

// NOTE: We use a quadratic algorithm to determine which members override/hide
// each other (i.e. we compare them pairwise). We could move to a linear
// algorithm that builds the closure set of overridden/hidden members and then
// uses that set to filter the candidate, but that would still involve realizing
// a lot of PE symbols. Instead, we partition the candidates by containing type.
// With this information, we can efficiently skip checks where the (potentially)
// overriding or hiding member is not in a subtype of the type containing the
// (potentially) overridden or hidden member.

@Youssef1313
Copy link
Member Author

@cston I see the comment says "We could move to a linear algorithm". I'm curious if this is the possible fix you're investigating?

@cston
Copy link
Member
cston commented May 24, 2023

@Youssef1313, I think we could keep the change simple by targeting extension methods only which are not part of a class hierarchy.

@Youssef1313
Copy link
Member Author

@cston The performance issue is still present in Preview 3. Can't tell for sure if it has improved a bit, but anyway it's still veeeery slow, really.

@CyrusNajmabadi
Copy link
Member

@Youssef1313 do you have an updated trace?

@Youssef1313
Copy link
Member Author

@CyrusNajmabadi Yup sure. Already uploading, but...

image

@Youssef1313
Copy link
Member Author

@cston
Copy link
Member
cston commented Feb 15, 2024

FWIW, it looks like there are a number of CompletionProvider instances invoked by CompletionService.GetCompletionsAsync when typing . in the method, and the CompletionProvider instances bind the modified method at least 4 times with distinct SemanticModel instances, so there is no sharing of BoundLambda instances between those cases. Two of the CompletionProvider instances are from Roslyn:

  • Microsoft.CodeAnalysis.CSharp.Completion.Providers.KeywordCompletionProvider
  • Microsoft.CodeAnalysis.CSharp.Completion.Providers.SymbolCompletionProvider

If those two CompletionProvider instances could share a SemanticModel, at least when not using a SpeculativeSemanticModel, that might increase Intellisense perf slightly.

cc @CyrusNajmabadi

@CyrusNajmabadi
Copy link
Member

@sharwell can you look into sharing the semantic model with KeywordCompletionProvider and SymbolCompletionProvider

@sharwell
Copy link
Member

@cston How did you determine that separate semantic models were being used for those two providers?

@sharwell
Copy link
Member
sharwell commented Feb 16, 2024

Ah, found it. The two instances are using a shared cache (SharedSyntaxContextsWithSpeculativeModel.GetSyntaxContextAsync). Both show up as causing the creation of a semantic model, but not during the same completion request.

@cston
Copy link
Member
cston commented Feb 16, 2024

How did you determine that separate semantic models were being used for those two providers?

The lambda expressions were bound for each provider, and the BoundLambda cache was not shared.

The two instances are using a shared cache (SharedSyntaxContextsWithSpeculativeModel.GetSyntaxContextAsync). Both show up as causing the creation of a semantic model, but not during the same completion request.

Are there multiple completion requests when typing a single . in the editor? And if so, are the completion requests executed serially?

@cston
Copy link
Member
cston commented Feb 29, 2024

It looks like typing . in the Visual Studio editor results in binding the method body 10 times. It's not clear how many of those method body bindings are required for displaying the completion list though.

@cston
Copy link
Member
cston commented Mar 1, 2024

It looks like typing . in the Visual Studio editor results in binding the method body 10 times.

@sharwell, @CyrusNajmabadi, do we expect to bind the containing method 10 times in this scenario? Again, it's not clear how many of those bindings block completion but perhaps the additional work in other threads may affect overall perf of completion.

@CyrusNajmabadi
Copy link
Member

We have lots of features waking up and doing things. What matters to me is what the threads are doing that are computing the items for completion.

@cston
Copy link
Member
cston commented Apr 5, 2024

For overload resolution cases where there are many overloads and potentially nested lambda expressions, it should be possible to reduce the number of times lambda bodies are bound by:

  • using explicit parameter types and even return types for lambda expressions, or
  • using explicit type arguments for generic method calls with lambda arguments, or
  • reducing nesting of lambda expressions.

@jeromelaban, @Youssef1313, sorry for not covering that earlier. Would one or more of those changes work in this case? Are there overload scenarios where those changes are not appropriate? Thanks.

@Youssef1313
Copy link
Member Author

The issue here is that it's not something we encounter only in our codebase where we can workaround the perf issue, but that's the API we ship for C# Markup and the performance affects our users' code. So, we really want our users to write the code in the way they want (keeping in mind that options 1 and 2 are very much verbose anyways).

@sharwell
Copy link
Member
sharwell commented Apr 8, 2024

@Youssef1313 For that situation, you'll want to provide an API that discourages the use of patterns that cause these problems. My understanding is the compiler is interested in introducing a warning when type inference reaches a particular complexity threshold, but that work is not yet complete.

we really want our users to write the code in the way they want

No objection to this, as long as either "the way they want" doesn't involve deeply nested lambdas without types specified or the user is willing to accept severe performance problems.

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

No branches or pull requests

7 participants