Skip to content

Commit 2a06d37

Browse files
author
Andrew Hall
authored
Remove dispatcher from DocumentVersionCache (#9026)
Takes document version tracking off the dispatcher thread requirement and uses a locking mechanism instead.
1 parent 7a3db42 commit 2a06d37

File tree

7 files changed

+82
-89
lines changed

7 files changed

+82
-89
lines changed

‎src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/DefaultDocumentVersionCache.cs‎

Lines changed: 63 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,6 @@
44
using System;
55
using System.Collections.Generic;
66
using System.Diagnostics.CodeAnalysis;
7-
using System.Threading;
8-
using System.Threading.Tasks;
97
using Microsoft.CodeAnalysis.Razor;
108
using Microsoft.CodeAnalysis.Razor.ProjectSystem;
119

@@ -16,17 +14,16 @@ internal class DefaultDocumentVersionCache : DocumentVersionCache
1614
internal const int MaxDocumentTrackingCount = 20;
1715

1816
// Internal for testing
19-
internal readonly Dictionary<string, List<DocumentEntry>> DocumentLookup;
20-
private readonly ProjectSnapshotManagerDispatcher _dispatcher;
17+
internal readonly Dictionary<string, List<DocumentEntry>> DocumentLookup_NeedsLock;
18+
private readonly ReadWriterLocker _lock = new();
2119
private ProjectSnapshotManagerBase? _projectSnapshotManager;
2220

2321
private ProjectSnapshotManagerBase ProjectSnapshotManager
2422
=> _projectSnapshotManager ?? throw new InvalidOperationException("ProjectSnapshotManager accessed before Initialized was called.");
2523

26-
public DefaultDocumentVersionCache(ProjectSnapshotManagerDispatcher dispatcher)
24+
public DefaultDocumentVersionCache()
2725
{
28-
_dispatcher = dispatcher ?? throw new ArgumentNullException(nameof(dispatcher));
29-
DocumentLookup = new Dictionary<string, List<DocumentEntry>>(FilePathComparer.Instance);
26+
DocumentLookup_NeedsLock = new(FilePathComparer.Instance);
3027
}
3128

3229
public override void TrackDocumentVersion(IDocumentSnapshot documentSnapshot, int version)
@@ -36,27 +33,34 @@ public override void TrackDocumentVersion(IDocumentSnapshot documentSnapshot, in
3633
throw new ArgumentNullException(nameof(documentSnapshot));
3734
}
3835

39-
_dispatcher.AssertDispatcherThread();
40-
4136
var filePath = documentSnapshot.FilePath.AssumeNotNull();
37+
using var upgradeableReadLock = _lock.EnterUpgradeAbleReadLock();
38+
TrackDocumentVersion(documentSnapshot, version, filePath, upgradeableReadLock);
39+
}
4240

43-
if (!DocumentLookup.TryGetValue(filePath, out var documentEntries))
41+
private void TrackDocumentVersion(IDocumentSnapshot documentSnapshot, int version, string filePath, ReadWriterLocker.UpgradeableReadLock upgradeableReadLock)
42+
{
43+
// Need to ensure the write lock covers all uses of documentEntries, not just DocumentLookup
44+
using (upgradeableReadLock.EnterWriteLock())
4445
{
45-
documentEntries = new List<DocumentEntry>();
46-
DocumentLookup[filePath] = documentEntries;
47-
}
46+
if (!DocumentLookup_NeedsLock.TryGetValue(filePath, out var documentEntries))
47+
{
48+
documentEntries = new List<DocumentEntry>();
49+
DocumentLookup_NeedsLock[filePath] = documentEntries;
50+
}
4851

49-
if (documentEntries.Count == MaxDocumentTrackingCount)
50-
{
51-
// Clear the oldest document entry
52+
if (documentEntries.Count == MaxDocumentTrackingCount)
53+
{
54+
// Clear the oldest document entry
5255

53-
// With this approach we'll slowly leak memory as new documents are added to the system. We don't clear up
54-
// document file paths where where all of the corresponding entries are expired.
55-
documentEntries.RemoveAt(0);
56-
}
56+
// With this approach we'll slowly leak memory as new documents are added to the system. We don't clear up
57+
// document file paths where where all of the corresponding entries are expired.
58+
documentEntries.RemoveAt(0);
59+
}
5760

58-
var entry = new DocumentEntry(documentSnapshot, version);
59-
documentEntries.Add(entry);
61+
var entry = new DocumentEntry(documentSnapshot, version);
62+
documentEntries.Add(entry);
63+
}
6064
}
6165

