Skip to content

Another small set of package initialization optimizations #77646

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 5 commits into from
Mar 18, 2025
Merged

Another small set of package initialization optimizations #77646

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
566a1c7
Another small set of package initialization optimizations
ToddGrun Mar 17, 2025
581ab0e
lint
ToddGrun Mar 17, 2025
670e2b6
move MiscellaneousFilesWorkspace to get created on a bg thread
ToddGrun Mar 18, 2025
f03bd7c
fix accidental changes from subsequent PR
ToddGrun Mar 18, 2025
70f4569
move the misc workspace back to initialization on the main thread to …
ToddGrun Mar 18, 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: 0 additions & 3 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 @@ -14,11 +14,9 @@
using Microsoft.CodeAnalysis.Options;
using Microsoft.CodeAnalysis.Shared.TestHooks;
using Microsoft.CodeAnalysis.Threading;
using Microsoft.VisualStudio;
using Microsoft.VisualStudio.LanguageServices.Implementation.LanguageService;
using Microsoft.VisualStudio.Settings;
using Microsoft.VisualStudio.Shell;
using Microsoft.VisualStudio.Shell.Interop;
using Task = System.Threading.Tasks.Task;

namespace Microsoft.CodeAnalysis.ColorSchemes;
Expand All @@ -42,7 +40,6 @@ internal sealed partial class ColorSchemeApplier
[Obsolete(MefConstruction.ImportingConstructorMessage, error: true)]
public ColorSchemeApplier(
IThreadingContext threadingContext,
IVsService<SVsFontAndColorStorage, IVsFontAndColorStorage> fontAndColorStorage,
IGlobalOptionService globalOptions,
SVsServiceProvider serviceProvider,
IAsynchronousOperationListenerProvider listenerProvider)
Expand Down
47 changes: 27 additions & 20 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,32 +58,32 @@ private async Task PackageInitializationMainThreadAsync(IProgress<ServiceProgres
RegisterEditorFactory(editorFactory);
}

RegisterLanguageService(typeof(TLanguageService), async cancellationToken =>
{
// Ensure we're on the BG when creating the language service.
await TaskScheduler.Default;

// Create the language service, tell it to set itself up, then store it in a field
// so we can notify it that it's time to clean up.
_languageService = CreateLanguageService();
await _languageService.SetupAsync(cancellationToken).ConfigureAwait(false);

return _languageService.ComAggregate!;
});

var miscellaneousFilesWorkspace = this.ComponentModel.GetService<MiscellaneousFilesWorkspace>();
Copy link
Member

@jasonmalinowski jasonmalinowski Mar 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Moving this to the background should be safe -- this is not written to have UI thread assumptions in the constructors. See the comments in OpenTextBufferProvider's constructor specifically for ensuring that's safe. This wasn't the case a few years back but this was generally modernized.

If you're wanting to keep the changes small to manage risk better, do this in a follow up PR (but don't forget about it.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I originally tried that and it threw. Let me see if I can find out where that dependency is.


// awaiting an IVsTask guarantees to return on the captured context
await shell.LoadPackageAsync(Guids.RoslynPackageId);

RegisterMiscellaneousFilesWorkspaceInformation(miscellaneousFilesWorkspace);
packageRegistrationTasks.AddTask(
isMainThreadTask: false,
task: (IProgress<ServiceProgressData> progress, PackageRegistrationTasks packageRegistrationTasks, CancellationToken cancellationToken) =>
{
RegisterLanguageService(typeof(TLanguageService), async cancellationToken =>
{
// Ensure we're on the BG when creating the language service.
await TaskScheduler.Default;
Comment on lines +72 to +73
Copy link
Member

@jasonmalinowski jasonmalinowski Mar 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

We already said isMainThreadTask: false, why saying this a second time? I'd frankly can't explain why that RegisterLanguageServer function exists in the first place, if that was the concern. It has this comment:

// When registering a language service, we need to take its ComAggregate wrapper.

But there's nothing special it's doing with it...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's the callback when the service is created, I don't know if we can guarantee what context we are on when that happens.

Copy link
Member

Choose a reason for hiding this comment

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

Oh right of course. That'd be something we should follow up with -- hopefully we have some way to say we can be created on a background thread -- it'd be silly for the service system to bounce to the UI thread if we didn't need it.


if (!_shell.IsInCommandLineMode())
{
// not every derived package support object browser and for those languages
// this is a no op
RegisterObjectBrowserLibraryManager();
}
// Create the language service, tell it to set itself up, then store it in a field
// so we can notify it that it's time to clean up.
_languageService = CreateLanguageService();
await _languageService.SetupAsync(cancellationToken).ConfigureAwait(false);

return _languageService.ComAggregate!;
});

RegisterMiscellaneousFilesWorkspaceInformation(miscellaneousFilesWorkspace);

return Task.CompletedTask;
});
}

