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

[release/6.0-rc1] Migrate LoggerMessageGenerator to IIncrementalGenerator #58271

Merged
merged 3 commits into from
Aug 28, 2021

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Aug 27, 2021

Backport of #58068 to release/6.0-rc1

Contributes to #56702

Customer Impact

Customers are reporting sluggish IDE behavior when targeting net6.0 in large solutions.

VS typing performance quickly degrades (hangs, up to several seconds pauses between characters typing, etc.) after working with several cs files for like 20 minutes or more in the context of a medium-sized DotNet 6 Preview 6 solution with maybe 70 projects, each with ~ 10 to 500 files.

This change is converting the Microsoft.Extensions.Logging source generator to use the new IIncrementalGenerator Roslyn API. This allows for the source generator to do considerably less work in the background of the IDE, especially when no code in the solution uses the source generator.

Testing

All automated tests continue to pass.

I also tested manually using VS 2022 that the source generator still works after converting to the new Roslyn API.

The responsiveness of VS 2022 in a large solution on my Standard_D8as_v4 Azure VM went from ~2 seconds to open the Intellisense window to almost instantaneous.

Risk

Since the IIncrementalGenerator Roslyn API is new in Roslyn v4.0, it doesn't work in VS 2019. This means a customer using the 6.0-rc1 NuGet package in VS 2019 will get a warning:

Warning CS8032 An instance of analyzer Microsoft.Extensions.Logging.Generators.LoggerMessageGenerator cannot be created from C:\Users\eerhardt\.nuget\packages\microsoft.extensions.logging.abstractions\7.0.0-dev\analyzers\dotnet\cs\Microsoft.Extensions.Logging.Generators.dll : Could not load file or assembly 'Microsoft.CodeAnalysis, Version=4.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35' or one of its dependencies. The system cannot find the file specified.

The plan is to enable "multi-targeting" different Roslyn versions in 6.0-rc2. This will allow us to ship 2 versions of the source generator in the NuGet package - one targeting the 3.x Roslyn APIs and one targeting the 4.0 Roslyn APIs, with the performance of the 4.0 Roslyn version being much better.

cc @chsienki @ericstj @jaredpar @sharwell

This reduces the time spent in the background in VS running the source generator, since we only need to respond to methods that have the LoggerMessageAttribute on them.

Contributes to #56702
@ghost
Copy link

ghost commented Aug 27, 2021

Tagging subscribers to this area: @maryamariyan
See info in area-owners.md if you want to be subscribed.

Issue Details

Backport of #58068 to release/6.0-rc1

/cc @eerhardt

Customer Impact

Testing

Risk

Author: github-actions[bot]
Assignees: -
Labels:

area-Extensions-Logging

Milestone: -

@eerhardt eerhardt added the Servicing-consider Issue for next servicing release review label Aug 27, 2021
@AraHaan
Copy link
Member

AraHaan commented Aug 27, 2021

I think a better option would be to allow generators to target stuff like netstandard2.0-roslyn{major version of roslyn}.{minor version of roslyn} where 3.x would be for vs2019 and 4.x folder would be for vs2022+

@eerhardt
Copy link
Member

I think a better option would be to allow generators to target stuff like netstandard2.0-roslyn{major version of roslyn}.{minor version of roslyn} where 3.x would be for vs2019 and 4.x folder would be for vs2022+

That is the plan listed in the Risk section above: "multi-targeting" different Roslyn versions in 6.0-rc2.

@danmoseley danmoseley added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Aug 27, 2021
@Anipik Anipik merged commit 7bfbc96 into release/6.0-rc1 Aug 28, 2021
@AraHaan
Copy link
Member

AraHaan commented Aug 28, 2021

I think a better option would be to allow generators to target stuff like netstandard2.0-roslyn{major version of roslyn}.{minor version of roslyn} where 3.x would be for vs2019 and 4.x folder would be for vs2022+

That is the plan listed in the Risk section above: "multi-targeting" different Roslyn versions in 6.0-rc2.

Would this also require an update to Visual Studio 2019 and 2022 to be made so that way people who do not use sdk style projects would get the same benefits? My college still uses vs2019 without sdk style projects for the assignments but I plan to see if I can pr my techer’s template for github classroom to convert it to sdk style so that way my classmates would just have to worry about the .net sdk installed with visual studio for it all. Plus I found out that I am not the only one using a Mac for class 😂.

Also I feel like the templates for Visual Studio should include (.NET Framework, SDK Style) in the options between the already .NET Framework / .NET Core options that already exists for those templates for those who want to target only .NET Framework versions but also have the benefits of being sdk style right at project creation.

@lewing lewing deleted the backport/pr-58068-to-release/6.0-rc1 branch August 28, 2021 02:29
@eerhardt
Copy link
Member

Would this also require an update to Visual Studio 2019 and 2022 to be made so that way people who do not use sdk style projects would get the same benefits?

My hope is that it will still work even if the project doesn't use sdk-style projects.

Also I feel like the templates for Visual Studio should include...

Please open new issues for feedback like that. This isn't the best place to provide that feedback. Probably the best place would be to use the "Send Feedback" button in VS.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Extensions-Logging Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants