From ad07803b5c1a40efe6f73f60fae27d48c9f5e5b7 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Fri, 22 Mar 2024 11:05:38 -0700 Subject: [PATCH 1/5] Create stronger type to expression compilation tracker internal policy --- ...pilationTracker.CompilationTrackerState.cs | 43 +++++++------ ...tionCompilationState.CompilationTracker.cs | 63 ++++++++++--------- ...tionState.CompilationTracker_Generators.cs | 4 +- ...SolutionCompilationState.CreationPolicy.cs | 54 ++++++++++++++++ 4 files changed, 115 insertions(+), 49 deletions(-) create mode 100644 src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.CreationPolicy.cs diff --git a/src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.CompilationTracker.CompilationTrackerState.cs b/src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.CompilationTracker.CompilationTrackerState.cs index bf93c6484c821..abffc060a6830 100644 --- a/src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.CompilationTracker.CompilationTrackerState.cs +++ b/src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.CompilationTracker.CompilationTrackerState.cs @@ -35,8 +35,11 @@ private abstract class CompilationTrackerState /// whether we even have references -- it's pretty likely that running a generator might produce worse results /// than what we originally had. /// + /// This also controls if we will generate skeleton references for cross-language P2P references when + /// creating the compilation for a particular project. When entirely frozen, we do not want to do this due + /// to the enormous cost of emitting ref assemblies for cross language cases. /// - public readonly bool IsFrozen; + public readonly CreationPolicy CreationPolicy; /// /// The best compilation that is available that source generators have not ran on. May be an @@ -47,10 +50,10 @@ private abstract class CompilationTrackerState public CompilationTrackerGeneratorInfo GeneratorInfo { get; } protected CompilationTrackerState( - bool isFrozen, + CreationPolicy creationPolicy, CompilationTrackerGeneratorInfo generatorInfo) { - IsFrozen = isFrozen; + CreationPolicy = creationPolicy; GeneratorInfo = generatorInfo; } } @@ -80,12 +83,12 @@ private sealed class InProgressState : CompilationTrackerState public override Compilation CompilationWithoutGeneratedDocuments => LazyCompilationWithoutGeneratedDocuments.Value; public InProgressState( - bool isFrozen, + CreationPolicy creationPolicy, Lazy compilationWithoutGeneratedDocuments, CompilationTrackerGeneratorInfo generatorInfo, Lazy staleCompilationWithGeneratedDocuments, ImmutableList pendingTranslationActions) - : base(isFrozen, generatorInfo) + : base(creationPolicy, generatorInfo) { // Note: Intermediate projects can be empty. Contract.ThrowIfTrue(pendingTranslationActions is null); @@ -105,13 +108,13 @@ public InProgressState( } public InProgressState( - bool isFrozen, + CreationPolicy creationPolicy, Compilation compilationWithoutGeneratedDocuments, CompilationTrackerGeneratorInfo generatorInfo, Compilation? staleCompilationWithGeneratedDocuments, ImmutableList pendingTranslationActions) : this( - isFrozen, + creationPolicy, new Lazy(() => compilationWithoutGeneratedDocuments), generatorInfo, // Extracted as a method call to prevent captures. @@ -164,20 +167,20 @@ private sealed class FinalCompilationTrackerState : CompilationTrackerState public override Compilation CompilationWithoutGeneratedDocuments { get; } private FinalCompilationTrackerState( - bool isFrozen, + CreationPolicy creationPolicy, Compilation finalCompilationWithGeneratedDocuments, Compilation compilationWithoutGeneratedDocuments, bool hasSuccessfullyLoaded, CompilationTrackerGeneratorInfo generatorInfo, UnrootedSymbolSet unrootedSymbolSet) - : base(isFrozen, generatorInfo) + : base(creationPolicy, generatorInfo) { Contract.ThrowIfNull(finalCompilationWithGeneratedDocuments); // As a policy, all partial-state projects are said to have incomplete references, since the // state has no guarantees. this.CompilationWithoutGeneratedDocuments = compilationWithoutGeneratedDocuments; - HasSuccessfullyLoaded = hasSuccessfullyLoaded && !isFrozen; + HasSuccessfullyLoaded = hasSuccessfullyLoaded; FinalCompilationWithGeneratedDocuments = finalCompilationWithGeneratedDocuments; UnrootedSymbolSet = unrootedSymbolSet; @@ -202,7 +205,7 @@ private FinalCompilationTrackerState( /// Not held onto /// Not held onto public static FinalCompilationTrackerState Create( - bool isFrozen, + CreationPolicy creationPolicy, Compilation finalCompilationWithGeneratedDocuments, Compilation compilationWithoutGeneratedDocuments, bool hasSuccessfullyLoaded, @@ -217,7 +220,7 @@ public static FinalCompilationTrackerState Create( RecordAssemblySymbols(projectId, finalCompilationWithGeneratedDocuments, metadataReferenceToProjectId); return new FinalCompilationTrackerState( - isFrozen, + creationPolicy, finalCompilationWithGeneratedDocuments, compilationWithoutGeneratedDocuments, hasSuccessfullyLoaded, @@ -225,13 +228,15 @@ public static FinalCompilationTrackerState Create( unrootedSymbolSet); } - public FinalCompilationTrackerState WithIsFrozen() - => new(isFrozen: true, - FinalCompilationWithGeneratedDocuments, - CompilationWithoutGeneratedDocuments, - HasSuccessfullyLoaded, - GeneratorInfo, - UnrootedSymbolSet); + public FinalCompilationTrackerState WithCreationPolicy(CreationPolicy creationPolicy) + => creationPolicy == this.CreationPolicy + ? this + : new(creationPolicy, + FinalCompilationWithGeneratedDocuments, + CompilationWithoutGeneratedDocuments, + HasSuccessfullyLoaded, + GeneratorInfo, + UnrootedSymbolSet); private static void RecordAssemblySymbols(ProjectId projectId, Compilation compilation, Dictionary? metadataReferenceToProjectId) { diff --git a/src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.CompilationTracker.cs b/src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.CompilationTracker.cs index 4134f3996f5ce..6c5d530ee27db 100644 --- a/src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.CompilationTracker.cs +++ b/src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.CompilationTracker.cs @@ -152,7 +152,7 @@ public ICompilationTracker Fork( }; var newState = new InProgressState( - state.IsFrozen, + state.CreationPolicy, compilationWithoutGeneratedDocuments, state.GeneratorInfo, staleCompilationWithGeneratedDocuments, @@ -334,12 +334,13 @@ InProgressState BuildInProgressStateFromNoCompilationState() var compilationWithoutGeneratedDocuments = CreateEmptyCompilation(); - // We only got here when we had no compilation state at all. So we couldn't have gotten - // here from a frozen state (as a frozen state always ensures we have a - // WithCompilationTrackerState). As such, we can safely still preserve that we're not - // frozen here. + // We only got here when we had no compilation state at all. So we couldn't have gotten here + // from a frozen state (as a frozen state always ensures we have a WithCompilationTrackerState). + // As such, we want to start initially in the state where we will both run generators and create + // skeleton references for p2p references. That will ensure the most correct state for our + // compilation the first time we create it. var allSyntaxTreesParsedState = new InProgressState( - isFrozen: false, + CreationPolicy.Create, new Lazy(CreateEmptyCompilation), CompilationTrackerGeneratorInfo.Empty, staleCompilationWithGeneratedDocuments: s_lazyNullCompilation, @@ -380,7 +381,7 @@ async Task CollapseInProgressStateAsync(InProgressState initial // generator docs back to the uncomputed state from that point onwards. We'll just keep // whateverZ generated docs we have. currentState = new InProgressState( - currentState.IsFrozen, + currentState.CreationPolicy, compilationWithoutGeneratedDocuments, generatorInfo, staleCompilationWithGeneratedDocuments, @@ -472,7 +473,7 @@ async Task FinalizeCompilationWorkerAsync(InProgre Contract.ThrowIfTrue(inProgressState.PendingTranslationActions.Count > 0); // The final state we produce will be frozen or not depending on if a frozen state was passed into it. - var isFrozen = inProgressState.IsFrozen; + var creationPolicy = inProgressState.CreationPolicy; var generatorInfo = inProgressState.GeneratorInfo; var compilationWithoutGeneratedDocuments = inProgressState.CompilationWithoutGeneratedDocuments; var staleCompilationWithGeneratedDocuments = inProgressState.LazyStaleCompilationWithGeneratedDocuments.Value; @@ -524,27 +525,33 @@ await compilationState.GetCompilationAsync( { // Not a submission. Add as a metadata reference. - if (isFrozen) + if (creationPolicy.SkeletonReferenceCreationPolicy is SkeletonReferenceCreationPolicy.Create) { - // In the frozen case, attempt to get a partial reference, or fallback to the last - // successful reference for this project if we can find one. - var metadataReference = compilationState.GetPartialMetadataReference(projectReference, this.ProjectState); + var metadataReference = await compilationState.GetMetadataReferenceAsync(projectReference, this.ProjectState, cancellationToken).ConfigureAwait(false); + AddMetadataReference(projectReference, metadataReference); + } + else + { + Contract.ThrowIfFalse(creationPolicy.SkeletonReferenceCreationPolicy is SkeletonReferenceCreationPolicy.CreateIfAbsent or SkeletonReferenceCreationPolicy.DoNotCreate); + // If not asked to explicit create an up to date skeleton, attempt to get a partial + // reference, or fallback to the last successful reference for this project if we can + // find one. + var metadataReference = compilationState.GetPartialMetadataReference(projectReference, this.ProjectState); if (metadataReference is null) { - // if we failed to get the metadata and we were frozen, check to see if we - // previously had existing metadata and reuse it instead. var inProgressCompilationNotRef = staleCompilationWithGeneratedDocuments ?? compilationWithoutGeneratedDocuments; metadataReference = inProgressCompilationNotRef.ExternalReferences.FirstOrDefault( r => GetProjectId(inProgressCompilationNotRef.GetAssemblyOrModuleSymbol(r) as IAssemblySymbol) == projectReference.ProjectId); } - AddMetadataReference(projectReference, metadataReference); - } - else - { - // For the non-frozen case, attempt to get the full metadata reference. - var metadataReference = await compilationState.GetMetadataReferenceAsync(projectReference, this.ProjectState, cancellationToken).ConfigureAwait(false); + // If we still failed, but our policy is to create when absent, then do the work to + // create a real skeleton here. + if (metadataReference is null && creationPolicy.SkeletonReferenceCreationPolicy is SkeletonReferenceCreationPolicy.CreateIfAbsent) + { + metadataReference = await compilationState.GetMetadataReferenceAsync(projectReference, this.ProjectState, cancellationToken).ConfigureAwait(false); + } + AddMetadataReference(projectReference, metadataReference); } } @@ -563,7 +570,7 @@ await compilationState.GetCompilationAsync( // We will finalize the compilation by adding full contents here. var (compilationWithGeneratedDocuments, generatedDocuments, generatorDriver) = await AddExistingOrComputeNewGeneratorInfoAsync( - isFrozen, + creationPolicy, compilationState, compilationWithoutGeneratedDocuments, generatorInfo, @@ -574,7 +581,7 @@ await compilationState.GetCompilationAsync( var nextGeneratorInfo = new CompilationTrackerGeneratorInfo(generatedDocuments, generatorDriver); var finalState = FinalCompilationTrackerState.Create( - isFrozen, + creationPolicy, compilationWithGeneratedDocuments, compilationWithoutGeneratedDocuments, hasSuccessfullyLoaded, @@ -669,11 +676,11 @@ public ICompilationTracker FreezePartialState(CancellationToken cancellationToke if (state is FinalCompilationTrackerState finalState) { - // If we're finalized and already frozen, we can just use ourselves. Otherwise, flip the frozen bit - // so that any future forks keep things frozen. - return finalState.IsFrozen + // Attempt to transition our state to one where we do not run generators or create skeleton references. + var newFinalState = finalState.WithCreationPolicy(CreationPolicy.DoNotCreate); + return newFinalState == finalState ? this - : new CompilationTracker(this.ProjectState, finalState.WithIsFrozen(), skeletonReferenceCacheToClone: _skeletonReferenceCache); + : new CompilationTracker(this.ProjectState, newFinalState, skeletonReferenceCacheToClone: _skeletonReferenceCache); } // Non-final state currently. Produce an in-progress-state containing the forked change. Note: we @@ -722,7 +729,7 @@ public ICompilationTracker FreezePartialState(CancellationToken cancellationToke return new CompilationTracker( frozenProjectState, new InProgressState( - isFrozen: true, + CreationPolicy.DoNotCreate, lazyCompilationWithoutGeneratedDocuments, CompilationTrackerGeneratorInfo.Empty, lazyCompilationWithGeneratedDocuments, @@ -748,7 +755,7 @@ public ICompilationTracker FreezePartialState(CancellationToken cancellationToke return new CompilationTracker( frozenProjectState, new InProgressState( - isFrozen: true, + CreationPolicy.DoNotCreate, compilationWithoutGeneratedDocuments, generatorInfo, compilationWithGeneratedDocuments, diff --git a/src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.CompilationTracker_Generators.cs b/src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.CompilationTracker_Generators.cs index 73fbee3c5a223..ecb953cc80b3d 100644 --- a/src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.CompilationTracker_Generators.cs +++ b/src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.CompilationTracker_Generators.cs @@ -27,14 +27,14 @@ internal partial class SolutionCompilationState private partial class CompilationTracker : ICompilationTracker { private async Task<(Compilation compilationWithGeneratedFiles, TextDocumentStates generatedDocuments, GeneratorDriver? generatorDriver)> AddExistingOrComputeNewGeneratorInfoAsync( - bool isFrozen, + CreationPolicy creationPolicy, SolutionCompilationState compilationState, Compilation compilationWithoutGeneratedFiles, CompilationTrackerGeneratorInfo generatorInfo, Compilation? compilationWithStaleGeneratedTrees, CancellationToken cancellationToken) { - if (isFrozen) + if (creationPolicy.GeneratedDocumentCreationPolicy is GeneratedDocumentCreationPolicy.DoNotCreate) { // We're frozen. So we do not want to go through the expensive cost of running generators. Instead, we // just whatever prior generated docs we have. diff --git a/src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.CreationPolicy.cs b/src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.CreationPolicy.cs new file mode 100644 index 0000000000000..90fbfdf68cc7b --- /dev/null +++ b/src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.CreationPolicy.cs @@ -0,0 +1,54 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +namespace Microsoft.CodeAnalysis; + +internal partial class SolutionCompilationState +{ + /// + /// Flags controlling if generator documents should be created or not. + /// + private enum GeneratedDocumentCreationPolicy + { + /// + /// Source generators should be run and should produce up to date results. + /// + Create, + + /// + /// Source generators should not run. Whatever results were previously computed should be reused. + /// + DoNotCreate, + } + + /// + /// Flags controlling if skeleton references should be created or not. + /// + private enum SkeletonReferenceCreationPolicy + { + /// + /// Skeleton references should be created, and should be up to date with the project they are created for. + /// + Create, + + /// + /// Skeleton references should only be created for a compilation if no existing skeleton exists for their + /// project from some point in the past. + /// + CreateIfAbsent, + + /// + /// Skeleton references should not be created at all. + /// + DoNotCreate, + } + + private readonly record struct CreationPolicy( + GeneratedDocumentCreationPolicy GeneratedDocumentCreationPolicy, + SkeletonReferenceCreationPolicy SkeletonReferenceCreationPolicy) + { + public static readonly CreationPolicy Create = new(GeneratedDocumentCreationPolicy.Create, SkeletonReferenceCreationPolicy.Create); + public static readonly CreationPolicy DoNotCreate = new(GeneratedDocumentCreationPolicy.DoNotCreate, SkeletonReferenceCreationPolicy.DoNotCreate); + } +} From aed1da109779a2b491f01427149898cc76501cfe Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Fri, 22 Mar 2024 11:07:27 -0700 Subject: [PATCH 2/5] Fix comment --- .../Solution/SolutionCompilationState.CompilationTracker.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.CompilationTracker.cs b/src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.CompilationTracker.cs index 6c5d530ee27db..5dcbf200836a8 100644 --- a/src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.CompilationTracker.cs +++ b/src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.CompilationTracker.cs @@ -335,7 +335,7 @@ InProgressState BuildInProgressStateFromNoCompilationState() var compilationWithoutGeneratedDocuments = CreateEmptyCompilation(); // We only got here when we had no compilation state at all. So we couldn't have gotten here - // from a frozen state (as a frozen state always ensures we have a WithCompilationTrackerState). + // from a frozen state (as a frozen state always ensures we have at least an InProgressState). // As such, we want to start initially in the state where we will both run generators and create // skeleton references for p2p references. That will ensure the most correct state for our // compilation the first time we create it. From 7bf8603dc91f946dc18b97f7496117292ca5d51b Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Fri, 22 Mar 2024 11:08:54 -0700 Subject: [PATCH 3/5] Docs --- .../Solution/SolutionCompilationState.CreationPolicy.cs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.CreationPolicy.cs b/src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.CreationPolicy.cs index 90fbfdf68cc7b..ff61c5044a6a4 100644 --- a/src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.CreationPolicy.cs +++ b/src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.CreationPolicy.cs @@ -48,7 +48,15 @@ private readonly record struct CreationPolicy( GeneratedDocumentCreationPolicy GeneratedDocumentCreationPolicy, SkeletonReferenceCreationPolicy SkeletonReferenceCreationPolicy) { + /// + /// Create up to date source generator docs and create up to date skeleton references when needed. + /// public static readonly CreationPolicy Create = new(GeneratedDocumentCreationPolicy.Create, SkeletonReferenceCreationPolicy.Create); + + /// + /// Do not create up to date source generator docs and do not create up to date skeleton references for P2P + /// references. For both, use whatever has been generated most recently. + /// public static readonly CreationPolicy DoNotCreate = new(GeneratedDocumentCreationPolicy.DoNotCreate, SkeletonReferenceCreationPolicy.DoNotCreate); } } From 450625c974b6feff79c11c2b4ded105a6b326896 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Fri, 22 Mar 2024 11:10:24 -0700 Subject: [PATCH 4/5] Docs --- .../SolutionCompilationState.CompilationTracker.cs | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.CompilationTracker.cs b/src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.CompilationTracker.cs index 5dcbf200836a8..1ba446df1396d 100644 --- a/src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.CompilationTracker.cs +++ b/src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.CompilationTracker.cs @@ -674,10 +674,15 @@ public ICompilationTracker FreezePartialState(CancellationToken cancellationToke { var state = this.ReadState(); + // We're freezing the solution for features where latency performance is parameter. Do not run SGs or + // create skeleton references at this point. Just use whatever we've already generated for each in the + // past. + var desiredCreationPolicy = CreationPolicy.DoNotCreate; + if (state is FinalCompilationTrackerState finalState) { // Attempt to transition our state to one where we do not run generators or create skeleton references. - var newFinalState = finalState.WithCreationPolicy(CreationPolicy.DoNotCreate); + var newFinalState = finalState.WithCreationPolicy(desiredCreationPolicy); return newFinalState == finalState ? this : new CompilationTracker(this.ProjectState, newFinalState, skeletonReferenceCacheToClone: _skeletonReferenceCache); @@ -729,7 +734,7 @@ public ICompilationTracker FreezePartialState(CancellationToken cancellationToke return new CompilationTracker( frozenProjectState, new InProgressState( - CreationPolicy.DoNotCreate, + desiredCreationPolicy, lazyCompilationWithoutGeneratedDocuments, CompilationTrackerGeneratorInfo.Empty, lazyCompilationWithGeneratedDocuments, @@ -755,7 +760,7 @@ public ICompilationTracker FreezePartialState(CancellationToken cancellationToke return new CompilationTracker( frozenProjectState, new InProgressState( - CreationPolicy.DoNotCreate, + desiredCreationPolicy, compilationWithoutGeneratedDocuments, generatorInfo, compilationWithGeneratedDocuments, From 1b24cc9cb6c990944de0698a4fb627a7855bc45a Mon Sep 17 00:00:00 2001 From: Fredric Silberberg Date: Fri, 22 Mar 2024 13:08:53 -0700 Subject: [PATCH 5/5] Skip failing test Opened https://github.com/dotnet/roslyn/issues/72678 for investigation. --- src/Compilers/CSharp/Test/EndToEnd/EndToEndTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Compilers/CSharp/Test/EndToEnd/EndToEndTests.cs b/src/Compilers/CSharp/Test/EndToEnd/EndToEndTests.cs index ccc34c3e368d9..64cc4a4f131e7 100644 --- a/src/Compilers/CSharp/Test/EndToEnd/EndToEndTests.cs +++ b/src/Compilers/CSharp/Test/EndToEnd/EndToEndTests.cs @@ -172,7 +172,7 @@ void M2() { // This test is a canary attempting to make sure that we don't regress the # of fluent calls that // the compiler can handle. - [Fact, WorkItem("https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1874763")] + [Fact(Skip = "https://github.com/dotnet/roslyn/issues/72678"), WorkItem("https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1874763")] public void OverflowOnFluentCall_ExtensionMethods() { int numberFluentCalls = (IntPtr.Size, ExecutionConditionUtil.Configuration, RuntimeUtilities.IsDesktopRuntime, RuntimeInformation.IsOSPlatform(OSPlatform.OSX)) switch