protected override async Task OnAfterPackageLoadedAsync(CancellationToken cancellationToken)
Expand All @@ -92,6 +92,13 @@ protected override async Task OnAfterPackageLoadedAsync(CancellationToken cancel

await JoinableTaskFactory.SwitchToMainThreadAsync(cancellationToken);

if (_shell != null && !_shell.IsInCommandLineMode())
{
// not every derived package support object browser and for those languages
// this is a no op
RegisterObjectBrowserLibraryManager();
}

LoadComponentsInUIContextOnceSolutionFullyLoadedAsync(cancellationToken).Forget();
}

Expand Down
19 changes: 7 additions & 12 deletions src/VisualStudio/Core/Def/ProjectSystem/MiscellaneousFilesWorkspace.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,13 @@
// See the LICENSE file in the project root for more information.

using System;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.ComponentModel.Composition;
using System.IO;
using System.Linq;
using System.Threading.Tasks;
using System.Threading;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.Editor;
using Microsoft.CodeAnalysis.Editor.Shared.Extensions;
Expand All @@ -17,11 +18,9 @@
using Microsoft.CodeAnalysis.Host.Mef;
using Microsoft.CodeAnalysis.MetadataAsSource;
using Microsoft.CodeAnalysis.Text;
using Microsoft.VisualStudio.Shell;
using Microsoft.VisualStudio.Shell.Interop;
using Microsoft.VisualStudio.Text;
using Microsoft.VisualStudio.TextManager.Interop;
using Microsoft.VisualStudio.Threading;
using Roslyn.Utilities;

namespace Microsoft.VisualStudio.LanguageServices.Implementation.ProjectSystem;
Expand All @@ -34,7 +33,7 @@ internal sealed partial class MiscellaneousFilesWorkspace : Workspace, IOpenText
private readonly OpenTextBufferProvider _openTextBufferProvider;
private readonly IMetadataAsSourceFileService _fileTrackingMetadataAsSourceService;

private readonly Dictionary<Guid, LanguageInformation> _languageInformationByLanguageGuid = [];
private readonly ConcurrentDictionary<Guid, LanguageInformation> _languageInformationByLanguageGuid = [];

/// <summary>
/// <see cref="WorkspaceRegistration"/> instances for all open buffers being tracked by by this object
Expand All @@ -50,8 +49,6 @@ internal sealed partial class MiscellaneousFilesWorkspace : Workspace, IOpenText

private readonly ImmutableArray<MetadataReference> _metadataReferences;

private IVsTextManager? _textManager;

[ImportingConstructor]
[Obsolete(MefConstruction.ImportingConstructorMessage, error: true)]
public MiscellaneousFilesWorkspace(
Expand All @@ -72,11 +69,6 @@ public MiscellaneousFilesWorkspace(
_openTextBufferProvider.AddListener(this);
}

public async Task InitializeAsync()
{
_textManager = await _textManagerService.GetValueAsync().ConfigureAwait(false);
}

void IOpenTextBufferEventListener.OnOpenDocument(string moniker, ITextBuffer textBuffer, IVsHierarchy? _) => TrackOpenedDocument(moniker, textBuffer);

void IOpenTextBufferEventListener.OnCloseDocument(string moniker) => TryUntrackClosingDocument(moniker);
Expand Down Expand Up @@ -115,9 +107,12 @@ public void RegisterLanguage(Guid languageGuid, string languageName, string scri

private LanguageInformation? TryGetLanguageInformation(string filename)
{
_threadingContext.ThrowIfNotOnUIThread();

LanguageInformation? languageInformation = null;

if (_textManager != null && ErrorHandler.Succeeded(_textManager.MapFilenameToLanguageSID(filename, out var fileLanguageGuid)))
var textManager = _threadingContext.JoinableTaskFactory.Run(() => _textManagerService.GetValueOrNullAsync(CancellationToken.None));
if (textManager != null && ErrorHandler.Succeeded(textManager.MapFilenameToLanguageSID(filename, out var fileLanguageGuid)))
{
_languageInformationByLanguageGuid.TryGetValue(fileLanguageGuid, out languageInformation);
}
Expand Down
7 changes: 0 additions & 7 deletions src/VisualStudio/Core/Def/RoslynPackage.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,13 +183,6 @@ private Task PackageInitializationBackgroundThreadAsync(IProgress<ServiceProgres
serviceBrokerContainer.Proffer(
ManagedHotReloadLanguageServiceDescriptor.Descriptor,
(_, _, _, _) => ValueTaskFactory.FromResult<object?>(new ManagedEditAndContinueLanguageServiceBridge(this.ComponentModel.GetService<EditAndContinueLanguageService>())));

packageRegistrationTasks.AddTask(
isMainThreadTask: false,
task: async (progress, packageRegistrationTasks, cancellationToken) =>
{
await miscellaneousFilesWorkspace.InitializeAsync().ConfigureAwait(false);
});
});

return Task.CompletedTask;
Expand Down
Loading