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

Async lightbulb performance improvements #56843

Closed
2 tasks done
mavasani opened this issue Sep 29, 2021 · 3 comments
Closed
2 tasks done

Async lightbulb performance improvements #56843

mavasani opened this issue Sep 29, 2021 · 3 comments
Assignees
@mavasani
Labels
Area-Analyzers Area-IDE Feature Request Tenet-Performance Regression in measured performance of the product from goals. User Story A single user-facing feature. Can be grouped under an epic.
Milestone
17.1

Comments

@mavasani
Copy link
Contributor

mavasani commented Sep 29, 2021 •
edited
Loading

While investigating lightbulb perf, I identified the following performance improvements that can be done for async light bulb feature:

  • High priority fixes: Currently, we only run the compiler analyzer for high priority code fixes (add using and merge conflict resolution). However, the shortcomings of the current analyzer API doesn't allow compiler analyzer to work in span-based mode for semantic diagnostics (see here), even though SemanticModel APIs for semantic diagnostics take an optional filter span. We can workaround this in the analyzer driver by special casing the compiler analyzer and threading in the input filter span to the SemanticModelAnalysisContext, thereby improving the performance for computing compiler diagnostics and populating the High priority code fixes in async light bulb.

  • Normal priority fixes: Currently, all the analyzers that are not high-pri are added to the normal priority bucket and executed at once. We can further break down the normal priority bucket as follow:

    1. Normal: Analyzers that produce at least one fixable diagnostic, i.e. have a corresponding code fixer.
    2. Low: Analyzers without any fixable diagnostics, i.e. have no corresponding code fixer. These diagnostics will only have Suppression/Configuration quick actions, which are lower priority and always show up at the bottom of the light bulb.

    This new bucketing would allow us to reduce the number of analyzers that are executed in the Normal priority bucket and hence generate all the non-suppression/configuration code fixes faster in async light bulb.
@mavasani mavasani added Area-IDE Area-Analyzers Feature Request Tenet-Performance Regression in measured performance of the product from goals. labels Sep 29, 2021
@mavasani mavasani added this to the 17.0 milestone Sep 29, 2021
@mavasani mavasani self-assigned this Sep 29, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged Issues and PRs which have not yet been triaged by a lead label Sep 29, 2021
@mavasani
Copy link
Contributor Author

FYI @CyrusNajmabadi @sharwell @jinujoseph @vatsalyaagrawal

I am planning to work on these improvements this week. Let me know your thoughts.

mavasani added a commit to mavasani/roslyn that referenced this issue Sep 29, 2021
@mavasani
Async lightbulb performance improvement
9addb73 
Addresses the first bullet point in dotnet#56843.

Prior to this change, compiler diagnostic analyzer could only be run from the IDE for entire document span. This is primarily due to lack of an analyzer API that allows the analyzer to register a span-based semantic diagnostic callback. Though we can consider designing and adding such an analyzer API, it would be purely an IDE-only analyzer action, as analyzers are never executed for a span in batch compilation mode - this would make it difficult to justify adding such an API.

With this PR, we add a workaround in the analyzer driver to allow executing compiler analyzer's semantic model action scoped to a filter span. This should speed up executing the compiler analyzer for lightbulb scenarions in the IDE.

Verified that both the compiler and IDE tests added with this PR fail prior to the product changes.
mavasani added a commit to mavasani/roslyn that referenced this issue Sep 29, 2021
@mavasani
Addresses the first bullet point in dotnet#56843.
38b82c3 
Prior to this change, compiler diagnostic analyzer could only be run from the IDE for entire document span. This is primarily due to lack of an analyzer API that allows the analyzer to register a span-based semantic diagnostic callback. Though we can consider designing and adding such an analyzer API, it would be purely an IDE-only analyzer action, as analyzers are never executed for a span in batch compilation mode - this would make it difficult to justify adding such an API.

With this PR, we add a workaround in the analyzer driver to allow executing compiler analyzer's semantic model action scoped to a filter span. This should speed up executing the compiler analyzer for lightbulb scenarios in the IDE, which are always scoped to current line span.

Verified that both the compiler and IDE tests added with this PR fail prior to the product changes.

**NOTE:** This change would not give us any perceivable improvement in non-async lightbulb scenario, as we are extremely likely to have at least one other analyzer that needs to run on the entire document. The performance improvement should be perceivable in async lightbulb scenario as the high priority code fixes bucket (add usings and merge conflict resolution) only run the compiler analyzer.
@mavasani mavasani mentioned this issue Sep 29, 2021
Merged
@CyrusNajmabadi
Copy link
Member

