Update MarkdownPreview to use Daytona.

[jgonz120]

Bug

Fixes: https://github.com/NuGet/Client.Engineering/issues/3109
Fixes: https://github.com/NuGet/Client.Engineering/issues/3104

Description

Update the Microsoft.VisualStudio.Markdown.Platform package to version 17.13.161. Set editor options in the PreviewBuilder to so MarkdownPreview renders content in Daytona.

PR Checklist

• Meaningful title, helpful description and a linked NuGet/Home issue
• Added tests
• Link to an issue or pull request to update docs if this PR changes settings, environment variables, new feature, etc.

[jeffkl]

Can't wait to see it in action!

[davkean]

The lines that pull components from MEF will increase the number of times this method hangs as it will force types and assemblies to be faulted in on the UI thread. It is already is hanging, this will increase those hangs.

Retrieve the INuGetFeatureFlagService and IEditorOptionsFactoryService off the UI thread;

await TaskScheduler.Default;

var nuGetFeatureFlagService = await ServiceLocator.GetComponentModelServiceAsync<INuGetFeatureFlagService>();
....

var editorOptionsFactoryService = await ServiceLocator.GetComponentModelServiceAsync<IEditorOptionsFactoryService>();

await SwitchToMainThreadAsync(cancellationToken);

Note you need a cancellation token triggered by the UI from the Dispose, otherwise the comment I said yesterday will apply; it will crash VS on resume if the UI has been closed.

[jgonz120]

I used the token from NuGetPackage. This function is called before the window is created so if I close the IDE while waiting for the service locators to run the dispose isn't called.

[davkean]

You have to worry about the reverse; what happens if you close the UI before this method finishes? it should be easy to test; add a Task.Delay(15000) within here and close the UI without closing the IDE, what happens? I see it touch UI when it resumes on the UI thread, so I don't see how it doesn't crash/fail. You should be combining the tokens at least.

[jgonz120]

Which UI? The pm UI doesn't open until after this initialization completes.

[davkean]

There is UI, this is a method on PackageManagerControl. You are saying, it's not visible at this point?

So walk me through this, let's say the MEF cache is out of date and needs to be rebuilt. This takes at minimum of 15 seconds in a very fast machine in the lab, probably upwards of a minute on slower machines. You are saying the user goes to open the Package Manager and:

1. They are confused as nothing appears to happen while that cache is being rebuilt in the background.

-or-

2. They decide not to wait and instead close VS (let's say they need to update Setup), the package now blocks on that cache rebuild, VS UI disappears but this stops VS process from exiting, and setup complains that VS is still open?

These aren't hypertheticals, I constantly see 2) in VS hang dumps. This should be super easy to test; simply add a Task.Delay here for 60 seconds, and do above, what happens does the VS process exit (watch Task Manager) straight away?

[jgonz120]

Well, I moved these to avoid the one-time initializations in the component loaded event. I thought it would make sense to put these all in the same place so when we need to refactor, we don't need to search the code for the different locations.

So, is it the case that each MEF load can add an additional 15-60s? There are 5 MEF objects being loaded in this area, 2 I added, so is it the case that we could be hanging up to 5 minutes because of these loads? If that's the case Is 5 minutes significantly worse than 3? At this point at least the UI would be responsive since I've moved the load off of the UI thread.

I'm happy to move these loads to another location but I would need one to occur when the control that loads the tab is being loaded, which does not have a loading icon, because it needs the INuGetFeatureFlagService to determine if the README tab is even available.

So would this mean I should put these loads in the constructor of both of these user controls?

[davkean]

The first person to retrieve IComponentModel pays the cost of the load. The retrieval of services after that is still costly and we want off the UI thread, but will be faster than that.

1. The first retrieval of IComponentModel costs at least between 400 - 520ms when the cache is up-to-date in the lab:

2. When it is out of date (~5-10% of sessions right now), it can be many factors longer than that. While this out of dateness isn't always a rebuild, full rebuilds do occur and it takes 15 seconds in the lab:

The lab machines are very fast machines, on slower machines or when the machine is busy, it will be much longer than this. The correct approach for this async "package manager window" is use an async tool window so that users know that something is loading, but failing that you can do after the UI has shown.

[zivkan]

I just remembered that NuGet has a second AsyncPackage, RestoreManagerPackage, that autoloads when any solution is open, so that it can handle packages.config (and legacy csproj PackageReference) restore on build events. It uses MEF.

So, if the 15 second delay is caused by the first thing that uses MEF, then MEF will already be loaded by the time the customer tries to open PM UI.

The dotnet/project-system (or CPS?) communicates with NuGet by importing a type that NuGet exports. So SDK style project loads also need MEF to be loaded at solution open.

That means the only way this PR makes perf worse is by waiting for the markdown control to be loaded before displaying PM UI. But the 15 second is an impossible hypothetical unless there are significant rewrites of other parts of NuGet (and other components that need NuGet?) first. As such, I think it's way out of scope for this work. But it's good to increase our team's knowledge of ways that NuGet's implementation has design flaws that would be nice to improve when we have time.

[davkean]

It doesn't matter that there's another unrelated NuGet package that loads MEF, unless its waited on or blocked on by this path. What matters is the registered package and the path that causes this tool window be instantiated, it's the only thing guaranteed to have finished loading before the tool window is opened. That appears to be this package, but TBH, it was a little hard to follow as NuGet instantiates this window very differently to other folks around the VS tree.

I would not assume this path is fast, I can see, just on the UI thread (doesn't include time spent on background thread) along this path had about 13,000 UI freezes over 1 second in the past 21 days. I'll let you folks figure this one out, but I would not block on MEF on any path on the UI thread, or give no indication anything is happening while you wait for it.

[jgonz120]

Is this in scope for this PR though? There are 3 MEF components already in this method, 1 from a downstream method that I moved here so it can be done off of the UI path. At this point I've added 1 additional MEF component load, is there a significant impact to performance by adding that additional MEF component?

[davkean]

@nkolev92 It seems this extension method that is attached to JoinableTask is forcing folks to RunAsync for fire and forget methods when it does nothing but overhead to these paths. That does not seem correct, see #6190 (comment).

[davkean]

Does CreateDocWindowForSolutionAsync end up waiting on this right?

Re-reading how this code works trying to understand it for the MEF conversation, I think we might have introduced a race here; previously CreateDocWindowForSolutionAsync assumed that FindExistingSolutionWindowFrameAsync had already checked if the window was open but that check is only relevant while you are on the UI thread (someone can open the window behind your back when you get off UI thread).

I presume previously despite being marked as async, any awaits on this path weren't really yielded and stayed on the UI thread, effectively making it a synchronous path.

Now, we're actually hitting real "yields" here (TaskScheduler.Default, any yields in service retrieval, etc) there's a race; if you invoke the command twice in succession very quickly, this path I think will be hit twice and the window will get opened twice or break state (I've not confirmed or looked at it).

You can simulate what this be like on slow machines or high thread pool latency by putting a Task.Delay(15000) here, and then invoke the method command twice in a row; what happens?

Other VS tool windows don't have this problem, because they say on the UI thread between the check and the creation of the window, or use the built-in async tool window logic for asynchronously loading the tool window.