Skip to content

Revert System.Text.Json source generator changes from PR #124077#124186

Merged
eiriktsarpalis merged 5 commits intomainfrom
copilot/revert-stj-source-generator-changes
Feb 11, 2026
Merged

Revert System.Text.Json source generator changes from PR #124077#124186
eiriktsarpalis merged 5 commits intomainfrom
copilot/revert-stj-source-generator-changes

Conversation

Copy link
Contributor

Copilot AI commented Feb 9, 2026 •
edited
Loading

Description

Reverts System.Text.Json changes from PR #124077 per @eiriktsarpalis feedback. The change caused a performance regression by breaking the 1-1 correspondence between Compilation and KnownTypeSymbols instances, creating fresh instances for every JsonSerializerContext instead of caching per compilation.

LoggerMessageGenerator changes remain unchanged.

Changes

Restored the original incremental pipeline pattern in JsonSourceGenerator.Roslyn4.0.cs:

Before (PR #124077):

contextGenerationSpecs = context.SyntaxProvider
    .ForAttributeWithMetadataName(
        ...,
        (context, cancellationToken) =>
        {
            var knownTypeSymbols = new KnownTypeSymbols(context.SemanticModel.Compilation);
            Parser parser = new(knownTypeSymbols);
            // ... parsing
        })

After (this PR):

IncrementalValueProvider<KnownTypeSymbols> knownTypeSymbols = context.CompilationProvider
    .Select((compilation, _) => new KnownTypeSymbols(compilation));

contextGenerationSpecs = context.SyntaxProvider
    .ForAttributeWithMetadataName(
        ...,
        (context, _) => (ContextClass: context.TargetNode, context.SemanticModel))
    .Combine(knownTypeSymbols)
    .Select(static (tuple, cancellationToken) =>
    {
        Parser parser = new(tuple.Right);  // KnownTypeSymbols
        // ... parsing using tuple.Left.ContextClass, tuple.Left.SemanticModel
    })

This ensures KnownTypeSymbols is cached per compilation in the incremental pipeline, eliminating redundant type symbol resolution.

Testing

  • Built clr+libs successfully
  • All 154 System.Text.Json.SourceGeneration unit tests pass
Original prompt

Context

PR #124077 ("Improve LoggerMessageGenerator and JsonSourceGenerator incrementality") was merged and included changes to both the LoggerMessageGenerator and JsonSourceGenerator. However, per feedback from @eiriktsarpalis in #124077 (comment), the System.Text.Json changes should be reverted because:

  1. Passing Compilation in the incremental graph does not automatically imply re-runs of the source generator every time it gets updated. This is only true in cases where such updates flow all the way to the SourceProductionContext. The STJ generator does not do this (source production is only triggered on the basis of equality of the resultant ContextGenerationSpec) and they have testing that validates that property.
  2. More importantly, the change actually regresses performance: KnownTypeSymbols defines a cache of known type symbols that the generator needs to reference. By removing it from the incremental graph, there is no longer a 1-1 correspondence between Compilation instances and KnownTypeSymbols instances, instead creating a fresh instance every time a JsonSerializerContext is processed for a given compilation.

What needs to be done

Revert ONLY the changes to src/libraries/System.Text.Json/gen/JsonSourceGenerator.Roslyn4.0.cs introduced by PR #124077 (merge commit 5709f35ed344f59062042d234b9a68683ec7e7a6).

The file was changed to:

  • Remove the CompilationProvider dependency and the knownTypeSymbols incremental value provider
  • Move KnownTypeSymbols instantiation into the ForAttributeWithMetadataName transform callback
  • Restructure the pipeline to no longer use .Combine(knownTypeSymbols) and .Select()

The revert should restore the original pattern where:

  • KnownTypeSymbols is created via context.CompilationProvider.Select(...) as an IncrementalValueProvider<KnownTypeSymbols>
  • The attribute provider uses .Combine(knownTypeSymbols)
  • A subsequent .Select() performs the parsing using tuple.Left (for the context class + semantic model) and tuple.Right (for the known type symbols)
  • The ForAttributeWithMetadataName transform only extracts (ContextClass, SemanticModel) without doing any parsing

Do NOT revert any LoggerMessageGenerator changes — those are correct and should remain as-is.

This pull request was created from Copilot chat.

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

@eiriktsarpalis
Revert System.Text.Json source generator changes from PR #124077
cf6a601 
Restore the original pattern where KnownTypeSymbols is created via
CompilationProvider to maintain 1-1 correspondence between Compilation
and KnownTypeSymbols instances, avoiding performance regression.

Co-authored-by: eiriktsarpalis <[email protected]>
@github-actions github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Feb 9, 2026
@tarekgh tarekgh mentioned this pull request Feb 9, 2026
Merged
@tarekgh tarekgh added area-System.Text.Json source-generator Indicates an issue with a source generator feature and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Feb 9, 2026
@tarekgh tarekgh added this to the 11.0.0 milestone Feb 9, 2026
@dotnet-policy-service
Copy link
Contributor

dotnet-policy-service bot commented Feb 9, 2026

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

Copilot AI changed the title [WIP] Revert System.Text.Json changes from LoggerMessageGenerator update Revert System.Text.Json source generator changes from PR #124077 Feb 9, 2026
Copilot AI requested a review from eiriktsarpalis February 9, 2026 17:45
@eiriktsarpalis eiriktsarpalis marked this pull request as ready for review February 9, 2026 17:45
Copilot AI review requested due to automatic review settings February 9, 2026 17:45
@eiriktsarpalis eiriktsarpalis enabled auto-merge (squash) February 9, 2026 17:45
Copilot AI reviewed Feb 9, 2026
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR reverts the System.Text.Json source generator pipeline change from PR #124077 to restore per-compilation caching of KnownTypeSymbols, avoiding redundant symbol resolution and the associated performance regression.

Changes:

  • Reintroduced context.CompilationProvider.Select(...) to create an incremental KnownTypeSymbols value cached per Compilation.
  • Restored the .Combine(knownTypeSymbols).Select(...) pipeline shape so parsing uses the combined (SemanticModel, ContextClass) + cached KnownTypeSymbols.
  • Moved KnownTypeSymbols construction back out of the ForAttributeWithMetadataName transform.

This was referenced Feb 9, 2026
Open
Open
@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Feb 10, 2026

/ba-g test failures unrelated

@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Feb 10, 2026

/ba-g test failures unrelated

1 similar comment
@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Feb 10, 2026

/ba-g test failures unrelated

@eiriktsarpalis eiriktsarpalis merged commit 848b99b into main Feb 11, 2026
82 of 90 checks passed
@eiriktsarpalis eiriktsarpalis deleted the copilot/revert-stj-source-generator-changes branch February 11, 2026 08:01
@dotnet-maestro dotnet-maestro bot mentioned this pull request Feb 12, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@tarekgh tarekgh tarekgh approved these changes

+1 more reviewer

@eiriktsarpalis eiriktsarpalis eiriktsarpalis approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@eiriktsarpalis eiriktsarpalis

Copilot code review Copilot

Labels

area-System.Text.Json source-generator Indicates an issue with a source generator feature

Projects

None yet

Milestone

11.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@eiriktsarpalis @tarekgh