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

jcansdale · 2018-03-22T19:16:48Z

This workaround causes the Scanning new and updated MEF components... dialog to appear. Without this the GitHub pane will be blamed for degrading startup performance.

What this PR does

It provokes MEF to initialize itself using an old style non-async package. This will happen before any tool windows are activated in the GitSccProvider auto-load context.

All the InitializeMefPackage package does is call GetService(typeof(SComponentModel)) when it initializes. Visual Studio knows how to handle synchronous initialization of MEF and pops up the Scanning new and updated MEF components... dialog.

The InitializeMefPackage package will only cause the GitHub.VisualStudio assembly to load, and uses an auto-load context we already use in GitContextPackage. It shouldn't degrade performance beyond the initial MEF initialization hit immediately after install.

How to test

Open Visual Studio 2017
Ensure the GitHub pane is visible
Click on Tools > Clear MEF Component Cache...
Allow Visual Studio to restart
If the fix is working you will see:
If not would would see:

NOTE: Visual Studio shows this banner based on an average startup time. You might need to do this a few times to being the average startup time back down. 😢 It should then consistently not appear.

Notes

I tried more sophisticated approaches but they didn't work nearly as well (or at all). For example:

[wip] Allow MEF to refresh its cache on a background thread: [wip] Allow MEF to refresh its cache on a background thread #1554

When MEF is refreshing on a background thread, no Scanning new and updated MEF components... dialog appears. There were other issues that I wasn't able to resolve.

We could probably resolve this issue by converting the GitHub pane into an async initializing tool window. Unfortunately this is only supported on recent versions of Visual Studio 2017. We would need to support two code paths and deal with separate Visual Studio 2017 assemblies.

Tested both Visual Studio versions with the same result

👍 ✌️ "Because we can, can, can... Yes we can, can, can, can..."

jcansdale · 2018-03-23T20:32:31Z

@bchavez Thanks for testing it. I'm relieved to hear it's working! 👍

Pre-initializing MEF is only required by GitHubPane.

AArnott · 2018-03-26T14:29:58Z

I'm surprised that this actually avoids the warning infobar. It seems like it would just shift it to a different package, but still from your VSIX. @BertanAygun does this make sense to you?

AArnott · 2018-03-26T14:33:35Z

src/GitHub.VisualStudio/GitHubPanePackage.cs

+        /// </summary>
+        protected override void Initialize()
+        {
+            GetService(typeof(SComponentModel));


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.

BertanAygun · 2018-03-26T16:06:29Z

src/GitHub.VisualStudio/GitHubPanePackage.cs

+    // 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).


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.

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.

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?

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)?

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

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! 😄

BertanAygun · 2018-03-26T16:08:27Z

src/GitHub.VisualStudio/GitHubPanePackage.cs

+using Microsoft.VisualStudio.Shell;
+using Microsoft.VisualStudio.ComponentModelHost;
+
+namespace GitHub.VisualStudio


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.

BertanAygun · 2018-03-26T16:11:20Z

As @AArnott stated this would not solve any performance issues and just move cost between packages but since both packages are from same extension. If you run Visual Studio with /log switch, the activity log should output inclusive cost of each package and tool window to help with diagnosis.

jcansdale · 2018-03-26T19:51:59Z

@BertanAygun, @AArnott,

If you run Visual Studio with /log switch, the activity log should output inclusive cost of each package and tool window to help with diagnosis.

I've just tried that with this build. It appears that by using [ProvideAutoLoad(Guids.GitSccProviderId)], I'm passing the blame for GetService(typeof(SComponentModel)) on to the Visual Studio Source Control Integration Package.

I believe this is what we were inadvertently doing in the past and why we never triggered the startup performance warning until we moved to an AsyncPackage.

The thing is, we do want to trigger the following dialog so users know what is going on:

Is there any other way to ensure that this dialog gets displayed short of calling GetService(typeof(SComponentModel)) from an auto-loading, non-async Package?

BertanAygun · 2018-03-26T20:54:05Z

