Another small set of package initialization optimizations #77646
Conversation
1) Move RegisterLanguageService call in AbstractPackage<> to be on the background. This method was already thread safe. 2) Move RegisterMiscellaneousFilesWorkspaceInformation call in AbstractPackage<> to be on the background. This required changing the data structure holding these strings to be a concurrent dictionary. 3) Move RegisterObjectBrowserLibraryManager call in AbstractPackge package initialization to OnAfterPackageLoadedAsync 4) Change MiscellaneousFilesWorkspace to not require a bg thread initialize call and instead get the text manager when needed.
ghost
added
Area-IDE
untriaged
| // Ensure we're on the BG when creating the language service. | ||
| await TaskScheduler.Default; |
There was a problem hiding this comment.
We already said isMainThreadTask: false, why saying this a second time? I'd frankly can't explain why that RegisterLanguageServer function exists in the first place, if that was the concern. It has this comment:
// When registering a language service, we need to take its ComAggregate wrapper.
But there's nothing special it's doing with it...
There was a problem hiding this comment.
That's the callback when the service is created, I don't know if we can guarantee what context we are on when that happens.
There was a problem hiding this comment.
Oh right of course. That'd be something we should follow up with -- hopefully we have some way to say we can be created on a background thread -- it'd be silly for the service system to bounce to the UI thread if we didn't need it.
|
|
||
| return _languageService.ComAggregate!; | ||
| }); | ||
|
|
||
| var miscellaneousFilesWorkspace = this.ComponentModel.GetService<MiscellaneousFilesWorkspace>(); |
There was a problem hiding this comment.
Moving this to the background should be safe -- this is not written to have UI thread assumptions in the constructors. See the comments in OpenTextBufferProvider's constructor specifically for ensuring that's safe. This wasn't the case a few years back but this was generally modernized.
If you're wanting to keep the changes small to manage risk better, do this in a follow up PR (but don't forget about it.)
There was a problem hiding this comment.
I originally tried that and it threw. Let me see if I can find out where that dependency is.
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
…see if it makes integration tests happy
Move RegisterLanguageService call in AbstractPackage<> to be on the background. This method was already thread safe.
Move RegisterMiscellaneousFilesWorkspaceInformation call in AbstractPackage<> to be on the background. This required changing the data structure holding these strings to be a concurrent dictionary.
Move RegisterObjectBrowserLibraryManager call in AbstractPackge package initialization to OnAfterPackageLoadedAsync
Change MiscellaneousFilesWorkspace to not require a bg thread initialize call and instead get the text manager when needed.