I support all of this :-)

@jinujoseph jinujoseph modified the milestones: 17.0, 17.1 Sep 29, 2021
@jinujoseph jinujoseph removed the untriaged Issues and PRs which have not yet been triaged by a lead label Sep 29, 2021
mavasani added a commit to mavasani/roslyn that referenced this issue Oct 5, 2021
@mavasani
[Async lightbulb] Move suppression fixes to 'Low' priority bucket
f07ecea 
Addresses second item in dotnet#56843

Prior to this change, all the analyzers that are not high-pri are added to the normal priority bucket and executed at once. This PR further breaks down the normal priority bucket into two buckets as follow:

1. `Normal`: Analyzers that produce at least one fixable diagnostic, i.e. have a corresponding code fixer.
2. `Low`: Analyzers without any fixable diagnostics, i.e. have no corresponding code fixer. These diagnostics will only have Suppression/Configuration quick actions, which are lower priority and always show up at the bottom of the light bulb.

This new bucketing allows us to reduce the number of analyzers that are executed in the Normal priority bucket and hence generate all the non-suppression/configuration code fixes faster in async light bulb.
mavasani added a commit to mavasani/roslyn that referenced this issue Oct 5, 2021
@mavasani
[Async lightbulb] Move suppression fixes to 'Low' priority bucket
8ed96c8 
Addresses second item in dotnet#56843

Prior to this change, all the analyzers that are not high-pri are added to the normal priority bucket and executed at once. This PR further breaks down the normal priority bucket into two buckets as follow:

1. `Normal`: Analyzers that produce at least one fixable diagnostic, i.e. have a corresponding code fixer.
2. `Low`: Analyzers without any fixable diagnostics, i.e. have no corresponding code fixer. These diagnostics will only have Suppression/Configuration quick actions, which are lower priority and always show up at the bottom of the light bulb.

This new bucketing allows us to reduce the number of analyzers that are executed in the Normal priority bucket and hence generate all the non-suppression/configuration code fixes faster in async light bulb.
mavasani added a commit to mavasani/roslyn that referenced this issue Oct 5, 2021
@mavasani
[Async lightbulb] Move suppression fixes to 'Low' priority bucket
7472ba7 
Addresses second item in dotnet#56843

Prior to this change, all the analyzers that are not high-pri are added to the normal priority bucket and executed at once. This PR further breaks down the normal priority bucket into two buckets as follow:

1. `Normal`: Analyzers that produce at least one fixable diagnostic, i.e. have a corresponding code fixer.
2. `Low`: Analyzers without any fixable diagnostics, i.e. have no corresponding code fixer. These diagnostics will only have Suppression/Configuration quick actions, which are lower priority and always show up at the bottom of the light bulb.

This new bucketing allows us to reduce the number of analyzers that are executed in the Normal priority bucket and hence generate all the non-suppression/configuration code fixes faster in async light bulb.
mavasani added a commit to mavasani/roslyn that referenced this issue Oct 5, 2021
@mavasani
[Async lightbulb] Move suppression fixes to 'Low' priority bucket
d6f32c1 
Addresses second item in dotnet#56843

Prior to this change, all the analyzers that are not high-pri are added to the normal priority bucket and executed at once. This PR further breaks down the normal priority bucket into two buckets as follow:

1. `Normal`: Analyzers that produce at least one fixable diagnostic, i.e. have a corresponding code fixer.
2. `Low`: Analyzers without any fixable diagnostics, i.e. have no corresponding code fixer. These diagnostics will only have Suppression/Configuration quick actions, which are lower priority and always show up at the bottom of the light bulb.

