Skip to content

Make GlobalOptionService initialization synchronous. #77823

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 27, 2025
Merged

Make GlobalOptionService initialization synchronous. #77823

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
97408bb
Make GlobalOptionService initialization synchronous.
ToddGrun Mar 25, 2025
5eb4ec8
fix indent
ToddGrun Mar 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 19 additions & 27 deletions src/VisualStudio/Core/Def/Options/VisualStudioOptionPersisterProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,7 @@
using System.Collections.Generic;
using System.Collections.Immutable;
using System.ComponentModel.Composition;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.Editor.Shared.Utilities;
using Microsoft.CodeAnalysis.ErrorReporting;
using Microsoft.CodeAnalysis.Host.Mef;
using Microsoft.CodeAnalysis.Options;
Expand All @@ -24,46 +21,41 @@ namespace Microsoft.VisualStudio.LanguageServices.Options;
[Export(typeof(VisualStudioOptionPersisterProvider))]
internal sealed class VisualStudioOptionPersisterProvider : IOptionPersisterProvider
{
private readonly IAsyncServiceProvider _serviceProvider;
private readonly ILegacyGlobalOptionService _legacyGlobalOptions;
private readonly IServiceProvider _serviceProvider;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might want to add a comment we're using IServiceProvider versus the async forms since we know this is a constrained case.

private readonly Lazy<ILegacyGlobalOptionService> _legacyGlobalOptions;

// maps config name to a read fallback:
private readonly ImmutableDictionary<string, Lazy<IVisualStudioStorageReadFallback, OptionNameMetadata>> _readFallbacks;

// Use vs-threading's JTF-aware AsyncLazy<T>. Ensure only one persister instance is created (even in the face of
// parallel requests for the value) because the constructor registers global event handler callbacks.
private readonly Threading.AsyncLazy<IOptionPersister> _lazyPersister;
// Ensure only one persister instance is created (even in the face of parallel requests for the value)
// because the constructor registers global event handler callbacks.
private readonly Lazy<IOptionPersister> _lazyPersister;

[ImportingConstructor]
[Obsolete(MefConstruction.ImportingConstructorMessage, error: true)]
public VisualStudioOptionPersisterProvider(
[Import(typeof(SAsyncServiceProvider))] IAsyncServiceProvider serviceProvider,
[Import(typeof(SVsServiceProvider))] IServiceProvider serviceProvider,
[ImportMany] IEnumerable<Lazy<IVisualStudioStorageReadFallback, OptionNameMetadata>> readFallbacks,
IThreadingContext threadingContext,
ILegacyGlobalOptionService legacyGlobalOptions)
Lazy<ILegacyGlobalOptionService> legacyGlobalOptions)
{
_serviceProvider = serviceProvider;
_legacyGlobalOptions = legacyGlobalOptions;
_readFallbacks = readFallbacks.ToImmutableDictionary(item => item.Metadata.ConfigName, item => item);
_lazyPersister = new Threading.AsyncLazy<IOptionPersister>(() => CreatePersisterAsync(threadingContext.DisposalToken), threadingContext.JoinableTaskFactory);
_lazyPersister = new Lazy<IOptionPersister>(() => CreatePersister());
}

public ValueTask<IOptionPersister> GetOrCreatePersisterAsync(CancellationToken cancellationToken)
=> new(_lazyPersister.GetValueAsync(cancellationToken));
public IOptionPersister GetOrCreatePersister()
=> _lazyPersister.Value;

private async Task<IOptionPersister> CreatePersisterAsync(CancellationToken cancellationToken)
private IOptionPersister CreatePersister()
{
// Obtain services before creating instances. This avoids state corruption in the event cancellation is
// requested (some of the constructors register event handlers that could leak if cancellation occurred
// in the middle of construction).
var settingsManager = await GetFreeThreadedServiceAsync<SVsSettingsPersistenceManager, ISettingsManager>().ConfigureAwait(false);
var settingsManager = GetFreeThreadedService<SVsSettingsPersistenceManager, ISettingsManager>();
Assumes.Present(settingsManager);
var localRegistry = await GetFreeThreadedServiceAsync<SLocalRegistry, ILocalRegistry4>().ConfigureAwait(false);

var localRegistry = GetFreeThreadedService<SLocalRegistry, ILocalRegistry4>();
Assumes.Present(localRegistry);
var featureFlags = await GetFreeThreadedServiceAsync<SVsFeatureFlags, IVsFeatureFlags>().ConfigureAwait(false);

// Cancellation is not allowed after this point
cancellationToken = CancellationToken.None;
var featureFlags = GetFreeThreadedService<SVsFeatureFlags, IVsFeatureFlags>();

return new VisualStudioOptionPersister(
new VisualStudioSettingsOptionPersister(RefreshOption, _readFallbacks, settingsManager),
Expand All @@ -73,23 +65,23 @@ private async Task<IOptionPersister> CreatePersisterAsync(CancellationToken canc

private void RefreshOption(OptionKey2 optionKey, object? newValue)
{
if (_legacyGlobalOptions.GlobalOptions.RefreshOption(optionKey, newValue))
if (_legacyGlobalOptions.Value.GlobalOptions.RefreshOption(optionKey, newValue))
{
// We may be updating the values of internally defined public options.
// Update solution snapshots of all workspaces to reflect the new values.
_legacyGlobalOptions.UpdateRegisteredWorkspaces();
_legacyGlobalOptions.Value.UpdateRegisteredWorkspaces();
}
}

/// <summary>
/// Returns a service without doing a transition to the UI thread to cast the service to the interface type. This should only be called for services that are
/// well-understood to be castable off the UI thread, either because they are managed or free-threaded COM.
/// </summary>
private async ValueTask<I?> GetFreeThreadedServiceAsync<T, I>() where I : class
private I? GetFreeThreadedService<T, I>() where I : class
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be done as a follow-up, but do we need this extension anymore? If all the underlying helpers from the shell are now smarter, maybe we can delete it.

{
try
{
return (I?)await _serviceProvider.GetServiceAsync(typeof(T)).ConfigureAwait(false);
return (I?)_serviceProvider.GetService(typeof(T));
}
catch (Exception e) when (FatalError.ReportAndPropagate(e))
{
Expand Down
25 changes: 3 additions & 22 deletions src/VisualStudio/Core/Def/RoslynPackage.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
// See the LICENSE file in the project root for more information.

using System;
using System.Collections.Immutable;
using System.ComponentModel.Design;
using System.Runtime.InteropServices;
using System.Threading;
Expand All @@ -18,12 +17,9 @@
using Microsoft.CodeAnalysis.Notification;
using Microsoft.CodeAnalysis.Options;
using Microsoft.CodeAnalysis.Remote.ProjectSystem;
using Microsoft.CodeAnalysis.Shared.TestHooks;
using Microsoft.VisualStudio.ComponentModelHost;
using Microsoft.VisualStudio.LanguageServices.EditorConfigSettings;
using Microsoft.VisualStudio.LanguageServices.Implementation.Diagnostics;
using Microsoft.VisualStudio.LanguageServices.Implementation.LanguageService;
using Microsoft.VisualStudio.LanguageServices.Implementation.ProjectSystem;
using Microsoft.VisualStudio.LanguageServices.Implementation.ProjectSystem.RuleSets;
using Microsoft.VisualStudio.LanguageServices.Implementation.Suppression;
using Microsoft.VisualStudio.LanguageServices.Implementation.SyncNamespaces;
Expand Down Expand Up @@ -106,6 +102,9 @@ protected override void RegisterOnAfterPackageLoadedAsyncWork(PackageLoadTasks a

Task OnAfterPackageLoadedBackgroundThreadAsync(PackageLoadTasks afterPackageLoadedTasks, CancellationToken cancellationToken)
{
// Ensure the options persisters are loaded since we have to fetch options from the shell
_ = ComponentModel.GetService<IGlobalOptionService>();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_ = ComponentModel.GetService();

I debated just not having this, but I wanted to keep it similar to before in that it explicitly ensures the service is at least created by this point.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this point we're not eagerly creating any persisters, right? It's just creating the service, but the persisters themselves are still lazy? In that case, I'd toss this. It's not doing anything (but looks like it might be.)


var colorSchemeApplier = ComponentModel.GetService<ColorSchemeApplier>();
colorSchemeApplier.RegisterInitializationWork(afterPackageLoadedTasks);

Expand All @@ -115,9 +114,6 @@ Task OnAfterPackageLoadedBackgroundThreadAsync(PackageLoadTasks afterPackageLoad
_solutionEventMonitor = new SolutionEventMonitor(globalNotificationService);
TrackBulkFileOperations(globalNotificationService);

// Ensure the options persisters are loaded since we have to fetch options from the shell
LoadOptionPersistersAsync(this.ComponentModel, cancellationToken).Forget();

return Task.CompletedTask;
}

Expand Down Expand Up @@ -145,21 +141,6 @@ private async Task ProfferServiceBrokerServicesAsync(CancellationToken cancellat
(_, _, _, _) => ValueTaskFactory.FromResult<object?>(new ManagedEditAndContinueLanguageServiceBridge(this.ComponentModel.GetService<EditAndContinueLanguageService>())));
}

private async Task LoadOptionPersistersAsync(IComponentModel componentModel, CancellationToken cancellationToken)
{
// Ensure on a background thread to ensure assembly loads don't show up as UI delays attributed to
// InitializeAsync.
Contract.ThrowIfTrue(JoinableTaskFactory.Context.IsOnMainThread);

var listenerProvider = componentModel.GetService<IAsynchronousOperationListenerProvider>();
using var token = listenerProvider.GetListener(FeatureAttribute.Workspace).BeginAsyncOperation(nameof(LoadOptionPersistersAsync));

var persisterProviders = componentModel.GetExtensions<IOptionPersisterProvider>().ToImmutableArray();

foreach (var provider in persisterProviders)
await provider.GetOrCreatePersisterAsync(cancellationToken).ConfigureAwait(true);
}

protected override async Task LoadComponentsAsync(CancellationToken cancellationToken)
{
await TaskScheduler.Default;
Expand Down
2 changes: 1 addition & 1 deletion src/VisualStudio/IntegrationTest/New.IntegrationTests/Options/GlobalOptionsTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ public async Task ValidateAllOptions()
{
var globalOptions = (GlobalOptionService)await TestServices.Shell.GetComponentModelServiceAsync<IGlobalOptionService>(HangMitigatingCancellationToken);
var provider = await TestServices.Shell.GetComponentModelServiceAsync<VisualStudioOptionPersisterProvider>(HangMitigatingCancellationToken);
var vsSettingsPersister = (VisualStudioOptionPersister)await provider.GetOrCreatePersisterAsync(HangMitigatingCancellationToken);
var vsSettingsPersister = (VisualStudioOptionPersister)provider.GetOrCreatePersister();

var optionsInfo = OptionsTestInfo.CollectOptions(Path.GetDirectoryName(typeof(GlobalOptionsTest).Assembly.Location!));
var allLanguages = new[] { LanguageNames.CSharp, LanguageNames.VisualBasic };
Expand Down
63 changes: 10 additions & 53 deletions src/Workspaces/Core/Portable/Options/GlobalOptionService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,9 @@
using System.Collections.Immutable;
using System.Composition;
using System.Diagnostics;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.Host.Mef;
using Microsoft.CodeAnalysis.PooledObjects;
using Microsoft.CodeAnalysis.Shared.Collections;
using Microsoft.CodeAnalysis.Shared.Utilities;
using Roslyn.Utilities;

namespace Microsoft.CodeAnalysis.Options;
Expand All @@ -21,62 +18,23 @@ namespace Microsoft.CodeAnalysis.Options;
[method: ImportingConstructor]
[method: Obsolete(MefConstruction.ImportingConstructorMessage, error: true)]
internal sealed class GlobalOptionService(
[Import(AllowDefault = true)] IWorkspaceThreadingService? workspaceThreadingService,
[ImportMany] IEnumerable<Lazy<IOptionPersisterProvider>> optionPersisters) : IGlobalOptionService
[ImportMany] IEnumerable<Lazy<IOptionPersisterProvider>> optionPersisterProviders) : IGlobalOptionService
{
private readonly ImmutableArray<Lazy<IOptionPersisterProvider>> _optionPersisterProviders = [.. optionPersisters];

private readonly object _gate = new();

#region Guarded by _gate

private ImmutableArray<IOptionPersister> _lazyOptionPersisters;
private readonly Lazy<ImmutableArray<IOptionPersister>> _optionPersisters = new(() => GetOptionPersisters(optionPersisterProviders));
private ImmutableDictionary<OptionKey2, object?> _currentValues = ImmutableDictionary.Create<OptionKey2, object?>();

#endregion

private readonly WeakEvent<OptionChangedEventArgs> _optionChanged = new();

private ImmutableArray<IOptionPersister> GetOptionPersisters()
private static ImmutableArray<IOptionPersister> GetOptionPersisters(IEnumerable<Lazy<IOptionPersisterProvider>> optionPersisterProviders)
{
if (_lazyOptionPersisters.IsDefault)
{
// Option persisters cannot be initialized while holding the global options lock
// https://dev.azure.com/devdiv/DevDiv/_workitems/edit/1353715
Debug.Assert(!Monitor.IsEntered(_gate));

ImmutableInterlocked.InterlockedInitialize(
ref _lazyOptionPersisters,
GetOptionPersistersSlow(workspaceThreadingService, _optionPersisterProviders, CancellationToken.None));
}

return _lazyOptionPersisters;

// Local functions
static ImmutableArray<IOptionPersister> GetOptionPersistersSlow(
IWorkspaceThreadingService? workspaceThreadingService,
ImmutableArray<Lazy<IOptionPersisterProvider>> persisterProviders,
CancellationToken cancellationToken)
{
if (workspaceThreadingService is not null && workspaceThreadingService.IsOnMainThread)
{
// speedometer tests report jtf.run calls from background threads, so we try to avoid those.
return workspaceThreadingService.Run(() => GetOptionPersistersAsync(persisterProviders, cancellationToken));
}
else
{
return GetOptionPersistersAsync(persisterProviders, cancellationToken).WaitAndGetResult_CanCallOnBackground(cancellationToken);
}
}

static async Task<ImmutableArray<IOptionPersister>> GetOptionPersistersAsync(
ImmutableArray<Lazy<IOptionPersisterProvider>> persisterProviders,
CancellationToken cancellationToken)
{
return await persisterProviders.SelectAsArrayAsync(
static (lazyProvider, cancellationToken) => lazyProvider.Value.GetOrCreatePersisterAsync(cancellationToken),
cancellationToken).ConfigureAwait(false);
}
return optionPersisterProviders.SelectAsArray(
static provider => provider.Value.GetOrCreatePersister());
}

private static object? LoadOptionFromPersisterOrGetDefault(OptionKey2 optionKey, ImmutableArray<IOptionPersister> persisters)
Expand Down Expand Up @@ -108,16 +66,15 @@ public T GetOption<T>(PerLanguageOption2<T> option, string language)

public T GetOption<T>(OptionKey2 optionKey)
{
// Ensure the option persisters are available before taking the global lock
var persisters = GetOptionPersisters();

// Performance: This is called very frequently, with the vast majority (> 99%) of calls requesting a previously
// added key. In those cases, we can avoid taking the lock as _currentValues is an immutable structure.
if (_currentValues.TryGetValue(optionKey, out var value))
{
return (T)value!;
}

// Ensure the option persisters are available before taking the global lock
var persisters = _optionPersisters.Value;
lock (_gate)
{
return (T)GetOption_NoLock(ref _currentValues, optionKey, persisters)!;
Expand All @@ -126,8 +83,6 @@ public T GetOption<T>(OptionKey2 optionKey)

public ImmutableArray<object?> GetOptions(ImmutableArray<OptionKey2> optionKeys)
{
// Ensure the option persisters are available before taking the global lock
var persisters = GetOptionPersisters();
using var values = TemporaryArray<object?>.Empty;

// Performance: The vast majority of calls are for previously added keys. In those cases, we can avoid taking the lock
Expand All @@ -146,6 +101,8 @@ public T GetOption<T>(OptionKey2 optionKey)

if (values.Count != optionKeys.Length)
{
// Ensure the option persisters are available before taking the global lock
var persisters = _optionPersisters.Value;
lock (_gate)
{
foreach (var optionKey in optionKeys)
Expand Down Expand Up @@ -190,7 +147,7 @@ public bool SetGlobalOptions(ImmutableArray<KeyValuePair<OptionKey2, object?>> o
private bool SetGlobalOptions(OneOrMany<KeyValuePair<OptionKey2, object?>> options)
{
using var _ = ArrayBuilder<(OptionKey2, object?)>.GetInstance(options.Count, out var changedOptions);
var persisters = GetOptionPersisters();
var persisters = _optionPersisters.Value;

lock (_gate)
{
Expand Down
6 changes: 1 addition & 5 deletions src/Workspaces/Core/Portable/Options/IOptionPersisterProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,6 @@
// 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.Threading;
using System.Threading.Tasks;

namespace Microsoft.CodeAnalysis.Options;

internal interface IOptionPersisterProvider
Expand All @@ -16,7 +13,6 @@ internal interface IOptionPersisterProvider
/// This method is safe for concurrent use from any thread. No guarantees are made regarding the use of the UI
/// thread.
/// </remarks>
/// <param name="cancellationToken">A cancellation token the operation may observe.</param>
/// <returns>The option persister.</returns>
ValueTask<IOptionPersister> GetOrCreatePersisterAsync(CancellationToken cancellationToken);
IOptionPersister GetOrCreatePersister();
}
Loading