Defer parsing syntax trees when computing frozen partial solutions #72090

CyrusNajmabadi · 2024-02-13T20:23:29Z

Followup to #72045

CyrusNajmabadi · 2024-02-13T20:46:42Z

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

+                {
+                    // Otherwise, we're not finalized, or the document has changed.  We'll create an in-progress state
+                    // to represent the new state of the project, with all the necessary translation steps to
+                    // incorporate the new document.


view with whitespace off.

note: teh logic here is very similar to the old logic. just operating on DocStates instead of trees. A lot of the diff is also because i updated the comments to reflect thsi. But i otherwise tried to keep the semantics the same as before.

CyrusNajmabadi · 2024-02-13T20:47:25Z

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

+                            // produced an empty one, and removed any documents, so inProgressProject.DocumentStates would
+                            // have been empty originally.
+                            pendingActions = [(inProgressProject, new CompilationAndGeneratorDriverTranslationAction.TouchDocumentAction(oldState, docState))];
+                            inProgressProject = inProgressProject.UpdateDocument(docState, contentChanged: true);


note: i think i'm passing in inProgressProject property to the pending actions. it represents the state prior to performing the action.

and here we see teh core concept change. instead of just manipulating the compilations directly (And thus needing the trees up front), we instead leverage totally normal pending actions, and we create an inprogress state that still has a teeny bit more work to do. That work is going to be the same as today (namely parsing the syntax trees), but that work is now handled in the normal translation-step processing code we already have.

Also, nicely, instead of needing to get the syntax trees for the N linked-file siblings of this document up front, that work can be delayed until someone actually needs it (by asking for syntax trees for them, or for getting compilations for their projects).

(by asking for syntax trees for them, or for getting compilations for their projects).

This is one small thing I do like about this approach: if we have features that grab frozen partials right away, and then later decide they had to do no work, we didn't end up paying the cost up front.

just a small thing? :)

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

CyrusNajmabadi · 2024-02-13T20:49:20Z

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


-                    // This is likely a bug.  It seems possible to pass out a partial compilation state that we don't
-                    // properly record assembly symbols for.
-                    metadataReferenceToProjectId = null;


i really liked being able to remove this. as we're now not transitioning directly to a FinalState, we don't need to ahve this computed.

src/Workspaces/CoreTest/SolutionTests/SolutionTests.cs

CyrusNajmabadi · 2024-02-13T22:33:03Z

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

                    compilationWithoutGeneratedDocuments,
                    compilationWithoutGeneratedDocuments.AddSyntaxTrees(generatorInfo.Documents.States.Values.Select(state => state.GetSyntaxTree(cancellationToken))));
-
-                // Now add in back a consistent set of project references.  For project references


this glorious code went away. since we already do it when transitioning an inprogress state to a finalstate :)

Wonderful discovery!

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

…ationState.CompilationTracker.cs

…Tree

CyrusNajmabadi · 2024-02-14T18:07:02Z

...le/Workspace/Solution/SolutionCompilationState.CompilationTracker.CompilationTrackerState.cs

                    CompilationTrackerGeneratorInfo generatorInfo,
                    Compilation? staleCompilationWithGeneratedDocuments,
-                    ImmutableList<(ProjectState state, CompilationAndGeneratorDriverTranslationAction action)> pendingTranslationSteps)
+                    ImmutableList<(ProjectState oldState, CompilationAndGeneratorDriverTranslationAction action)> pendingTranslationSteps)


naming for consistency.

CyrusNajmabadi · 2024-02-14T18:07:22Z

...le/Workspace/Solution/SolutionCompilationState.CompilationTracker.CompilationTrackerState.cs

-                    HasSuccessfullyLoaded = hasSuccessfullyLoaded;
+
+                    // As a policy, all partial-state projects are said to have incomplete references, since the
+                    // state has no guarantees.


moved from a different location here, so that we're consistent on what this bit means wrt frozen states.