@jcansdale, I would really recommend avoiding initializing MEF synchronously. Andrew has already good recommendations to be able to do that and starting in 15.6 you can use async tool window to make some of those changes easier.

Unfortunately initializing MEF synchronously in startup will make sure all users that have GitHub installed to have really bad startup experience.

jcansdale · 2018-03-28T15:06:11Z

@AArnott, @BertanAygun,

There's an implementation of what you suggested here:

Initialize GitHubPane and menus asynchronously Initialize GitHubPane and menus asynchronously #1569

If you could take a look/review, that would be very much appreciated!

Unfortunately initializing MEF synchronously in startup will make sure all users that have GitHub installed to have really bad startup experience.

Before writing the above PR, I assumed the hit from MEF was only from when it initializes its cache after install. I hadn't appreciated the few second delay the first time the SCompoentModel is requested. I see why you were keep we don't auto-load it! 😉

jcansdale · 2018-04-03T19:48:17Z

Made obsolete by #1569.

jcansdale requested review from shana, grokys and meaghanlewis and removed request for shana March 22, 2018 20:55

This was referenced Mar 22, 2018

[wip] Allow MEF to refresh its cache on a background thread #1554

Closed

Extension likely caused X seconds of unresponsiveness #1516

Open

Give GitHubPane its own Package

7740a89

Pre-initializing MEF is only required by GitHubPane.

AArnott reviewed Mar 26, 2018

View reviewed changes

BertanAygun suggested changes Mar 26, 2018

View reviewed changes

jcansdale mentioned this pull request Mar 28, 2018

Initialize GitHubPane and menus asynchronously #1569

Merged

jcansdale closed this Apr 3, 2018

jcansdale deleted the fixes/1550-initialize-mef branch April 3, 2018 19:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jcansdale commented Mar 22, 2018 •

edited

Loading

bchavez commented Mar 23, 2018 •

edited

Loading

jcansdale commented Mar 23, 2018

AArnott commented Mar 26, 2018

AArnott Mar 26, 2018

BertanAygun Mar 26, 2018

jcansdale Mar 26, 2018

BertanAygun Mar 26, 2018

jcansdale Mar 26, 2018 •

edited

Loading

BertanAygun Mar 26, 2018 •

edited

Loading

jcansdale Mar 26, 2018

BertanAygun Mar 26, 2018

BertanAygun commented Mar 26, 2018

jcansdale commented Mar 26, 2018 •

edited

Loading

BertanAygun commented Mar 26, 2018

jcansdale commented Mar 28, 2018

jcansdale commented Apr 3, 2018

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

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

Conversation

jcansdale commented Mar 22, 2018 • edited Loading

What this PR does

How to test

Notes

Related

bchavez commented Mar 23, 2018 • edited Loading

Tested both Visual Studio versions with the same result

jcansdale commented Mar 23, 2018

AArnott commented Mar 26, 2018

AArnott Mar 26, 2018

Choose a reason for hiding this comment

BertanAygun Mar 26, 2018

Choose a reason for hiding this comment

jcansdale Mar 26, 2018

Choose a reason for hiding this comment

BertanAygun Mar 26, 2018

Choose a reason for hiding this comment

jcansdale Mar 26, 2018 • edited Loading

Choose a reason for hiding this comment

BertanAygun Mar 26, 2018 • edited Loading

Choose a reason for hiding this comment

jcansdale Mar 26, 2018

Choose a reason for hiding this comment

BertanAygun Mar 26, 2018

Choose a reason for hiding this comment

BertanAygun commented Mar 26, 2018

jcansdale commented Mar 26, 2018 • edited Loading

BertanAygun commented Mar 26, 2018

jcansdale commented Mar 28, 2018

jcansdale commented Apr 3, 2018

jcansdale commented Mar 22, 2018 •

edited

Loading

bchavez commented Mar 23, 2018 •

edited

Loading

jcansdale Mar 26, 2018 •

edited

Loading

BertanAygun Mar 26, 2018 •

edited

Loading

jcansdale commented Mar 26, 2018 •

edited

Loading