Make GlobalOptionService initialization synchronous. #77823
Conversation
As Tomas pointed out in an earlier PR, all the hoops that are jumped through in GlobalOptionService to make it's initialization asynchronous aren't really necessary if the services obtained in VisualStudioOptionPersisterProvider were obtained synchronously. Doing just that allows GlobalOptionService to be quite a bit simpler and to no longer require a potential JTF.Run call when getting the first option. The concern one could have with obtaining these services synchronously would be whether they were expensive to get and whether the first call would come through on the main thread. The first call to get an option comes through on a background thread (I've seen it come from either the background processing in after package load, or from the workspace initialization call roslyn gets from project system). Additionally, the measurements that I've taken of obtaining those services haven't shown those as expensive to obtain, so this does seem like a better approach than earlier attempts.
ghost
added
Area-IDE
untriaged
|
Hopefully third times a charm |
| @@ -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>(); | |||
There was a problem hiding this comment.
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.)
|
@jasonmalinowski @sharwell -- this approach good for you? |
|
I strongly support the approach here. If we had a time machine, then we would just make GetOption async and that'd be fine. That would have been handy in the past too when we were moving stuff OOP and wished that we could go to VS to fetch options lazily (and not block in the ServiceHub process while doing so.) But lacking a time machine I think this is fine. The primary concern we've had here is deadlock risk -- this code has been troublesome over the years because UI thread transitions would sneak in during the service fetch, and then we'd deadlock since some random part of Roslyn that though it was free-threaded during initialization wasn't. And since the code predated JTF introduction into Roslyn, we weren't following JTF patterns. (And since GetOption wasn't async, it wasn't a great choice anyways....) But if the services are now free-threaded for good, which they just should be, then the deadlock concern goes away. I'm not concerned at all either about the fact we're using the synchronous GetService rather than GetServiceAsync, because:
|
| @@ -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; | |||
There was a problem hiding this comment.
You might want to add a comment we're using IServiceProvider versus the async forms since we know this is a constrained case.
| } | ||
| } | ||
|
|
||
| /// <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 |
There was a problem hiding this comment.
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.
| @@ -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>(); | |||
There was a problem hiding this comment.
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.)
As Tomas pointed out in an earlier PR, all the hoops that are jumped through in GlobalOptionService to make it's initialization asynchronous aren't really necessary if the services obtained in VisualStudioOptionPersisterProvider were obtained synchronously. Doing just that allows GlobalOptionService to be quite a bit simpler and to no longer require a potential JTF.Run call when getting the first option.
The concern one could have with obtaining these services synchronously would be whether they were expensive to get and whether the first call would come through on the main thread. The first call to get an option comes through on a background thread (I've seen it come from either the background processing in after package load, or from the workspace initialization call roslyn gets from project system). Additionally, the measurements that I've taken of obtaining those services haven't shown those as expensive to obtain, so this does seem like a better approach than earlier attempts.