WIP: First attempt at cleaning up package load to do less thread switching during InitializeAsync #77368

ToddGrun · 2025-02-27T22:16:04Z

This is going to be a dev18 PR, just wanted to get something out to start the conversation about what I've currently got.

Manual testing of opening Roslyn.sln with a single file opened in the sln showed the stopwatch time spent in CSharpPackage.InitializeAsync from about 2000 ms without this change to about 700 ms with this change.

Test insertion to see if speedometer shows anything:
commit 6: https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_build/results?buildId=11106739&view=results

… during InitializeAsync

davkean · 2025-02-27T22:17:10Z

src/VisualStudio/Core/Def/LanguageService/AbstractPackage.cs

@@ -27,16 +27,20 @@ internal IComponentModel ComponentModel

    protected override async Task InitializeAsync(CancellationToken cancellationToken, IProgress<ServiceProgressData> progress)
    {
-        await base.InitializeAsync(cancellationToken, progress).ConfigureAwait(true);
+        // Should only be called from a threadpool. Opinionated, as package load sequence thread switches are impactful.
+        Contract.ThrowIfTrue(JoinableTaskFactory.Context.IsOnMainThread);


We inline you on UI thread if sync blocked so this will fire.

This is a base class, the derived classes ensure we aren't on the main thread

In general, we don't want packages to make that decision; we inline you on the UI thread because its not doing anything, if you queue to the thread pool you will now be affected by latency of the thread pool, instead of just immediately running on the thread that is currently blocked on you,

src/VisualStudio/Core/Def/LanguageService/AbstractPackage.cs

davkean · 2025-02-27T22:18:50Z

src/VisualStudio/Core/Def/LanguageService/AbstractPackage.cs


-        _componentModel_doNotAccessDirectly = (IComponentModel?)await GetServiceAsync(typeof(SComponentModel)).ConfigureAwait(true);
+        _componentModel_doNotAccessDirectly = await GetServiceAsync<SComponentModel, IComponentModel>(throwOnFailure: true, cancellationToken).ConfigureAwait(true);


You don't need to retrieve this here, your package unneccesarily waits on the MEF cache to read or be rebuilt here, just retrieve at exact moment you need.

I started going down that path, and maybe it's something to pursue, but there are a lot of consumers of the ComponentModel property, and making them async wasn't going to be simple

Took another look, and this is not going to be a simple thing to defer (for long, at least).

For example, in RoslynPackage.InitializeAsync, it calls base.InitializeAsync (which sets _componentModel_doNotAccessDirectly), and then two of the next three statements end up accessing ComponentModel and calling methods on the items returned from the MEF catalog.

Took some stopwatch data. About 500 ms of the 700 ms I see in InitializeAsync is due to obtaining MEF. Need to think on this more.

That's when its up-to-date that's 15 seconds+ on a full rebuild.

Good to know. Let's bite off pieces as we can. I'll be coming back from vacation on Monday and can help Todd out with this.

We'll likely have several PRs to get through the bulk of the work, depending on how invasive the changes are.

@toddgrunke feel free to open an issue to help track upcoming immediate work. Thanks!

Also, we should def be measuring both "warm Mef" and "full Mef rebuild time"

@davekean how possible would it be to have dedicated speedometer tests for both? It would be great for measuring our progress here over time, and especially good for preventing regressions.

If we already have tests here (say... Time to open a basic project, and time to open a single file in a project), could you link is to those?

Thanks!

davkean · 2025-02-27T22:19:47Z

src/VisualStudio/Core/Def/LanguageService/AbstractPackage`2.cs

-        Assumes.Present(shell);
-        Assumes.Present(solution);
+        // Should only be called from a threadpool. Opinionated, as package load sequence thread switches are impactful.
+        Contract.ThrowIfTrue(JoinableTaskFactory.Context.IsOnMainThread);


I don't think this can be removed, and is safe to do as the derived classes make sure this assumption is valid.

src/VisualStudio/Core/Def/LanguageService/AbstractPackage`2.cs

davkean · 2025-02-27T22:21:36Z

src/VisualStudio/Core/Def/LanguageService/AbstractPackage`2.cs


        var miscellaneousFilesWorkspace = this.ComponentModel.GetService<MiscellaneousFilesWorkspace>();
        RegisterMiscellaneousFilesWorkspaceInformation(miscellaneousFilesWorkspace);

+        await shell.LoadPackageAsync(Guids.RoslynPackageId);


I know we're already doing this, but would want to know why we need to do this.

I think it's necessary, but it's maybe something which could be improved. Is there a way to specify inter-package dependencies other than manually loading like this?

Package loads generally shouldn't have side-effects, so any behavior its depending on should be expressed in a form that is more explicit such as a service retrieval.

src/VisualStudio/Core/Def/LanguageService/AbstractPackage`2.cs

src/VisualStudio/Core/Def/RoslynPackage.cs

2) move getting the MiscellaneousFilesWorkspace to the bg thread 3) Get rid of some useless calls into base class (AsyncPackage)

…rom main thread 2) change call to base.OnAfterPackageLoadedAsync to use ConfigureAwait(true) to ensure we capture the calling context.

…previous commit

remove assumption about LoadComponentsAsync calling thread

CyrusNajmabadi · 2025-03-01T07:57:18Z

src/VisualStudio/Core/Def/LanguageService/AbstractPackage.cs

-        await JoinableTaskFactory.SwitchToMainThreadAsync(cancellationToken);
-
-        _componentModel_doNotAccessDirectly = (IComponentModel?)await GetServiceAsync(typeof(SComponentModel)).ConfigureAwait(true);
+        _componentModel_doNotAccessDirectly = await GetServiceAsync<SComponentModel, IComponentModel>(throwOnFailure: true, cancellationToken).ConfigureAwait(true);


Ok to not do the base initialize?

CyrusNajmabadi · 2025-03-01T07:58:18Z

src/VisualStudio/Core/Def/LanguageService/AbstractPackage.cs

        Assumes.Present(_componentModel_doNotAccessDirectly);
    }

    protected override async Task OnAfterPackageLoadedAsync(CancellationToken cancellationToken)
    {
-        await base.OnAfterPackageLoadedAsync(cancellationToken).ConfigureAwait(false);


Just trying to understand why this is ok. Is it just known that the base does nothing?

I documented this.

The default implementation of this method does nothing and there is no requirement to call it in an override.

CyrusNajmabadi · 2025-03-01T08:03:35Z

Could you write up what work may remain? Like deferring the work to access Mef? Thanks!

First attempt at cleaning up package load to do less thread switching…

13c0b8a

… during InitializeAsync

dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Feb 27, 2025

ToddGrun requested a review from davkean February 27, 2025 22:16