Move various bits of code to the after package load code in RoslynPackage #77745
Conversation
…kage 1) Move the *very expensive* color schema applier initialization code to occur after package load. I talked with Joey about this and he indicated this could (quite infrequently) cause a recolorization in the editor upon open, but only when the user had previously changed themes or the very first open of a C# file after install. 2) Move the global notification service construction to happen on a bg thread, bright before it's use. 3) Move the SolutionEventMonitor construction and bulk file notification registration to occur after solution load. The bulk notification for the solution open would have already been initiated if the package load occurs during solution load, so this shouldn't affect that notification. 4) A bit of cleanup from earlier PRs: (VB lambda param cleanup, removing duplicated MiscellaneousFilesWorkspace service retrieval)
ghost
added
Area-IDE
untriaged
| var settingsEditorFactory = this.ComponentModel.GetService<SettingsEditorFactory>(); | ||
|
|
||
| packageInitializationTasks.AddTask( | ||
| isMainThreadTask: true, |
There was a problem hiding this comment.
so, this goes to my feedback from before. why are we initializing on the BG just to kick this tack over to the foreground? why not just do this on the FG?
(note: i'm not saying this is wrong. i'm just saying i don't understand the 'why?' part fo this, and i really wish the code was doc'ed for future understanding).
There was a problem hiding this comment.
This one is done because SettingsEditorFactory is fairly expensive to create, thus wanting it done on a bg thread and the main thread work depends on that being initialized. I'll create a follow up PR to make that ctor less expensive so it's not a big deal to create it on the main thread.
There was a problem hiding this comment.
I think in this specific case, SettingsEditorFactory is fairly expensive to create via MEF. There's a more general piece of work to clean that up since really should be cheap to create and pass to RegsiterEditorFactory, but that's not how it's written right now.
There was a problem hiding this comment.
my ask is that we then doc these decisions in the code. :)
| Assumes.Present(globalNotificationService); | ||
|
|
||
| _solutionEventMonitor = new SolutionEventMonitor(globalNotificationService); | ||
| TrackBulkFileOperations(globalNotificationService); |
There was a problem hiding this comment.
consider inlining this. it's more confusing to me as a call out to a function.
There was a problem hiding this comment.
I think I'd prefer to keep that method separate. It's fairly long and has two local functions inside it already.
|
|
||
| // We are at the VS layer, so we know we must be able to get the IGlobalOperationNotificationService here. | ||
| var globalNotificationService = this.ComponentModel.GetService<IGlobalOperationNotificationService>(); | ||
| Assumes.Present(globalNotificationService); |
There was a problem hiding this comment.
no GetRequiredService or anything like that?
There was a problem hiding this comment.
MEF throws if there isn't exactly one matching export. I'll remove the Assumes.Present as it probably makes that less obvious.
| packageInitializationTasks.AddTask( | ||
| isMainThreadTask:=False, | ||
| task:=Function(packageInitializationTasks2, cancellationToken) As Task | ||
| task:=Function() As Task |
There was a problem hiding this comment.
Oh that's right I forgot VB lets you do this. I figured remove it from the C# side too....
|
@ToddGrun Help me understand the benefit here -- is the advantage of this going after package load is that other things blocking on package load can continue their work while this all runs? |
|
Yes, essentially allowing higher priority items during solution/package load to be less contended. And I guess this helps the case where our packages are loaded but don't need this information right away. In reply to: 2744576389 |
4 participants
Move the very expensive color schema applier initialization code to occur after package load. I talked with Joey about this and he indicated this could (quite infrequently) cause a recolorization in the editor upon open, but only when the user had previously changed themes or the very first open of a C# file after install.
Move the global notification service construction to happen on a bg thread, bright before it's use.
Move the SolutionEventMonitor construction and bulk file notification registration to occur after solution load. The bulk notification for the solution open would have already been initiated if the package load occurs during solution load, so this shouldn't affect that notification.
A bit of cleanup from earlier PRs: (VB lambda param cleanup, removing duplicated MiscellaneousFilesWorkspace service retrieval)