Further clean up Roslyn's package load#80598
Conversation
| base.RegisterInitializeAsyncWork(packageInitializationTasks); | ||
|
|
||
| packageInitializationTasks.AddTask(isMainThreadTask: false, task: PackageInitializationBackgroundThreadAsync); | ||
| packageInitializationTasks.AddTask(isMainThreadTask: true, task: PackageInitializationMainThreadAsync); |
There was a problem hiding this comment.
Woohoo! Nothing needed here anymore.
| base.RegisterOnAfterPackageLoadedAsyncWork(afterPackageLoadedTasks); | ||
|
|
||
| afterPackageLoadedTasks.AddTask(isMainThreadTask: false, task: OnAfterPackageLoadedBackgroundThreadAsync); | ||
| afterPackageLoadedTasks.AddTask(isMainThreadTask: true, task: OnAfterPackageLoadedMainThreadAsync); |
There was a problem hiding this comment.
This will be a follow-up to switch this to a background thread, since LoadComponentsInUIContextOnceSolutionFullyLoadedAsync is just waiting for a UIContext to be active before doing more background loading.
| var registerEditors = await GetServiceAsync<SVsRegisterEditors, IVsRegisterEditors>(throwOnFailure: true, cancellationToken).ConfigureAwait(true); | ||
| Assumes.Present(registerEditors); | ||
|
|
||
| ErrorHandler.ThrowOnFailure(registerEditors.RegisterEditor(editorFactory.GetType().GUID, editorFactory, out _)); |
There was a problem hiding this comment.
Looks like this was made threadsafe a couple months ago, so this is nice!
Not related to this PR, but when looking this up, the first usage of RegisterEditor I found was still doing a main thread switch before calling RegisterEditor in VSCorePackage.InitializeAsync, which is a core package load sequence so I would have thought DavKean would have ensured it was changed already. Which makes me question whether I understand whether this is thread safe.
I guess I'm also a little confused why this RegisterEditorFactoryAsync isn't defined in the Shell's Package class. Maybe the additional public surface area wasn't worth it for getting the IVsRegisterEditors via GetServiceAsync?
There was a problem hiding this comment.
Yes, there really should be a method we can call here. FYI to @davkean for the other questions.
|
Running an internal validation build, this is breaking the product because this change assumes that CreateEditorFactories() is safe to call off the UI thread. TypeScript's is not and has a direct assertion to the contrary. Moving this to draft before somebody tries to merge this. |
|
The TypeScript team is going to fix their editor factories to allow them to be created off the UI thread; once that's merged we'll go ahead with this change again. |
jasonmalinowski
force-pushed
the
clean-up-package-load
branch
from
7365c0f to
9fdfa47
Compare
|
TypeScript has inserted their changes and an internal validation run is clean; so this is unblocked. I'll still need to respond to the current code review feedback before marking this as ready for review. |
The implementation now imports all the persisters lazily, so this won't even have any benefit.
This can be done off the UI thread now.
This is free-threaded compared to asking IVsShell.
This isn't needed anymore since nothing on the main thread is still consuming the ComponentModel.
jasonmalinowski
force-pushed
the
clean-up-package-load
branch
from
9fdfa47 to
1b2c755
Compare
|
This is now ready for review, and the draft feedback is responded to. |
4 participants
This simplifies our package initialization further, and moves more stuff off the UI thread when free-threaded alternatives exist. This PR is still a stepping stone to the ultimate goal which is we get rid of our PackageLoadTasks infrastructure since at this point there's very little left that needs it. But that will be a separate PR after this one.
Commit-at-a-time recommended for the individual changes.