Move remaining legacy workspace event listeners over to new APIs. #78333
Conversation
If there were any questions about the event needing the main thread, this PR attempts to be safe in the assumptions. Future PRs can judiciously look at moving event handlers to work on background threads.
jasonmalinowski
left a comment
jasonmalinowski
left a comment
There was a problem hiding this comment.
My comments were mostly two general thoughts:
- For places where we're doing a Connect/Disconnect pattern, it might be wise to have a Debug.Assert() that we're not overwriting a disposer if it already had one. When we're just using event subscription it'd be possible for things to get out of order, but less so here. I don't imagine we're relying on it, but the assertion might be useful.
- There's some cases where I'd be OK with is just making the disposers read only to remove the null checking entirely.
But neither of these are critical, and don't need to block merging of this. (Or need to be done at all.)
| { | ||
| _workspace = workspace; | ||
| _workspace.WorkspaceChanged += UpdateProviders; | ||
| _ = workspace.RegisterWorkspaceChangedHandler(UpdateProviders); |
There was a problem hiding this comment.
I'm not exactly sure what this type is doing, but it might be a good place to see if we can have this unsubscribe when an editor isn't closed again.
There was a problem hiding this comment.
It's not clear to me either how this type is used. I'd be hesitant to add much logic around removing this handler in certain scenarios without fully understanding the type and that's a bit out of scope for this PR.
|
|
||
| _workspace.WorkspaceChanged += OnWorkspaceChanged; | ||
| // Main thread as OnWorkspaceChanged's call to IDiagnosticAnalyzerService.RequestDiagnosticRefresh isn't clear on | ||
| // threading requirements |
There was a problem hiding this comment.
Are you planning tracking bugs for these?
There was a problem hiding this comment.
I plan on attacking the ones that show up using heavy CPU on background threads, or moderate CPU on the main thread, but I wasn't planning on creating a bug to modify each of these to not require the main thread.
If you think it's important for us to do that, I can log a bug, but it wasn't on my radar to get us completely off the main thread for non-trivial implementations.
src/EditorFeatures/Core/EditAndContinue/PdbMatchingSourceTextProvider.cs
Show resolved
Hide resolved
...eatures/Core/Shared/Tagging/EventSources/TaggerEventSources.ParseOptionChangedEventSource.cs
Show resolved
Hide resolved
src/VisualStudio/Core/Def/StackTraceExplorer/StackTraceExplorerViewModel.cs
Show resolved
Hide resolved
… advise/unadvise working 2) Don't null out a disposer in a class that doesn't follow that mechanism 3) Add comments suggested from an unrelated PR that had already been merged.
3 participants
About 4.1% of main thread CPU costs during solution load are in workspace eventing. This moves nearly all those costs to background threads. The majority of the workspace changed handler costs are associated with ProjectCodeModelFactoy.
If there were any questions about the event handler needing the main thread, this PR attempts to be safe, and continue to use the main thread.
Future PRs can judiciously look at moving event handlers to work on background threads.
Test insertion PR: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/631924
*** Before, all threads ***
*** Before, filtered to main thead ***
*** After, all threads ***
*** After, filtered to main thead ***
