Skip to content

Swithc to acquiring the component model on a BG thread. #58351

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
Dec 15, 2021
Merged

Swithc to acquiring the component model on a BG thread. #58351

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
10 changes: 5 additions & 5 deletions src/VisualStudio/Core/Def/Implementation/LanguageService/AbstractPackage`2.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
using Microsoft.VisualStudio.Shell;
using Microsoft.VisualStudio.Shell.Interop;
using Microsoft.VisualStudio.Threading;
using Roslyn.Utilities;
using Task = System.Threading.Tasks.Task;

namespace Microsoft.VisualStudio.LanguageServices.Implementation.LanguageService
Expand All @@ -41,6 +42,9 @@ protected override async Task InitializeAsync(CancellationToken cancellationToke
{
await base.InitializeAsync(cancellationToken, progress).ConfigureAwait(true);

await TaskScheduler.Default;
_componentModel_doNotAccessDirectly = await this.GetServiceAsync<SComponentModel, IComponentModel>(throwOnFailure: true).ConfigureAwait(false);
Copy link
Member Author

@CyrusNajmabadi CyrusNajmabadi Dec 15, 2021

Choose a reason for hiding this comment

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

@jasonmalinowski do you see any issue doing things this way?

Copy link
Member Author

@CyrusNajmabadi CyrusNajmabadi Dec 15, 2021

Choose a reason for hiding this comment

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

@sharwell is the switch to the the BG necessary/desirable here? or would we just acquire from our async thread, adn allow that impl to be on the BG if it wanted to do that itself?

Copy link
Member

@jasonmalinowski jasonmalinowski Dec 15, 2021

Choose a reason for hiding this comment

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

@CyrusNajmabadi Functionally it should be fine. My only "concern" would be whether we're able to instead do the async fetch closer to the actual thing that needs it. The downside generally putting things into package load is you're making everything that uses the package wait until this runs, even if the operation then didn't use it. Package load can be triggered for all sorts of random things, some of which may not use MEF and those are being slowed down. At some point we'll get asked why our package load is slow, we'll see this, and then get to sort out if we can remove this and push the work somewhere closer to its actual need.

Now getting off the soapbox, I admit it's not entirely clear to me what the "ideal" approach would be in this case. The editor factory needs the MEF container to actually create editor types. But we have to call RegisterEditorFactory and pass a concrete IVsEditorFactory instance (we can't pass an async lambda to produce it once it's needed), and the EditorFactory APIs (even IVsEditorFactory4) has no async entrypoints. So I'm happy to sign off on this, but would love it if @olegtk or somebody is able to chime in and explain how we're supposed to do this.

Copy link
Member

@jasonmalinowski jasonmalinowski Dec 15, 2021

Choose a reason for hiding this comment

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

Actually there might be one small alternative you could try would be to make that property be an async method and then thread through everything, but I suspect that'll touch a lot more than you want. But it'll make it much clearer what stuff is potentially being slowed down by fetching MEF.

Copy link
Member Author

@CyrusNajmabadi CyrusNajmabadi Dec 15, 2021

Choose a reason for hiding this comment

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

i will try that and see if it's managable.

Copy link
Member Author

@CyrusNajmabadi CyrusNajmabadi Dec 15, 2021

Choose a reason for hiding this comment

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

It was definitely not managable. So many sync paths that would run into this.

Copy link
Member Author

@CyrusNajmabadi CyrusNajmabadi Dec 15, 2021

Choose a reason for hiding this comment

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

@CyrusNajmabadi Functionally it should be fine. My only "concern" would be whether we're able to instead do the async fetch closer to the actual thing that needs it. The downside generally putting things into package load is you're making everything that uses the package wait until this runs, even if the operation then didn't use it.

So here's the thing. InitializeAsync already directly needs the component model later due to:

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

And one of the things we're solving is that the CreateEditorFactories call needs this as well:

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

            await TaskScheduler.Default;
            _componentModel_doNotAccessDirectly = await this.GetServiceAsync<SComponentModel, IComponentModel>(throwOnFailure: true).ConfigureAwait(false);

            await JoinableTaskFactory.SwitchToMainThreadAsync(cancellationToken);

            var shell = (IVsShell7)await GetServiceAsync(typeof(SVsShell)).ConfigureAwait(true);
            var solution = (IVsSolution)await GetServiceAsync(typeof(SVsSolution)).ConfigureAwait(true);
            cancellationToken.ThrowIfCancellationRequested();
            Assumes.Present(shell);
            Assumes.Present(solution);

            foreach (var editorFactory in CreateEditorFactories())

So all we're doing is ensuring we can get this component model in an non-blocking fashion prior to it being needed, vs having to immediately block the UI thread just a few instructions later.

Copy link
Contributor

@sharwell sharwell Dec 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@sharwell is the switch to the the BG necessary/desirable here? or would we just acquire from our async thread, adn allow that impl to be on the BG if it wanted to do that itself?

The explicit switch to background thread is not desirable. GetServiceAsync will yield on its own if the requested value is not available. The expected pattern would be:

  1. Move this line after the explicit switch to main thread
  2. Use ConfigureAwait(true)

The end result will match what you see for IVsShell7 and IVsSolution a few lines below.

Copy link
Member Author

@CyrusNajmabadi CyrusNajmabadi Dec 17, 2021

Choose a reason for hiding this comment

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

will do. on it now.


await JoinableTaskFactory.SwitchToMainThreadAsync(cancellationToken);

var shell = (IVsShell7)await GetServiceAsync(typeof(SVsShell)).ConfigureAwait(true);
Expand Down Expand Up @@ -106,11 +110,7 @@ internal IComponentModel ComponentModel
{
get
{
ThreadHelper.ThrowIfNotOnUIThread();

if (_componentModel_doNotAccessDirectly == null)
_componentModel_doNotAccessDirectly = (IComponentModel)GetService(typeof(SComponentModel));

Contract.ThrowIfNull(_componentModel_doNotAccessDirectly);
return _componentModel_doNotAccessDirectly;
}
}
Expand Down