CyrusNajmabadi · 2024-02-14T18:07:53Z

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

            {
                var allDocumentIds = @this.SolutionState.GetRelatedDocumentIds(documentId);
-                using var _ = ArrayBuilder<(DocumentState, SyntaxTree)>.GetInstance(allDocumentIds.Length, out var statesAndTrees);
+                using var _ = ArrayBuilder<DocumentState>.GetInstance(allDocumentIds.Length, out var documentStates);


much cleaner now. we just grab the document states we want to force to be in the frozen solution snapshot.

CyrusNajmabadi · 2024-02-14T18:12:09Z

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

                {
-                    Contract.ThrowIfTrue(tree != null);
+                    var frozenFinalState = finalState.WithIsFrozen();
+                    return new CompilationTracker(this.ProjectState, frozenFinalState, this.SkeletonReferenceCache.Clone());


note: this logic is technically optional. i could just always make an inprogressstate here with no translation actions attached. however, we have tests that validate you get the same compilation instance back in that case and it felt good to preserve.

Specifically.

GetCompilation from normal solution.

Get document from that solution.

freeze that document (without any changes)

GetCompilation from that frozen document's project.

Tests validate you get teh exact same instance as before. So this maintains that invariant. Note: you will still get a frozen state, so further mutations will not run generators.

we have tests that validate you get the same compilation instance back in that case and it felt good to preserve.

I agree that seems worth preserving, but it seems a bit surprising to me that returning an InProgressState with no further translation actions and it frozen would have resulted in a different compilation being made. That feels like we have a bug in the finalization step that's recreating a compilation even if it doesn't need to.

But this is small enough to preserve that I don't object to this logic here.

CyrusNajmabadi · 2024-02-14T18:19:58Z

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

-                }
+                    // Transition us to a frozen in progress state.  With the best compilations up to this point, and the
+                    // pending actions we'll need to perform on it to get to the final state.
+                    var inProgressState = InProgressState.Create(


this is core part of the change. Instead of going to be in a final state, where we manually did all the steps ourselves, we go back to a normal frozen in-progress-state where there are still 1-2 pending translations to perform in the standard fashion.

CyrusNajmabadi · 2024-02-14T18:21:12Z

@ToddGrun @jasonmalinowski ptal.

ToddGrun · 2024-02-14T21:17:18Z

...le/Workspace/Solution/SolutionCompilationState.CompilationTracker.CompilationTrackerState.cs

                }

+                public FinalCompilationTrackerState WithIsFrozen()
+                    => new(isFrozen: true,


new

no check to return this if already frozen?

fair. i can do that.

put into: #72111

ToddGrun · 2024-02-14T21:22:50Z

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

+                // any future forks keep things frozen.
+                if (state is FinalCompilationTrackerState finalState)
+                {
+                    var frozenFinalState = finalState.WithIsFrozen();


finalState

I assume we can't just return this here too if finalstate is already frozen?

put into: #72111

CyrusNajmabadi · 2024-02-14T22:17:22Z

@ToddGrun can i make those changes in followup?

ToddGrun · 2024-02-15T00:05:05Z

LGTM, but I'm going to defer approval to @jasonmalinowski as the metadataReference part of the change wasn't clear to me

CyrusNajmabadi · 2024-02-15T00:12:03Z

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

-
-                metadataReferenceToProjectId = [];
-
-                foreach (var projectReference in this.ProjectState.ProjectReferences)


@ToddGrun So this code simply attempts to ensure that the p2p refs in the frozen view of the world point at something. because it's partial and we want to be fast, a p2p ref will point at the compilation without generated docs for the linked project.

previously, we did this work ehre. However, because we're now just making ordinary (albeit frozen) inprogressstates, we can get rid of this. Instead, when we then do the similar pass when transitioning an inprogressstate to a finalstate, we do this same work (deviating only slightly to preserve the isfrozen speedup).

CyrusNajmabadi · 2024-02-15T00:13:41Z

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

                                }
                                else
                                {
-                                    var metadataReference = await compilationState.GetMetadataReferenceAsync(


@ToddGrun this loop (if you look upwards to the containing for-loop) is what is setting up p2p references. It stays the same for normal states, but is tweaked for the frozen case. In the frozen case we just use the compilation-without-generated-docs (which is why we can be fast and not async here).

ToddGrun

ToddGrun · 2024-02-15T15:41:14Z

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

+                    var frozenFinalState = finalState.WithIsFrozen();
+                    return new CompilationTracker(this.ProjectState, frozenFinalState, this.SkeletonReferenceCache.Clone());
                }
                else


else

Out of curiosity, why did you add this in the last commit?

Just a style preference. I preferred the indentation

jasonmalinowski

This turned out far more awesome than I anticipated, especially with your realization we can ditch all the duplicate code messing with references. Nice job!

...le/Workspace/Solution/SolutionCompilationState.CompilationTracker.CompilationTrackerState.cs

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

jasonmalinowski · 2024-02-15T23:24:57Z

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

                    compilationWithoutGeneratedDocuments,
                    compilationWithoutGeneratedDocuments.AddSyntaxTrees(generatorInfo.Documents.States.Values.Select(state => state.GetSyntaxTree(cancellationToken))));
-
-                // Now add in back a consistent set of project references.  For project references


Wonderful discovery!

jasonmalinowski · 2024-02-15T23:30:50Z

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

                {
-                    Contract.ThrowIfTrue(tree != null);
+                    var frozenFinalState = finalState.WithIsFrozen();
+                    return new CompilationTracker(this.ProjectState, frozenFinalState, this.SkeletonReferenceCache.Clone());


we have tests that validate you get the same compilation instance back in that case and it felt good to preserve.

I agree that seems worth preserving, but it seems a bit surprising to me that returning an InProgressState with no further translation actions and it frozen would have resulted in a different compilation being made. That feels like we have a bug in the finalization step that's recreating a compilation even if it doesn't need to.

But this is small enough to preserve that I don't object to this logic here.

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

jasonmalinowski · 2024-02-15T23:44:06Z

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

-                            var oldTree = oldState.GetSyntaxTree(cancellationToken);
-
-                            compilationPair = compilationPair.ReplaceSyntaxTree(oldTree, tree);
+                            pendingActions = [(inProgressProject, new CompilationAndGeneratorDriverTranslationAction.TouchDocumentAction(oldState, docState))];


You may wish to consider adding an assert that says:

The old state had a syntax tree and

That syntax tree is present in compilationPair.

Since if at any point that gets out of sync in the current form we'd know that right away (and it'd be easy to debug) since ReplaceSyntaxTree would have thrown right away. If that invariant were to get screwed up now, we won't know until the later call which is running the translation actions and we might have lost the state here of why we chose that state.

And to be very clear: I think the logic is fine, but just paranoia since bugs we've had in the past here are very hard to debug when the things go wrong only after this did the wrong thing.

Paranoia is fine. I will add in followup pr

jasonmalinowski · 2024-02-15T23:44:51Z

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

+                            // produced an empty one, and removed any documents, so inProgressProject.DocumentStates would
+                            // have been empty originally.
+                            pendingActions = [(inProgressProject, new CompilationAndGeneratorDriverTranslationAction.TouchDocumentAction(oldState, docState))];
+                            inProgressProject = inProgressProject.UpdateDocument(docState, contentChanged: true);


(by asking for syntax trees for them, or for getting compilations for their projects).

This is one small thing I do like about this approach: if we have features that grab frozen partials right away, and then later decide they had to do no work, we didn't end up paying the cost up front.

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

src/Workspaces/CoreTest/SolutionTests/SolutionTests.cs

jasonmalinowski · 2024-02-16T00:10:29Z

@CyrusNajmabadi If I had one general concern with this change, if there's one thing the old code did well (and I use "well" loosely here!) was the old code always immediately put us into FinalState, so it's obvious that a later call to GetCompilationAsync() won't do anything expensive. Now, we're deferring that a bit further. And that's good! But it does mean there's a risk we might accidentally let something else expensive slip in. Now if we were to regress that we might know easily enough from telemetry (especially @genlu pointing out completion got slower again!), but I wonder if we can add some extra asserts in the code or tests or something. But for example if a test today is calling FreezeWithDocument and then later calling GetCompilationAsync() that we absolutely know that GetCompilationAsync() should never have yielded, and the task returned should be completed. Because yes, there might have been a few translation actions to process, we had to do some work, but that should still never yield.

CyrusNajmabadi · 2024-02-16T00:19:17Z

Rolling feedback into followup.

CyrusNajmabadi added 2 commits February 13, 2024 11:55

Merge branch 'main' into frozenPartialSyntaxTree

e27bb84

Defer parsing syntax trees when computing frozen partial solutions

92e8fdb

ghost added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Feb 13, 2024

CyrusNajmabadi added 5 commits February 13, 2024 12:24

simplify

39dff43

docs

85ba620

preserve

0d03232

fix

e3c6c2f

Update test

ae90a86

CyrusNajmabadi commented Feb 13, 2024

View reviewed changes

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

CyrusNajmabadi commented Feb 13, 2024

View reviewed changes

src/Workspaces/CoreTest/SolutionTests/SolutionTests.cs Show resolved Hide resolved

CyrusNajmabadi added 2 commits February 13, 2024 14:30

Simplify

4ec51f6

Simplify

ac5b220

CyrusNajmabadi commented Feb 13, 2024

View reviewed changes

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

CyrusNajmabadi commented Feb 13, 2024

View reviewed changes

src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.CompilationTracker.cs Outdated Show resolved Hide resolved

CyrusNajmabadi added 5 commits February 13, 2024 14:34

Update src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompil…

6c7fdb7

…ationState.CompilationTracker.cs

Merge branch 'main' into frozenPartialSyntaxTree

cbd1e58

Simplify

4e98024

Separate methods

52a13be

Merge remote-tracking branch 'upstream/main' into frozenPartialSyntax…

6f7d2a4

…Tree

CyrusNajmabadi commented Feb 14, 2024

View reviewed changes

indent

9db05f5

CyrusNajmabadi commented Feb 14, 2024

View reviewed changes

CyrusNajmabadi marked this pull request as ready for review February 14, 2024 18:21

CyrusNajmabadi requested a review from a team as a code owner February 14, 2024 18:21

lint

a16da4e

jasonmalinowski self-requested a review February 14, 2024 18:59

CyrusNajmabadi mentioned this pull request Feb 14, 2024

Simplify how frozen partial documents are produced. #72106

Merged

CyrusNajmabadi requested a review from ToddGrun February 14, 2024 20:03

ToddGrun reviewed Feb 14, 2024

View reviewed changes

CyrusNajmabadi mentioned this pull request Feb 14, 2024

Reuse compilation tracker states when possible. #72111

Merged

CyrusNajmabadi commented Feb 15, 2024

View reviewed changes

ToddGrun approved these changes Feb 15, 2024

View reviewed changes

ToddGrun reviewed Feb 15, 2024

View reviewed changes

jasonmalinowski approved these changes Feb 16, 2024

View reviewed changes

CyrusNajmabadi merged commit ad97cb5 into dotnet:main Feb 16, 2024

CyrusNajmabadi deleted the frozenPartialSyntaxTree branch February 16, 2024 00:19

ghost added this to the Next milestone Feb 16, 2024

Cosifne mentioned this pull request Feb 21, 2024

[Automated] PRs inserted in VS build main-34620.00 #72198

Closed

jjonescz modified the milestones: Next, 17.10 P2 Feb 27, 2024


		metadataReferenceToProjectId = [];

		foreach (var projectReference in this.ProjectState.ProjectReferences)

Defer parsing syntax trees when computing frozen partial solutions #72090

Defer parsing syntax trees when computing frozen partial solutions #72090

Uh oh!

Conversation

CyrusNajmabadi commented Feb 13, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Feb 14, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Feb 14, 2024

Uh oh!

ToddGrun commented Feb 15, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ToddGrun left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonmalinowski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!