Simple and safe performance changes to package load code. by ToddGrun · Pull Request #77561 · dotnet/roslyn

ToddGrun · 2025-03-12T17:26:09Z

This is the 2nd part of a multi-part PR series focused on improving the performance of loading the roslyn packages.

This PR focuses mostly on removing some extraneous potential thread switches.

ToddGrun · 2025-03-12T17:28:03Z

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


    protected override async Task InitializeAsync(CancellationToken cancellationToken, IProgress<ServiceProgressData> progress)
    {
-        await base.InitializeAsync(cancellationToken, progress).ConfigureAwait(true);


InitializeAsync

Base class documents that calling it's InitializeAsync and OnAfterPackageLoadedAsync methods is not necessary.

ToddGrun · 2025-03-12T17:28:41Z

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

-
-        await JoinableTaskFactory.SwitchToMainThreadAsync(cancellationToken);
-
-        _componentModel_doNotAccessDirectly = (IComponentModel?)await GetServiceAsync(typeof(SComponentModel)).ConfigureAwait(true);


GetServiceAsync

There is no main thread requirement for getting/using this service.

ToddGrun · 2025-03-12T17:29:38Z

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


    protected override async Task InitializeAsync(CancellationToken cancellationToken, IProgress<ServiceProgressData> progress)
    {
-        await base.InitializeAsync(cancellationToken, progress).ConfigureAwait(true);


true

no need to capture the context here as the next line will move us back to the UI thread

ToddGrun · 2025-03-12T17:30:06Z

src/VisualStudio/Core/Def/RoslynPackage.cs


        await JoinableTaskFactory.SwitchToMainThreadAsync(cancellationToken);

-        cancellationToken.ThrowIfCancellationRequested();


cancellationToken.ThrowIfCancellationRequested();

The SwitchToMainThreadAsync call captures this check

ToddGrun · 2025-03-12T17:30:34Z

src/VisualStudio/Core/Def/RoslynPackage.cs


    protected override async Task InitializeAsync(CancellationToken cancellationToken, IProgress<ServiceProgressData> progress)
    {
-        await base.InitializeAsync(cancellationToken, progress).ConfigureAwait(true);


true

No need to ensure we're on the captured context if the next line moves us to the main thread.

ToddGrun · 2025-03-12T17:30:54Z

src/VisualStudio/CSharp/Impl/CSharpPackage.cs

            try
            {
                await base.InitializeAsync(cancellationToken, progress).ConfigureAwait(true);
-                await JoinableTaskFactory.SwitchToMainThreadAsync(cancellationToken);


SwitchToMainThreadAsync

RegisterService doesn't require main thread

do we need/want CA(true) on the prior line? if so, doc.

no, we don't want that either, I just missed it in this PR. I'll go ahead and change.

doc that as well plz

ToddGrun · 2025-03-12T17:31:18Z

src/VisualStudio/VisualBasic/Impl/LanguageService/VisualBasicPackage.vb

        Protected Overrides Async Function InitializeAsync(cancellationToken As CancellationToken, progress As IProgress(Of ServiceProgressData)) As Task
            Try
                Await MyBase.InitializeAsync(cancellationToken, progress).ConfigureAwait(True)
-                Await JoinableTaskFactory.SwitchToMainThreadAsync(cancellationToken)


SwitchToMainThreadAsync

Neither RegisterLanguageService nor RegisterService require the main thread.

do we need/want CA(true) on the prior line? if so, doc.

CyrusNajmabadi

signing off. but i think it wouild be good for these critical sections to doc the intentional behavior here.

ToddGrun · 2025-03-12T17:33:32Z

i think it wouild be good for these critical sections to doc the intentional behavior here.

Agreed, I will add some documentation once things get a little closer to the end target.

JoeRobich · 2025-03-12T17:53:11Z

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

-        _componentModel_doNotAccessDirectly = (IComponentModel?)await GetServiceAsync(typeof(SComponentModel)).ConfigureAwait(true);
+        _componentModel_doNotAccessDirectly = (IComponentModel?)await GetServiceAsync(typeof(SComponentModel)).ConfigureAwait(false);
        Assumes.Present(_componentModel_doNotAccessDirectly);
+


nit: feel free to do in a follow up

Suggested change

whoops! I had a whole bunch of logging that I was using for local verification, and looks like I didn't remove the newline that I had next to it.

Simple and safe performance changes to package load code.

0b605d4

ToddGrun requested a review from a team as a code owner March 12, 2025 17:26

ghost added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Mar 12, 2025

ToddGrun requested review from CyrusNajmabadi, davkean and jasonmalinowski March 12, 2025 17:26

ToddGrun commented Mar 12, 2025

View reviewed changes

CyrusNajmabadi approved these changes Mar 12, 2025

View reviewed changes

Missed a ConfigureAwait(true) that is potentially bad

b91f604

JoeRobich reviewed Mar 12, 2025

View reviewed changes

remove extra newline

603bb4e

ToddGrun merged commit 5fa4c2c into dotnet:release/dev18.0 Mar 12, 2025
25 checks passed

ToddGrun mentioned this pull request Mar 14, 2025

Attempt to further reduce thread switches during package load. #77602

Merged

This was referenced Apr 19, 2025

[Automated] PRs inserted in VS build main-10518.29 #78209

Closed

[Automated] PRs inserted in VS build feature.debugger.main-10521.160 #78252

Closed

dotnet-bot mentioned this pull request May 31, 2025

[Automated] PRs inserted in VS build feature.telemetry.d18OpenTelemetry-10630.96 #78782

Closed


		await JoinableTaskFactory.SwitchToMainThreadAsync(cancellationToken);

		_componentModel_doNotAccessDirectly = (IComponentModel?)await GetServiceAsync(typeof(SComponentModel)).ConfigureAwait(true);

Conversation

ToddGrun commented Mar 12, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi left a comment

Choose a reason for hiding this comment

Uh oh!

ToddGrun commented Mar 12, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants