Move source-generators out of ProjectState and entirely into SolutinoCompilationState #72834

CyrusNajmabadi · 2024-04-01T22:31:11Z

This will help us control the lifetimes of these objects. The goal here is to only instantiate them on the OOP side. That way we don't have any issues where we create and cache the generators once on the VS side. In VS we can't reload them as we're netfx. Isolating the places where we access the generators means we can always defer to OOP to do anything we need in this regard.

This PR is just about isolating. Next will be actually moving the in proc calls to not instantiate the generators.

CyrusNajmabadi · 2024-04-01T22:34:51Z

src/Workspaces/Core/Portable/Workspace/Solution/ProjectState.cs

-        }
-    }
-
-    public IEnumerable<ISourceGenerator> SourceGenerators


this was too broad a surface area. we def did not want to expose inside the workspace that you could get to ISourceGenerator instances.

CyrusNajmabadi · 2024-04-01T22:35:07Z

src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.CompilationTracker.cs

@@ -422,7 +422,7 @@ async Task<InProgressState> CollapseInProgressStateAsync(InProgressState initial
                            // Also transform the compilation that has generated files; we won't do that though if the transformation either would cause problems with
                            // the generated documents, or if don't have any source generators in the first place.
                            if (translationAction.CanUpdateCompilationWithStaleGeneratedTreesIfGeneratorsGiveSameOutput &&
-                                translationAction.OldProjectState.SourceGenerators.Any())
+                                GetSourceGenerators(translationAction.OldProjectState).Any())


Note: this needs to not instantiate generators, but instead call into OOP to determine this info.

Considering that the SDK delivers source generators that are enabled by default, it might make sense to just assume the .Any() is true and focus on optimizing the specific case where none of the source generators adds any documents to the compilation.

CyrusNajmabadi · 2024-04-01T22:35:26Z

src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.CompilationTracker.cs

