Repository navigation
Update .Net source generators to use new SyntaxValueProvider.ForAttributeWithMetadataName api. #70754
Description
Roslyn recently added a new API for greatly speeding up SGs that use the pattern of looking for nodes that have an attribute on it with a particular fully qualified name for the attribute. For example, the json serialization generator looks for this attribute:
Currently, this is extremely expensive as on every edit, the generator must hit every attributed class in every file and semantically check if it has an attribute that binds to this fully qualified name. This may mean thousands of files bound, with expensive semantic models and binding checks performed, all for nodes+attributes completely unrelated to this SG.
The added API provides a new entry point to get the same functionality as previously possible, but with much better performance. Now, the generator says what attributes they care about (passing in teh fully qualified metadata name) and roslyn can smartly examine the source code and not even bother doing any binding or semantics when it would be pointless. Intuitively, you can think of it that when roslyn sees [FooBar] it goes "well, FooBar (and FooBarAttribute) could not ever bind to JsonSerializableAttribute, so it's pointless to even try". This massively reduces the amount of work done (>40x in roslyn itself) especially when the attribute is never, or almost never, used.
Once this api is available in a consumable roslyn package, we should update all the runtime's SGs to use it. That includes the generators for
Regex. Done in Polyfill the incremental generator ForAttributeWithMetadataName from roslyn. #70911. This also added the main polyfill code.Json. Done in Polyfill the incremental generator ForAttributeWithMetadataName from roslyn (for JsonGenerator). #71653Logging. Done in Polyfill the incremental generator ForAttributeWithMetadataName from roslyn (for LoggingGenerator). #71651eventsource. Done in Polyfill the incremental generator ForAttributeWithMetadataName from roslyn (for EventSourceGeneration). #71662- libraryimport. in progress: Polyfill the incremental generator ForAttributeWithMetadataName from roslyn (for LibraryImportGenerator). #71652
Activity
CyrusNajmabadi commentedon Jun 14, 2022
Tagging @stephentoub @ViktorHofer for visibility. I'm happy to contribute the changes on the runtime side for this if that can help out.
jkoritzinsky commentedon Jun 14, 2022
We need to make sure that we not only keep compatibility for downlevel generators (so introduce another build variant of the out-of-band packaged generators that uses the new roslyn API version) and ensure that we don't break uplevel repos like aspnetcore by adopting a Roslyn version that is newer than what many of them use (as we turn on in-box generators by default, which causes build failures when the Roslyn version the upstream repo uses is too old).
Generally, we try to keep the version of Roslyn that the "new" generators build against to at newest the version that ships with the SDK that dotnet/arcade is using to ensure we won't cause compile errors upstack.
runtime/eng/Versions.props
Lines 34 to 39 in 042fdf4
For generators that don't ship outside of dotnet/runtime, we can update them as soon as we have a NuGet package (so that includes the EventSource generator and the XUnitWrapperGenerator)
CyrusNajmabadi commentedon Jun 14, 2022
Note: teh major offenders i'm seeing in traces are Json and Logging. So it woudl be great to be able to update these asap and get these into a VS release that has the new updated API.
jkoritzinsky commentedon Jun 15, 2022
Since logging ships out-of-band, we could add another build dimension to it now against the newer Roslyn API version since we can put it into a different path in the NuGet package based on Roslyn version.
JSON ships in-box too, so we can't update the in-box version without breaking upstream repos, but we could update the package version for downlevel consumers immediately if we wanted to.
Basically we can update every generator project that isn't in the following list in some manner now (either by introducing new projects for the out-of-band generators or updating the in-repo ones directly), and then we can update the ones in the list after we know we won't break upstream repos.
runtime/src/libraries/NetCoreAppLibrary.props
Lines 187 to 191 in 6896e1b
danmoseley commentedon Jun 15, 2022
cc @joperezr who I believe is looking at source generator perf at the moment.
CyrusNajmabadi commentedon Jun 15, 2022
@stephentoub @jkoritzinsky
There's a potential hybrid option we can take.
So what does
most**mean? Well, currently the API depends on some internal functionality in order to support a very unlikely, but usefulfull correctnesscase. Specifically, the API will understand if you do the following:File1:
File2:
In other words, it knows that that global alias exists, and as such the json-generator should run on
Whatever. If you were willing to say that that was a currently unsupported scenario, we could port over the entirety of the new API except teh part that supports global aliases. Practically speaking, i think all users would still be fine. It would be up to you if this functionality break would be worth the massive speedup we're seeing (over 40x in roslyn itself).To me, given just how bad the perf problem is here, my preference is that we take the perf gain for all the generators, and we accept a highly unlikely functionality break. Then, when you can finally move all generators over to the new api we'll have teh best of both worlds. High performance, and complete correctness.
Any thoughts from your perspective?
ViktorHofer commentedon Jun 15, 2022
Here are my thoughts on this:
Based on that I would propose the following:
VS 17.4 but I could be mistakenEDIT: VS 17.3 Preview 3.cc @eerhardt @jaredpar
eerhardt commentedon Jun 15, 2022
The issue with this proposal is that projects building with the
6.0.x00SDK will now see degraded performance when they use the7.0.0System.Text.Json nupkg. With the6.0.xSystem.Text.Json nupkg, they will use theroslyn4.0version, which has incremental build support. When the project upgrades to the7.0.0nupkg, there is noroslyn4.0built version, so they will fall back toroslyn3.11and now all the incremental support we took advantage of in .NET 6 is removed.It is pretty easy to get into this situation AFAICT. You just need to be on a VS 2022 version that doesn't include .NET 7 - so let's say VS
17.0. You are using the6.0.0System.Text.Json nupkg, and NuGet Manager says a new version is out in the "Updates" tab, so you update to STJ7.0.0.ViktorHofer commentedon Jun 15, 2022
Agreed, that's a problem and needs to be solved, thanks for pointing that out. What makes this problematic is that we currently don't have a lifecycle policy for source generators when they ship inside packages. I just recently had this discussion with @eerhardt.
Visual Studio 2019, which only works with the roslyn3.11 source generators is supported until April 2029. Based on that, we would need to build and package our roslyn3.11 source generators for the next 6-7 years.
Visual Studio 2022 17.0 is supported until July 2023, 17.2 until January 2024 and the support date for 17.3 isn't yet announced. Because of these support dates, we would need to keep our roslyn4.0 targeted source generators around until at least .NET 9.
It's very likely that we will again depend on newer roslyn features going forward and with that build and package even more versions of our source generators. Packages will grow unbounded and the main branch hosts all different versions of the roslyn source generators.
I don't think there's a good solution to mitigate the increasing build complexity, package growth and ever expanding matrix of source generators. One option that we might want to discuss is to couple our package support policy with toolset versions. Just theoretically speaking, we could mark the System.Text.Json 7.0 package to not support Visual Studio 2019 or older SDKs that don't have a specific roslyn version available. I don't suggest that we should to that but wanted mention it.
27 remaining items
danmoseley commentedon Aug 24, 2022
Roslyn bug was fixed over a week ago. Are we still waiting on the flow?
jkoritzinsky commentedon Aug 24, 2022
If we update the generators to use the new Roslyn here, we need to update all consuming repositories as well.
I think the best solution will be to update the SDK in Arcade to use an SDK with the fix so we don’t need to manually upgrade the Roslyn version used in each repo.
danmoseley commentedon Aug 29, 2022
Who owns doing that -- is that you @jkoritzinsky , @joperezr or someone else?
joperezr commentedon Aug 29, 2022
I'm planning to take care of it this week.
CyrusNajmabadi commentedon Sep 2, 2022
@joperezr Anything left to do with this item? Or can we close it out?
joperezr commentedon Sep 6, 2022
PR isn't merged yet 😄. PR should be ready to go but I'm still waiting on making sure that all upstack repos have ingested an arcade update that will bump their compiler and SDK versions so that they'll be able to ingest the new source generators.
CyrusNajmabadi commentedon Oct 6, 2022
@joperezr Anything left to do with this item? Or can we close it out?
joperezr commentedon Oct 6, 2022
This can be closed now.
CyrusNajmabadi commentedon Oct 7, 2022
Thanks!