-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Port the incremental source generator polyfill code to the 6.0 servicing branch. #72219
Conversation
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
@@ -0,0 +1,125 @@ | |||
// Licensed to the .NET Foundation under one or more agreements. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exact copy of 7.0 line.
@@ -1,7 +1,10 @@ | |||
// Licensed to the .NET Foundation under one or more agreements. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exact copy of 7.0 line.
@@ -0,0 +1,74 @@ | |||
// Licensed to the .NET Foundation under one or more agreements. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exact copy of 7.0 line.
@@ -0,0 +1,24 @@ | |||
// Licensed to the .NET Foundation under one or more agreements. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exact copy of 7.0 line.
@@ -0,0 +1,66 @@ | |||
// Licensed to the .NET Foundation under one or more agreements. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exact copy of 7.0 line.
@@ -0,0 +1,42 @@ | |||
// Licensed to the .NET Foundation under one or more agreements. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exact copy of 7.0 line.
@@ -0,0 +1,29 @@ | |||
// Licensed to the .NET Foundation under one or more agreements. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exact copy of 7.0 line.
@@ -0,0 +1,201 @@ | |||
// Licensed to the .NET Foundation under one or more agreements. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exact copy of 7.0 line.
@@ -0,0 +1,400 @@ | |||
// Licensed to the .NET Foundation under one or more agreements. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exact copy of 7.0 line.
private const string JsonSourceGenerationOptionsAttributeFullName = "System.Text.Json.Serialization.JsonSourceGenerationOptionsAttribute"; | ||
|
||
internal const string JsonSerializableAttributeFullName = "System.Text.Json.Serialization.JsonSerializableAttribute"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
single deviation. In 7.0 line this was renamed (from Serializer -> Serializable). I kept the 7.0 name (more correct), requiring teh update to line 243 below.
@@ -18,7 +18,7 @@ public partial class LoggerMessageGenerator | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exact copy of 7.0 line.
@@ -7,6 +7,7 @@ | |||
using System.Text; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exact copy of 7.0 line.
@@ -8,6 +8,20 @@ | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exact copy of 7.0 line.
@@ -11,6 +11,7 @@ | |||
using Microsoft.CodeAnalysis; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exact copy of 7.0 line.
@@ -13,4 +13,18 @@ | |||
<Compile Include="JsonSourceGenerator.Roslyn4.0.cs" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exact copy of 7.0 line.
@@ -6,7 +6,6 @@ | |||
</PropertyGroup> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exact copy of 7.0 line.
src/libraries/System.Text.RegularExpressions/src/System.Text.RegularExpressions.csproj
Outdated
Show resolved
Hide resolved
…th packages that need to be serviced.
The failure in OSX seems to be related. Looks like an issue with the Logging generator:
|
cc: @layomia @eiriktsarpalis @steveharter for the Json Source generator changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
STJ changes LGTM.
Tagging subscribers to this area: @dotnet/area-extensions-logging Issue DetailsDraft until i make sure i didn't break anything. Backport of: #71653 and #71651. Bulk of code added in #70911. However that PR also updated the RegexGenerator (which does not exist in 6.0 and thus does not need to be serviced). Note: this is a polyfill of an API that roslyn has added. Once SDK can move to a version of roslyn containing the api, it can reference that API directly and remove the 1000 lines of added code for the polyfill. Servicing Template:Description:Primary goal here is to use a more efficient incrementally built up system for determining what classes should be examined to see if we need the source generators to run. Previously, all types in a solution would be examined (costly in CPU and memory), now only types with attributes on it (that we can show could actually bind to the desired attribute) are checked. As in practice this is a tiny subset of actual types in a solution (often zero) this vastly decreases the amount of work we needed to be done. -- Port was very clean, requiring only one deviating line between it and the code that exists in the 7.0 line. Customer Impact:Improved performance of 'in box' generators when in visual studio. The existing two generators cause lots of CPU and memory churn when processing. This is because each of them effectively walks all files/types looking for potential matches. This forces continual reparsing of files, and then lots of semantic binding on potential matches. For large projects (consider thousands of files being normal), this is an enormous amount of work that can use tons of CPU and memory. In Roslyn this can be particularly bad, leading to our server getting bogged down and slowing down all language services. The new approach uses the incremental generator engine to incrementally build up a list of 'actually viable classes' to examine. The intuition generally being: if you only care about types with "JsonSerializable" on them, then only check types that have that attribute on it syntactically. This is slightly more involved than just a trivial name check because things like aliases/global-aliases mean that someone could go Regression?Yes. This was a perf (but not functionality) regression for anyone with a large solution and who moved to .net 6. Note taht a customer doesn't need to be using the generator to have the perf hit. Just referencing the SDK is enough as it will cause Roslyn to run the generator, which does all expensive work, just to determine that is has nothing to generate. RiskI would consider this low. This is a non-trivial amount of code going in. However, it has been extensively tested in roslyn, with a large test suite, and has been validated for the 7.0 sdk. That said, incremental source generation is itself complex (one of the reasons that the perf issue arose in teh first place), so this is definitely more than a trivial fix. Link the PR to the original issue and to the PR to main.Issue PR: Main PRs: This was multiple PRs as i separated out the initial polyfill, and then did a single PR per generator to update. Note any packaging impact. (If the changes touch code in a package that we ship out of band (OOB) then we will need to take care to ensure those packages ship.)Yes. This impacts the Microsoft.Extensions.Logging.Abstractions NuGet package as well as the System.Text.Json NuGet package. Those two ship source generators that are being edited here so we will need to service those two. Note any ref pack impact. (Touches files contained in Microsoft.NETCore.App.Ref, Microsoft.AspNetCore.App.Ref, or Microsoft.WindowsDesktop.App.Ref).Yes. This impacts the Microsoft.NETCore.App.Ref since the Json Source generator ships in the ref pack as well, and it is being serviced by this change.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logging generator changes LGTM
Merge depends on 17.4 Preview 1 testing. |
Tagging subscribers to this area: @dotnet/area-meta Issue DetailsBackport of: #71653 and #71651. Bulk of code added in #70911. However that PR also updated the RegexGenerator (which does not exist in 6.0 and thus does not need to be serviced). Note: this is a polyfill of an API that roslyn has added. Once SDK can move to a version of roslyn containing the api, it can reference that API directly and remove the 1000 lines of added code for the polyfill. Servicing Template:Description:Primary goal here is to use a more efficient incrementally built up system for determining what classes should be examined to see if we need the source generators to run. Previously, all types in a solution would be examined (costly in CPU and memory), now only types with attributes on it (that we can show could actually bind to the desired attribute) are checked. As in practice this is a tiny subset of actual types in a solution (often zero) this vastly decreases the amount of work we needed to be done. -- Port was very clean, requiring only one deviating line between it and the code that exists in the 7.0 line. Customer Impact:Improved performance of 'in box' generators when in visual studio. The existing two generators cause lots of CPU and memory churn when processing. This is because each of them effectively walks all files/types looking for potential matches. This forces continual reparsing of files, and then lots of semantic binding on potential matches. For large projects (consider thousands of files being normal), this is an enormous amount of work that can use tons of CPU and memory. In Roslyn this can be particularly bad, leading to our server getting bogged down and slowing down all language services. The new approach uses the incremental generator engine to incrementally build up a list of 'actually viable classes' to examine. The intuition generally being: if you only care about types with "JsonSerializable" on them, then only check types that have that attribute on it syntactically. This is slightly more involved than just a trivial name check because things like aliases/global-aliases mean that someone could go Regression?Yes. This was a perf (but not functionality) regression for anyone with a large solution and who moved to .net 6. Note taht a customer doesn't need to be using the generator to have the perf hit. Just referencing the SDK is enough as it will cause Roslyn to run the generator, which does all expensive work, just to determine that is has nothing to generate. RiskI would consider this low. This is a non-trivial amount of code going in. However, it has been extensively tested in roslyn, with a large test suite, and has been validated for the 7.0 sdk. That said, incremental source generation is itself complex (one of the reasons that the perf issue arose in teh first place), so this is definitely more than a trivial fix. Link the PR to the original issue and to the PR to main.Issue PR: Main PRs: This was multiple PRs as i separated out the initial polyfill, and then did a single PR per generator to update. Note any packaging impact. (If the changes touch code in a package that we ship out of band (OOB) then we will need to take care to ensure those packages ship.)Yes. This impacts the Microsoft.Extensions.Logging.Abstractions NuGet package as well as the System.Text.Json NuGet package. Those two ship source generators that are being edited here so we will need to service those two. Note any ref pack impact. (Touches files contained in Microsoft.NETCore.App.Ref, Microsoft.AspNetCore.App.Ref, or Microsoft.WindowsDesktop.App.Ref).Yes. This impacts the Microsoft.NETCore.App.Ref since the Json Source generator ships in the ref pack as well, and it is being serviced by this change.
|
I labeled it Meta as it is covering multiple areas. |
@rbhanda who should take care of this so I can merge it? |
@rbhanda @joperezr @CyrusNajmabadi is this good to merge or do we still need confirmation of the testing? Today is the due date for merging backports that we intend to add to the September servicing release. |
Tactics approved. |
Backport of: #71653 and #71651.
Bulk of code added in #70911. However that PR also updated the RegexGenerator (which does not exist in 6.0 and thus does not need to be serviced).
Note: this is a polyfill of an API that roslyn has added. Once SDK can move to a version of roslyn containing the api, it can reference that API directly and remove the 1000 lines of added code for the polyfill.
Servicing Template:
Description:
Primary goal here is to use a more efficient incrementally built up system for determining what classes should be examined to see if we need the source generators to run. Previously, all types in a solution would be examined (costly in CPU and memory), now only types with attributes on it (that we can show could actually bind to the desired attribute) are checked. As in practice this is a tiny subset of actual types in a solution (often zero) this vastly decreases the amount of work we needed to be done.
--
Port was very clean, requiring only one deviating line between it and the code that exists in the 7.0 line.
Customer Impact:
Improved performance of 'in box' generators when in visual studio. The existing two generators cause lots of CPU and memory churn when processing. This is because each of them effectively walks all files/types looking for potential matches. This forces continual reparsing of files, and then lots of semantic binding on potential matches. For large projects (consider thousands of files being normal), this is an enormous amount of work that can use tons of CPU and memory. In Roslyn this can be particularly bad, leading to our server getting bogged down and slowing down all language services.
The new approach uses the incremental generator engine to incrementally build up a list of 'actually viable classes' to examine. The intuition generally being: if you only care about types with "JsonSerializable" on them, then only check types that have that attribute on it syntactically. This is slightly more involved than just a trivial name check because things like aliases/global-aliases mean that someone could go
global using Foo = System.Text.Json.JsonSerializable;
in one file and then[Foo] class MyClass
in another. However, with this, we can narrow down to only the set of types that are actually relevant, not all types. Furthermore, in projects that reference the sdk but do not use the generator at all, there will be zero potential matches, and so no semantic work has to happen at all.Regression?
Yes. This was a perf (but not functionality) regression for anyone with a large solution and who moved to .net 6. Note taht a customer doesn't need to be using the generator to have the perf hit. Just referencing the SDK is enough as it will cause Roslyn to run the generator, which does all expensive work, just to determine that is has nothing to generate.
Risk
I would consider this low. This is a non-trivial amount of code going in. However, it has been extensively tested in roslyn, with a large test suite, and has been validated for the 7.0 sdk. That said, incremental source generation is itself complex (one of the reasons that the perf issue arose in teh first place), so this is definitely more than a trivial fix.
Link the PR to the original issue and to the PR to main.
Issue PR:
#70754
Main PRs:
#71653
#71651
#70911
This was multiple PRs as i separated out the initial polyfill, and then did a single PR per generator to update.
Note any packaging impact. (If the changes touch code in a package that we ship out of band (OOB) then we will need to take care to ensure those packages ship.)
Yes. This impacts the Microsoft.Extensions.Logging.Abstractions NuGet package as well as the System.Text.Json NuGet package. Those two ship source generators that are being edited here so we will need to service those two.
Note any ref pack impact. (Touches files contained in Microsoft.NETCore.App.Ref, Microsoft.AspNetCore.App.Ref, or Microsoft.WindowsDesktop.App.Ref).
Yes. This impacts the Microsoft.NETCore.App.Ref since the Json Source generator ships in the ref pack as well, and it is being serviced by this change.