Remove CWT from skeleton caching and simplify impl

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

@@ -385,6 +385,12 @@ public Task<VersionStamp> GetLatestDocumentVersionAsync(CancellationToken cancel
         public async Task<VersionStamp> GetSemanticVersionAsync(CancellationToken cancellationToken = default)
         {
             var docVersion = await _lazyLatestDocumentTopLevelChangeVersion.GetValueAsync(cancellationToken).ConfigureAwait(false);
+
+            // This is unfortunate, however the impact of this is that *any* change to our project-state version will 
+            // cause us to think the semantic version of the project has changed.  Thus, any change to a project property
+            // that does *not* flow into the compiler still makes us think the semantic version has changed.  This is 
+            // likely to not be too much of an issue as these changes should be rare, and it's better to be conservative
+            // and assume there was a change than to wrongly presume there was not.
             return docVersion.GetNewerVersion(this.Version);
         }
 

src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.SkeletonReferenceCache.cs

@@ -36,22 +36,14 @@ internal partial class SolutionState
     /// <para/>
     /// The implementation works by keeping metadata references around associated with a specific <see cref="VersionStamp"/>
     /// for a project. As long as the <see cref="Project.GetDependentSemanticVersionAsync"/> for that project
-    /// is the same, then all the references of it can be reused.  When an <see cref="SolutionState.ICompilationTracker"/> forks
+    /// is the same, then all the references of it can be reused.  When an <see cref="ICompilationTracker"/> forks
     /// itself, it  will also <see cref="Clone"/> this, allowing previously computed references to be used by later forks.
     /// However, this means that later forks (esp. ones that fail to produce a skeleton, or which produce a skeleton for 
     /// different semantics) will not leak backward to a prior <see cref="ProjectState"/>, causing it to see a view of the world
     /// inapplicable to its current snapshot.
     /// </summary>
     private partial class SkeletonReferenceCache
     {
-        /// <summary>
-        /// Mapping from compilation instance to metadata-references for it.  This allows us to associate the same
-        /// <see cref="SkeletonReferenceSet"/> to different compilations that may not be the same as the original
-        /// compilation we generated the set from.  This allows us to use compilations as keys as long as they're
-        /// alive, but also associate the set with new compilations that are generated in the future if the older
-        /// compilations were thrown away.
-        /// </summary>
-        private static readonly ConditionalWeakTable<Compilation, SkeletonReferenceSet> s_compilationToReferenceMap = new();
         private static readonly EmitOptions s_metadataOnlyEmitOptions = new(metadataOnly: true);
 
         /// <summary>
@@ -115,84 +107,68 @@ public SkeletonReferenceCache Clone()
             MetadataReferenceProperties properties,
             CancellationToken cancellationToken)
         {
-            // First, just see if we have cached a reference set that is complimentary with the version of the project
-            // being passed in.  If so, we can just reuse what we already computed before.
-            var workspace = solution.Workspace;
             var version = await compilationTracker.GetDependentSemanticVersionAsync(solution, cancellationToken).ConfigureAwait(false);
-            var metadataReference = TryGetReferenceSet(version)?.GetMetadataReference(properties);
-            if (metadataReference != null)
-            {
-                workspace.LogTestMessage($"Reusing the already cached skeleton assembly for {compilationTracker.ProjectState.Id}");
-                return metadataReference;
-            }
-
-            var compilation = await compilationTracker.GetCompilationAsync(solution, cancellationToken).ConfigureAwait(false);
-
-            // Didn't have a direct mapping to a reference set.  Compute one for ourselves.
-            var referenceSet = await GetOrBuildReferenceSetAsync(workspace, version, compilation, cancellationToken).ConfigureAwait(false);
-
-            // another thread may have come in and beaten us to computing this.  So attempt to actually cache this
-            // in the global map.  if it succeeds, use our computed version.  If it fails, use the one the other
-            // thread succeeded in storing.
-            referenceSet = s_compilationToReferenceMap.GetValue(compilation, _ => referenceSet);
-
-            lock (_stateGate)
-            {
-                // whoever won, still store this reference set against us with the provided version.
-                _version = version;
-                _skeletonReferenceSet = referenceSet;
-            }
-
-            return referenceSet.GetMetadataReference(properties);
+            var referenceSet = await TryGetOrCreateReferenceSetAsync(
+                compilationTracker, solution, version, cancellationToken).ConfigureAwait(false);
+            return referenceSet?.GetMetadataReference(properties);
         }
 
-        private async Task<SkeletonReferenceSet> GetOrBuildReferenceSetAsync(
-            Workspace workspace,
+        private async Task<SkeletonReferenceSet?> TryGetOrCreateReferenceSetAsync(
+            ICompilationTracker compilationTracker,
+            SolutionState solution,
             VersionStamp version,
-            Compilation compilation,
             CancellationToken cancellationToken)
         {
-            var referenceSet = TryGetExistingReferenceSet(version, compilation);
-            if (referenceSet != null)
+            // First, just see if we have cached a reference set that is complimentary with the version of the project
+            // being passed in.  If so, we can just reuse what we already computed before.
+            if (TryReadSkeletonReferenceSetAtThisVersion(version, out var referenceSet))
                 return referenceSet;
 
-            // okay, we don't have one. so create one now.
-            ITemporaryStreamStorage? storage;
-
+            // okay, we don't have anything cached with this version. so create one now.  Note: this is expensive
+            // so ensure only one thread is doing hte work to actually make the compilation and emit it.
             using (await _emitGate.DisposableWaitAsync(cancellationToken).ConfigureAwait(false))
             {
                 // after taking the gate, another thread may have succeeded.  See if we can use their version if so:
-                referenceSet = TryGetExistingReferenceSet(version, compilation);
-                if (referenceSet != null)
+                if (TryReadSkeletonReferenceSetAtThisVersion(version, out referenceSet))
                     return referenceSet;
 
-                storage = TryCreateMetadataStorage(workspace, compilation, cancellationToken);
-            }
+                // Ok, first thread to get in and actually do this work.  Build the compilation and try to emit it.
+                // Regardless of if we succeed or fail, store this result so this only happens once.
 
-            if (storage == null)
-            {
-                // unfortunately, we couldn't create one. see if we have one from previous compilation., it might be
-                // out-of-date big time, but better than nothing.
-                referenceSet = TryGetReferenceSet(version: null);
-                if (referenceSet != null)
+                var compilation = await compilationTracker.GetCompilationAsync(solution, cancellationToken).ConfigureAwait(false);
+                var storage = TryCreateMetadataStorage(solution.Workspace, compilation, cancellationToken);
+
+                lock (_stateGate)
                 {
-                    workspace.LogTestMessage($"We failed to create metadata so we're using the one we just found from an earlier version.");
-                    return referenceSet;
+                    // If we successfully created the metadata storage, then create the new set that points to it.
+                    // if we didn't, that's ok too, we'll just say that for this requested version, that we can
+                    // return any prior computed reference set (including 'null' if we've never successfully made
+                    // a skeleton).
+                    if (storage != null)
+                        _skeletonReferenceSet = new SkeletonReferenceSet(storage, compilation.AssemblyName, new DeferredDocumentationProvider(compilation));
+
+                    _version = version;
+
+                    return _skeletonReferenceSet;
                 }
             }
-
-            return new SkeletonReferenceSet(storage, compilation.AssemblyName, new DeferredDocumentationProvider(compilation));
         }
 
-        private SkeletonReferenceSet? TryGetExistingReferenceSet(VersionStamp version, Compilation compilation)
+        private bool TryReadSkeletonReferenceSetAtThisVersion(VersionStamp version, out SkeletonReferenceSet? result)
         {
-            // first, check if we have a direct mapping from this compilation to a reference set. If so, use it.  This
-            // ensures the same compilations will get same metadata reference.
-            if (s_compilationToReferenceMap.TryGetValue(compilation, out var referenceSet))
-                return referenceSet;
+            lock (_stateGate)
+            {
+                // if we're asking about the same version as we've cached, then return whatever have (regardless of
+                // whether it succeeded or not.
+                if (version == _version)
+                {
+                    result = _skeletonReferenceSet;
+                    return true;
+                }
+            }
 
-            // Then see if we already have a reference set for this version.  if so, we're done and can return that.
-            return TryGetReferenceSet(version);
+            result = null;
+            return false;
         }
 
         private static ITemporaryStreamStorage? TryCreateMetadataStorage(Workspace workspace, Compilation compilation, CancellationToken cancellationToken)
@@ -205,8 +181,6 @@ private async Task<SkeletonReferenceSet> GetOrBuildReferenceSetAsync(
 
                 using (Logger.LogBlock(FunctionId.Workspace_SkeletonAssembly_EmitMetadataOnlyImage, cancellationToken))
                 {
-                    // TODO: make it to use SerializableBytes.WritableStream rather than MemoryStream so that
-                    //       we don't allocate anything for skeleton assembly.
                     using var stream = SerializableBytes.CreateWritableStream();
                     // note: cloning compilation so we don't retain all the generated symbols after its emitted.
                     // * REVIEW * is cloning clone p2p reference compilation as well?
@@ -252,39 +226,6 @@ private async Task<SkeletonReferenceSet> GetOrBuildReferenceSetAsync(
             }
         }
 
-        /// <summary>
-        /// Tries to get the <see cref="SkeletonReferenceSet"/> for this project matching <paramref name="version"/>.
-        /// if <paramref name="version"/> is <see langword="null"/>, any cached <see cref="SkeletonReferenceSet"/> 
-        /// can be returned, even if it doesn't correspond to that version.  This is useful in error tolerance cases
-        /// as building a skeleton assembly may easily fail. In that case it's better to use the last successfully 
-        /// built skeleton than just have no semantic information for that project at all.
-        /// </summary>
-        private SkeletonReferenceSet? TryGetReferenceSet(VersionStamp? version)
-        {
-            // Otherwise, we don't have a direct mapping stored.  Try to see if the cached reference we have is
-            // applicable to this project semantic version.
-            lock (_stateGate)
-            {
-                // if we don't have a skeleton cached, then we have nothing to return.
-                if (_skeletonReferenceSet == null)
-                    return null;
-
-                // if the caller is requiring a particular semantic version, it much match what we have cached.
-                if (version != null && version != _version)
-                    return null;
-
-                return _skeletonReferenceSet;
-            }
-        }
-
-        /// <summary>
-        /// Return a metadata reference if we already have a reference-set computed for this particular <paramref name="version"/>.
-        /// If a reference already exists for the provided <paramref name="properties"/>, the same instance will be returned.  Otherwise,
-        /// a fresh instance will be returned.
-        /// </summary>
-        public MetadataReference? TryGetReference(VersionStamp version, MetadataReferenceProperties properties)
-            => TryGetReferenceSet(version)?.GetMetadataReference(properties);
-
         private sealed class SkeletonReferenceSet
         {
             /// <summary>

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

@@ -1910,9 +1910,6 @@ public SolutionState WithFrozenSourceGeneratedDocument(SourceGeneratedDocumentId
                 // referenced project's compilation and re-importing it.
                 using (Logger.LogBlock(FunctionId.Workspace_SkeletonAssembly_GetMetadataOnlyImage, cancellationToken))
                 {
-                    var workspace = this.Workspace;
-                    workspace.LogTestMessage($"Looking for a cached skeleton assembly for {projectReference.ProjectId} before taking the lock...");
-
                     var properties = new MetadataReferenceProperties(aliases: projectReference.Aliases, embedInteropTypes: projectReference.EmbedInteropTypes);
                     return await tracker.SkeletonReferenceCache.GetOrBuildReferenceAsync(
                         tracker, this, properties, cancellationToken).ConfigureAwait(false);