Additional work to release unused PreviewWorkspace memory #68054

sharwell · 2023-05-02T14:05:07Z

Local heap dump shows 6.8GB memory retained by unused PreviewWorkspace instances. The GC root is linked through event handlers on GlobalOptionService. Since this has been a problem in the past (#67142), I took a multi-approach solution:

More aggressively track and dispose PreviewWorkspace instances. They are stored in ReferenceCountedDisposable<> from the point of initial creation, with ownership transferred by a pattern of duplication of a reference followed by releasing the original. Outside of exceptions thrown by the constructor(s), instances should now be disposed in the event of any exception.
~~Force the use of a weak event handler pattern for GlobalOptionService. In the event a PreviewWorkspace fails to be released, it will no longer be GC rooted through this event.~~ Moved to Use a weak event handler pattern for GlobalOptionService #68062

sharwell · 2023-05-02T14:08:19Z

src/EditorFeatures/Core.Wpf/QuickInfo/LazyToolTip.cs

+                    // Avoid propagating the exception within a UI cleanup layer
+                }
+            }
+
            private void OnToolTipOpening(object sender, ToolTipEventArgs e)


📝 If an exception was no the original source of a PreviewWorkspace leaking, this is the second most likely case. This type assumes that there is a strict matching of calls to OnToolTipOpening and OnToolTipClosing. If there is ever a case where OnToolTipOpening is called more times than OnToolTipClosing, the additional calls would have leaked a PreviewWorkspace instance.

The updated code partially relaxes this assumption. If a stray workspace is detected in the call to OnToolTipOpening, it will be cleaned up at that time. We also add a new handler for ITableEntriesSnapshot.Dispose() (the owner of these entries) to force any remaining items to be disposed when the snapshot is disposed.

CyrusNajmabadi · 2023-05-02T15:15:18Z

src/EditorFeatures/Core.Wpf/QuickInfo/ContentControlService.cs


-            return new DisposableToolTip(toolTip, workspace);
+            return DisposableToolTip.Create(toolTip, workspace);


visually, this just looks weird. It looks like you're returning something that is holding onto something that is explicitly being disposed. needs comments to explain why this is ok.

This is the correct pattern for correctly transferring ownership of a ReferenceCountedDisposable<T>. An RCD passed by value is only assured to be alive until the end of the call. The callee must obtain its own reference if it needs to store the value to a field. The behavior of the caller depends on the source of the RCD: if the caller created the RCD (as is the case here) it must release it before returning. If the caller received the RCD via an argument, it doesn't own that instance and can just return.

This is the correct pattern for correctly transferring ownership of a ReferenceCountedDisposable

literally nothing in the code states taht it's an RCD though. It just looks like a bog standard captured disposable. RCD is fine here because the using is only affecting the ref-count. And, when that is clear, is totally ok. But in cases like this, there's no indication of ref-counting at all, so it immediatley jumps out as looking problematic.

➡️ Renamed Create methods that return RCD to CreateReferenceCounted

CyrusNajmabadi · 2023-05-02T15:16:30Z

src/EditorFeatures/Core.Wpf/QuickInfo/LazyToolTip.cs

@@ -18,17 +19,17 @@ internal partial class ContentControlService
        /// <summary>
        /// Class which allows us to provide a delay-created tooltip for our reference entries.
        /// </summary>
-        private class LazyToolTip : ForegroundThreadAffinitizedObject
+        private class LazyToolTip : ForegroundThreadAffinitizedObject, IDisposable


perhaps LazyDisposableToolTip?

➡️ Keeping the current name because 1) it's impossible to create an instance of this without realizing it's disposable (the Create method returns RCD) and 2) there is no corresponding non-disposable type.

CyrusNajmabadi · 2023-05-02T15:17:28Z

src/EditorFeatures/Core.Wpf/QuickInfo/LazyToolTip.cs

@@ -58,8 +75,11 @@ private void OnToolTipOpening(object sender, ToolTipEventArgs e)
                    Debug.Assert(_element.ToolTip == this);
                    Debug.Assert(_disposableToolTip == null);

