Have the workspace cache the semantic model (after it is acquired) for the active document (if one exists). #73074

CyrusNajmabadi · 2024-04-17T22:24:54Z

We're seeing a lot of cases where both the local solution and OOP solution end up recreating the semantic model for the active document repeatedly. This is because we intentionally do not hold onto semantic models strongly, so tehy can drop away after use. this is valuable so that features that access lots of docs don't end up rooting a huge amount of memory. However, it has the downside that heavily used, and very important docs (like the user's actively edited document) aren't held onto, leading to features getting the semantic model, and then having it drop away prior to the next feature waking up and obtaining it. As features run on very different cadences, and GCs are happening very regularly, these models often drop away.

In prior versions of roslyn, we had complex caches with complex rules for populating and purging. These caches were not well understood, and often caused major problems for us (like rooting 10s of GB of memory, and only releasing them on some long time-based cadence). We ended up removing them, getting to a much better point where our memory usage would now peak far lower, and would stay relatively stable.

This PR adds back in a cache, but in a very targeted fashion. It only caches one semantic model around, only if the requested model is for the active document. This concept works in both the local host and in the OOP server, allowing both to keep that sole model around once asked until either edits come in, or hte active doc changes. The logic here is then quite simple, and the amount of memory used will be bounded to the whatever that single model needs.

--

Going to run speedometer tests to ensure this causes no issue (and we're hoping to see benefits here as well).

PR1: https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_build/results?buildId=9434937&view=results
PR2: https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_build/results?buildId=9434940&view=results

CyrusNajmabadi · 2024-04-17T22:25:16Z

src/EditorFeatures/CSharpTest/NavigateTo/NavigateToSearcherTests.cs

@@ -7,8 +7,8 @@
 using System.Linq;
 using System.Threading;
 using System.Threading.Tasks;
+using Microsoft.CodeAnalysis.Editor.Test;


helper type moved into common location.

Also, view with whitespcace off.

CyrusNajmabadi · 2024-04-17T22:25:28Z

src/EditorFeatures/CSharpTest/NavigateTo/NavigateToSearcherTests.cs

@@ -24,7 +24,7 @@ namespace Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.NavigateTo
    [Trait(Traits.Feature, Traits.Features.NavigateTo)]
    public sealed class NavigateToSearcherTests
    {
-        private static readonly TestComposition FirstActiveAndVisibleComposition = EditorTestCompositions.EditorFeatures.AddParts(typeof(FirstDocIsActiveAndVisibleDocumentTrackingService.Factory));
+        private static readonly TestComposition FirstActiveAndVisibleComposition = EditorTestCompositions.EditorFeatures.AddParts(typeof(FirstDocumentIsActiveAndVisibleDocumentTrackingService.Factory));


renamed to not have abbrs.

CyrusNajmabadi · 2024-04-17T22:25:52Z

src/EditorFeatures/Core/Remote/SolutionChecksumUpdater.cs

    /// <summary>
    /// Queue to push out text changes in a batched fashion when we hear about them.  Because these should be short
    /// operations (only syncing text changes) we don't cancel this when we enter the paused state.  We simply don't
    /// start queuing more requests into this until we become unpaused.
    /// </summary>
-    private readonly AsyncBatchingWorkQueue<(Document? oldDocument, Document? newDocument)> _textChangeQueue;
+    private readonly AsyncBatchingWorkQueue<(Document oldDocument, Document newDocument)> _textChangeQueue;


change this queue so that we don't bother adding to it if the doc is null, instead of filtering those out on the other end when we process th queue items.

src/EditorFeatures/Core/Remote/SolutionChecksumUpdater.cs

CyrusNajmabadi · 2024-04-17T22:26:50Z

src/EditorFeatures/Core/Remote/SolutionChecksumUpdater.cs

+
+        var solution = _workspace.CurrentSolution;
+        await client.TryInvokeAsync<IRemoteAssetSynchronizationService>(
+            (service, cancellationToken) => service.SynchronizeActiveDocumentAsync(activeDocument, cancellationToken),


note: this call doesn't use CurrentSolution, or pass over any solution data that needs to sync. It's solely about passing along the current active doc id.

CyrusNajmabadi · 2024-04-17T22:27:15Z

src/EditorFeatures/Core/Remote/SolutionChecksumUpdater.cs

@@ -194,15 +231,15 @@ async ValueTask SynchronizeTextChangesAsync(Document oldDocument, Document newDo
            }

            // get text changes
-            var textChanges = newText.GetTextChanges(oldText);
-            if (textChanges.Count == 0)
+            var textChanges = newText.GetTextChanges(oldText).AsImmutable();


changed the API to take an immutable array as well.

Would something like ToImmutableArrayOrEmpty prevent recreating the array?

That's what AsImmutable should do. If not, we need ot unify :)

Yes, I find it very confusing that there as so many ways to convert from IEnumerable to ImmutableArray, but I think most of them don't do the check for it already being an immutable array

This bottoms at at:

public static ImmutableArray<T> CreateRange<T>(IEnumerable<T> items) { Requires.NotNull(items, "items"); if (items is IImmutableArray immutableArray) { Array array = immutableArray.Array; if (array == null) { throw new InvalidOperationException(System.SR.InvalidOperationOnDefaultArray); } return new ImmutableArray<T>((T[])array); }

So it should allow moving direct to a new immutablearray backed by the same array.

ultimately, it's the underlying array that matters. as boxing and unboxing the immutable array doesn't actually give the same IA again for obvious reasons.

Oh, I didn't track it into CreateRange, I didn't think ImmutableArray had that built in. Nice!

CyrusNajmabadi · 2024-04-17T22:27:31Z

src/EditorFeatures/TestUtilities/DocumentTracking/TestDocumentTrackingService.cs

+namespace Microsoft.CodeAnalysis.Editor.Test;
+
+[ExportWorkspaceService(typeof(IDocumentTrackingService), ServiceLayer.Test), Shared, PartNotDiscoverable]
+internal sealed class TestDocumentTrackingService : IDocumentTrackingService


this is a move. t hsi type already existed.

CyrusNajmabadi · 2024-04-17T22:28:19Z

src/Features/Core/Portable/SolutionCrawler/IDocumentTrackingService.cs

-// The .NET Foundation licenses this file to you under the MIT license.
-// See the LICENSE file in the project root for more information.
-
-using System;


this was moved to Workspace level, but was cleaned up so much, it shows as a remove+add.

CyrusNajmabadi · 2024-04-17T22:28:25Z

src/Features/Core/Portable/SolutionCrawler/IDocumentTrackingService.cs

-    /// <summary>
-    /// Raised when a text buffer that's not part of a workspace is changed.
-    /// </summary>
-    event EventHandler<EventArgs> NonRoslynBufferTextChanged;


never used. removed.

CyrusNajmabadi · 2024-04-17T22:28:30Z

src/Features/Core/Portable/SolutionCrawler/IDocumentTrackingService.cs

-
-internal interface IDocumentTrackingService : IWorkspaceService
-{
-    bool SupportsDocumentTracking { get; }


not needed. removed.

CyrusNajmabadi · 2024-04-17T22:29:01Z

src/VisualStudio/Core/Test.Next/Services/SolutionServiceTests.cs

-            using var workspace = TestWorkspace.CreateCSharp(code);
-            using var remoteWorkspace = CreateRemoteWorkspace();
+    [Fact]
+    public void TestNoActiveDocumentSemanticModelNotCached()


a bunch of tests to validate the strong vs weak holding of semantic models based on active-doc, along with tests that oop respects that.

CyrusNajmabadi · 2024-04-17T22:29:21Z

src/Workspaces/Core/Portable/Workspace/DocumentTracking/IDocumentTrackingService.cs

+/// return altogether.  Hosts are free to implement this service to do nothing at all, always returning empty/default
+/// values for the members within.  As per the above, this should never affect correctness, but it may impede a
+/// feature's ability to provide results in as timely a manner as possible for a client.
+/// </summary>


new docs for clarity.

CyrusNajmabadi · 2024-04-17T22:32:00Z

src/Workspaces/Remote/Core/IRemoteAssetSynchronizationService.cs

+    /// hold on weakly.  Given how much work happens on the active document, this can help avoid the remote side from
+    /// continually creating and then throwing away that data.
+    /// </summary>
+    ValueTask SynchronizeActiveDocumentAsync(DocumentId? documentId, CancellationToken cancellationToken);


CyrusNajmabadi · 2024-04-17T22:32:21Z

...kspaces/Remote/ServiceHub/Services/AssetSynchronization/RemoteAssetSynchronizationService.cs

+    {
+        var documentTrackingService = GetWorkspace().Services.GetRequiredService<IDocumentTrackingService>() as RemoteDocumentTrackingService;
+        documentTrackingService?.SetActiveDocument(documentId);
+        return ValueTaskFactory.CompletedTask;


impl on OOP side is trivial. we literally just set the value in the oop doc tracking service.

src/EditorFeatures/TestUtilities/DocumentTracking/TestDocumentTrackingService.cs

ToddGrun · 2024-04-18T00:34:01Z

src/Workspaces/Remote/ServiceHub/Services/ServiceHubDocumentTrackingService.cs

+[ExportWorkspaceService(typeof(IDocumentTrackingService), ServiceLayer.Host), Shared]
+[method: ImportingConstructor]
+[method: Obsolete(MefConstruction.ImportingConstructorMessage, error: true)]
+internal sealed class RemoteDocumentTrackingService() : IDocumentTrackingService


RemoteDocumentTrackingService

nit: rename file

if i do all this work myself, what work will be left for the poor AIs?

CyrusNajmabadi · 2024-04-18T01:24:01Z

@ToddGrun what features were you seeing that were doing nullable analysis? I want to check if those are candidates for opting out of that analysis.

ToddGrun · 2024-04-18T04:45:36Z

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

-            SemanticModel? semanticModel;
-            if (disableNullableAnalysis)
+        var semanticModel = await GetSemanticModelWorkerAsync().ConfigureAwait(false);
+        this.Project.Solution.OnSemanticModelObtained(this.Id, semanticModel);


OnSemanticModelObtained

No need to indicate in the cache the value of disableNullableAnalysis?

Never mind, it's never queried from that cache, strictly used as a mechanism to keep it alive.

(although it looks like this won't be able to keep alive both the semantic models for the active document)

Finally looked in the SemanticModelCaching code, and it's able to detect that and keeps both. Nice!

Haha. A wild journey in four steps. :-)

ToddGrun

CyrusNajmabadi · 2024-04-18T04:58:59Z

Will check the perf results tomorrow morning and move forward if things are good.

…adi/roslyn into semanticModelCaching

CyrusNajmabadi · 2024-04-18T17:22:01Z

No regressions. Merging in.

CyrusNajmabadi · 2024-04-24T21:16:55Z

@jasonmalinowski For review when you get back.

CyrusNajmabadi added 13 commits April 17, 2024 12:33

Add initial caching logic

e2059ce

Add remote api

a84d7a1

Move where cache is

9921a01

Simplify and docs

0bc3830

Add support for remoting the active file

9c5cc52

tests and simplification

f23e03e

Fixes

adb0f2e

Fix test

49c30fa

Fix test

27aab15

Add tesT

e49c5b6

Fix test

58c82d8

Add test

837bc32

Fix tests

5b1e9f4

CyrusNajmabadi requested a review from a team as a code owner April 17, 2024 22:24

dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Apr 17, 2024

CyrusNajmabadi commented Apr 17, 2024

View reviewed changes

src/EditorFeatures/Core/Remote/SolutionChecksumUpdater.cs Outdated Show resolved Hide resolved

Apply suggestions from code review

f468e96

CyrusNajmabadi commented Apr 17, 2024

View reviewed changes

Primary constructor

302392f

CyrusNajmabadi commented Apr 17, 2024

View reviewed changes

CyrusNajmabadi requested a review from ToddGrun April 17, 2024 22:32

CyrusNajmabadi added 2 commits April 17, 2024 16:36

Merge branch 'main' into semanticModelCaching

51d353c

Remove

6f1271a

ToddGrun reviewed Apr 17, 2024

View reviewed changes

src/EditorFeatures/TestUtilities/DocumentTracking/TestDocumentTrackingService.cs Outdated Show resolved Hide resolved

CyrusNajmabadi added 3 commits April 17, 2024 17:03

Remove

a8e7e87

Remove

08a45dd

Simplify

118a257

ToddGrun reviewed Apr 18, 2024

View reviewed changes

CyrusNajmabadi added 2 commits April 17, 2024 17:39

rename file

b99374b

simplify

83bfccc

ToddGrun reviewed Apr 18, 2024

View reviewed changes

ToddGrun approved these changes Apr 18, 2024

View reviewed changes

CyrusNajmabadi added 5 commits April 17, 2024 23:44

Merge branch 'main' into semanticModelCaching

7a6613b

remove unused event from interface that was never subscribed to

8f2659a

Merge branch 'removeUnusedEvent' into semanticModelCaching

3804d10

Merge branch 'semanticModelCaching' of https://github.com/CyrusNajmab…

103e38b

…adi/roslyn into semanticModelCaching

fix

1037a13

CyrusNajmabadi merged commit afa1eb6 into dotnet:main Apr 18, 2024
25 checks passed

CyrusNajmabadi deleted the semanticModelCaching branch April 18, 2024 17:22

dotnet-policy-service bot added this to the Next milestone Apr 18, 2024

This was referenced Apr 20, 2024

[Automated] PRs inserted in VS build main-34819.162 #73114

Closed

[Automated] PRs inserted in VS build feature.debugger.main-34823.102 #73204

Closed

dibarbet modified the milestones: Next, 17.11 P1 Apr 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Have the workspace cache the semantic model (after it is acquired) for the active document (if one exists). #73074

Have the workspace cache the semantic model (after it is acquired) for the active document (if one exists). #73074

CyrusNajmabadi commented Apr 17, 2024 •

edited

CyrusNajmabadi Apr 17, 2024

CyrusNajmabadi Apr 17, 2024

CyrusNajmabadi Apr 17, 2024

CyrusNajmabadi Apr 17, 2024

CyrusNajmabadi Apr 17, 2024

ToddGrun Apr 17, 2024

CyrusNajmabadi Apr 17, 2024

ToddGrun Apr 17, 2024

CyrusNajmabadi Apr 18, 2024

CyrusNajmabadi Apr 18, 2024 •

edited

ToddGrun Apr 18, 2024

CyrusNajmabadi Apr 17, 2024

CyrusNajmabadi Apr 17, 2024

CyrusNajmabadi Apr 17, 2024

CyrusNajmabadi Apr 17, 2024

CyrusNajmabadi Apr 17, 2024

CyrusNajmabadi Apr 17, 2024

CyrusNajmabadi Apr 17, 2024

CyrusNajmabadi Apr 17, 2024

ToddGrun Apr 18, 2024

CyrusNajmabadi Apr 18, 2024

CyrusNajmabadi commented Apr 18, 2024

ToddGrun Apr 18, 2024

ToddGrun Apr 18, 2024

ToddGrun Apr 18, 2024

ToddGrun Apr 18, 2024

CyrusNajmabadi Apr 18, 2024

ToddGrun left a comment

CyrusNajmabadi commented Apr 18, 2024

CyrusNajmabadi commented Apr 18, 2024

CyrusNajmabadi commented Apr 24, 2024

Have the workspace cache the semantic model (after it is acquired) for the active document (if one exists). #73074

Have the workspace cache the semantic model (after it is acquired) for the active document (if one exists). #73074

Conversation

CyrusNajmabadi commented Apr 17, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi Apr 18, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Apr 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ToddGrun left a comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Apr 18, 2024

CyrusNajmabadi commented Apr 18, 2024

CyrusNajmabadi commented Apr 24, 2024

CyrusNajmabadi commented Apr 17, 2024 •

edited

CyrusNajmabadi Apr 18, 2024 •

edited