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.

Sign up for GitHub

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

Jump to bottom

Add FilterTree/FilterSpan API on remaining analysis contexts #68052

Merged
merged 11 commits into from
Jun 9, 2023
Merged

Add FilterTree/FilterSpan API on remaining analysis contexts #68052

merged 11 commits into from
Jun 9, 2023

Conversation

mavasani
Copy link
Contributor

@mavasani mavasani commented May 2, 2023 •
edited
Loading

Work towards #68033

  • Add FilterTree/FilterSpan API on all the remaing analysis contexts
  • Move many IDE code style analyzers to respect the FilterTree/FilterSpan API

Performance measurements

All analyzers

  • I carried out performance measurements for executing all analyzers for line span on few large C# files in Microsoft.CodeAnalysis.CSharp.csproj, with bits built out of current Roslyn main branch and bits built from this PR branch
  • I see ~10-20% speedup in computation (screenshot below).

image

All high + normal priority analyzers for lightbulb (excludes SymbolStart/End and SemanticModel analyzers)

  • I carried similar performance measurements as above for high + normal priority analyzers for lightbulb
  • I see ~20-25% speedup in computation (screenshot below).

image

SymbolStart/End analyzers (known expensive analyzers)

  • I carried out performance measurements for executing SymbolStart/End analyzers for line span on few large C# files in Microsoft.CodeAnalysis.CSharp.csproj, with bits built out of current Roslyn main branch and bits built from this PR branch
  • I see ~10% speedup in computation (screenshot below).
  • I was hoping for a bigger improvement, but digging further I identified majority of the time is coming from method body binding to generate all the compilation events/operation trees. For example, invoking SemanticModel.GetDiagnostics() on LanguageParser.cs takes ~1.8 seconds, which is about 75% of the time (~2.2 seconds) for computing analyzer semantic diagnostics for the same file.

image

@mavasani
Add FilterTree/FilterSpan API on SymbolStartAnalysisContext and Symbo…
eb371ec 
…lAnalysisContext

Work towards dotnet#68033

- Add FilterTree/FilterSpan API on SymbolStartAnalysisContext and SymbolAnalysisContext
- Move IDE code style SymbolStart analyzers to respect the FilterTree/FilterSpan API
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Analyzers untriaged Issues and PRs which have not yet been triaged by a lead labels May 2, 2023
@mavasani mavasani mentioned this pull request May 2, 2023
Closed
@CyrusNajmabadi
Copy link
Member

I was hoping for a bigger improvement, but digging further I identified majority of the time is coming from method body binding to generate all the compilation events/operation trees.

Can you expand on that? Are we generating events and iop trees that are not needed?

@mavasani
Copy link
Contributor Author

mavasani commented May 2, 2023

I was hoping for a bigger improvement, but digging further I identified majority of the time is coming from method body binding to generate all the compilation events/operation trees.

Can you expand on that? Are we generating events and iop trees that are not needed?

No, this is not redundant - the core thing is the driver has to pessimistically assume that at least one of the SymbolStart analyzer would execute on the entire symbol declaration (including all method bodies within it). Current approach for driving analyzer execution is via relevant compilation events which are generated by invoking SemanticModel.GetDiagnostics() on all files with named type symbol's partial definition. An alternate approach for driving analysis was attempted in #67165, but that showed the execution times were just moved to later when we create all operation trees for all method bodies. So, we would always get this hit when executing SymbolStart analyzers. The performance gain in this PR is primarily coming from the analyzers themselves doing much lesser analysis.

@mavasani mavasani added the Performance-Scenario-Diagnostics This issue affects diagnostics computation performance for lightbulb, background analysis, tagger. label May 4, 2023
mavasani added a commit to mavasani/roslyn that referenced this pull request May 9, 2023
@mavasani
Add FilterSpan API for remaining analysis contexts
5e6998a 
Closes dotnet#68033
Follow-up to dotnet#68052
@mavasani mavasani changed the title Add FilterTree/FilterSpan API on SymbolStartAnalysisContext and SymbolAnalysisContext Add FilterTree/FilterSpan API on remaining analysis contexts May 9, 2023
@mavasani mavasani marked this pull request as ready for review May 19, 2023 09:00
@mavasani mavasani requested review from a team as code owners May 19, 2023 09:00
333fred
333fred approved these changes May 20, 2023
View reviewed changes
Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

Compiler changes seem fine to me. Have not reviewed outside the compiler layer.

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalysisScope.cs Outdated Show resolved Hide resolved
CyrusNajmabadi
CyrusNajmabadi approved these changes May 22, 2023
View reviewed changes
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi left a comment

Choose a reason for hiding this comment

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

signing off IDE side. didn't look at compiler side.

slightly nervous about Location instances, but we can see in traces if its an issue. likely we get so much savings that this is worth it.

CyrusNajmabadi
CyrusNajmabadi approved these changes May 22, 2023
View reviewed changes
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi left a comment

Choose a reason for hiding this comment

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

signing off IDE side. didn't look at compiler side.

slightly nervous about Location instances, but we can see in traces if its an issue. likely we get so much savings that this is worth it.

@mavasani
Copy link
Contributor Author

@dotnet/roslyn-compiler for a second review

@mavasani
Copy link
Contributor Author

mavasani commented Jun 8, 2023

@dotnet/roslyn-compiler for a second review. This PR gives us an observable performance gain for lightbulb scenarios.

cston
cston approved these changes Jun 9, 2023
View reviewed changes
Copy link
Member

@cston cston left a comment

Choose a reason for hiding this comment

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

Compiler changes LGTM.

@mavasani
Copy link
Contributor Author

mavasani commented Jun 9, 2023

Thanks!

@mavasani mavasani merged commit 4f0feeb into dotnet:main Jun 9, 2023
@ghost ghost added this to the Next milestone Jun 9, 2023
@mavasani mavasani deleted the SymbolStartSpanApi branch June 9, 2023 16:19
@allisonchou allisonchou mentioned this pull request Jun 16, 2023
Closed
@RikkiGibson RikkiGibson modified the milestones: Next, 17.7 P3 Jun 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@333fred 333fred 333fred approved these changes

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi approved these changes

@cston cston cston approved these changes

Assignees
No one assigned
Labels
Area-Analyzers Area-Compilers Performance-Scenario-Diagnostics This issue affects diagnostics computation performance for lightbulb, background analysis, tagger. untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Milestone
17.7 P3
Development

Successfully merging this pull request may close these issues.
5 participants
@mavasani @CyrusNajmabadi @333fred @cston @RikkiGibson