@@ -774,7 +774,7 @@ public ICompilationTracker FreezePartialState(CancellationToken cancellationToke
                SolutionCompilationState compilationState, CancellationToken cancellationToken)
            {
                // If we don't have any generators, then we know we have no generated files, so we can skip the computation entirely.
-                if (!this.ProjectState.SourceGenerators.Any())
+                if (!GetSourceGenerators(this.ProjectState).Any())


CyrusNajmabadi · 2024-04-01T22:35:32Z

src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.CompilationTracker.cs

@@ -787,7 +787,7 @@ public ICompilationTracker FreezePartialState(CancellationToken cancellationToke
            public async ValueTask<ImmutableArray<Diagnostic>> GetSourceGeneratorDiagnosticsAsync(
                SolutionCompilationState compilationState, CancellationToken cancellationToken)
            {
-                if (!this.ProjectState.SourceGenerators.Any())
+                if (!GetSourceGenerators(this.ProjectState).Any())


CyrusNajmabadi · 2024-04-01T22:35:36Z

src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.CompilationTracker.cs

@@ -816,7 +816,7 @@ public ICompilationTracker FreezePartialState(CancellationToken cancellationToke

            public async ValueTask<GeneratorDriverRunResult?> GetSourceGeneratorRunResultAsync(SolutionCompilationState compilationState, CancellationToken cancellationToken)
            {
-                if (!this.ProjectState.SourceGenerators.Any())
+                if (!GetSourceGenerators(this.ProjectState).Any())


CyrusNajmabadi · 2024-04-01T22:35:47Z

...s/Core/Portable/Workspace/Solution/SolutionCompilationState.CompilationTracker_Generators.cs

@@ -233,7 +233,7 @@ private partial class CompilationTracker : ICompilationTracker
            // the "InCurrentProcess" call so that it will normally run only in the OOP process, thus ensuring that we
            // get accurate information about what SourceGenerators we actually have (say, in case they they are rebuilt
            // by the user while VS is running).
-            if (!this.ProjectState.SourceGenerators.Any())
+            if (!GetSourceGenerators(this.ProjectState).Any())


this call is fine. We're already in OOP.

CyrusNajmabadi · 2024-04-01T22:36:00Z

...s/Core/Portable/Workspace/Solution/SolutionCompilationState.CompilationTracker_Generators.cs

@@ -293,7 +295,7 @@ private partial class CompilationTracker : ICompilationTracker
                if (IsGeneratorRunResultToIgnore(generatorResult))
                    continue;

-                var generatorAnalyzerReference = this.ProjectState.GetAnalyzerReferenceForGenerator(generatorResult.Generator);
+                var generatorAnalyzerReference = GetAnalyzerReference(this.ProjectState, generatorResult.Generator);


this call is fine. We're already in OOP.

CyrusNajmabadi · 2024-04-01T22:36:03Z

...s/Core/Portable/Workspace/Solution/SolutionCompilationState.CompilationTracker_Generators.cs

@@ -394,7 +396,7 @@ static GeneratorDriver CreateGeneratorDriver(ProjectState projectState)

                return compilationFactory.CreateGeneratorDriver(
                    projectState.ParseOptions!,
-                    projectState.SourceGenerators.ToImmutableArray(),
+                    GetSourceGenerators(projectState),


this call is fine. We're already in OOP.

CyrusNajmabadi · 2024-04-01T22:36:17Z

...paces/Core/Portable/Workspace/Solution/SolutionCompilationState.TranslationAction_Actions.cs

@@ -355,7 +355,7 @@ public override GeneratorDriver TransformGeneratorDriver(GeneratorDriver _)
                    .ReplaceAdditionalTexts(this.NewProjectState.AdditionalDocumentStates.SelectAsArray(static documentState => documentState.AdditionalText))
                    .WithUpdatedParseOptions(this.NewProjectState.ParseOptions!)
                    .WithUpdatedAnalyzerConfigOptions(this.NewProjectState.AnalyzerOptions.AnalyzerConfigOptionsProvider)
-                    .ReplaceGenerators(this.NewProjectState.SourceGenerators.ToImmutableArray());
+                    .ReplaceGenerators(GetSourceGenerators(this.NewProjectState));


this need to not happen on the VS side.

CyrusNajmabadi · 2024-04-02T14:33:57Z

@ToddGrun ptal :-)

sharwell

Overall seems acceptable. Would like the perf question answered before proceeding.

src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState_SourceGenerators.cs

question answered

src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState_SourceGenerators.cs

ToddGrun

CyrusNajmabadi · 2024-04-24T21:14:38Z

@jasonmalinowski For review when you get back.

CyrusNajmabadi added 5 commits April 1, 2024 12:44

Merge remote-tracking branch 'upstream/main' into sgOnlyOop2

8af4d04

Merge remote-tracking branch 'upstream/main' into sgOnlyOop2

b625c7a

in progress

7d13eae

Use tuple

d26d6d5

Simplify

e3b7872

CyrusNajmabadi requested a review from a team as a code owner April 1, 2024 22:31

dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Apr 1, 2024

CyrusNajmabadi requested a review from ToddGrun April 1, 2024 22:34

CyrusNajmabadi commented Apr 1, 2024

View reviewed changes

sharwell previously requested changes Apr 2, 2024

View reviewed changes

src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState_SourceGenerators.cs Show resolved Hide resolved

sharwell approved these changes Apr 2, 2024

View reviewed changes

ToddGrun reviewed Apr 2, 2024

View reviewed changes

src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState_SourceGenerators.cs Show resolved Hide resolved

ToddGrun approved these changes Apr 2, 2024

View reviewed changes

CyrusNajmabadi merged commit 055d1d4 into dotnet:main Apr 2, 2024
27 checks passed

CyrusNajmabadi deleted the sgOnlyOop2 branch April 2, 2024 17:23

dotnet-policy-service bot added this to the Next milestone Apr 2, 2024

This was referenced Apr 13, 2024

[Automated] PRs inserted in VS build main-34812.241 #73011

Closed

[Automated] PRs inserted in VS build feature.debugger.main-34815.163 #73037

Closed

dibarbet modified the milestones: Next, 17.11 P1 Apr 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move source-generators out of ProjectState and entirely into SolutinoCompilationState #72834

Move source-generators out of ProjectState and entirely into SolutinoCompilationState #72834

CyrusNajmabadi commented Apr 1, 2024 •

edited

Loading

CyrusNajmabadi Apr 1, 2024

CyrusNajmabadi Apr 1, 2024

sharwell Apr 2, 2024 •

edited

Loading

CyrusNajmabadi Apr 1, 2024

CyrusNajmabadi Apr 1, 2024

CyrusNajmabadi Apr 1, 2024

CyrusNajmabadi Apr 1, 2024

CyrusNajmabadi Apr 1, 2024

CyrusNajmabadi Apr 1, 2024

CyrusNajmabadi Apr 1, 2024

CyrusNajmabadi commented Apr 2, 2024

sharwell left a comment

ToddGrun left a comment

CyrusNajmabadi commented Apr 24, 2024

Move source-generators out of ProjectState and entirely into SolutinoCompilationState #72834

Move source-generators out of ProjectState and entirely into SolutinoCompilationState #72834

Conversation

CyrusNajmabadi commented Apr 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharwell Apr 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Apr 2, 2024

sharwell left a comment

Choose a reason for hiding this comment

ToddGrun left a comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Apr 24, 2024

CyrusNajmabadi commented Apr 1, 2024 •

edited

Loading

sharwell Apr 2, 2024 •

edited

Loading