Skip to content

Move various bits of code to the after package load code in RoslynPackage #77745

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 1 commit into from
Mar 22, 2025
Merged

Move various bits of code to the after package load code in RoslynPackage #77745

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions src/VisualStudio/Core/Def/ColorSchemes/ColorSchemeApplier.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,10 +67,10 @@ public void RegisterInitializationWork(PackageLoadTasks packageInitializationTas
_isInitialized = true;
}

packageInitializationTasks.AddTask(isMainThreadTask: false, task: PackageInitializationBackgroundThreadAsync);
packageInitializationTasks.AddTask(isMainThreadTask: false, task: AfterPackageLoadedBackgroundThreadAsync);
}

private async Task PackageInitializationBackgroundThreadAsync(PackageLoadTasks packageInitializationTasks, CancellationToken cancellationToken)
private async Task AfterPackageLoadedBackgroundThreadAsync(PackageLoadTasks afterPackageLoadedTasks, CancellationToken cancellationToken)
{
var settingsManager = await _asyncServiceProvider.GetServiceAsync<SVsSettingsPersistenceManager, ISettingsManager>(_threadingContext.JoinableTaskFactory).ConfigureAwait(false);

Expand Down
3 changes: 3 additions & 0 deletions src/VisualStudio/Core/Def/LanguageService/AbstractPackage`2.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,9 @@ private async Task PackageInitializationMainThreadAsync(PackageLoadTasks package
RegisterEditorFactory(editorFactory);
}

// Misc workspace has to be up and running by the time our package is usable so that it can track running
// doc events and appropriately map files to/from it and other relevant workspaces (like the
// metadata-as-source workspace).
var miscellaneousFilesWorkspace = this.ComponentModel.GetService<MiscellaneousFilesWorkspace>();

// awaiting an IVsTask guarantees to return on the captured context
Expand Down
29 changes: 10 additions & 19 deletions src/VisualStudio/Core/Def/RoslynPackage.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
using System;
using System.Collections.Immutable;
using System.ComponentModel.Design;
using System.IO;
using System.Runtime.InteropServices;
using System.Threading;
using System.Threading.Tasks;
Expand All @@ -20,7 +19,6 @@
using Microsoft.CodeAnalysis.Options;
using Microsoft.CodeAnalysis.Remote.ProjectSystem;
using Microsoft.CodeAnalysis.Shared.TestHooks;
using Microsoft.CodeAnalysis.SolutionCrawler;
using Microsoft.VisualStudio.ComponentModelHost;
using Microsoft.VisualStudio.LanguageServices.EditorConfigSettings;
using Microsoft.VisualStudio.LanguageServices.Implementation.Diagnostics;
Expand All @@ -32,7 +30,6 @@
using Microsoft.VisualStudio.LanguageServices.Implementation.TableDataSource;
using Microsoft.VisualStudio.LanguageServices.Implementation.UnusedReferences;
using Microsoft.VisualStudio.LanguageServices.InheritanceMargin;
using Microsoft.VisualStudio.LanguageServices.Options;
using Microsoft.VisualStudio.LanguageServices.ProjectSystem;
using Microsoft.VisualStudio.LanguageServices.ProjectSystem.BrokeredService;
using Microsoft.VisualStudio.LanguageServices.StackTraceExplorer;
Expand All @@ -54,7 +51,6 @@ internal sealed class RoslynPackage : AbstractPackage
private static RoslynPackage? s_lazyInstance;

private RuleSetEventHandler? _ruleSetEventHandler;
private ColorSchemeApplier? _colorSchemeApplier;
private SolutionEventMonitor? _solutionEventMonitor;

internal static async ValueTask<RoslynPackage?> GetOrLoadAsync(IThreadingContext threadingContext, IAsyncServiceProvider serviceProvider, CancellationToken cancellationToken)
Expand Down Expand Up @@ -85,13 +81,6 @@ protected override void RegisterInitializeAsyncWork(PackageLoadTasks packageInit

private async Task PackageInitializationBackgroundThreadAsync(PackageLoadTasks packageInitializationTasks, CancellationToken cancellationToken)
{
_colorSchemeApplier = ComponentModel.GetService<ColorSchemeApplier>();
_colorSchemeApplier.RegisterInitializationWork(packageInitializationTasks);

// We are at the VS layer, so we know we must be able to get the IGlobalOperationNotificationService here.
var globalNotificationService = this.ComponentModel.GetService<IGlobalOperationNotificationService>();
Assumes.Present(globalNotificationService);

await ProfferServiceBrokerServicesAsync().ConfigureAwait(true);

var settingsEditorFactory = this.ComponentModel.GetService<SettingsEditorFactory>();
Expand All @@ -100,16 +89,8 @@ private async Task PackageInitializationBackgroundThreadAsync(PackageLoadTasks p
isMainThreadTask: true,
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Mar 21, 2025

Choose a reason for hiding this comment

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

so, this goes to my feedback from before. why are we initializing on the BG just to kick this tack over to the foreground? why not just do this on the FG?

(note: i'm not saying this is wrong. i'm just saying i don't understand the 'why?' part fo this, and i really wish the code was doc'ed for future understanding).

Copy link
Contributor Author

@ToddGrun ToddGrun Mar 21, 2025

Choose a reason for hiding this comment

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

This one is done because SettingsEditorFactory is fairly expensive to create, thus wanting it done on a bg thread and the main thread work depends on that being initialized. I'll create a follow up PR to make that ctor less expensive so it's not a big deal to create it on the main thread.

Copy link
Member

@jasonmalinowski jasonmalinowski Mar 21, 2025

Choose a reason for hiding this comment

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

I think in this specific case, SettingsEditorFactory is fairly expensive to create via MEF. There's a more general piece of work to clean that up since really should be cheap to create and pass to RegsiterEditorFactory, but that's not how it's written right now.

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Mar 21, 2025

Choose a reason for hiding this comment

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

my ask is that we then doc these decisions in the code. :)

task: (packageInitializationTasks, cancellationToken) =>
{
_solutionEventMonitor = new SolutionEventMonitor(globalNotificationService);
TrackBulkFileOperations(globalNotificationService);

RegisterEditorFactory(settingsEditorFactory);

// Misc workspace has to be up and running by the time our package is usable so that it can track running
// doc events and appropriately map files to/from it and other relevant workspaces (like the
// metadata-as-source workspace).
var miscellaneousFilesWorkspace = this.ComponentModel.GetService<MiscellaneousFilesWorkspace>();

return Task.CompletedTask;
});
}
Expand All @@ -125,6 +106,16 @@ protected override void RegisterOnAfterPackageLoadedAsyncWork(PackageLoadTasks a

Task OnAfterPackageLoadedBackgroundThreadAsync(PackageLoadTasks afterPackageLoadedTasks, CancellationToken cancellationToken)
{
var colorSchemeApplier = ComponentModel.GetService<ColorSchemeApplier>();
colorSchemeApplier.RegisterInitializationWork(afterPackageLoadedTasks);

// We are at the VS layer, so we know we must be able to get the IGlobalOperationNotificationService here.
var globalNotificationService = this.ComponentModel.GetService<IGlobalOperationNotificationService>();
Assumes.Present(globalNotificationService);
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Mar 21, 2025

Choose a reason for hiding this comment

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

no GetRequiredService or anything like that?

Copy link
Contributor Author

@ToddGrun ToddGrun Mar 21, 2025

Choose a reason for hiding this comment

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

MEF throws if there isn't exactly one matching export. I'll remove the Assumes.Present as it probably makes that less obvious.


_solutionEventMonitor = new SolutionEventMonitor(globalNotificationService);
TrackBulkFileOperations(globalNotificationService);
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Mar 21, 2025

Choose a reason for hiding this comment

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

consider inlining this. it's more confusing to me as a call out to a function.

Copy link
Contributor Author

@ToddGrun ToddGrun Mar 21, 2025

Choose a reason for hiding this comment

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

I think I'd prefer to keep that method separate. It's fairly long and has two local functions inside it already.


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

Expand Down
2 changes: 1 addition & 1 deletion src/VisualStudio/VisualBasic/Impl/LanguageService/VisualBasicPackage.vb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ Namespace Microsoft.VisualStudio.LanguageServices.VisualBasic

packageInitializationTasks.AddTask(
isMainThreadTask:=False,
task:=Function(packageInitializationTasks2, cancellationToken) As Task
task:=Function() As Task
Copy link
Member

@jasonmalinowski jasonmalinowski Mar 21, 2025

Choose a reason for hiding this comment

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

Oh that's right I forgot VB lets you do this. I figured remove it from the C# side too....

Try
RegisterLanguageService(GetType(IVbCompilerService), Function() Task.FromResult(_comAggregate))

Expand Down
Loading