+                    // We don't expect _disposableToolTip to be non-null here, but we still make sure it's not leaking
+                    _disposableToolTip?.Dispose();


what about just not doing work in that case? if hte tooltip was already created, we could just immediately exit?

➡️ Updated

CyrusNajmabadi · 2023-05-02T15:20:03Z

src/EditorFeatures/Core/Preview/AbstractPreviewFactoryService.cs

-            var rightWorkspace = new PreviewWorkspace(document.Project.Solution);
-            rightWorkspace.OpenDocument(document.Id, newBuffer.AsTextContainer());
+            using var rightWorkspace = PreviewWorkspace.Create(document.Project.Solution);
+            rightWorkspace.Target.OpenDocument(document.Id, newBuffer.AsTextContainer());

 #pragma warning disable CA2007 // Consider calling ConfigureAwait on the awaited task (containing method uses JTF)
            return await CreateAddedDocumentPreviewViewCoreAsync(newBuffer, rightWorkspace, document, zoomLevel, cancellationToken);
 #pragma warning restore CA2007 // Consider calling ConfigureAwait on the awaited task


same points about returning something that captures a local which appears to be disposed when we get to end of scope.

➡️ Covered by https://github.com/dotnet/roslyn/pull/68054/files#r1182949296

CyrusNajmabadi · 2023-05-02T15:21:54Z

src/EditorFeatures/Core/Preview/AbstractPreviewFactoryService.cs

            }

-            return new DifferenceViewerPreview(diffViewer);
+            return differenceViewer.TryAddReference()!;


for my own edification, this should always succeed, yes? (i presume that's also why we have the !). I feel ike it would be nice on these RefCountedDisposables to have a method (or extension) that adds and asserts/throws if that somehow failed (Similar to GetRequiredDocument vs GetDocument).

⏱️ Will add a AddReference() method

➡️ Implemented in b228912

CyrusNajmabadi · 2023-05-02T15:22:42Z

src/EditorFeatures/Core/Preview/DifferenceViewerPreview.cs

        {
            Contract.ThrowIfNull(viewer);
            _viewer = viewer;
+            LeftWorkspace = leftWorkspace?.TryAddReference();
+            RightWorkspace = rightWorkspace?.TryAddReference();


why ?.? You never are passed null instances in right?

Also, is this another location where we could use the asserting AddRef methods?

also, i'd get rid of nullable disable in this file. it seems like it would catch a few things.

Multiple locations pass in null

➡️ These can be null. See 6a08a73

CyrusNajmabadi · 2023-05-02T15:23:23Z

src/EditorFeatures/Core/Preview/DifferenceViewerPreview.cs

@@ -33,6 +48,9 @@ public void Dispose()
            }

            _viewer = null;
+
+            LeftWorkspace?.Dispose();
+            RightWorkspace?.Dispose();


no need for ?. afaict.

➡️ Covered by 6a08a73

CyrusNajmabadi · 2023-05-02T15:23:56Z

src/EditorFeatures/Test/Options/GlobalOptionsTests.cs

+            => throw new NotImplementedException();
+
+        public void RemoveOptionChangedHandler(object target, EventHandler<OptionChangedEventArgs> handler)
+            => throw new NotImplementedException();


would it be possible to make two PRs? one for the refcounted stuff, and one for the weak event handlerS?

➡️ Extracted #68062

CyrusNajmabadi · 2023-05-02T15:25:18Z

src/VisualStudio/Core/Def/FindReferences/Entries/DocumentSpanEntry.cs

                        CreateDisposableToolTip(_excerptResult.Document, _excerptResult.Span));

+                    disposeCallback += () =>


wow. that this is being += is subtle. Can you doc htat? I was def confused a bit. :)

dumb question. i see what you're doing here, creating a side-table of disposables that go away when the snapshot goes away. Is it not possible though to just reutrn FrameworkElements that do this cleanup once they themselves are disposed?

Feels like it would be more natural to have hte lifetimes of these disposables track with that, versus in a side table.

📝 FrameworkElement is not disposable, and WPF does not notify on final release

➡️ Documented the use of the += operator

CyrusNajmabadi · 2023-05-02T15:25:34Z

src/VisualStudio/Core/Def/FindReferences/Entries/DocumentSpanEntry.cs

+                    {
+                        try
+                        {
+                            disposable?.Dispose();


why ?. here?

➡️ Updated to .

CyrusNajmabadi · 2023-05-02T15:26:12Z

src/VisualStudio/Core/Def/FindReferences/TableEntriesSnapshot.cs

@@ -31,6 +36,23 @@ public TableEntriesSnapshot(ImmutableList<Entry> entries, int versionNumber)

            public override int Count => _entries.Count;

+            public override void Dispose()
+            {
+                foreach (var callback in _disposeCallbacks)


nit: consider threading assert.

➡️ Updated to remove affinity further down stream

CyrusNajmabadi · 2023-05-02T15:26:27Z

src/VisualStudio/Core/Def/FindReferences/TableEntriesSnapshot.cs

@@ -20,6 +24,7 @@ private class TableEntriesSnapshot : WpfTableEntriesSnapshotBase
            private readonly int _versionNumber;

            private readonly ImmutableList<Entry> _entries;
+            private readonly List<Action> _disposeCallbacks = new();


➡️ Small amount of docs added

CyrusNajmabadi · 2023-05-02T15:29:02Z

src/VisualStudio/Core/Def/Preview/PreviewEngine.cs

        {
-            _updater?.CloseWorkspace();
+            _updater?.Dispose();


➡️ _updater is lazily created

CyrusNajmabadi · 2023-05-02T15:29:08Z

src/VisualStudio/Core/Def/Preview/PreviewEngine.cs

@@ -218,15 +229,27 @@ public void UpdatePreview(DocumentId documentId, SpanChange spanSource)
            var document = updatedSolution.GetTextDocument(documentId);
            if (document != null)
            {
-                _updater.UpdateView(document, spanSource);
+                using var updater = _updater?.TryAddReference();


➡️ Switched to Contract.ThrowIfNull at this location

CyrusNajmabadi · 2023-05-02T15:29:30Z

src/VisualStudio/Core/Def/Preview/PreviewEngine.cs

@@ -218,15 +229,27 @@ public void UpdatePreview(DocumentId documentId, SpanChange spanSource)
            var document = updatedSolution.GetTextDocument(documentId);
            if (document != null)
            {
-                _updater.UpdateView(document, spanSource);
+                using var updater = _updater?.TryAddReference();
+                RoslynDebug.AssertNotNull(updater);


soundsl ike a place for RequireAddReference :)

📝 It would just be AddReference()

➡️ Switched to AddReference at this location

CyrusNajmabadi · 2023-05-02T15:30:50Z

src/VisualStudio/Core/Def/Preview/PreviewUpdater.cs

    {
-        private PreviewDialogWorkspace? _previewWorkspace;
+        private ReferenceCountedDisposable<PreviewDialogWorkspace>? _previewWorkspace;


wow... this is lazily created and assigned.... i hate this :D (same problem before your code).

➡️ Planning to leave this one as-is since it's the same as before

CyrusNajmabadi · 2023-05-02T15:32:00Z

src/VisualStudio/Core/Def/Preview/PreviewUpdater.cs

+            {
+                var newWorkspace = PreviewDialogWorkspace.Create(document.Project.Solution);
+                if (Interlocked.CompareExchange(ref _previewWorkspace, newWorkspace, null) is not null)
+                    newWorkspace.Dispose();
            }


i'm def confused about why this is all written this way. i would presume the PreviewUpdater is for something like the options page. and deferring the whole workspace stuff just seems totally unnecessary (And makes things so contorted and hard to follow).

➡️ Planning to leave this one as-is since it's the same as before

CyrusNajmabadi · 2023-05-02T15:32:10Z

src/VisualStudio/Core/Def/Preview/PreviewUpdater.cs

+            RoslynDebug.AssertNotNull(_previewWorkspace);
+
+            using var previewWorkspace2 = _previewWorkspace.TryAddReference();
+            RoslynDebug.AssertNotNull(previewWorkspace2);


RequireAddReference :)

➡️ Part of b228912

src/Workspaces/Core/Portable/Options/GlobalOptionService.cs

src/Workspaces/Core/Portable/Options/IGlobalOptionService.cs

CyrusNajmabadi · 2023-05-02T15:36:07Z

overall LGTM. a few strong recommendations:

break out event handlers from preview workspace.
more aggressive asserts on refcounteddisposable.
some light docs in teh code would be good. esp. around passing ownership around.
nullable enable some files
consider changing some lifetimes of some objects to be clearer.

not signing off yet as i would like to do at least more pass after some cleanup has happened. Thanks.

Use ReferenceCountedDisposable to ensure PreviewWorkspace is disposed

1bc63b6

sharwell requested review from a team as code owners May 2, 2023 14:05

dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels May 2, 2023

sharwell commented May 2, 2023

View reviewed changes

CyrusNajmabadi reviewed May 2, 2023

View reviewed changes

runfoapp bot mentioned this pull request May 2, 2023

testtypeinheritance jaredpar/runfo#125

Open

CyrusNajmabadi reviewed May 2, 2023

View reviewed changes

src/Workspaces/Core/Portable/Options/GlobalOptionService.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed May 2, 2023

View reviewed changes

src/Workspaces/Core/Portable/Options/IGlobalOptionService.cs Outdated Show resolved Hide resolved

sharwell mentioned this pull request May 2, 2023

Use a weak event handler pattern for GlobalOptionService #68062

Merged

Propagate disposable preview objects forward through a cast to object

7d17da2

sharwell added 2 commits May 2, 2023 14:33

Implement AddReference extension for ReferenceCountedDisposable<T>

b228912

Enable nullable reference types for DifferenceViewerPreview

6a08a73

sharwell force-pushed the tagger-changed branch from b58779e to 6a08a73 Compare May 2, 2023 19:35

A few simplifications and documentation

556ec70

sharwell force-pushed the tagger-changed branch from 430003b to 556ec70 Compare May 2, 2023 20:00

build-analysis bot mentioned this pull request May 2, 2023

Ubuntu 1ES hosted pool images hitting No space left on device dotnet/arcade#13036

Closed

5 tasks

sharwell added 2 commits May 15, 2023 21:54

Rename methods to indicate return types are reference counted

4f4ff87

Clarify use of uncommon delegate combination pattern

e5bb750

dotnet deleted a comment from azure-pipelines bot May 16, 2023

Merge remote-tracking branch 'dotnet/main' into tagger-changed

ea3e08f


		return new DisposableToolTip(toolTip, workspace);
		return DisposableToolTip.Create(toolTip, workspace);

		CreateDisposableToolTip(_excerptResult.Document, _excerptResult.Span));

		disposeCallback += () =>

Additional work to release unused PreviewWorkspace memory #68054

Are you sure you want to change the base?

Additional work to release unused PreviewWorkspace memory #68054

Conversation

sharwell commented May 2, 2023 • edited Loading

sharwell May 2, 2023 • edited Loading

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

sharwell May 2, 2023 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi May 2, 2023 • edited Loading

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

sharwell May 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharwell May 2, 2023 • edited Loading

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

sharwell May 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharwell May 2, 2023 • edited Loading

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 May 2, 2023

sharwell commented May 2, 2023 •

edited

Loading

sharwell May 2, 2023 •

edited

Loading

sharwell May 2, 2023 •

edited

Loading

CyrusNajmabadi May 2, 2023 •

edited

Loading

sharwell May 2, 2023 •

edited

Loading

sharwell May 2, 2023 •

edited

Loading

sharwell May 2, 2023 •

edited

Loading

sharwell May 2, 2023 •

edited

Loading