From c276b0af628621ed6b7ce3892637791997472a6d Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Sun, 7 Apr 2024 14:07:36 -0700 Subject: [PATCH 1/4] Switch to a batching work queue for saving sourcetexts --- ...coverableTextAndVersion.RecoverableText.cs | 42 +++++++++---------- .../Solution/RecoverableTextAndVersion.cs | 7 +++- 2 files changed, 26 insertions(+), 23 deletions(-) diff --git a/src/Workspaces/Core/Portable/Workspace/Solution/RecoverableTextAndVersion.RecoverableText.cs b/src/Workspaces/Core/Portable/Workspace/Solution/RecoverableTextAndVersion.RecoverableText.cs index 1e996fc9eb57b..2eeb49242ef6d 100644 --- a/src/Workspaces/Core/Portable/Workspace/Solution/RecoverableTextAndVersion.RecoverableText.cs +++ b/src/Workspaces/Core/Portable/Workspace/Solution/RecoverableTextAndVersion.RecoverableText.cs @@ -6,6 +6,7 @@ using System.Diagnostics.CodeAnalysis; using System.Threading; using System.Threading.Tasks; +using Microsoft.CodeAnalysis.Collections; using Microsoft.CodeAnalysis.Shared.TestHooks; using Microsoft.CodeAnalysis.Text; using Roslyn.Utilities; @@ -23,8 +24,16 @@ internal sealed partial class RecoverableTextAndVersion private sealed partial class RecoverableText { // enforce saving in a queue so save's don't overload the thread pool. - private static Task s_latestTask = Task.CompletedTask; - private static readonly NonReentrantLock s_taskGuard = new(); + private static AsyncBatchingWorkQueue<(RecoverableText recoverableText, SourceText sourceText)> s_saveQueue; + + static RecoverableText() + { + s_saveQueue = new AsyncBatchingWorkQueue<(RecoverableText recoverableText, SourceText sourceText)>( + TimeSpan.Zero, + SaveAllAsync, + AsynchronousOperationListenerProvider.NullListener, + CancellationToken.None); + } /// /// Lazily created. Access via the property. @@ -136,27 +145,16 @@ private void UpdateWeakReferenceAndEnqueueSaveTask_NoLock(SourceText instance) if (!_saved) { _saved = true; - using (s_taskGuard.DisposableWait()) - { - // force all save tasks to be in sequence so we don't hog all the threads. - s_latestTask = s_latestTask.SafeContinueWithFromAsync(async _ => - { - // Now defer to our subclass to actually save the instance to secondary storage. - await SaveAsync(instance, CancellationToken.None).ConfigureAwait(false); - - // Only set _initialValue to null if the saveTask completed successfully. If the save did not complete, - // we want to keep it around to service future requests. Once we do clear out this value, then all - // future request will either retrieve the value from the weak reference (if anyone else is holding onto - // it), or will recover from underlying storage. - _initialValue = null; - }, - CancellationToken.None, - // Ensure we run continuations asynchronously so that we don't start running the continuation while - // holding s_taskGuard. - TaskContinuationOptions.RunContinuationsAsynchronously, - TaskScheduler.Default); - } + + s_saveQueue.AddWork((this, instance)); } } + + private static async ValueTask SaveAllAsync( + ImmutableSegmentedList<(RecoverableText recoverableText, SourceText sourceText)> list, CancellationToken cancellationToken) + { + foreach (var (recoverableText, sourceText) in list) + await recoverableText.SaveAsync(sourceText, cancellationToken).ConfigureAwait(false); + } } } diff --git a/src/Workspaces/Core/Portable/Workspace/Solution/RecoverableTextAndVersion.cs b/src/Workspaces/Core/Portable/Workspace/Solution/RecoverableTextAndVersion.cs index 392acdfbfec8c..7cc1f3f1ba2b8 100644 --- a/src/Workspaces/Core/Portable/Workspace/Solution/RecoverableTextAndVersion.cs +++ b/src/Workspaces/Core/Portable/Workspace/Solution/RecoverableTextAndVersion.cs @@ -17,7 +17,6 @@ namespace Microsoft.CodeAnalysis; /// internal sealed partial class RecoverableTextAndVersion(ITextAndVersionSource initialSource, SolutionServices services) : ITextAndVersionSource { - // Starts as ITextAndVersionSource and is replaced with RecoverableText when the TextAndVersion value is requested. // At that point the initial source is no longer referenced and can be garbage collected. private object _initialSourceOrRecoverableText = initialSource; @@ -192,6 +191,12 @@ private async Task SaveAsync(SourceText text, CancellationToken cancellationToke // make sure write is done before setting _storage field Interlocked.CompareExchange(ref _storage, storage, null); + + // Only set _initialValue to null if the saveTask completed successfully. If the save did not complete, we + // want to keep it around to service future requests. Once we do clear out this value, then all future + // request will either retrieve the value from the weak reference (if anyone else is holding onto it), or + // will recover from underlying storage. + _initialValue = null; } public bool TryGetTextVersion(LoadTextOptions options, out VersionStamp version) From c65fa4451959895e4381fff94793d22880bbb46d Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Sun, 7 Apr 2024 14:08:54 -0700 Subject: [PATCH 2/4] Change comment --- .../Workspace/Solution/RecoverableTextAndVersion.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Workspaces/Core/Portable/Workspace/Solution/RecoverableTextAndVersion.cs b/src/Workspaces/Core/Portable/Workspace/Solution/RecoverableTextAndVersion.cs index 7cc1f3f1ba2b8..bcc498768b875 100644 --- a/src/Workspaces/Core/Portable/Workspace/Solution/RecoverableTextAndVersion.cs +++ b/src/Workspaces/Core/Portable/Workspace/Solution/RecoverableTextAndVersion.cs @@ -192,10 +192,10 @@ private async Task SaveAsync(SourceText text, CancellationToken cancellationToke // make sure write is done before setting _storage field Interlocked.CompareExchange(ref _storage, storage, null); - // Only set _initialValue to null if the saveTask completed successfully. If the save did not complete, we - // want to keep it around to service future requests. Once we do clear out this value, then all future - // request will either retrieve the value from the weak reference (if anyone else is holding onto it), or - // will recover from underlying storage. + // Only set _initialValue to null once writing to the storage service completes fully. If the save did not + // complete, we want to keep it around to service future requests. Once we do clear out this value, then + // all future request will either retrieve the value from the weak reference (if anyone else is holding onto + // it), or will recover from underlying storage. _initialValue = null; } From d1e723be2ac3483970c705791cb1869b18158763 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Sun, 7 Apr 2024 14:34:29 -0700 Subject: [PATCH 3/4] lint --- .../Solution/RecoverableTextAndVersion.RecoverableText.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Workspaces/Core/Portable/Workspace/Solution/RecoverableTextAndVersion.RecoverableText.cs b/src/Workspaces/Core/Portable/Workspace/Solution/RecoverableTextAndVersion.RecoverableText.cs index 2eeb49242ef6d..035e0b00d5416 100644 --- a/src/Workspaces/Core/Portable/Workspace/Solution/RecoverableTextAndVersion.RecoverableText.cs +++ b/src/Workspaces/Core/Portable/Workspace/Solution/RecoverableTextAndVersion.RecoverableText.cs @@ -24,7 +24,7 @@ internal sealed partial class RecoverableTextAndVersion private sealed partial class RecoverableText { // enforce saving in a queue so save's don't overload the thread pool. - private static AsyncBatchingWorkQueue<(RecoverableText recoverableText, SourceText sourceText)> s_saveQueue; + private static readonly AsyncBatchingWorkQueue<(RecoverableText recoverableText, SourceText sourceText)> s_saveQueue; static RecoverableText() { From 356b54c6bde0cb44b031d9139c20f9fe75cbf5bf Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Sun, 7 Apr 2024 14:38:07 -0700 Subject: [PATCH 4/4] Simplify --- .../RecoverableTextAndVersion.RecoverableText.cs | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/src/Workspaces/Core/Portable/Workspace/Solution/RecoverableTextAndVersion.RecoverableText.cs b/src/Workspaces/Core/Portable/Workspace/Solution/RecoverableTextAndVersion.RecoverableText.cs index 035e0b00d5416..f8ded8d24ae96 100644 --- a/src/Workspaces/Core/Portable/Workspace/Solution/RecoverableTextAndVersion.RecoverableText.cs +++ b/src/Workspaces/Core/Portable/Workspace/Solution/RecoverableTextAndVersion.RecoverableText.cs @@ -24,16 +24,11 @@ internal sealed partial class RecoverableTextAndVersion private sealed partial class RecoverableText { // enforce saving in a queue so save's don't overload the thread pool. - private static readonly AsyncBatchingWorkQueue<(RecoverableText recoverableText, SourceText sourceText)> s_saveQueue; - - static RecoverableText() - { - s_saveQueue = new AsyncBatchingWorkQueue<(RecoverableText recoverableText, SourceText sourceText)>( - TimeSpan.Zero, + private static readonly AsyncBatchingWorkQueue<(RecoverableText recoverableText, SourceText sourceText)> s_saveQueue = + new(TimeSpan.Zero, SaveAllAsync, AsynchronousOperationListenerProvider.NullListener, CancellationToken.None); - } /// /// Lazily created. Access via the property.