From bcfc36c6e765b8977333f0a60ed5a5f9e7bbbc90 Mon Sep 17 00:00:00 2001 From: Dustin Campbell Date: Mon, 24 Mar 2025 14:41:23 -0700 Subject: [PATCH 1/6] GeneratedOutputSource: Hold output in WeakReference This change brings back the behavior to hold the generated RazorCodeDocument produced by a document snapshot in a WeakReference. This avoids memory retention problems when document snapshots are rooted in the Roslyn workspace by the DynamicFileInfo system. --- .../Sources/GeneratedOutputSource.cs | 29 +++++++++++++++---- 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSystem/Sources/GeneratedOutputSource.cs b/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSystem/Sources/GeneratedOutputSource.cs index 218a6b5b305..79beb362a83 100644 --- a/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSystem/Sources/GeneratedOutputSource.cs +++ b/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSystem/Sources/GeneratedOutputSource.cs @@ -1,6 +1,7 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the MIT license. See License.txt in the project root for license information. +using System; using System.Diagnostics.CodeAnalysis; using System.Threading; using System.Threading.Tasks; @@ -13,12 +14,21 @@ internal sealed class GeneratedOutputSource { private readonly SemaphoreSlim _gate = new(initialCount: 1); - private RazorCodeDocument? _output; + // Hold the output in a WeakReference to avoid memory leaks in the case of a long-lived + // document snapshots. In particular, the DynamicFileInfo system results in the Roslyn + // workspace holding onto document snapshots. + private WeakReference? _output; public bool TryGetValue([NotNullWhen(true)] out RazorCodeDocument? result) { - result = _output; - return result is not null; + var output = _output; + if (output is null) + { + result = null; + return false; + } + + return output.TryGetTarget(out result); } public async ValueTask GetValueAsync(DocumentSnapshot document, CancellationToken cancellationToken) @@ -39,11 +49,20 @@ public async ValueTask GetValueAsync(DocumentSnapshot documen var projectEngine = project.ProjectEngine; var compilerOptions = project.CompilerOptions; - _output = await CompilationHelpers + result = await CompilationHelpers .GenerateCodeDocumentAsync(document, projectEngine, compilerOptions, cancellationToken) .ConfigureAwait(false); - return _output; + if (_output is null) + { + _output = new WeakReference(result); + } + else + { + _output.SetTarget(result); + } + + return result; } } } From fef1b1135c4d0f163983b9d340a1a857191da624 Mon Sep 17 00:00:00 2001 From: Dustin Campbell Date: Mon, 24 Mar 2025 15:48:38 -0700 Subject: [PATCH 2/6] Make DocumentSnapshot own GeneratedOutputSource instead of DocumentState Now that GeneratedOutputSource holds its RazorCodeDocument in a WeakReference, there's not much point in keeping it on DocumentState. Instead, it can move to DocumentSnapshot, which is really more appropriate since GeneratedOutputSource needed a DocumentSnapshot to perform computation. --- .../ProjectSystem/DocumentSnapshot.cs | 20 +- .../ProjectSystem/DocumentState.cs | 18 +- .../ProjectSystem/ProjectState.cs | 6 +- .../Sources/GeneratedOutputSource.cs | 9 +- .../ProjectSystem/DocumentStateTest.cs | 96 --------- .../ProjectStateGeneratedOutputTest.cs | 195 ------------------ 6 files changed, 24 insertions(+), 320 deletions(-) delete mode 100644 src/Razor/test/Microsoft.CodeAnalysis.Razor.Workspaces.Test/ProjectSystem/ProjectStateGeneratedOutputTest.cs diff --git a/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSystem/DocumentSnapshot.cs b/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSystem/DocumentSnapshot.cs index 9a31327616c..17799b0211c 100644 --- a/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSystem/DocumentSnapshot.cs +++ b/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSystem/DocumentSnapshot.cs @@ -7,15 +7,25 @@ using Microsoft.AspNetCore.Razor.Language; using Microsoft.AspNetCore.Razor.ProjectSystem; using Microsoft.CodeAnalysis.Razor.ProjectSystem.Legacy; +using Microsoft.CodeAnalysis.Razor.ProjectSystem.Sources; using Microsoft.CodeAnalysis.Text; namespace Microsoft.CodeAnalysis.Razor.ProjectSystem; -internal sealed class DocumentSnapshot(ProjectSnapshot project, DocumentState state) : IDocumentSnapshot, ILegacyDocumentSnapshot, IDesignTimeCodeGenerator +internal sealed class DocumentSnapshot : IDocumentSnapshot, ILegacyDocumentSnapshot, IDesignTimeCodeGenerator { - public ProjectSnapshot Project { get; } = project; + private readonly GeneratedOutputSource _generatedOutputSource; - private readonly DocumentState _state = state; + public ProjectSnapshot Project { get; } + + private readonly DocumentState _state; + + public DocumentSnapshot(ProjectSnapshot project, DocumentState state) + { + Project = project; + _state = state; + _generatedOutputSource = new(this); + } public HostDocument HostDocument => _state.HostDocument; @@ -40,10 +50,10 @@ public ValueTask GetTextVersionAsync(CancellationToken cancellatio => _state.GetTextVersionAsync(cancellationToken); public bool TryGetGeneratedOutput([NotNullWhen(true)] out RazorCodeDocument? result) - => _state.TryGetGeneratedOutput(out result); + => _generatedOutputSource.TryGetValue(out result); public ValueTask GetGeneratedOutputAsync(CancellationToken cancellationToken) - => _state.GetGeneratedOutputAsync(this, cancellationToken); + => _generatedOutputSource.GetValueAsync(cancellationToken); public IDocumentSnapshot WithText(SourceText text) { diff --git a/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSystem/DocumentState.cs b/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSystem/DocumentState.cs index 91ff97c3e46..e2f589905ef 100644 --- a/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSystem/DocumentState.cs +++ b/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSystem/DocumentState.cs @@ -4,7 +4,6 @@ using System.Diagnostics.CodeAnalysis; using System.Threading; using System.Threading.Tasks; -using Microsoft.AspNetCore.Razor.Language; using Microsoft.CodeAnalysis.Razor.ProjectSystem.Sources; using Microsoft.CodeAnalysis.Text; @@ -16,14 +15,12 @@ internal sealed partial class DocumentState public int Version { get; } private readonly ITextAndVersionSource _textAndVersionSource; - private readonly GeneratedOutputSource _generatedOutputSource; private DocumentState(HostDocument hostDocument, ITextAndVersionSource textAndVersionSource) { HostDocument = hostDocument; Version = 1; _textAndVersionSource = textAndVersionSource; - _generatedOutputSource = new(); } private DocumentState(DocumentState oldState, ITextAndVersionSource textAndVersionSource) @@ -31,7 +28,6 @@ private DocumentState(DocumentState oldState, ITextAndVersionSource textAndVersi HostDocument = oldState.HostDocument; Version = oldState.Version + 1; _textAndVersionSource = textAndVersionSource; - _generatedOutputSource = new(); } public static DocumentState Create(HostDocument hostDocument, SourceText text) @@ -46,12 +42,6 @@ private static ConstantTextAndVersionSource CreateTextAndVersionSource(SourceTex private static LoadableTextAndVersionSource CreateTextAndVersionSource(TextLoader textLoader) => new(textLoader); - public bool TryGetGeneratedOutput([NotNullWhen(true)] out RazorCodeDocument? result) - => _generatedOutputSource.TryGetValue(out result); - - public ValueTask GetGeneratedOutputAsync(DocumentSnapshot document, CancellationToken cancellationToken) - => _generatedOutputSource.GetValueAsync(document, cancellationToken); - public bool TryGetTextAndVersion([NotNullWhen(true)] out TextAndVersion? result) => _textAndVersionSource.TryGetValue(out result); @@ -110,13 +100,7 @@ async ValueTask GetTextVersionCoreAsync(CancellationToken cancella } } - public DocumentState WithConfigurationChange() - => new(this, _textAndVersionSource); - - public DocumentState WithImportsChange() - => new(this, _textAndVersionSource); - - public DocumentState WithProjectWorkspaceStateChange() + public DocumentState UpdateVersion() => new(this, _textAndVersionSource); public DocumentState WithText(SourceText text, VersionStamp textVersion) diff --git a/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSystem/ProjectState.cs b/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSystem/ProjectState.cs index eb308e9f6b8..35b05d94492 100644 --- a/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSystem/ProjectState.cs +++ b/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSystem/ProjectState.cs @@ -259,7 +259,7 @@ public ProjectState WithHostProject(HostProject hostProject) return this; } - var documents = UpdateDocuments(static x => x.WithConfigurationChange()); + var documents = UpdateDocuments(static x => x.UpdateVersion()); // If the host project has changed then we need to recompute the imports map var importsToRelatedDocuments = BuildImportsMap(documents.Values, ProjectEngine); @@ -277,7 +277,7 @@ public ProjectState WithProjectWorkspaceState(ProjectWorkspaceState projectWorks return this; } - var documents = UpdateDocuments(static x => x.WithProjectWorkspaceStateChange()); + var documents = UpdateDocuments(static x => x.UpdateVersion()); return new(this, HostProject, projectWorkspaceState, documents, ImportsToRelatedDocuments, retainProjectEngine: true); } @@ -378,7 +378,7 @@ private ImmutableDictionary UpdateRelatedDocumentsIfNeces return documents; } - var updates = relatedDocuments.Select(x => KeyValuePair.Create(x, documents[x].WithImportsChange())); + var updates = relatedDocuments.Select(x => KeyValuePair.Create(x, documents[x].UpdateVersion())); return documents.SetItems(updates); } diff --git a/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSystem/Sources/GeneratedOutputSource.cs b/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSystem/Sources/GeneratedOutputSource.cs index 79beb362a83..796ec7742ee 100644 --- a/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSystem/Sources/GeneratedOutputSource.cs +++ b/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSystem/Sources/GeneratedOutputSource.cs @@ -10,8 +10,9 @@ namespace Microsoft.CodeAnalysis.Razor.ProjectSystem.Sources; -internal sealed class GeneratedOutputSource +internal sealed class GeneratedOutputSource(DocumentSnapshot document) { + private readonly DocumentSnapshot _document = document; private readonly SemaphoreSlim _gate = new(initialCount: 1); // Hold the output in a WeakReference to avoid memory leaks in the case of a long-lived @@ -31,7 +32,7 @@ public bool TryGetValue([NotNullWhen(true)] out RazorCodeDocument? result) return output.TryGetTarget(out result); } - public async ValueTask GetValueAsync(DocumentSnapshot document, CancellationToken cancellationToken) + public async ValueTask GetValueAsync(CancellationToken cancellationToken) { if (TryGetValue(out var result)) { @@ -45,12 +46,12 @@ public async ValueTask GetValueAsync(DocumentSnapshot documen return result; } - var project = document.Project; + var project = _document.Project; var projectEngine = project.ProjectEngine; var compilerOptions = project.CompilerOptions; result = await CompilationHelpers - .GenerateCodeDocumentAsync(document, projectEngine, compilerOptions, cancellationToken) + .GenerateCodeDocumentAsync(_document, projectEngine, compilerOptions, cancellationToken) .ConfigureAwait(false); if (_output is null) diff --git a/src/Razor/test/Microsoft.CodeAnalysis.Razor.Workspaces.Test/ProjectSystem/DocumentStateTest.cs b/src/Razor/test/Microsoft.CodeAnalysis.Razor.Workspaces.Test/ProjectSystem/DocumentStateTest.cs index 1162177b1f8..abbd343b4da 100644 --- a/src/Razor/test/Microsoft.CodeAnalysis.Razor.Workspaces.Test/ProjectSystem/DocumentStateTest.cs +++ b/src/Razor/test/Microsoft.CodeAnalysis.Razor.Workspaces.Test/ProjectSystem/DocumentStateTest.cs @@ -61,100 +61,4 @@ public async Task DocumentState_WithTextLoader_CreatesNewState() var text = await state.GetTextAsync(DisposalToken); Assert.Same(_text, text); } - - [Fact] - public void DocumentState_WithConfigurationChange_CachesSnapshotText() - { - // Arrange - var original = DocumentState.Create(_hostDocument, EmptyTextLoader.Instance) - .WithText(_text, VersionStamp.Create()); - - // Act - var state = original.WithConfigurationChange(); - - // Assert - Assert.True(state.TryGetText(out _)); - Assert.True(state.TryGetTextVersion(out _)); - } - - [Fact] - public async Task DocumentState_WithConfigurationChange_CachesLoadedText() - { - // Arrange - var original = DocumentState.Create(_hostDocument, EmptyTextLoader.Instance) - .WithTextLoader(_textLoader); - - await original.GetTextAsync(DisposalToken); - - // Act - var state = original.WithConfigurationChange(); - - // Assert - Assert.True(state.TryGetText(out _)); - Assert.True(state.TryGetTextVersion(out _)); - } - - [Fact] - public void DocumentState_WithImportsChange_CachesSnapshotText() - { - // Arrange - var original = DocumentState.Create(_hostDocument, EmptyTextLoader.Instance) - .WithText(_text, VersionStamp.Create()); - - // Act - var state = original.WithImportsChange(); - - // Assert - Assert.True(state.TryGetText(out _)); - Assert.True(state.TryGetTextVersion(out _)); - } - - [Fact] - public async Task DocumentState_WithImportsChange_CachesLoadedText() - { - // Arrange - var original = DocumentState.Create(_hostDocument, EmptyTextLoader.Instance) - .WithTextLoader(_textLoader); - - await original.GetTextAsync(DisposalToken); - - // Act - var state = original.WithImportsChange(); - - // Assert - Assert.True(state.TryGetText(out _)); - Assert.True(state.TryGetTextVersion(out _)); - } - - [Fact] - public void DocumentState_WithProjectWorkspaceStateChange_CachesSnapshotText() - { - // Arrange - var original = DocumentState.Create(_hostDocument, EmptyTextLoader.Instance) - .WithText(_text, VersionStamp.Create()); - - // Act - var state = original.WithProjectWorkspaceStateChange(); - - // Assert - Assert.True(state.TryGetText(out _)); - Assert.True(state.TryGetTextVersion(out _)); - } - - [Fact] - public async Task DocumentState_WithProjectWorkspaceStateChange_CachesLoadedText() - { - // Arrange - var original = DocumentState.Create(_hostDocument, EmptyTextLoader.Instance) - .WithTextLoader(_textLoader); - - await original.GetTextAsync(DisposalToken); - - // Act - var state = original.WithProjectWorkspaceStateChange(); - - // Assert - Assert.True(state.TryGetText(out _)); - Assert.True(state.TryGetTextVersion(out _)); - } } diff --git a/src/Razor/test/Microsoft.CodeAnalysis.Razor.Workspaces.Test/ProjectSystem/ProjectStateGeneratedOutputTest.cs b/src/Razor/test/Microsoft.CodeAnalysis.Razor.Workspaces.Test/ProjectSystem/ProjectStateGeneratedOutputTest.cs deleted file mode 100644 index 00e59121da3..00000000000 --- a/src/Razor/test/Microsoft.CodeAnalysis.Razor.Workspaces.Test/ProjectSystem/ProjectStateGeneratedOutputTest.cs +++ /dev/null @@ -1,195 +0,0 @@ -// Copyright (c) .NET Foundation. All rights reserved. -// Licensed under the MIT license. See License.txt in the project root for license information. - -using System.Collections.Immutable; -using System.Threading.Tasks; -using Microsoft.AspNetCore.Razor.Language; -using Microsoft.AspNetCore.Razor.ProjectSystem; -using Microsoft.AspNetCore.Razor.Test.Common; -using Microsoft.AspNetCore.Razor.Test.Common.Workspaces; -using Xunit; -using Xunit.Abstractions; - -namespace Microsoft.CodeAnalysis.Razor.ProjectSystem; - -public class ProjectStateGeneratedOutputTest : WorkspaceTestBase -{ - private readonly HostDocument _hostDocument; - private readonly HostProject _hostProject; - private readonly HostProject _hostProjectWithConfigurationChange; - private readonly ImmutableArray _someTagHelpers; - - public ProjectStateGeneratedOutputTest(ITestOutputHelper testOutput) - : base(testOutput) - { - _hostProject = TestProjectData.SomeProject with { Configuration = FallbackRazorConfiguration.MVC_2_0 }; - _hostProjectWithConfigurationChange = TestProjectData.SomeProject with { Configuration = FallbackRazorConfiguration.MVC_1_0 }; - - _someTagHelpers = ImmutableArray.Create( - TagHelperDescriptorBuilder.Create("Test1", "TestAssembly").Build()); - - _hostDocument = TestProjectData.SomeProjectFile1; - } - - protected override void ConfigureProjectEngine(RazorProjectEngineBuilder builder) - { - builder.SetImportFeature(new TestImportProjectFeature(HierarchicalImports.Legacy)); - } - - [Fact] - public async Task AddDocument_CachesOutput() - { - // Arrange - var state = ProjectState - .Create(_hostProject, CompilerOptions, ProjectEngineFactoryProvider) - .AddEmptyDocument(_hostDocument); - - var output = await GetGeneratedOutputAsync(state, _hostDocument); - - // Act - var newState = state.AddEmptyDocument(TestProjectData.AnotherProjectFile1); - var newOutput = await GetGeneratedOutputAsync(newState, _hostDocument); - - // Assert - Assert.Same(output, newOutput); - } - - [Fact] - public async Task AddDocument_Import_DoesNotCacheOutput() - { - // Arrange - var state = ProjectState - .Create(_hostProject, CompilerOptions, ProjectEngineFactoryProvider) - .AddEmptyDocument(_hostDocument); - - var output = await GetGeneratedOutputAsync(state, _hostDocument); - - // Act - var newState = state.AddEmptyDocument(TestProjectData.SomeProjectImportFile); - var newOutput = await GetGeneratedOutputAsync(newState, _hostDocument); - - // Assert - Assert.NotSame(output, newOutput); - } - - [Fact] - public async Task WithDocumentText_DoesNotCacheOutput() - { - // Arrange - var state = ProjectState - .Create(_hostProject, CompilerOptions, ProjectEngineFactoryProvider) - .AddEmptyDocument(_hostDocument) - .AddEmptyDocument(TestProjectData.SomeProjectImportFile); - - var output = await GetGeneratedOutputAsync(state, _hostDocument); - - // Act - var newState = state.WithDocumentText(_hostDocument.FilePath, TestMocks.CreateTextLoader("@using System")); - var newOutput = await GetGeneratedOutputAsync(newState, _hostDocument); - - // Assert - Assert.NotSame(output, newOutput); - } - - [Fact] - public async Task WithDocumentText_Import_DoesNotCacheOutput() - { - // Arrange - var state = ProjectState - .Create(_hostProject, CompilerOptions, ProjectEngineFactoryProvider) - .AddEmptyDocument(_hostDocument) - .AddEmptyDocument(TestProjectData.SomeProjectImportFile); - - var output = await GetGeneratedOutputAsync(state, _hostDocument); - - // Act - var newState = state.WithDocumentText(TestProjectData.SomeProjectImportFile.FilePath, TestMocks.CreateTextLoader("@using System")); - var newOutput = await GetGeneratedOutputAsync(newState, _hostDocument); - - // Assert - Assert.NotSame(output, newOutput); - } - - [Fact] - public async Task RemoveDocument_Import_DoesNotCacheOutput() - { - // Arrange - var state = ProjectState - .Create(_hostProject, CompilerOptions, ProjectEngineFactoryProvider) - .AddEmptyDocument(_hostDocument) - .AddEmptyDocument(TestProjectData.SomeProjectImportFile); - - var output = await GetGeneratedOutputAsync(state, _hostDocument); - - // Act - var newState = state.RemoveDocument(TestProjectData.SomeProjectImportFile.FilePath); - var newOutput = await GetGeneratedOutputAsync(newState, _hostDocument); - - // Assert - Assert.NotSame(output, newOutput); - } - - [Fact] - public async Task WithProjectWorkspaceState_CachesOutput_EvenWhenNewerProjectWorkspaceState() - { - // Arrange - var state = ProjectState - .Create(_hostProject, CompilerOptions, ProjectEngineFactoryProvider) - .AddEmptyDocument(_hostDocument); - - var output = await GetGeneratedOutputAsync(state, _hostDocument); - - // Act - var newState = state.WithProjectWorkspaceState(ProjectWorkspaceState.Default); - var newOutput = await GetGeneratedOutputAsync(newState, _hostDocument); - - // Assert - Assert.Same(output, newOutput); - } - - [Fact] - public async Task WithProjectWorkspaceState_TagHelperChange_DoesNotCacheOutput() - { - // Arrange - var state = ProjectState - .Create(_hostProject, CompilerOptions, ProjectEngineFactoryProvider) - .AddEmptyDocument(_hostDocument); - - var output = await GetGeneratedOutputAsync(state, _hostDocument); - - // Act - var newState = state.WithProjectWorkspaceState(ProjectWorkspaceState.Create(_someTagHelpers)); - var newOutput = await GetGeneratedOutputAsync(newState, _hostDocument); - - // Assert - Assert.NotSame(output, newOutput); - } - - [Fact] - public async Task WithHostProject_DoesNotCacheOutput() - { - // Arrange - var state = ProjectState - .Create(_hostProject, CompilerOptions, ProjectEngineFactoryProvider) - .AddEmptyDocument(_hostDocument); - - var output = await GetGeneratedOutputAsync(state, _hostDocument); - - // Act - var newState = state.WithHostProject(_hostProjectWithConfigurationChange); - var newOutput = await GetGeneratedOutputAsync(newState, _hostDocument); - - // Assert - Assert.NotSame(output, newOutput); - } - - private ValueTask GetGeneratedOutputAsync(ProjectState project, HostDocument hostDocument) - { - var document = project.Documents[hostDocument.FilePath]; - - var projectSnapshot = new ProjectSnapshot(project); - var documentSnapshot = new DocumentSnapshot(projectSnapshot, document); - - return documentSnapshot.GetGeneratedOutputAsync(DisposalToken); - } -} From ca595e14aba45afafa32baa0fa119a7ff0178286 Mon Sep 17 00:00:00 2001 From: Dustin Campbell Date: Mon, 24 Mar 2025 16:53:12 -0700 Subject: [PATCH 3/6] BackgroundDocumentGenerator: Stop forcing compilation The BackgroundDocumentGenerator doesn't need to force compilation before updating the DynamicFileInfo system with a document snapshot. When the Roslyn workspace receives the DynamicFileInfo, compilation will occur through the TextLoader. --- .../DynamicFiles/BackgroundDocumentGenerator.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Razor/src/Microsoft.VisualStudio.LanguageServices.Razor/DynamicFiles/BackgroundDocumentGenerator.cs b/src/Razor/src/Microsoft.VisualStudio.LanguageServices.Razor/DynamicFiles/BackgroundDocumentGenerator.cs index 176ee3622eb..4019de0e859 100644 --- a/src/Razor/src/Microsoft.VisualStudio.LanguageServices.Razor/DynamicFiles/BackgroundDocumentGenerator.cs +++ b/src/Razor/src/Microsoft.VisualStudio.LanguageServices.Razor/DynamicFiles/BackgroundDocumentGenerator.cs @@ -86,11 +86,11 @@ public void Dispose() protected Task WaitUntilCurrentBatchCompletesAsync() => _workQueue.WaitUntilCurrentBatchCompletesAsync(); - protected virtual async Task ProcessDocumentAsync(DocumentSnapshot document, CancellationToken cancellationToken) + protected virtual Task ProcessDocumentAsync(DocumentSnapshot document, CancellationToken cancellationToken) { - await document.GetGeneratedOutputAsync(cancellationToken).ConfigureAwait(false); - UpdateFileInfo(document); + + return Task.CompletedTask; } public virtual void EnqueueIfNecessary(DocumentKey documentKey) From 18f87c243046c70da7d820f8532f255cea695943 Mon Sep 17 00:00:00 2001 From: Dustin Campbell Date: Mon, 24 Mar 2025 16:54:46 -0700 Subject: [PATCH 4/6] RazorDynamicFileInfoProvider: A bit of clean up --- .../RazorDynamicFileInfoProvider.cs | 185 +++++++----------- 1 file changed, 66 insertions(+), 119 deletions(-) diff --git a/src/Razor/src/Microsoft.VisualStudio.LanguageServices.Razor/DynamicFiles/RazorDynamicFileInfoProvider.cs b/src/Razor/src/Microsoft.VisualStudio.LanguageServices.Razor/DynamicFiles/RazorDynamicFileInfoProvider.cs index 83738b06df1..057a4b8fbf9 100644 --- a/src/Razor/src/Microsoft.VisualStudio.LanguageServices.Razor/DynamicFiles/RazorDynamicFileInfoProvider.cs +++ b/src/Razor/src/Microsoft.VisualStudio.LanguageServices.Razor/DynamicFiles/RazorDynamicFileInfoProvider.cs @@ -6,6 +6,7 @@ using System.Collections.Generic; using System.ComponentModel.Composition; using System.Diagnostics; +using System.Diagnostics.CodeAnalysis; using System.Text; using System.Threading; using System.Threading.Tasks; @@ -68,15 +69,8 @@ public void UpdateLSPFileInfo(Uri documentUri, IDynamicDocumentContainer documen { Debug.Assert(!_languageServerFeatureOptions.UseRazorCohostServer, "Should never be called in cohosting"); - if (documentUri is null) - { - throw new ArgumentNullException(nameof(documentUri)); - } - - if (documentContainer is null) - { - throw new ArgumentNullException(nameof(documentContainer)); - } + ArgHelper.ThrowIfNull(documentUri); + ArgHelper.ThrowIfNull(documentContainer); // This endpoint is only called in LSP cases when the file is open(ed) // We report diagnostics are supported to Roslyn in this case @@ -109,10 +103,7 @@ public void UpdateFileInfo(ProjectKey projectKey, IDynamicDocumentContainer docu { Debug.Assert(!_languageServerFeatureOptions.UseRazorCohostServer, "Should never be called in cohosting"); - if (documentContainer is null) - { - throw new ArgumentNullException(nameof(documentContainer)); - } + ArgHelper.ThrowIfNull(documentContainer); // This endpoint is called either when: // 1. LSP: File is closed @@ -123,8 +114,7 @@ public void UpdateFileInfo(ProjectKey projectKey, IDynamicDocumentContainer docu // There's a possible race condition here where we're processing an update // and the project is getting unloaded. So if we don't find an entry we can // just ignore it. - var projectId = TryFindProjectIdForProjectKey(projectKey); - if (projectId is null) + if (!TryFindProjectIdForProjectKey(projectKey, out var projectId)) { return; } @@ -147,46 +137,35 @@ public void PromoteBackgroundDocument(Uri documentUri, IRazorDocumentPropertiesS { Debug.Assert(!_languageServerFeatureOptions.UseRazorCohostServer, "Should never be called in cohosting"); - if (documentUri is null) - { - throw new ArgumentNullException(nameof(documentUri)); - } - - if (propertiesService is null) - { - throw new ArgumentNullException(nameof(propertiesService)); - } + ArgHelper.ThrowIfNull(documentUri); + ArgHelper.ThrowIfNull(propertiesService); var filePath = GetProjectSystemFilePath(documentUri); - foreach (var associatedKvp in GetAllKeysForPath(filePath)) + foreach (var (key, entry) in GetAllKeysForPath(filePath)) { - var associatedKey = associatedKvp.Key; - var associatedEntry = associatedKvp.Value; - - var projectId = associatedKey.ProjectId; - var projectKey = TryFindProjectKeyForProjectId(projectId); - if (projectKey is not ProjectKey key) + var projectId = key.ProjectId; + if (!TryFindProjectKeyForProjectId(projectId, out var projectKey)) { Debug.Fail("Could not find project key for project id. This should never happen."); continue; } - var filename = _filePathService.GetRazorCSharpFilePath(key, associatedKey.FilePath); + var filename = _filePathService.GetRazorCSharpFilePath(projectKey, key.FilePath); // To promote the background document, we just need to add the passed in properties service to // the dynamic file info. The properties service contains the client name and allows the C# // server to recognize the document. - var documentServiceProvider = associatedEntry.Current.DocumentServiceProvider; + var documentServiceProvider = entry.Current.DocumentServiceProvider; var excerptService = documentServiceProvider.GetService(); var spanMappingService = documentServiceProvider.GetService(); var mappingService = documentServiceProvider.GetService(); var emptyContainer = new PromotedDynamicDocumentContainer( - documentUri, propertiesService, excerptService, spanMappingService, mappingService, associatedEntry.Current.TextLoader); + documentUri, propertiesService, excerptService, spanMappingService, mappingService, entry.Current.TextLoader); - lock (associatedEntry.Lock) + lock (entry.Lock) { - associatedEntry.Current = new RazorDynamicFileInfo( - filename, associatedEntry.Current.SourceCodeKind, associatedEntry.Current.TextLoader, _factory.Create(emptyContainer)); + entry.Current = new RazorDynamicFileInfo( + filename, entry.Current.SourceCodeKind, entry.Current.TextLoader, _factory.Create(emptyContainer)); } } @@ -217,8 +196,7 @@ public void SuppressDocument(DocumentKey documentKey) // There's a possible race condition here where we're processing an update // and the project is getting unloaded. So if we don't find an entry we can // just ignore it. - var projectId = TryFindProjectIdForProjectKey(documentKey.ProjectKey); - if (projectId is null) + if (!TryFindProjectIdForProjectKey(documentKey.ProjectKey, out var projectId)) { return; } @@ -250,25 +228,17 @@ public void SuppressDocument(DocumentKey documentKey) return SpecializedTasks.Null(); } - if (projectFilePath is null) - { - throw new ArgumentNullException(nameof(projectFilePath)); - } - - if (filePath is null) - { - throw new ArgumentNullException(nameof(filePath)); - } + ArgHelper.ThrowIfNull(projectFilePath); + ArgHelper.ThrowIfNull(filePath); // We are activated for all Roslyn projects that have a .cshtml or .razor file, but they are not necessarily // C# projects that we expect. - var projectKey = TryFindProjectKeyForProjectId(projectId); - if (projectKey is not { } razorProjectKey) + if (!TryFindProjectKeyForProjectId(projectId, out var projectKey)) { return SpecializedTasks.Null(); } - _fallbackProjectManager.DynamicFileAdded(projectId, razorProjectKey, projectFilePath, filePath, cancellationToken); + _fallbackProjectManager.DynamicFileAdded(projectId, projectKey, projectFilePath, filePath, cancellationToken); var key = new Key(projectId, filePath); var entry = _entries.GetOrAdd(key, _createEmptyEntry); @@ -280,23 +250,15 @@ public Task RemoveDynamicFileInfoAsync(ProjectId projectId, string? projectFileP { Debug.Assert(!_languageServerFeatureOptions.UseRazorCohostServer, "Should never be called in cohosting"); - if (projectFilePath is null) - { - throw new ArgumentNullException(nameof(projectFilePath)); - } - - if (filePath is null) - { - throw new ArgumentNullException(nameof(filePath)); - } + ArgHelper.ThrowIfNull(projectFilePath); + ArgHelper.ThrowIfNull(filePath); - var projectKey = TryFindProjectKeyForProjectId(projectId); - if (projectKey is not { } razorProjectKey) + if (!TryFindProjectKeyForProjectId(projectId, out var projectKey)) { return Task.CompletedTask; } - _fallbackProjectManager.DynamicFileRemoved(projectId, razorProjectKey, projectFilePath, filePath, cancellationToken); + _fallbackProjectManager.DynamicFileRemoved(projectId, projectKey, projectFilePath, filePath, cancellationToken); // ---------------------------------------------------------- NOTE & CAUTION -------------------------------------------------------------- // @@ -330,8 +292,6 @@ public static string GetProjectSystemFilePath(Uri uri) return uri.AbsolutePath; } - public TestAccessor GetTestAccessor() => new(this); - private void ProjectManager_Changed(object? sender, ProjectChangeEventArgs args) { Debug.Assert(!_languageServerFeatureOptions.UseRazorCohostServer, "Should never be called in cohosting"); @@ -352,7 +312,7 @@ private void ProjectManager_Changed(object? sender, ProjectChangeEventArgs args) { var removedProject = args.Older.AssumeNotNull(); - if (TryFindProjectIdForProjectKey(removedProject.Key) is { } projectId) + if (TryFindProjectIdForProjectKey(removedProject.Key, out var projectId)) { foreach (var documentFilePath in removedProject.DocumentFilePaths) { @@ -366,56 +326,61 @@ private void ProjectManager_Changed(object? sender, ProjectChangeEventArgs args) } } - private ProjectId? TryFindProjectIdForProjectKey(ProjectKey key) + private bool TryFindProjectIdForProjectKey(ProjectKey key, [NotNullWhen(true)] out ProjectId? projectId) { var workspace = _workspaceProvider.GetWorkspace(); if (workspace.CurrentSolution.TryGetProject(key, out var project)) { - return project.Id; + projectId = project.Id; + return true; } - return null; + projectId = null; + return false; } - private ProjectKey? TryFindProjectKeyForProjectId(ProjectId projectId) + private bool TryFindProjectKeyForProjectId(ProjectId projectId, out ProjectKey projectKey) { var workspace = _workspaceProvider.GetWorkspace(); - return workspace.CurrentSolution.GetProject(projectId) is { Language: LanguageNames.CSharp } project - ? project.ToProjectKey() - : null; + if (workspace.CurrentSolution.GetProject(projectId) is { Language: LanguageNames.CSharp } project) + { + projectKey = project.ToProjectKey(); + return true; + } + + projectKey = default; + return false; } private RazorDynamicFileInfo CreateEmptyInfo(Key key) { - var projectKey = TryFindProjectKeyForProjectId(key.ProjectId).AssumeNotNull(); + Assumed.True(TryFindProjectKeyForProjectId(key.ProjectId, out var projectKey)); + var filename = _filePathService.GetRazorCSharpFilePath(projectKey, key.FilePath); var textLoader = new EmptyTextLoader(filename); + return new RazorDynamicFileInfo(filename, SourceCodeKind.Regular, textLoader, _factory.CreateEmpty()); } private RazorDynamicFileInfo CreateInfo(Key key, IDynamicDocumentContainer document) { - var projectKey = TryFindProjectKeyForProjectId(key.ProjectId).AssumeNotNull(); + Assumed.True(TryFindProjectKeyForProjectId(key.ProjectId, out var projectKey)); + var filename = _filePathService.GetRazorCSharpFilePath(projectKey, key.FilePath); var textLoader = document.GetTextLoader(filename); + return new RazorDynamicFileInfo(filename, SourceCodeKind.Regular, textLoader, _factory.Create(document)); } // Using a separate handle to the 'current' file info so that can allow Roslyn to send // us the add/remove operations, while we process the update operations. - private class Entry + private sealed class Entry(RazorDynamicFileInfo current) { - public Entry(RazorDynamicFileInfo current) - { - Current = current; - Lock = new object(); - } - - public RazorDynamicFileInfo Current { get; set; } + public RazorDynamicFileInfo Current { get; set; } = current; - public object Lock { get; } + public object Lock { get; } = new object(); public override string ToString() { @@ -426,28 +391,17 @@ public override string ToString() } } - private readonly struct Key : IEquatable + private readonly struct Key(ProjectId projectId, string filePath) : IEquatable { - public readonly ProjectId ProjectId; - public readonly string FilePath; - - public Key(ProjectId projectId, string filePath) - { - ProjectId = projectId; - FilePath = filePath; - } + public readonly ProjectId ProjectId = projectId; + public readonly string FilePath = filePath; public bool Equals(Key other) - { - return - ProjectId.Equals(other.ProjectId) && - FilePathComparer.Instance.Equals(FilePath, other.FilePath); - } + => ProjectId.Equals(other.ProjectId) && + FilePathComparer.Instance.Equals(FilePath, other.FilePath); public override bool Equals(object? obj) - { - return obj is Key other && Equals(other); - } + => obj is Key other && Equals(other); public override int GetHashCode() { @@ -458,22 +412,18 @@ public override int GetHashCode() } } - private class EmptyTextLoader : TextLoader + private class EmptyTextLoader(string filePath) : TextLoader { - private readonly string _filePath; - private readonly VersionStamp _version; + // Providing an encoding here is important for debuggability. Without this edit-and-continue + // won't work for projects with Razor files. + private static readonly SourceText s_emptyText = SourceText.From("", Encoding.UTF8); - public EmptyTextLoader(string filePath) - { - _filePath = filePath; - _version = VersionStamp.Default; // Version will never change so this can be reused. - } + private readonly string _filePath = filePath; public override Task LoadTextAndVersionAsync(LoadTextOptions options, CancellationToken cancellationToken) { - // Providing an encoding here is important for debuggability. Without this edit-and-continue - // won't work for projects with Razor files. - return Task.FromResult(TextAndVersion.Create(SourceText.From("", Encoding.UTF8), _version, _filePath)); + var version = VersionStamp.Default; // Version will never change so this can be reused. + return Task.FromResult(TextAndVersion.Create(s_emptyText, version, _filePath)); } } @@ -512,14 +462,11 @@ public void SetSupportsDiagnostics(bool value) public IRazorMappingService? GetMappingService() => _mappingService; } - public class TestAccessor - { - private readonly RazorDynamicFileInfoProvider _provider; + public TestAccessor GetTestAccessor() => new(this); - public TestAccessor(RazorDynamicFileInfoProvider provider) - { - _provider = provider; - } + public class TestAccessor(RazorDynamicFileInfoProvider provider) + { + private readonly RazorDynamicFileInfoProvider _provider = provider; public async Task GetDynamicFileInfoAsync(ProjectId projectId, string filePath, CancellationToken cancellationToken) { From 24ba58a09b6c664753e298d8829b75a0ece23788 Mon Sep 17 00:00:00 2001 From: Dustin Campbell Date: Sat, 22 Mar 2025 05:39:06 -0700 Subject: [PATCH 5/6] ProjectSnapshot: Set capacity and don't create map until needed ProjectSnapshot.TryGetDocument(...) turns out to be a significant source of dictionary growth. However, we always know the number of documents in a particular ProjectSnapshot from its ProjectState, so we can set the capacity up front. --- .../ProjectSystem/ProjectSnapshot.cs | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSystem/ProjectSnapshot.cs b/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSystem/ProjectSnapshot.cs index 07ba138b574..6c8ee99348a 100644 --- a/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSystem/ProjectSnapshot.cs +++ b/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSystem/ProjectSnapshot.cs @@ -20,7 +20,7 @@ internal sealed class ProjectSnapshot(ProjectState state) : IProjectSnapshot, IL private readonly ProjectState _state = state; private readonly object _gate = new(); - private readonly Dictionary _filePathToDocumentMap = new(FilePathNormalizingComparer.Instance); + private Dictionary? _filePathToDocumentMap; public HostProject HostProject => _state.HostProject; public RazorCompilerOptions CompilerOptions => _state.CompilerOptions; @@ -53,8 +53,12 @@ public bool ContainsDocument(string filePath) // ImmutableDictionary<,>, which has O(log n) lookup. So, checking _filePathToDocumentMap // first is faster if the DocumentSnapshot has already been created. - return _filePathToDocumentMap.ContainsKey(filePath) || - _state.Documents.ContainsKey(filePath); + if (_filePathToDocumentMap is not null && _filePathToDocumentMap.ContainsKey(filePath)) + { + return true; + } + + return _state.Documents.ContainsKey(filePath); } } @@ -63,7 +67,8 @@ public bool TryGetDocument(string filePath, [NotNullWhen(true)] out DocumentSnap lock (_gate) { // Have we already seen this document? If so, return it! - if (_filePathToDocumentMap.TryGetValue(filePath, out var snapshot)) + if (_filePathToDocumentMap is not null && + _filePathToDocumentMap.TryGetValue(filePath, out var snapshot)) { document = snapshot; return true; @@ -78,6 +83,8 @@ public bool TryGetDocument(string filePath, [NotNullWhen(true)] out DocumentSnap // If we have DocumentState, go ahead and create a new DocumentSnapshot. snapshot = new DocumentSnapshot(this, state); + + _filePathToDocumentMap ??= new(capacity: _state.Documents.Count, FilePathNormalizingComparer.Instance); _filePathToDocumentMap.Add(filePath, snapshot); document = snapshot; From 527fddf5c01e617cb7e56b995e42d33689ef67ea Mon Sep 17 00:00:00 2001 From: Dustin Campbell Date: Thu, 27 Mar 2025 12:32:06 -0700 Subject: [PATCH 6/6] GeneratedOutputSource: Use target-typed new expression --- .../ProjectSystem/Sources/GeneratedOutputSource.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSystem/Sources/GeneratedOutputSource.cs b/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSystem/Sources/GeneratedOutputSource.cs index 796ec7742ee..60cd649c033 100644 --- a/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSystem/Sources/GeneratedOutputSource.cs +++ b/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSystem/Sources/GeneratedOutputSource.cs @@ -56,7 +56,7 @@ public async ValueTask GetValueAsync(CancellationToken cancel if (_output is null) { - _output = new WeakReference(result); + _output = new(result); } else {