Skip to content

Simple and safe performance changes to package load code. #77561

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 3 commits into from
Mar 12, 2025
Merged

Simple and safe performance changes to package load code. #77561

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0b605d4
Simple and safe performance changes to package load code.
ToddGrun Mar 12, 2025
b91f604
Missed a ConfigureAwait(true) that is potentially bad
ToddGrun Mar 12, 2025
603bb4e
remove extra newline
ToddGrun Mar 12, 2025
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
3 changes: 1 addition & 2 deletions src/VisualStudio/CSharp/Impl/CSharpPackage.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,7 @@ protected override async Task InitializeAsync(CancellationToken cancellationToke
{
try
{
await base.InitializeAsync(cancellationToken, progress).ConfigureAwait(true);
await JoinableTaskFactory.SwitchToMainThreadAsync(cancellationToken);
Copy link
Contributor Author

@ToddGrun ToddGrun Mar 12, 2025

Choose a reason for hiding this comment

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

SwitchToMainThreadAsync

RegisterService doesn't require main thread

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Mar 12, 2025

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@ToddGrun ToddGrun Mar 12, 2025

Choose a reason for hiding this comment

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

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

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Mar 12, 2025

Choose a reason for hiding this comment

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

doc that as well plz

await base.InitializeAsync(cancellationToken, progress).ConfigureAwait(false);

this.RegisterService<ICSharpTempPECompilerService>(async ct =>
{
Expand Down
8 changes: 1 addition & 7 deletions src/VisualStudio/Core/Def/LanguageService/AbstractPackage.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,18 +27,12 @@ internal IComponentModel ComponentModel

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

@ToddGrun ToddGrun Mar 12, 2025

Choose a reason for hiding this comment

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

InitializeAsync

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


await JoinableTaskFactory.SwitchToMainThreadAsync(cancellationToken);

_componentModel_doNotAccessDirectly = (IComponentModel?)await GetServiceAsync(typeof(SComponentModel)).ConfigureAwait(true);
Copy link
Contributor Author

@ToddGrun ToddGrun Mar 12, 2025

Choose a reason for hiding this comment

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

GetServiceAsync

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

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

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

// TODO: remove, workaround for https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1985204
var globalOptions = ComponentModel.GetService<IGlobalOptionService>();
if (globalOptions.GetOption(SemanticSearchFeatureFlag.Enabled))
Expand Down
2 changes: 1 addition & 1 deletion 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 @@ -36,7 +36,7 @@ protected AbstractPackage()

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

@ToddGrun ToddGrun Mar 12, 2025

Choose a reason for hiding this comment

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

true

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

await base.InitializeAsync(cancellationToken, progress).ConfigureAwait(false);

await JoinableTaskFactory.SwitchToMainThreadAsync(cancellationToken);

Expand Down
4 changes: 1 addition & 3 deletions src/VisualStudio/Core/Def/RoslynPackage.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,12 +144,10 @@ protected override void OnSaveOptions(string key, Stream stream)

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

@ToddGrun ToddGrun Mar 12, 2025

Choose a reason for hiding this comment

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

true

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

await base.InitializeAsync(cancellationToken, progress).ConfigureAwait(false);

await JoinableTaskFactory.SwitchToMainThreadAsync(cancellationToken);

cancellationToken.ThrowIfCancellationRequested();
Copy link
Contributor Author

@ToddGrun ToddGrun Mar 12, 2025

Choose a reason for hiding this comment

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

cancellationToken.ThrowIfCancellationRequested();

The SwitchToMainThreadAsync call captures this check


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

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

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)
Copy link
Contributor Author

@ToddGrun ToddGrun Mar 12, 2025

Choose a reason for hiding this comment

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

SwitchToMainThreadAsync

Neither RegisterLanguageService nor RegisterService require the main thread.

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Mar 12, 2025

Choose a reason for hiding this comment

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

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

Await MyBase.InitializeAsync(cancellationToken, progress).ConfigureAwait(False)

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

Expand Down
Loading