System.Text.Json incremental source generator performance issues

[eiriktsarpalis]

The System.Text.Json.SourceGeneration.Roslyn4.0.Tests project defines ~35 large JsonSerializerContext classes, resulting in approximately 3500 generated source files. The size of the project has had a detrimental effect on performance, both when building but especially when editing the project: on my primary dev machine the IDE essentially goes unresponsive when attempting to make any change in the project.

I ran a performance investigation and came up with the following findings:

1. There appear to be performance issues inherent in Roslyn ingesting such big numbers of generated source files. On my machine a build will max out all 20 cores of my CPU and consume 4-7 GB of memory. It can be isolated to making a large number of calls to the SourceProductionContext.AddSource method and is not related to the source generator itself.
2. Our incremental source generator implementation will regenerate every single source file on every keystroke. This is attributable to a combination of two factors:
   • Our incremental values are combining the entire Compilation object. This is explicitly called out as an anti-pattern in the incremental source generation doc, since any change will force re-evaluation of the source generator.
   • We are calling Collect() on the ClassDeclarationSyntax values provider, feeding every single JsonSerializerContext declaration to the source generator regardless of whether it's a newly updated or cached value.
3. The source generator is not honoring the CancellationToken passed by Roslyn. Fixed in Make json source generation cancellable #69332

I'm not sure if 1) could be improved, however it definitely seems like 2) is a performance bug in our source generator implementation that we should try to fix. Not sure how it could be done without some refactoring of the Parser class.

cc @layomia @ericstj @eerhardt

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis
See info in area-owners.md if you want to be subscribed.

Issue Details

---

The System.Text.Json.SourceGeneration.Roslyn4.0.Tests project defines ~35 large JsonSerializerContext classes, resulting in approximately 3500 generated source files. The size of the project has had a detrimental effect on performance, both when building but especially when editing the project: on my primary dev machine the IDE essentially goes unresponsive when attempting to make any change in the project.

I ran a performance investigation and came up with the following findings:

1. There appear to be performance issues inherent in Roslyn ingesting such big numbers of generated source files. On my machine a build will max out all 20 cores of my CPU and consume 4-7 GB of memory. It can be isolated to making a large number of calls to the SourceProductionContext.AddSource method and is not related to the source generator itself.
2. Our incremental source generator implementation will regenerate every single source file on every keystroke. This is attributable to a combination of two factors:
   • Our incremental values are combining the entire Compilation object. This is explicitly called out as an anti-pattern in the incremental source generation doc, since any change will force re-evaluation of the source generator.
   • We are calling Collect() on the ClassDeclarationSyntax values provider, feeding every single JsonSerializerContext declaration to the source generator regardless of whether it's a newly updated or cached value.
3. The source generator is not honoring the CancellationToken passed by Roslyn.

I'm not sure if 1) could be improved, however it definitely seems like 2) is a performance bug in our source generator implementation that we should try to fix. Not sure how it could be done without some refactoring of the Parser class.

cc @layomia @ericstj @eerhardt

Author:	eiriktsarpalis
Assignees:	-
Labels:	area-System.Text.Json, untriaged
Milestone:	-

[eiriktsarpalis]

cc @chsienki

[eiriktsarpalis]

Here's a naive workaround that improves performance when editing the project in Visual Studio. It avoids regenerating sources for unchanged context classes by avoiding Collect() and invoking the source generator separately for each newly updated class. It also employs a custom equality comparer that ignores the Compilation component so that arbitrary changes do not trigger source generation.

As it stands though the workaround suffers from a couple of issues:

1. Invoking the source generator independently for every source class is suboptimal in batch compilation. I was looking for a variant of IncrementValuesProvider.Collect() that only surfaces the newly updated values but such functionality doesn't seem to exist.
2. The source generator will only trigger if the context classes themselves are modified, but not if any of their JsonSerializableAttribute dependencies are. A refinement of this might need to resolve the syntax nodes from the attribute symbols but I'm not sure if and how this might be possible.

[ericstj]

We clearly have a lot more to learn about building these. Do we have the right inputs and APIs available to make our source-regeneration incremental? I know @eiriktsarpalis and @sharwell looked at this before and made improvements.

One thing to be careful of -- which I noticed for our own SLN -- is that it seemed to be loading the "first" source generator for JSON that it hit which was sometimes the old non-incremental one. It wouldn't load the incremental version since it had the exact same name. That's an internal only problem since customers would see only the one appropriate for their compiler version.

[layomia]

[With this comment I'm assuming that generating less files might have less impact on memory and cores, even if the material content generated remains the same]

It can be isolated to making a large number of calls to the SourceProductionContext.AddSource method and is not related to the source generator itself.

I recall @eerhardt gave feedback about reducing the number of files we output. Today we generate individual files for the following:

1. the template context implementation including ctors, intialization of source-generated options, helpers to get converters
2. the JsonSerializerContext.GetTypeInfo implementation
3. JsonEncodedText-typed members for property names used for fast-path deserialization
4. unique files for every type specified on the context, including types nested in their object graphs (this can be a lot)

Since we've identified this as a perf bottleneck, we should consider combining these files (especially the first 3). Unique files for each type is nice for debugging source-generated code for a single type, so it might be helpful to define a file-generation strategy, say based on an msbuild property.

[layomia]

Issues 2 & 3 should be looked into and addressed.

One thing to be careful of -- which I noticed for our own SLN -- is that it seemed to be loading the "first" source generator for JSON that it hit which was sometimes the old non-incremental one. It wouldn't load the incremental version since it had the exact same name. That's an internal only problem since customers would see only the one appropriate for their compiler version.

Definitely worth looking into resolving this.

[sharwell]

It also employs a custom equality comparer that ignores the Compilation component

Never do this. This action instructs the command line compiler that it is allowed to produce inaccurate outputs in an actual .dll or .exe, and will allow a production machine to publish/sign/ship that invalid output.

[sharwell]

will regenerate every single source file on every keystroke

There were several Roslyn bugs that were causing this to occur when it shouldn't. Roslyn's design is to not regenerate sources on every keystroke regardless of the implementation of the source generator itself, and either 17.2 or 17.3 has been updated to better reflect this desired behavior.

[eiriktsarpalis]

One thing to be careful of -- which I noticed for our own SLN -- is that it seemed to be loading the "first" source generator for JSON that it hit which was sometimes the old non-incremental one. It wouldn't load the incremental version since it had the exact same name. That's an internal only problem since customers would see only the one appropriate for their compiler version.

Definitely worth looking into resolving this.

FWIW I deleted the Roslyn3.1 projects locally to ensure no interference and can confirm that the performance problems persist even with just the 4.0 projects present.

[eiriktsarpalis]

It also employs a custom equality comparer that ignores the Compilation component

Never do this. This action instructs the command line compiler that it is allowed to produce inaccurate outputs in an actual .dll or .exe, and will allow a production machine to publish/sign/ship that invalid output.

That's interesting, I would have assumed there were no incremental caching concerns when building from the command line. I can see how this can cause the source generator to fire less frequently than is desirable in interactive scenaria, but I don't think it should be capable of passing unsound inputs to the source generator.

[eiriktsarpalis]

will regenerate every single source file on every keystroke

There were several Roslyn bugs that were causing this to occur when it shouldn't. Roslyn's design is to not regenerate sources on every keystroke regardless of the implementation of the source generator itself, and either 17.2 or 17.3 has been updated to better reflect this desired behavior.

Hmmm, I tried attaching the Roslyn process from the latest VS 17.3 to the debugger and it would seem like every edit still triggers a new compilation.