Skip to content
This repository was archived by the owner on Jun 21, 2023. It is now read-only.

Ensure that MEF is initialized using an old style non-async package #1560

Closed
wants to merge 2 commits into from
Closed

Ensure that MEF is initialized using an old style non-async package #1560

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fcbf4f4
Ensure that MEF is initialized using an old style non-async package
jcansdale Mar 22, 2018
7740a89
Give GitHubPane its own Package
jcansdale Mar 24, 2018
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
1 change: 1 addition & 0 deletions src/GitHub.Exports/Settings/Guids.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ public static class Guids
public const string StartPagePackageId = "3b764d23-faf7-486f-94c7-b3accc44a70e";
public const string CodeContainerProviderId = "6CE146CB-EF57-4F2C-A93F-5BA685317660";
public const string InlineReviewsPackageId = "248325BE-4A2D-4111-B122-E7D59BF73A35";
public const string InitializeMefPackageId = "88d1bb06-1ce3-4d63-9c5f-2c0e8e4bb99d";
public const string PullRequestStatusPackageId = "5121BEC6-1088-4553-8453-0DDC7C8E2238";
public const string TeamExplorerWelcomeMessage = "C529627F-8AA6-4FDB-82EB-4BFB7DB753C3";
public const string LoginManagerId = "7BA2071A-790A-4F95-BE4A-0EEAA5928AAF";
Expand Down
1 change: 1 addition & 0 deletions src/GitHub.VisualStudio/GitHub.VisualStudio.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,7 @@
<Compile Include="Commands\ShowGitHubPaneCommand.cs" />
<Compile Include="Commands\OpenPullRequestsCommand.cs" />
<Compile Include="GitContextPackage.cs" />
<Compile Include="GitHubPanePackage.cs" />
<Compile Include="IServiceProviderPackage.cs" />
<Compile Include="Helpers\ActiveDocumentSnapshot.cs" />
<Compile Include="Commands\AddConnectionCommand.cs" />
Expand Down
1 change: 0 additions & 1 deletion src/GitHub.VisualStudio/GitHubPackage.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ namespace GitHub.VisualStudio
[Guid(Guids.guidGitHubPkgString)]
[ProvideMenuResource("Menus.ctmenu", 1)]
[ProvideAutoLoad(Guids.UIContext_Git, PackageAutoLoadFlags.BackgroundLoad)]
[ProvideToolWindow(typeof(GitHubPane), Orientation = ToolWindowOrientation.Right, Style = VsDockStyle.Tabbed, Window = EnvDTE.Constants.vsWindowKindSolutionExplorer)]
[ProvideOptionPage(typeof(OptionsPage), "GitHub for Visual Studio", "General", 0, 0, supportsAutomation: true)]
public class GitHubPackage : AsyncPackage
{
Expand Down
40 changes: 40 additions & 0 deletions src/GitHub.VisualStudio/GitHubPanePackage.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
using System;
using System.Runtime.InteropServices;
using GitHub.VisualStudio.UI;
using Microsoft.VisualStudio.Shell;
using Microsoft.VisualStudio.ComponentModelHost;

namespace GitHub.VisualStudio
Copy link
Contributor

Choose a reason for hiding this comment

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

I would not approve this change as this doesn't really solve any performance issues. In fact it seems to make things worse as it adds synchronous query of MEF to startup path for those users that have Git provider set as default source control provider.

{
/// <summary>
/// This is the host package for the <see cref="GitHubPane"/> tool window.
/// </summary>
/// <remarks>
/// We auto-load this package before its tool window is activated to ensure that MEF has
/// been initialized and the tool window won't be blamed for degrading startup performance.
/// See: https://github.com/github/VisualStudio/issues/1550
/// </remarks>
[PackageRegistration(UseManagedResourcesOnly = true)]
[Guid(Guids.InitializeMefPackageId)]
// This package must auto-load before its tool window is activated on startup.
// We're using the GitSccProvider UI context because this is triggered before
// tool windows are activated and we already use it elsewhere (the containing
// assembly would have been loaded anyway).
Copy link
Contributor

Choose a reason for hiding this comment

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

VS Shell will already ensure your package is loaded before the tool window is activated so you don't have to do it here.

This auto load ensures that your package is loaded for every startup session regardless of your tool window is open slowing down startup for all users instead of just those with the toolwindow visible.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

VS Shell will already ensure your package is loaded before the tool window is activated so you don't have to do it here.

If we allow that to happen, our package gets blamed for MEF initialization (which can take 10 seconds 😨).

This auto load ensures that your package is loaded for every startup session regardless of your tool window is open slowing down startup for all users instead of just those with the toolwindow visible.

We need to auto-load on this context anyway in order to install a custom UIContext. That's why I wasn't too concerned about loading this assembly.

Copy link
Contributor

Choose a reason for hiding this comment

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

As you pointed, the cost is still there just becomes part of SCC package which is unique in this case since it is the one that causes your auto load unlike other scenarios. At the end of the day startup is still impacted by 10 seconds which is the important part. I have to mention that SCC hiding cost of auto loads from the UI context it sets is a known issue that we will fix soon btw.

As for your auto load reason, would you be able to use a rule based UI context to activate your UI context instead of auto loading a package?

Copy link
Collaborator Author

@jcansdale jcansdale Mar 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I have to mention that SCC hiding cost of auto loads from the UI context it sets is a known issue that we will fix soon btw.

Thanks, this is good to know.

As for your auto load reason, would you be able to use a rule based UI context to activate your UI context instead of auto loading a package?

Our UI context is a subset of the GitSccProvider context (which appears to activate if VS was using Git the last time it launched). Is there a way to implement a custom UI context that doesn't involve loading a package?

Am I correct in thinking that a rule based UI context is a composite of existing UI context (you can't use them to create a completely custom context)?

Copy link
Contributor

@BertanAygun BertanAygun Mar 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

What are the conditions that you set the UI context in? If it is GIT SCC context and presence of your package, you can just define a rule based UI contexts that is only based on GIT SCC context. Since the rule based UI context will only be added when your pkgdef is deployed it will only be active when GIT SCC is active and user has your extension installed.

https://msdn.microsoft.com/en-us/library/mt750411.aspx

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What are the conditions that you set the UI context in?

Here is pseudo code for what we need:
If in GitSccProvider context && IGitExt.ActiveRepositories.Count > 0

We're pretty much always in the GitSccProvider context, because it's active even when there is no loaded solution but the previously loaded solution was in a Git repo. 😕 We therefore check the IGitExt service to find out if we're really in the context of an active repo.

We have made efforts to only load when absolutely necessary! 😄

[ProvideAutoLoad(Guids.GitSccProviderId)]
[ProvideToolWindow(typeof(GitHubPane), Orientation = ToolWindowOrientation.Right,
Style = VsDockStyle.Tabbed, Window = EnvDTE.Constants.vsWindowKindSolutionExplorer)]
public sealed class GitHubPanePackage : Package
{
/// <summary>
/// Ensure that MEF is initialized using an old style non-async package.
/// This causes the `Scanning new and updated MEF components...` dialog to appear.
/// Without this the GitHub pane will be blamed for degrading startup performance.
/// Initialize must be called before its tool window is activated (otherwise the
/// tool window still end up waiting for it).
/// </summary>
protected override void Initialize()
{
GetService(typeof(SComponentModel));
Copy link

Choose a reason for hiding this comment

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

Can we remove this line and fix the tool window to not synchronously block on this service? Your tool window initialization could display something else while asynchronously awaiting MEF's initialization, thus the user experience is that VS starts up much more quickly, even if github's tool window is visible.
The VS 15.6 async tool window feature is basically a temporary control displayed while the window initializes, so you could make that up in your VSIX yourself and thus support it downlevel on VS 2015.

}
}
}