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

BackgroundAnalysisScope enhancements #57146

Closed
mavasani opened this issue Oct 14, 2021 · 4 comments · Fixed by #57172
Closed

BackgroundAnalysisScope enhancements #57146

mavasani opened this issue Oct 14, 2021 · 4 comments · Fixed by #57172
Assignees
Labels
Area-Analyzers Area-IDE Feature Request Performance-Scenario-Responsiveness Performance issues related to UI responsiveness User Story A single user-facing feature. Can be grouped under an epic.

Comments

@mavasani
Copy link
Contributor

mavasani commented Oct 14, 2021

Currently, BackgroundAnalysisScope has three possible scopes that affects how analyzers and code fixes run in the IDE:

  1. ActiveFile or CurrentDocument: Runs analyzers and compiler diagnostics only on currently active document
  2. OpenFiles or OpenDocuments (default scope): Runs analyzers and compiler diagnostics on all open documents
  3. FullSolution or EntireSolution: Runs analyzers and compiler diagnostics on all documents and projects in the entire solution

NOTE: Code refactorings are not affected by this scope setting, they are always available for currently active line regardless of the current background analysis scope.

We are proposing following enhancements to the scopes:

  1. Add a new analysis scope None with the following semantics:

    1. All analyzers and corresponding code fixes are disabled on all documents
    2. Compiler diagnostics and corresponding code fixes are enabled on all open documents

    This scope gives a "compiler diagnostics only" editing experience, where the user intends to just get their code in a working/compilable state. No IDE code style suggestions or code quality diagnostics/fixes are provided in this mode, which should significantly improve the IDE responsiveness and performance while typing. Related issues: How to disable live code compilation/analysis in Visual Studio 2017? #24513 and Restrict background analysis scope to reduce CPU consumption in live analysis #38429

  2. Change the semantics of ActiveFile or CurrentDocument scope so that compiler diagnostics are enabled on all opened documents instead of just the current document, while all the analyzers continue to run just on the currently active document. Primary motivation for this change to make this scope more usable. It is pretty likely that user making top level changes to the current document, such as method signature changes, affects the compiler diagnostics in other documents, such as at all call sites, and we want to automatically refresh the error list with latest compiler diagnostics without requiring the user to open each affected document to get up-to-date compiler diagnostics. There is also a secondary motivation for this change, which is mentioned below.

  3. Make ActiveFile or CurrentDocument the default background analysis scope. With the semantic change to this scope mentioned above, we feel that this is the most appropriate analysis scope for best editing experience with optimum IDE performance characteristics. We plan to first roll out this change to our internal team members for dogfooding, and subsequently roll it out to everyone in Dev17.1

@mavasani mavasani added this to the 17.1 milestone Oct 14, 2021
@mavasani mavasani self-assigned this Oct 14, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged Issues and PRs which have not yet been triaged by a lead label Oct 14, 2021
@mavasani mavasani removed the untriaged Issues and PRs which have not yet been triaged by a lead label Oct 14, 2021
@CyrusNajmabadi
Copy link
Member

which should significantly improve the IDE responsiveness and performance while typing.

This past should not be true. Ide responsiveness and performance should be unaffected by analyzers. If that's not the case, we have a problem.

@mavasani
Copy link
Contributor Author

Ide responsiveness and performance should be unaffected by analyzers

@CyrusNajmabadi I believe we have already seen quite a few performance traces in the past showing that when there are extremely large number of open documents in VS combined with large number of analyzers enabled for the solution, analyzer execution from background analysis dominates lot of CPU cycles and allocations and IDE does become sluggish. Not sure if this changes with 64 bit VS, but regardless, doing lesser background analysis should improve perf.

@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Oct 14, 2021

So any bg analysis should not affect things, otherwise we have an issue. Analyzers, in particular, should be in another process, so allocations should not be impactful.

I'd want us to explore why this stuff is impacting the ide.

Note: I'm not pushing back on this proposal. Just pointing out that if it resolving these perf issues we should also be looking at resolving then without this flag.

Or, put another way, this flag should exist because it provides the UX a set of users want. Not because it solves a perf issue for them :-). Perf can be added gravy. It should not be the goal or the motivation imo.

mavasani added a commit to mavasani/roslyn that referenced this issue Oct 15, 2021
mavasani added a commit to mavasani/roslyn that referenced this issue Oct 15, 2021
mavasani added a commit to mavasani/roslyn that referenced this issue Oct 15, 2021
mavasani added a commit to mavasani/roslyn that referenced this issue Oct 15, 2021
mavasani added a commit to mavasani/roslyn that referenced this issue Oct 15, 2021
@jmarolf
Copy link
Contributor

jmarolf commented Oct 15, 2021

Add a new analysis scope None with the following semantics:

Like this change

Change the semantics of ActiveFile or CurrentDocument scope so that compiler diagnostics are enabled on all opened documents instead of just the current document

Also love this

Make ActiveFile or CurrentDocument the default background analysis scope

Skeptical of this. There are performance assertions made here without linking to perf measurements for customer bugs about perf. I do not want to change the defaults based on no evidence.

One thing I would love for us to do to the analysis engine is: Do no run info level analyzers except in the current document by default. Based on telemetry all the most expensive analyzers that customers are running are info level:

Expensive Analyzer Type Name Number of Times Reported
Microsoft.CodeAnalysis.CSharp.Diagnostics.SimplifyTypeNames.CSharpSimplifyTypeNamesDiagnosticAnalyzer 6,370
Microsoft.CodeAnalysis.CSharp.RemoveUnusedParametersAndValues.CSharpRemoveUnusedParametersAndValuesDiagnosticAnalyzer 2,752
Microsoft.CodeAnalysis.Formatting.FormattingDiagnosticAnalyzer 909
Microsoft.CodeAnalysis.CSharp.Diagnostics.NamingStyles.CSharpNamingStyleDiagnosticAnalyzer 533
Microsoft.CodeAnalysis.CSharp.UsePatternMatching.CSharpIsAndCastCheckDiagnosticAnalyzer 104
Microsoft.CodeAnalysis.UseSystemHashCode.UseSystemHashCodeDiagnosticAnalyzer 82
Microsoft.CodeAnalysis.CSharp.SimplifyThisOrMe.CSharpSimplifyThisOrMeDiagnosticAnalyzer 69
Microsoft.CodeAnalysis.CSharp.Diagnostics.TypeStyle.CSharpUseExplicitTypeDiagnosticAnalyzer 56
Microsoft.CodeAnalysis.CSharp.UsePatternMatching.CSharpIsAndCastCheckWithoutNameDiagnosticAnalyzer 27
Microsoft.CodeAnalysis.CSharp.RemoveUnnecessaryImports.CSharpRemoveUnnecessaryImportsDiagnosticAnalyzer 25

mavasani added a commit to mavasani/roslyn that referenced this issue Oct 20, 2021
@jmarolf jmarolf added the User Story A single user-facing feature. Can be grouped under an epic. label Dec 2, 2021
@jmarolf jmarolf removed the User Story A single user-facing feature. Can be grouped under an epic. label Jan 6, 2022
@mavasani mavasani modified the milestones: 17.1, 17.2 Jan 10, 2022
@mikadumont mikadumont added the User Story A single user-facing feature. Can be grouped under an epic. label Jan 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Analyzers Area-IDE Feature Request Performance-Scenario-Responsiveness Performance issues related to UI responsiveness User Story A single user-facing feature. Can be grouped under an epic.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants