Add GetOptionAsync calls to IGlobalOptionService #77808
Conversation
This is a potentially better way to address the speedometer regression "fixed" by dotnet#77788. That PR just replaced a JTF.Run on a bg thread with a .Result call (which doesn't get flagged by speedometer). Instead, go ahead and make there be an async path through this code that avoids the JTF.Run/Result code altogether. Note that only the first call to GetOption would hit this path, thus why I went down the expediant fix before. However, PR feedback has expressed a strong desire to not go that route, and thus the change here to add an async path to getting an option through IGlobalOptionService. There are *many* synchronous calls to GetOption, and this PR doesn't remove that codepath as that's outside the scope of the change that I'd like to make at this time. This PR only changes the GetOption calls that occur during package load as those were fairly easy to change and the most likely to cause the async work to occur.
ghost
added
Area-IDE
untriaged
| ref _lazyOptionPersisters, | ||
| lazyOptionPersisters); | ||
|
|
||
| return lazyOptionPersisters; |
There was a problem hiding this comment.
is this correct? or should you be grabbing _lazyOptionPErsisters?
There was a problem hiding this comment.
Yeah, _lazyOptionPersisters is probably better to use
| } | ||
| } | ||
|
|
||
| public ValueTask<T> GetOptionAsync<T>(OptionKey2 optionKey) |
There was a problem hiding this comment.
why doesn't this take a CT?
| async ValueTask<T> GetOptionSlowAsync(OptionKey2 optionKey) | ||
| { | ||
| // Ensure the option persisters are available before taking the global lock | ||
| var persisters = await GetOptionPersistersAsync(CancellationToken.None).ConfigureAwait(false); |
There was a problem hiding this comment.
seems odd to not be cancellable.
| /// <summary> | ||
| /// Gets the current value of the specific option. | ||
| /// </summary> | ||
| ValueTask<T> GetOptionAsync<T>(Option2<T> option); |
There was a problem hiding this comment.
Yeah, probably better to enforce sending over a CT from the get-go.
|
The only reason the code that retrieves persisters is async is because we are using GetServiceAsync to get the following services: Is it necessary to fetch these services asynchronously? |
| } | ||
|
|
||
| public ColorSchemeName GetConfiguredColorScheme() | ||
| public async Task<ColorSchemeName> GetConfiguredColorSchemeAsync(CancellationToken cancellationToken) |
There was a problem hiding this comment.
This code is using both global options service and ISettingsManager to get options/react on option changes.
This code might need some cleanup (why is it switching to UI thread?). Can it use global options changed event instead?
// We need to update the theme whenever the Editor Color Scheme setting changes.
await TaskScheduler.Default;
var settingsManager = await _asyncServiceProvider.GetServiceAsync<SVsSettingsPersistenceManager, ISettingsManager>(_threadingContext.JoinableTaskFactory).ConfigureAwait(false);
await _threadingContext.JoinableTaskFactory.SwitchToMainThreadAsync(cancellationToken);
settingsManager.GetSubset(ColorSchemeOptionsStorage.ColorSchemeSettingKey).SettingChangedAsync += ColorSchemeChangedAsync;
await TaskScheduler.Default;Can we avoid initializing the color scheme from package initialization? Is the comment still true?
// Try to migrate the `useEnhancedColorsSetting` to the new `ColorSchemeName` setting.
_settings.MigrateToColorSchemeSetting();
// Since the Roslyn colors are now defined in the Roslyn repo and no longer applied by the VS pkgdef built from EditorColors.xml,
// We attempt to apply a color scheme when the Roslyn package is loaded. This is our chance to update the configuration registry
// with the Roslyn colors before they are seen by the user. This is important because the MEF exported Roslyn classification
// colors are only applicable to the Blue and Light VS themes.
// If the color scheme has updated, apply the scheme.
await UpdateColorSchemeAsync(cancellationToken).ConfigureAwait(false);
There was a problem hiding this comment.
@tmat, if you'resuggesting the code snippet you included above, just a bit of feedback on that:
Please don't switch to a background thread just to asynchronously call GetServiceAsync and then switch back to the main thread. That only adds thread transitions, makes your code a little harder to follow, and slows down the overall completion time. If the service isn't already available, it can load w/o the main thread being involved even if the caller is on the main thread. That's why we made it async. :)
There was a problem hiding this comment.
The above (existing) code is what I'm suggesting should be cleaned up :)
There was a problem hiding this comment.
The above code (in the upper box) doesn't exist anymore in our 17.15 branch.
Maybe not. Let me look into that and put this PR on ice. |
OK, it does look better and seems ok with perf |
This is a potentially better way to address the speedometer regression "fixed" by #77788.
That PR just replaced a JTF.Run on a bg thread with a .Result call (which doesn't get flagged by speedometer). Instead, go ahead and make there be an async path through this code that avoids the JTF.Run/Result code altogether.
Note that only the first call to GetOption would hit this path, thus why I went down the expediant fix before. However, PR feedback has expressed a strong desire to not go that route, and thus the change here to add an async path to getting an option through IGlobalOptionService. There are many synchronous calls to GetOption, and this PR doesn't remove that codepath as that's outside the scope of the change that I'd like to make at this time. This PR only changes the GetOption calls that occur during package load as those were fairly easy to change and the most likely to cause the async work to occur.