This new bucketing allows us to reduce the number of analyzers that are executed in the Normal priority bucket and hence generate all the non-suppression/configuration code fixes faster in async light bulb.
@mavasani mavasani mentioned this issue Oct 5, 2021
Merged
RikkiGibson added a commit that referenced this issue Oct 19, 2021
@RikkiGibson @mavasani
@crummel @jcouv
Merge main to main-vs-deps (#57242)
07250d0 
* Addresses the first bullet point in #56843.

Prior to this change, compiler diagnostic analyzer could only be run from the IDE for entire document span. This is primarily due to lack of an analyzer API that allows the analyzer to register a span-based semantic diagnostic callback. Though we can consider designing and adding such an analyzer API, it would be purely an IDE-only analyzer action, as analyzers are never executed for a span in batch compilation mode - this would make it difficult to justify adding such an API.

With this PR, we add a workaround in the analyzer driver to allow executing compiler analyzer's semantic model action scoped to a filter span. This should speed up executing the compiler analyzer for lightbulb scenarios in the IDE, which are always scoped to current line span.

Verified that both the compiler and IDE tests added with this PR fail prior to the product changes.

**NOTE:** This change would not give us any perceivable improvement in non-async lightbulb scenario, as we are extremely likely to have at least one other analyzer that needs to run on the entire document. The performance improvement should be perceivable in async lightbulb scenario as the high priority code fixes bucket (add usings and merge conflict resolution) only run the compiler analyzer.

* Update src/Compilers/Core/Portable/DiagnosticAnalyzer/DiagnosticAnalysisContext.cs

* Add test and fix filtering by analysis scope.

* Address further feedback

* Add work item attribute to test

* Remove GetAdjustedSpanForCompilerAnalyzerAsync workaround in the IDE

* Unncessary usings

* Revert "Unncessary usings"

This reverts commit 510edb8.

* Revert "Remove GetAdjustedSpanForCompilerAnalyzerAsync workaround in the IDE"

This reverts commit 23e3310.

* Update test to account for workaround in IDE DocumentAnalysisExecutor for #1557

* Address PR feedback

* Fix VisualStudioActiveDocumentTracker to handle VSSELELEMID.SEID_WindowFrame

Fixes #57203. Issue description has more details

* Update src/Compilers/Core/Portable/DiagnosticAnalyzer/CompilationUnitCompletedEvent.cs

* Apply arcade-powered source-build patches (#55823)

* Don't include desktop artifacts that don't exist in source-build.
Source-build doesn't have these artifacts available, even when we eventually will build desktop TFMs, because Roslyn is one of the first builds in source-build.  Instead Roslyn is picking up reference packages that don't have the `lib` directory which is causing a build failure.  This disables the attempt to grab these desktop artifacts so source-build just skips them instead.

* Honor suppression on switch expression (#57204)

* Merge pull request #57227 from dotnet/dev/rigibson/update-feature-status-2

Update Language Feature Status.md

Co-authored-by: Manish Vasani <mavasani@microsoft.com>
Co-authored-by: Chris Rummel <crummel@microsoft.com>
Co-authored-by: Julien Couvreur <jcouv@users.noreply.github.com>
Co-authored-by: Rikki Gibson <rigibson@microsoft.com>
mavasani added a commit to mavasani/roslyn that referenced this issue Oct 29, 2021
@mavasani
[Async lightbulb] Move suppression fixes to 'Low' priority bucket
3e9bf10 
Addresses second item in dotnet#56843

Prior to this change, all the analyzers that are not high-pri are added to the normal priority bucket and executed at once. This PR further breaks down the normal priority bucket into two buckets as follow:

1. `Normal`: Analyzers that produce at least one fixable diagnostic, i.e. have a corresponding code fixer.
2. `Low`: Analyzers without any fixable diagnostics, i.e. have no corresponding code fixer. These diagnostics will only have Suppression/Configuration quick actions, which are lower priority and always show up at the bottom of the light bulb.

This new bucketing allows us to reduce the number of analyzers that are executed in the Normal priority bucket and hence generate all the non-suppression/configuration code fixes faster in async light bulb.
mavasani added a commit to mavasani/roslyn that referenced this issue Nov 18, 2021
@mavasani
[Async lightbulb] Move suppression fixes to 'Low' priority bucket
6930441 
Addresses second item in dotnet#56843

Prior to this change, all the analyzers that are not high-pri are added to the normal priority bucket and executed at once. This PR further breaks down the normal priority bucket into two buckets as follow:

1. `Normal`: Analyzers that produce at least one fixable diagnostic, i.e. have a corresponding code fixer.
2. `Low`: Analyzers without any fixable diagnostics, i.e. have no corresponding code fixer. These diagnostics will only have Suppression/Configuration quick actions, which are lower priority and always show up at the bottom of the light bulb.

This new bucketing allows us to reduce the number of analyzers that are executed in the Normal priority bucket and hence generate all the non-suppression/configuration code fixes faster in async light bulb.
@jmarolf jmarolf added the User Story A single user-facing feature. Can be grouped under an epic. label Dec 2, 2021
JoeRobich added a commit that referenced this issue Dec 6, 2021
@JoeRobich @Youssef1313
@sharwell @bradselw @mavasani @genlu @CyrusNajmabadi @ryzngard @bernd5 @allisonchou @davidwengier
Merge main to main-vs-deps (#58066)
3e5ced3 
* Remove IDE0051 suppression

* Add LayoutKind

* LayoutKind.Sequential

Co-authored-by: Sam Harwell <sam@tunnelvisionlabs.com>

* Create azure-pipelines-integration-dartlab.yml

* Update azure-pipelines-integration-dartlab.yml

* [Async lightbulb] Move suppression fixes to 'Low' priority bucket

Addresses second item in #56843

Prior to this change, all the analyzers that are not high-pri are added to the normal priority bucket and executed at once. This PR further breaks down the normal priority bucket into two buckets as follow:

1. `Normal`: Analyzers that produce at least one fixable diagnostic, i.e. have a corresponding code fixer.
2. `Low`: Analyzers without any fixable diagnostics, i.e. have no corresponding code fixer. These diagnostics will only have Suppression/Configuration quick actions, which are lower priority and always show up at the bottom of the light bulb.

This new bucketing allows us to reduce the number of analyzers that are executed in the Normal priority bucket and hence generate all the non-suppression/configuration code fixes faster in async light bulb.

* Cache last project and compilationWithAnalyzers

* Update test as normal priority bucket has one lesser code action

* Make shallow checkout optional for integration test CI

* Move pipeline checkout skips up a layer.

* Look up VS bootstrapper from build

* Revert skipPipelinesCheckout change

* Comment out checkout: none for now

* Added matrix

* Target topic branch in DartLab.Templates

* Update number of machines to 4

* Run test agent elevated

* Update pipeline description

* Default VS branch to main

* Address feedback

* Exclude unnecessary sqlite assemblies

* Convert namespace to file scoped when typing semicolon

* Working on tests

* Add tests

* Add comments

* Add comments

* Revert

* Revert

* Make static

* Add tests

* Pass AnalysisKind instead of int

* Honor option, and also improve formatting with comment

* Fix comment

* Add tests

* Fix await completion for expression body lambda

* Add new parser/lexer to the StackTraceAnalyzer (#57598)

No functional changes, just moving to the new API and cleaning up unused code

* Add comments

* Make it possible to analyze the dataflow of `ConstructorInitializerSyntax` and `PrimaryConstructorBaseTypeSyntax` (#57576)

Fixes #57577.

* Snap 17.1 P2 (#58041)

* Add new parser/lexer to the StackTraceAnalyzer (#57598) (#58050)

No functional changes, just moving to the new API and cleaning up unused code

* Read SourceLink info and call service to retrieve source from there (#57978)

* Merge pull request #58100 from dotnet/dev/jorobich/skip-test

Skip flaky CommandLine.ArgumentParsing test

See #58077 for more details

Co-authored-by: Youssef Victor <31348972+Youssef1313@users.noreply.github.com>
Co-authored-by: Sam Harwell <sam@tunnelvisionlabs.com>
Co-authored-by: Brad White <4261702+bradselw@users.noreply.github.com>
Co-authored-by: Manish Vasani <mavasani@microsoft.com>
Co-authored-by: Joey Robichaud <jorobich@microsoft.com>
Co-authored-by: Brad White <bradwhit@microsoft.com>
Co-authored-by: gel@microsoft.com <gel@microsoft.com>
Co-authored-by: Cyrus Najmabadi <cyrusn@microsoft.com>
Co-authored-by: Sam Harwell <Sam.Harwell@microsoft.com>
Co-authored-by: Andrew Hall <andrha@microsoft.com>
Co-authored-by: Bernd Baumanns <baumannsbernd@gmail.com>
Co-authored-by: Allison Chou <allichou@microsoft.com>
Co-authored-by: CyrusNajmabadi <cyrus.najmabadi@gmail.com>
Co-authored-by: David Wengier <david.wengier@microsoft.com>
Co-authored-by: Gen Lu <genlu@users.noreply.github.com>
@mavasani
Copy link
Contributor Author

This is now complete

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mavasani mavasani

Labels
Area-Analyzers Area-IDE Feature Request Tenet-Performance Regression in measured performance of the product from goals. User Story A single user-facing feature. Can be grouped under an epic.
Projects
None yet
Milestone
17.1
Development

No branches or pull requests
4 participants
@CyrusNajmabadi @jmarolf @mavasani @jinujoseph