Repository navigation
Make the STJ source generator cache friendly - #86121
Conversation
|
Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis Issue DetailsThis PR attempts to improve the cache friendliness of the incremental source generator by making the following changes:
Contributes to #68353.
|
| using System.Text.Json.Serialization; | ||
| using System.Text.Json.Reflection; | ||
| using System.Diagnostics; | ||
| using Microsoft.CodeAnalysis; |
There was a problem hiding this comment.
This file has been moved under the Model folder, but git seems to think it's been deleted.
| private const string JsonTypeInfoTypeRef = "global::System.Text.Json.Serialization.Metadata.JsonTypeInfo"; | ||
| private const string JsonTypeInfoResolverTypeRef = "global::System.Text.Json.Serialization.Metadata.IJsonTypeInfoResolver"; | ||
|
|
||
| private static DiagnosticDescriptor TypeNotSupported { get; } = new DiagnosticDescriptor( |
There was a problem hiding this comment.
Descriptors moved to the Parser class.
| string FormatNumber() => $"({type.FullyQualifiedName})({Convert.ToString(value, CultureInfo.InvariantCulture)})"; | ||
| } | ||
|
|
||
| private static bool ShouldGenerateMetadata(TypeGenerationSpec typeSpec) |
There was a problem hiding this comment.
Methods from here on have been moved over from the TypeGenerationSpec class.
| // Needed for converter validation | ||
| private readonly Type? _jsonConverterOfTType; | ||
|
|
||
| // Keeps track of generated context type names |
There was a problem hiding this comment.
Moved from the Emitter class.
| var typeRef = new TypeRef(type); | ||
| typeInfoPropertyName ??= type.GetTypeInfoPropertyName(); | ||
|
|
||
| if (classType is ClassType.TypeUnsupportedBySourceGen) |
There was a problem hiding this comment.
Diagnostic reports moved over from the Emitter class.
| out bool setterIsVirtual, | ||
| out bool setterIsInitOnly); | ||
|
|
||
| if (!isPublic && !memberType.IsPublic) |
There was a problem hiding this comment.
Checks extracted from the callsite.
| } | ||
|
|
||
| [Fact] | ||
| public void CanGetAttributes() |
There was a problem hiding this comment.
Test deleted since the source generator no longer surfaces "TypeWrapper" values. The reflection wrappers will eventually get removed in a follow-up PR.
| #else | ||
| public | ||
| #endif | ||
| enum JsonIgnoreCondition |
There was a problem hiding this comment.
They are part of the SG model which I made public for unit testing.
There was a problem hiding this comment.
If it's necessary for unit testing could we leave them as internal and use InternalsVisibleTo from the unit tests? It's probably not a huge deal, but exposing public surface area from the source generator does create an opportunity for someone to reference it and depend on these. We intend to not support that, but... yeah.
There was a problem hiding this comment.
I discussed this with @ericstj and @layomia and we figured that these wouldn't be public in practice since there's no easy way for users to reference them directly. The source generator classes themselves are public already but we don't particularly care about keeping them backward compatible. The SDK assemblies appear to also have public types for the same reason. I'd rather we fall back to IVT if there's strong pushback to the idea of making the model types public.
cc @ViktorHofer
There was a problem hiding this comment.
I don't have a strong preference here just wanted to share that I noticed some issues with IVT in source generators. I.e. winforms heavily depends on IVT for their testing and when adding polyfills to netstandard2.0 like NullableAttributes.cs (which runtime does implicitly you get compiler errors. That's because the internal types are visible by the consumer which clash with the ones provided via the framework, i.e. when targeting .NETCoreApp.
Unsure if our source generator tests reference the source generator output assemblies directly though.
Co-authored-by: Stephen Toub <stoub@microsoft.com>
| /// </summary> | ||
| [return: NotNullIfNotNull(nameof(location))] | ||
| public static Location? GetTrimmedLocation(this Location? location) | ||
| => location is null ? null : Location.Create(location.SourceTree?.FilePath ?? "", location.SourceSpan, location.GetLineSpan().Span); |
There was a problem hiding this comment.
We've now duplicated this in several of our generators. @chsienki, should Roslyn include helpers for this (and maybe more generally on a Diagnostic so that someone writing a source generator can easily avoid referencing the compilation from a Diagnostic they hold onto)?
There was a problem hiding this comment.
Yeah, that seems like a good candidate for a new API
stephentoub
left a comment
stephentoub
left a comment
There was a problem hiding this comment.
Have you validated this has the desired impact on recomputation in VS?
…ay.cs Co-authored-by: Stephen Toub <stoub@microsoft.com>
Yes, both in unit testing and debugging VS itself. Curiously enough though it appears to be re-triggering periodically every ~20 seconds or so regardless of whether it is needed or not. @chsienki @sharwell is this expected behavior? |
| foreach (TypeGenerationSpec spec in _currentContext.GeneratedTypes) | ||
| { | ||
| GenerateTypeInfo(typeGenerationSpec); | ||
| _typeIndex.Add(spec.TypeRef, spec); |
There was a problem hiding this comment.
nit: _typeCache is a bit more readable.
There was a problem hiding this comment.
Will address in a follow-up.
| namespace System.Text.Json.SourceGeneration | ||
| { | ||
| internal enum ClassType | ||
| public enum ClassType |
There was a problem hiding this comment.
nit: we can remove the ctors part of line 9. Also this is tangential but at this point are there any types that we don't emit source for? I remember multi-dim arrays but we might have switched to generating metadata that throws NotSupportedException in some way.
There was a problem hiding this comment.
We can, and we should eventually. I wanted to scope this PR to refactoring only and avoid any changes to the underlying logic.
|
|
||
| namespace System.Text.Json.SourceGeneration | ||
| { | ||
| /// <summary> |
There was a problem hiding this comment.
Could be used here to avoid duplication and future divergence?
| #if DEBUG | ||
| MetadataLoadContext = _metadataLoadContext, | ||
| #endif | ||
| ContextGenerationSpecs = contextGenSpecList.ToImmutableEquatableArray(), |
There was a problem hiding this comment.
nit: match the names
| ContextGenerationSpecs = contextGenSpecList.ToImmutableEquatableArray(), | |
| ContextGenerationSpecs = contextGenSpecs.ToImmutableEquatableArray(), |
There was a problem hiding this comment.
Will fix in a follow-up PR.
| out bool setterIsVirtual, | ||
| out bool setterIsInitOnly); | ||
|
|
||
| if (!isPublic && !memberType.IsPublic) |
| string FormatNumber() => $"({type.FullyQualifiedName})({Convert.ToString(value, CultureInfo.InvariantCulture)})"; | ||
| } | ||
|
|
||
| private static bool ShouldGenerateMetadata(TypeGenerationSpec typeSpec) |
There was a problem hiding this comment.
Should the bool value be a property on TypeGenerationSpec?
There was a problem hiding this comment.
Yes, I'm planning on doing this in a follow-up PR. Generally speaking the emitter should just emit and have no domain logic of its own. But I wanted to avoid adding new properties in the scope of this PR.
| private static bool IsGenerationModeSpecified(TypeGenerationSpec typeSpec, JsonSourceGenerationMode mode) | ||
| => typeSpec.GenerationMode == JsonSourceGenerationMode.Default || (mode & typeSpec.GenerationMode) != 0; | ||
|
|
||
| public static bool TryFilterSerializableProps( |
There was a problem hiding this comment.
We discussed before that this should be precomputed by the parser...
There was a problem hiding this comment.
Agreed, and I'll move in a follow-up PR. For this change I only wanted to make the minimum number of rearrangements to make caching viable without making substantial changes to the core logic.
| private static readonly CSharpParseOptions s_parseOptions = | ||
| new CSharpParseOptions(kind: SourceCodeKind.Regular, documentationMode: DocumentationMode.Parse); | ||
|
|
||
| #if ROSLYN4_0_OR_GREATER |
There was a problem hiding this comment.
Does this PR provide an opportunity to switch to some helpers in RoslynTestUtils? https://github.com/dotnet/runtime/blob/main/src/libraries/Common/tests/SourceGenerators/RoslynTestUtils.cs
When you say re-triggering, do you mean the whole generator is running, or just the inputs, and the generator finishes early because everything is cached? |
The whole thing E2E. |
ghost
locked as resolved and limited conversation to collaborators
This PR attempts to improve the cache friendliness of the incremental source generator by making the following changes:
SourceProductionContextdependencies from theParserclass.CompilationandISymboldependencies from theEmitterclass.SourceGenerationSpecmodel type immutable and structurally equatable so that it can be cached incrementally.Contributes to #68353.