Migrate to the editor 15.6.241 #23943
Conversation
| @@ -124,7 +125,7 @@ public static PartDiscovery CreatePartDiscovery(Resolver resolver) | |||
|
|
|||
| public static ExportProvider CreateExportProvider(ComposableCatalog catalog) | |||
| { | |||
| var configuration = CompositionConfiguration.Create(catalog.WithDesktopSupport().WithCompositionService()); | |||
There was a problem hiding this comment.
WithDesktopSupport() is deprecated, see https://github.com/Microsoft/vs-mef/blob/v15.5/src/Microsoft.VisualStudio.Composition/Configuration/NetFxAdapters.cs#L59
|
Is master-vs-deps the correct branch for this kind of change anyway, @jasonmalinowski ? |
Yes. Updates that ship in the latest stable release (15.5.x) can be merged into master, but updates coming in the next stable release go into master-vs-deps. |
| @@ -56,6 +56,8 @@ | |||
| <PackageReference Include="Microsoft.VisualStudio.VsInteractiveWindow" Version="$(MicrosoftVisualStudioVsInteractiveWindowVersion)" /> | |||
| <PackageReference Include="Microsoft.VisualStudio.ComponentModelHost" Version="$(MicrosoftVisualStudioComponentModelHostVersion)" /> | |||
| <PackageReference Include="System.Collections.Immutable" Version="$(SystemCollectionsImmutableVersion)" /> | |||
| <!-- Required to workaround double write of System.Threading.Tasks.Dataflow --> | |||
There was a problem hiding this comment.
What bug is this working around? It's not obvious from the comment. Is there a bug that can be linked to?
There was a problem hiding this comment.
Some questions and a request for full bug URLs.
More importantly: can you confirm that the editor APIs this is referencing have been inserted into d15.6stg? We cannot take this PR until they have been. (Even better, I'd like to know how to fish to confirm this myself.)
| @@ -85,6 +84,7 @@ | |||
| <PackageReference Include="Microsoft.VisualStudio.Validation" Version="$(MicrosoftVisualStudioValidationVersion)" /> | |||
| <PackageReference Include="Microsoft.VisualStudio.Threading" Version="$(MicrosoftVisualStudioThreadingVersion)" /> | |||
| <PackageReference Include="StreamJsonRpc" Version="$(StreamJsonRpcVersion)" /> | |||
| <PackageReference Include="System.Threading.Tasks.Dataflow" Version="$(SystemThreadingTasksDataflowVersion)" /> | |||
There was a problem hiding this comment.
What package is actually shipping this binary? Does VS ship it and we've been relying on that copy? I didn't think it was part of the framework.
There was a problem hiding this comment.
yes, it's shipping with VS as version 4.5.24. You were relying on it indirectly by referencing Microsoft.Tpl.Dataflow package, which contains System.Threading.Tasks.Dataflow.dll.
There was a problem hiding this comment.
@olegtk I guess my question is "somebody other than us is responsible for making sure it gets on the user's machine?"
| { | ||
| // In 15.6 the editor (QuickInfo in particular) took a dependency on | ||
| // IObscuringTipManager, which is only exported in VS editor layer. | ||
| // This is tracked by the editor bug #544569. |
There was a problem hiding this comment.
Full URL to bug please. Is this being fixed in a newer version of the packages?
There was a problem hiding this comment.
Bug is currently marked to be fixed in 15.8: https://devdiv.visualstudio.com/DevDiv/_workitems?id=544569
| // to be able to compose in any part of the Editor. | ||
| // Starting with 15.3 the editor took a dependency on JoinableTaskContext | ||
| // in Text.Logic and Intellisense layers as an editor host provided service. | ||
| [PartCreationPolicy(CreationPolicy.NonShared)] // JTC is "main thread" affinitized so should not be shared |
There was a problem hiding this comment.
The changes to this file seem somewhat fishy to me:
- You're no longer using the CurrentForegroundThreadData which means this type doesn't need to inherit from ForegroundThreadAffinitizedObject anymore.
- Because of the previous point, there's no real reason to create an instance of this type at all...which seems that the creation policy doesn't matter.
- I don't see why this would be ThreadStatic, as we really shouldn't have tests that care about this then other than the ones that use our WpfFact to run on a single thread.
- The part creation policy should only matter in a single composition, which wouldn't have an impact on if it's being used across threads.
Maybe we have to just chat in person on this one, but what's going on here? This seems very magic and there's a lot of context missing.
There was a problem hiding this comment.
let me look at it again...
There was a problem hiding this comment.
it does look fishy, I wish I knew a better way.
Essentially what I found:
- Without non-shared part creation policy most most tests (specifically I believe non WpfFact ones) creating text view fail because new QuickInfo text view creation listener throws for not being on the main thread. I think that's because JTC instances are cached by MinimalTestExportProvider between compositions and so being reused on other threads.
- Without [ThreadStatic] tests deadlock, presumably because we end up creating more than one JTC for same thread.
This needs further investigation, I just don't have enough context in the text infrastructure. Maybe it would be better not to export JTC here, but explicitly add new instance to each composition?
I suggest creating an issue to track it, but not block this PR. I don't mind spending more time on this.
@gundermanc Do you have any tests that compose like we do and sees if things worked? Would be pretty simple to write if not. |
|
@jasonmalinowski, commenting here since GitHub doesn't seem to let me respond to your comment on my feedback: We do have integration tests that ensure that the editor composes but none compose just the editor without the Microsoft.VisualStudio.Editor.Implementation assembly (the VS specific portion of the editor). We probably should add some tests and/or a nuget for hosting the editor in unit test scenarios like yours. Also, the quick info and pattern matcher changes are both in d15.6stg, however, I can't comment on whether or not this particular build of the Nugets has additional APIs that aren't in yet. @olegtk ? |
| <MicrosoftVisualStudioCompositionVersion>15.0.71</MicrosoftVisualStudioCompositionVersion> | ||
| <MicrosoftVisualStudioCoreUtilityVersion>15.6.161-preview</MicrosoftVisualStudioCoreUtilityVersion> | ||
| <MicrosoftVisualStudioCompositionVersion>15.5.23</MicrosoftVisualStudioCompositionVersion> | ||
| <MicrosoftVisualStudioCoreUtilityVersion>15.6.241-preview</MicrosoftVisualStudioCoreUtilityVersion> |
There was a problem hiding this comment.
These bits are already in d15.6stg (actually it has 15.6.245-preview already), see https://devdiv.visualstudio.com/DevDiv/Editor/_git/VS?path=%2F.corext%2FConfigs%2Fdefault.config&version=GBlab%2Fd15.6stg&line=301&lineStyle=plain&lineEnd=302&lineStartColumn=1&lineEndColumn=1
| @@ -56,6 +56,11 @@ | |||
| <PackageReference Include="Microsoft.VisualStudio.VsInteractiveWindow" Version="$(MicrosoftVisualStudioVsInteractiveWindowVersion)" /> | |||
| <PackageReference Include="Microsoft.VisualStudio.ComponentModelHost" Version="$(MicrosoftVisualStudioComponentModelHostVersion)" /> | |||
| <PackageReference Include="System.Collections.Immutable" Version="$(SystemCollectionsImmutableVersion)" /> | |||
| <!-- | |||
| Required to avoid double write of System.Threading.Tasks.Dataflow.dll caused by both | |||
| Microsoft.VisualStudio.ProjectSystem and Microsoft.VisualStudio.Composition containing it. | |||
There was a problem hiding this comment.
Is there a bug tracking this we can reference here?
There was a problem hiding this comment.
Who should get the bug though? @AArnott agreed Microsoft.VisualStudio.ProjectSystem should move to System.Threading.Tasks.Dataflow.dll instead of obsolete Microsoft.Tpl.Dataflow. I can file such bug against Microsoft.VisualStudio.ProjectSystem.
Also Nuget has related, but Won't fixed bug NuGet/Home#329
There was a problem hiding this comment.
Filed https://devdiv.visualstudio.com/DevDiv/_workitems/edit/547065 against CPS to move from Microsoft.Tpl.Dataflow. I will reference it here.
| { | ||
| [ThreadStatic] | ||
| private static JoinableTaskContext s_jtcThreadStatic; |
There was a problem hiding this comment.
It looks like you're solving the problem twice over: you're both using NonShared and you're using ThreadStatic here.
It seems ThreadStatic by itself should fix this, shouldn't it?
There was a problem hiding this comment.
Nope, just ThreadStatic is not enough, tons of tests fail because JTC.IsOnMainThread check fails.
@AArnott, what's the creation semantics of an exported property? I believe if the class is a shared part it's instantiated only once and its property is accessed only once too so we have a singleton JTC per catalog.
Roslyn tests reuse ExportProvider containing this JTC export so when a test runs on a different thread JTC.IsOnMainThread return false. That's why I thought just having CreationPolicy.NonShared would solve it. But for some reason that alone deadlocks (\\olegt-vmhost\tmp\xunit.runner.worker.dmp) and I don't fully understand why yet. I suspect we end up creating more than one JTC targeting the same thread as main they get stored in various editor components in that static ExportProvider. That's why ThreadStatic.
There was a problem hiding this comment.
what's the creation semantics of an exported property?
An exported property's getter is called for each and every importer. Basically the same thing as an exported type when it's tagged as a NonShared part.
My first guess about the deadlock is that different importers are getting unique instances of JTC even within the same test and MEF graph, which means they can't interact to resolve deadlocks. If you're all on the same (main) thread, this probably wouldn't actually be an issue. But if you actually have multiple threads going for an individual test, and you care about the simulated 'main' thread, then you need to ensure that you have one JTC instance shared for the whole test and you need a way for a non-main thread in the test to schedule work for the 'main' thread. This would require that you set up a SynchronizationContext on the 'main' thread that can schedule work, and then see that that thread actually pumps those messages. It's not hard, but it must be done within your test project.
Roslyn tests reuse ExportProvider
That's unfortunate. ExportProvider itself is actually really cheap. It's composing the graph that's expensive. I wonder if they've tried just constructing the catalog and graph once, and sharing the resulting ExportProviderFactory for all tests, but each individual test gets its own ExportProvider? That sounds like it would be the best combination of perf and test isolation.
But if you must share the ExportProvider, then we're basically saying that all tests need to share a JTC and thus share a 'main' thread. So there should be a static thread that's running a message pump for all tests to occasionally switch to during the test.
There was a problem hiding this comment.
The way I solved this in VsVim is to do the following:
- Define a new ExportProvider for JoinableTaskFactory https://github.com/jaredpar/VsVim/blob/master/Src/VimEditorHost/EditorHostFactory.JoinableTaskContextExportProvider.cs
- Add that to the ExportProviderList of the composition https://github.com/jaredpar/VsVim/blob/master/Src/VimEditorHost/EditorHostFactory.cs#L80
Given that all tests run on the main STA thread htis means the provider, and the resulting JoinableTaskContext, will be created on the main thread.
There was a problem hiding this comment.
Oh, I haven't realized the export was on a field, not a property. After turning it into a property I could remove part creation policy altogether, just leaving ThreadStatic as you suggested.
@jasonmalinowski, does the last iteration look reasonable enough to proceed with this PR?
There was a problem hiding this comment.
thanks, @jaredpar. I'm trying to follow that approach. Roslyn needs VS MEF ExportProvider though, so I'm trying to figure out how to create VS MEF Export provider for JTC and how to combine 2 VS MEF export providers.
I then again I suggest we don't block this PR on this and follow up with a separate issue.
There was a problem hiding this comment.
I guess it's not possible with VS MEF and switching to MEF v1 ExportProvider (like in VsVim) is not possible due to portable requirements. So the approach in this PR is the only realistic one I can think of.
@jasonmalinowski please review if I addressed the changes you requested in this PR.
There was a problem hiding this comment.
I wonder if they've tried just constructing the catalog and graph once, and sharing the resulting ExportProviderFactory for all tests, but each individual test gets its own ExportProvider? That sounds like it would be the best combination of perf and test isolation.
Roslyn has switched back and forth between sharing the actual ExportProvider and just sharing the catalog around 5 times now. It alternated between "oh goodness, the perf!" and "oh goodness, we're sharing test state!" that it made me wonder if everybody was suffering from memory loss. 😄 I want to say last time the switch was done was prior to us moving to VS MEF (and might predate VS MEF itself....?), and at the time creating a new composition while reusing the catalog was cheap, but once you have 50,000 tests doing that and it's more expensive than the five lines of actual code being tested, you start to get second thoughts. 😄
@olegtk I'm OK with this going in. I suspect it's still going to bite us eventually but I think that trap has already been set...
Awhile back I tried giving a shot at decoupling all the MEF shared state to also be strict about tests that are free-threaded (and can run in parallel) and tests that need a concept of a UI thread and it was a terrible rat hole. We had some tests that'd casually spin up a WPF text view on a random xunit test thread and weren't careful about that, or other sorts of fun. One Day™ I want to get it cleaned up, but you aren't making it any worse.
There was a problem hiding this comment.
@jaredpar I'd like to understand why you had to derive types for ExportProvider and/or JoinableTaskContext. You can 'inject' a JoinableTaskContext into a VS MEF composition using an exporting field/property and avoid any type deriving.
| @@ -80,6 +79,7 @@ | |||
| <PackageReference Include="Microsoft.VisualStudio.Text.UI" Version="$(MicrosoftVisualStudioTextUIVersion)" /> | |||
| <PackageReference Include="Microsoft.VisualStudio.Text.UI.Wpf" Version="$(MicrosoftVisualStudioTextUIWpfVersion)" /> | |||
| <PackageReference Include="Microsoft.VisualStudio.Validation" Version="$(MicrosoftVisualStudioValidationVersion)" /> | |||
| <PackageReference Include="System.Threading.Tasks.Dataflow" Version="$(SystemThreadingTasksDataflowVersion)" /> | |||
There was a problem hiding this comment.
Nit: if this is implied by Microsoft.VisualStudio.Composition and everything works by deleting this line, I'm OK with not stating it. Our projects today over-specify a lot of package versions which makes things get annoying sometimes.
|
Tagging @Pilchie for ask mode approval. Not sure if necessary or if the ask mode template makes sense here. This is @olegtk moving us forward to newer editor packages that are already in d15.6stg, so we can unblock people trying to move us onto any of the new APIs they're working on. That work is later, and possibly post-15.6. |
Migrate to the latest editor bits v15.6.241 (containing Async QuickInfo and NavigateTo PatternMatch support), also to clear up the way for migration to the new editor commanding.