6266
public override bool TryGetDocumentVersion(IDocumentSnapshot documentSnapshot, [NotNullWhen(true)] out int? version)
@@ -66,11 +70,10 @@ public override bool TryGetDocumentVersion(IDocumentSnapshot documentSnapshot, [
6670
throw new ArgumentNullException(nameof(documentSnapshot));
6771
}
6872

69-
_dispatcher.AssertDispatcherThread();
70-
7173
var filePath = documentSnapshot.FilePath.AssumeNotNull();
74+
using var _ = _lock.EnterReadLock();
7275

73-
if (!DocumentLookup.TryGetValue(filePath, out var documentEntries))
76+
if (!DocumentLookup_NeedsLock.TryGetValue(filePath, out var documentEntries))
7477
{
7578
version = null;
7679
return false;
@@ -98,22 +101,6 @@ public override bool TryGetDocumentVersion(IDocumentSnapshot documentSnapshot, [
98101
return true;
99102
}
100103

101-
public override Task<int?> TryGetDocumentVersionAsync(IDocumentSnapshot documentSnapshot, CancellationToken cancellationToken)
102-
{
103-
if (documentSnapshot is null)
104-
{
105-
throw new ArgumentNullException(nameof(documentSnapshot));
106-
}
107-
108-
return _dispatcher.RunOnDispatcherThreadAsync(
109-
() =>
110-
{
111-
TryGetDocumentVersion(documentSnapshot, out var version);
112-
return version;
113-
},
114-
cancellationToken);
115-
}
116-
117104
public override void Initialize(ProjectSnapshotManagerBase projectManager)
118105
{
119106
if (projectManager is null)
@@ -133,18 +120,21 @@ private void ProjectSnapshotManager_Changed(object? sender, ProjectChangeEventAr
133120
return;
134121
}
135122

136-
_dispatcher.AssertDispatcherThread();
123+
var upgradeableLock = _lock.EnterUpgradeAbleReadLock();
137124

138125
switch (args.Kind)
139126
{
140127
case ProjectChangeKind.DocumentChanged:
141-
var documentFilePath = args.DocumentFilePath!;
142-
if (DocumentLookup.ContainsKey(documentFilePath) &&
143-
!ProjectSnapshotManager.IsDocumentOpen(documentFilePath))
144-
{
145-
// Document closed, evict entry.
146-
DocumentLookup.Remove(documentFilePath);
147-
}
128+
var documentFilePath = args.DocumentFilePath!;
129+
if (DocumentLookup_NeedsLock.ContainsKey(documentFilePath) &&
130+
!ProjectSnapshotManager.IsDocumentOpen(documentFilePath))
131+
{
132+
using (upgradeableLock.EnterWriteLock())
133+
{
134+
// Document closed, evict entry.
135+
DocumentLookup_NeedsLock.Remove(documentFilePath);
136+
}
137+
}
148138

149139
break;
150140
}
@@ -158,27 +148,22 @@ private void ProjectSnapshotManager_Changed(object? sender, ProjectChangeEventAr
158148
return;
159149
}
160150

161-
CaptureProjectDocumentsAsLatest(project);
151+
CaptureProjectDocumentsAsLatest(project, upgradeableLock);
162152
}
163153

164154
// Internal for testing
165155
internal void MarkAsLatestVersion(IDocumentSnapshot document)
166156
{
167-
var filePath = document.FilePath.AssumeNotNull();
168-
169-
if (!TryGetLatestVersionFromPath(filePath, out var latestVersion))
170-
{
171-
return;
172-
}
173-
174-
// Update our internal tracking state to track the changed document as the latest document.
175-
TrackDocumentVersion(document, latestVersion.Value);
157+
using var upgradeableLock = _lock.EnterUpgradeAbleReadLock();
158+
MarkAsLatestVersion(document, upgradeableLock);
176159
}
177160

178161
// Internal for testing
179162
internal bool TryGetLatestVersionFromPath(string filePath, [NotNullWhen(true)] out int? version)
180163
{
181-
if (!DocumentLookup.TryGetValue(filePath, out var documentEntries))
164+
using var _ = _lock.EnterReadLock();
165+
166+
if (!DocumentLookup_NeedsLock.TryGetValue(filePath, out var documentEntries))
182167
{
183168
version = null;
184169
return false;
@@ -190,18 +175,33 @@ internal bool TryGetLatestVersionFromPath(string filePath, [NotNullWhen(true)] o
190175
return true;
191176
}
192177

193-
private void CaptureProjectDocumentsAsLatest(IProjectSnapshot projectSnapshot)
178+
private void CaptureProjectDocumentsAsLatest(IProjectSnapshot projectSnapshot, ReadWriterLocker.UpgradeableReadLock upgradeableReadLock)
194179
{
195180
foreach (var documentPath in projectSnapshot.DocumentFilePaths)
196181
{
197-
if (DocumentLookup.ContainsKey(documentPath) &&
182+
if (DocumentLookup_NeedsLock.ContainsKey(documentPath) &&
198183
projectSnapshot.GetDocument(documentPath) is { } document)
199184
{
200-
MarkAsLatestVersion(document);
185+
MarkAsLatestVersion(document, upgradeableReadLock);
201186
}
202187
}
203188
}
204189

190+
private void MarkAsLatestVersion(IDocumentSnapshot document, ReadWriterLocker.UpgradeableReadLock upgradeableReadLock)
191+
{
192+
var filePath = document.FilePath.AssumeNotNull();
193+
194+
if (!DocumentLookup_NeedsLock.TryGetValue(filePath, out var documentEntries))
195+
{
196+
return;
197+
}
198+
199+
var latestEntry = documentEntries[^1];
200+
201+
// Update our internal tracking state to track the changed document as the latest document.
202+
TrackDocumentVersion(document, latestEntry.Version, document.FilePath.AssumeNotNull(), upgradeableReadLock);
203+
}
204+
205205
internal class DocumentEntry
206206
{
207207
public DocumentEntry(IDocumentSnapshot document, int version)

‎src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/DocumentVersionCache.cs‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,17 +2,12 @@
22
// Licensed under the MIT license. See License.txt in the project root for license information.
33

44
using System.Diagnostics.CodeAnalysis;
5-
using System.Threading;
6-
using System.Threading.Tasks;
75
using Microsoft.CodeAnalysis.Razor.ProjectSystem;
86

97
namespace Microsoft.AspNetCore.Razor.LanguageServer;
108

119
internal abstract class DocumentVersionCache : ProjectSnapshotChangeTrigger
1210
{
1311
public abstract bool TryGetDocumentVersion(IDocumentSnapshot documentSnapshot, [NotNullWhen(true)] out int? version);
14-
15-
public abstract Task<int?> TryGetDocumentVersionAsync(IDocumentSnapshot documentSnapshot, CancellationToken cancellationToken);
16-
1712
public abstract void TrackDocumentVersion(IDocumentSnapshot documentSnapshot, int version);
1813
}

‎src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/HtmlFormatter.cs‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -35,8 +35,7 @@ public async Task<TextEdit[]> FormatAsync(
3535
throw new ArgumentNullException(nameof(context));
3636
}
3737

38-
var documentVersion = await _documentVersionCache.TryGetDocumentVersionAsync(context.OriginalSnapshot, cancellationToken).ConfigureAwait(false);
39-
if (documentVersion is null)
38+
if (!_documentVersionCache.TryGetDocumentVersion(context.OriginalSnapshot, out var documentVersion))
4039
{
4140
return Array.Empty<TextEdit>();
4241
}
@@ -63,8 +62,7 @@ public async Task<TextEdit[]> FormatOnTypeAsync(
6362
FormattingContext context,
6463
CancellationToken cancellationToken)
6564
{
66-
var documentVersion = await _documentVersionCache.TryGetDocumentVersionAsync(context.OriginalSnapshot, cancellationToken).ConfigureAwait(false);
67-
if (documentVersion == null)
65+
if (!_documentVersionCache.TryGetDocumentVersion(context.OriginalSnapshot, out var documentVersion))
6866
{
6967
return Array.Empty<TextEdit>();
7068
}

‎src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/DefaultDocumentContextFactoryTest.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ public class DefaultDocumentContextFactoryTest : LanguageServerTestBase
2424
public DefaultDocumentContextFactoryTest(ITestOutputHelper testOutput)
2525
: base(testOutput)
2626
{
27-
_documentVersionCache = new DefaultDocumentVersionCache(Dispatcher);
27+
_documentVersionCache = new DefaultDocumentVersionCache();
2828
}
2929

3030
[Fact]

‎src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/DefaultDocumentVersionCacheTest.cs‎

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ public DefaultDocumentVersionCacheTest(ITestOutputHelper testOutput)
2222
public void MarkAsLatestVersion_UntrackedDocument_Noops()
2323
{
2424
// Arrange
25-
var documentVersionCache = new DefaultDocumentVersionCache(LegacyDispatcher);
25+
var documentVersionCache = new DefaultDocumentVersionCache();
2626
var document = TestDocumentSnapshot.Create("C:/file.cshtml");
2727
documentVersionCache.TrackDocumentVersion(document, 123);
2828
var untrackedDocument = TestDocumentSnapshot.Create("C:/other.cshtml");
@@ -39,7 +39,7 @@ public void MarkAsLatestVersion_UntrackedDocument_Noops()
3939
public void MarkAsLatestVersion_KnownDocument_TracksNewDocumentAsLatest()
4040
{
4141
// Arrange
42-
var documentVersionCache = new DefaultDocumentVersionCache(LegacyDispatcher);
42+
var documentVersionCache = new DefaultDocumentVersionCache();
4343
var documentInitial = TestDocumentSnapshot.Create("C:/file.cshtml");
4444
documentVersionCache.TrackDocumentVersion(documentInitial, 123);
4545
var documentLatest = TestDocumentSnapshot.Create(documentInitial.FilePath);
@@ -56,7 +56,7 @@ public void MarkAsLatestVersion_KnownDocument_TracksNewDocumentAsLatest()
5656
public void TryGetLatestVersionFromPath_TrackedDocument_ReturnsTrue()
5757
{
5858
// Arrange
59-
var documentVersionCache = new DefaultDocumentVersionCache(LegacyDispatcher);
59+
var documentVersionCache = new DefaultDocumentVersionCache();
6060
var filePath = "C:/file.cshtml";
6161
var document1 = TestDocumentSnapshot.Create(filePath);
6262
var document2 = TestDocumentSnapshot.Create(filePath);
@@ -75,7 +75,7 @@ public void TryGetLatestVersionFromPath_TrackedDocument_ReturnsTrue()
7575
public void TryGetLatestVersionFromPath_UntrackedDocument_ReturnsFalse()
7676
{
7777
// Arrange
78-
var documentVersionCache = new DefaultDocumentVersionCache(LegacyDispatcher);
78+
var documentVersionCache = new DefaultDocumentVersionCache();
7979

8080
// Act
8181
var result = documentVersionCache.TryGetLatestVersionFromPath("C:/file.cshtml", out var version);
@@ -89,7 +89,7 @@ public void TryGetLatestVersionFromPath_UntrackedDocument_ReturnsFalse()
8989
public void ProjectSnapshotManager_Changed_DocumentRemoved_DoesNotEvictDocument()
9090
{
9191
// Arrange
92-
var documentVersionCache = new DefaultDocumentVersionCache(LegacyDispatcher);
92+
var documentVersionCache = new DefaultDocumentVersionCache();
9393
var projectSnapshotManager = TestProjectSnapshotManager.Create(ErrorReporter);
9494
projectSnapshotManager.AllowNotifyListeners = true;
9595
documentVersionCache.Initialize(projectSnapshotManager);
@@ -119,7 +119,7 @@ public void ProjectSnapshotManager_Changed_DocumentRemoved_DoesNotEvictDocument(
119119
public void ProjectSnapshotManager_Changed_OpenDocumentRemoved_DoesNotEvictDocument()
120120
{
121121
// Arrange
122-
var documentVersionCache = new DefaultDocumentVersionCache(LegacyDispatcher);
122+
var documentVersionCache = new DefaultDocumentVersionCache();
123123
var projectSnapshotManager = TestProjectSnapshotManager.Create(ErrorReporter);
124124
projectSnapshotManager.AllowNotifyListeners = true;
125125
documentVersionCache.Initialize(projectSnapshotManager);
@@ -151,7 +151,7 @@ public void ProjectSnapshotManager_Changed_OpenDocumentRemoved_DoesNotEvictDocum
151151
public void ProjectSnapshotManager_Changed_DocumentClosed_EvictsDocument()
152152
{
153153
// Arrange
154-
var documentVersionCache = new DefaultDocumentVersionCache(LegacyDispatcher);
154+
var documentVersionCache = new DefaultDocumentVersionCache();
155155
var projectSnapshotManager = TestProjectSnapshotManager.Create(ErrorReporter);
156156
projectSnapshotManager.AllowNotifyListeners = true;
157157
documentVersionCache.Initialize(projectSnapshotManager);
@@ -183,14 +183,14 @@ public void ProjectSnapshotManager_Changed_DocumentClosed_EvictsDocument()
183183
public void TrackDocumentVersion_AddsFirstEntry()
184184
{
185185
// Arrange
186-
var documentVersionCache = new DefaultDocumentVersionCache(LegacyDispatcher);
186+
var documentVersionCache = new DefaultDocumentVersionCache();
187187
var document = TestDocumentSnapshot.Create("C:/file.cshtml");
188188

189189
// Act
190190
documentVersionCache.TrackDocumentVersion(document, 1337);
191191

192192
// Assert
193-
var kvp = Assert.Single(documentVersionCache.DocumentLookup);
193+
var kvp = Assert.Single(documentVersionCache.DocumentLookup_NeedsLock);
194194
Assert.Equal(document.FilePath, kvp.Key);
195195
var entry = Assert.Single(kvp.Value);
196196
Assert.True(entry.Document.TryGetTarget(out var actualDocument));
@@ -202,7 +202,7 @@ public void TrackDocumentVersion_AddsFirstEntry()
202202
public void TrackDocumentVersion_EvictsOldEntries()
203203
{
204204
// Arrange
205-
var documentVersionCache = new DefaultDocumentVersionCache(LegacyDispatcher);
205+
var documentVersionCache = new DefaultDocumentVersionCache();
206206
var document = TestDocumentSnapshot.Create("C:/file.cshtml");
207207

208208
for (var i = 0; i < DefaultDocumentVersionCache.MaxDocumentTrackingCount; i++)
@@ -214,7 +214,7 @@ public void TrackDocumentVersion_EvictsOldEntries()
214214
documentVersionCache.TrackDocumentVersion(document, 1337);
215215

216216
// Assert
217-
var kvp = Assert.Single(documentVersionCache.DocumentLookup);
217+
var kvp = Assert.Single(documentVersionCache.DocumentLookup_NeedsLock);
218218
Assert.Equal(DefaultDocumentVersionCache.MaxDocumentTrackingCount, kvp.Value.Count);
219219
Assert.Equal(1337, kvp.Value.Last().Version);
220220
}
@@ -223,7 +223,7 @@ public void TrackDocumentVersion_EvictsOldEntries()
223223
public void TryGetDocumentVersion_UntrackedDocumentPath_ReturnsFalse()
224224
{
225225
// Arrange
226-
var documentVersionCache = new DefaultDocumentVersionCache(LegacyDispatcher);
226+
var documentVersionCache = new DefaultDocumentVersionCache();
227227
var document = TestDocumentSnapshot.Create("C:/file.cshtml");
228228

229229
// Act
@@ -238,7 +238,7 @@ public void TryGetDocumentVersion_UntrackedDocumentPath_ReturnsFalse()
238238
public void TryGetDocumentVersion_EvictedDocument_ReturnsFalse()
239239
{
240240
// Arrange
241-
var documentVersionCache = new DefaultDocumentVersionCache(LegacyDispatcher);
241+
var documentVersionCache = new DefaultDocumentVersionCache();
242242
var document = TestDocumentSnapshot.Create("C:/file.cshtml");
243243
var evictedDocument = TestDocumentSnapshot.Create(document.FilePath);
244244
documentVersionCache.TrackDocumentVersion(document, 1337);
@@ -255,7 +255,7 @@ public void TryGetDocumentVersion_EvictedDocument_ReturnsFalse()
255255
public void TryGetDocumentVersion_KnownDocument_ReturnsTrue()
256256
{
257257
// Arrange
258-
var documentVersionCache = new DefaultDocumentVersionCache(LegacyDispatcher);
258+
var documentVersionCache = new DefaultDocumentVersionCache();
259259
var document = TestDocumentSnapshot.Create("C:/file.cshtml");
260260
documentVersionCache.TrackDocumentVersion(document, 1337);
261261

‎src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/Formatting/TestRazorFormattingService.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ public static async Task<IRazorFormattingService> CreateWithFullSupportAsync(
2929
var mappingService = new RazorDocumentMappingService(TestLanguageServerFeatureOptions.Instance, new TestDocumentContextFactory(), loggerFactory);
3030

3131
var dispatcher = new LSPProjectSnapshotManagerDispatcher(loggerFactory);
32-
var versionCache = new DefaultDocumentVersionCache(dispatcher);
32+
var versionCache = new DefaultDocumentVersionCache();
3333
if (documentSnapshot is not null)
3434
{
3535
await dispatcher.RunOnDispatcherThreadAsync(() =>

0 commit comments

Comments
 (0)