Remove most remaining uses of WorkspaceChanged off the UI thread #78778

jasonmalinowski · 2025-05-31T00:24:55Z

Commit-at-a-time is recommended, since each use gets its own commit with further analysis there.

The remaining uses of eventing that hit the UI thread (of any kind of workspace event) are:

Inline rename subscribes when there is an active rename to cancel on changes it doesn't know about. Since this is only active during the session, it doesn't seem worth it change it.
MiscellanousFilesWorkspace subscribes to workspace registration changed, which is when a document is opened/closed, and that's not urgent to fix and restricted to just registration events.
XamlProjectService subscribes to document closed. This only happens if XAML is loaded in the first place, and then is only raised on the close path. I looked at making a small change to move it off but the code is generally quite scary and since it's document close only that's not chatty.

src/VisualStudio/Core/Impl/SolutionExplorer/DiagnosticItem/CpsDiagnosticItemSource.cs

src/Features/Core/Portable/Diagnostics/CodeAnalysisDiagnosticAnalyzerService.cs

ToddGrun · 2025-05-31T00:58:16Z

src/VisualStudio/Core/Def/StackTraceExplorer/StackTraceExplorerViewModel.cs

-        if (e.Kind == WorkspaceChangeKind.SolutionChanged)
-        {
-            Selection = null;
-            Frames.Clear();


Frames.Clear();

this just wasn't necessary?

My commit message here:

I'm unable to figure out what the intent was here -- it clears the list but only on a SolutionChanged event, which is the type of event raised if multiple projects are modified at once in a single workspace change -- any other type of event would have a more specific kind. I could imagine the intent might have been for solution close, but then I can imagine scenarios where the user might have pasted a stack and now needs to switch the solution to navigate from the stack.

Since this likely never ran, I'm just deleting it.

Initially I read the code as "ah, when the solution is closed, clear it"...and then a few seconds later realized that's not what it was.

ToddGrun

CyrusNajmabadi · 2025-06-02T15:07:03Z

...re/Shared/Tagging/EventSources/TaggerEventSources.DocumentActiveContextChangedEventSource.cs

-        // Require main thread on the callback as RaiseChanged implementors may have main thread dependencies.
        protected override void ConnectToWorkspace(Workspace workspace)
-            => _documentActiveContextChangedDisposer = workspace.RegisterDocumentActiveContextChangedHandler(OnDocumentActiveContextChanged, WorkspaceEventOptions.RequiresMainThreadOptions);
+            => _documentActiveContextChangedDisposer = workspace.RegisterDocumentActiveContextChangedHandler(OnDocumentActiveContextChanged);


might be worthwhile to have explicit docs stating why BG is appropriate here. It probably is sufficient to just say that tagger event sources are designed to fire on the BG just fine.

ITaggerEventSource was updated to add that comment -- would that be sufficient or also want a comment on RaiseChanged too?

src/Features/Core/Portable/Diagnostics/CodeAnalysisDiagnosticAnalyzerService.cs

All subscribers are working in thread-safe manners (they either take locks to clear caches, or use existing batching/threading primitives to queue new work).

Subscribers were already thread safe. I've also removed the IMPORTANT warning since it seems quite stale -- this is already in a delayed queue so it's not really clear what it meant.

I'm unable to figure out what the intent was here -- it clears the list but only on a SolutionChanged event, which is the type of event raised if multiple projects are modified at once in a single workspace change -- any other type of event would have a more specific kind. I could imagine the intent might have been for solution close, but then I can imagine scenarios where the user might have pasted a stack and now needs to switch the solution to navigate from the stack. Since this likely never ran, I'm just deleting it.

If the user expands a node in a CPS project wanting to look at the diagnostics under an analyzer, but we haven't been told about that analyzer let, we stick a WorkspaceChanged handler there to find it once it comes back. We were hopping to the UI thread to see if the CanonicalName of the item could have changed, but that's not really going to happen ever for these items, so we can just grab it once during creation and be done with it.

This looks to be safely callable from any thread.

…ead (dotnet#78778)" This reverts commit 67cce50, reversing changes made to aafd6eb.

…ged off the UI thread (#78778)" (#79366) This reverts commit 67cce50, reversing changes made to aafd6eb. Going to run a test insertion to see if this is the cause of regressions flagged in https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/650773

…ged off the UI thread (dotnet#78778)" (dotnet#79366) This reverts commit 67cce50, reversing changes made to aafd6eb. Going to run a test insertion to see if this is the cause of regressions flagged in https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/650773

* upstream/main: (698 commits) Use new dll name for hooking xunit dispose Fix helix timeout Replace Assert.False call Add console logger to ensure helix console log has detailed info Disable Assert that is causing hangs *** DO NOT MERGE: Revert "Remove most remaining uses of WorkspaceChanged off the UI thread (dotnet#78778)" (dotnet#79366) Update configs for snap nrt Ensure we collect dumps on hangs/crashes Null tolerance Ensure generated types come after top level statements Allow user to still create a new field/prop when offering to initialize an existing prop Fix issue where typeof/sizeof weren't classified properly in FindRefs Add test showing issue no longer reproes Add test Disable more tests for dotnet#79352 Track assembly names, not counts Fix Fix crash in replace property with methods Fix issue with remove unnecessary parens in vb ...

* Simplify code * Simplify code * Ensure server is loaded and unloaded with the solution * Check that a resulting ternary operator conversion is implicit before removing a cast in one of its branches * Extract method for handling source generated documents * Revert "Remove the 'intent' subsystem from roslyn (#79179)" This reverts commit 4e03ac9, reversing changes made to 93b1b3e. * Remove legacy intents that are no longer hooked up * Remove unnecessary folder in project file * More cases * More cases * remove tests * Reduce allocations of BoundBinaryOperator.UncommonData (#79200) * Remove duplicate method * Extensions: adjust SpecialName on implementation methods (#79068) * Update azure-pipelines-integration-dartlab.yml (#79206) * Rename * Remove obsolete code * Extract common lexer code into helpers (#79214) * Improve diagnostic for ambiguous predefined type (#79196) * Improve diagnostic for ambiguous predefined type * Update pre-existing tests * Keep translations * Revert unnecessary changes * Add a source package that can be used to connect to the roslyn build server (#78986) * Add BuildClient package * Allow sdk passing compiler hash * Reuse in Replay * Add a bit how we can and will break the APIs * Move more files to the shared folder * Use shared file list in build task * Address some known functionality gaps for extension operators (#79167) Related to #78968. Related to #76130. * Use spans in low level lexer char array handling code * Use spans in low level lexer char array handling code * VB tests * Remove duplicate code for processing arrays vs strings * Revert * Fix test * Add internal APIs for prototyping * Share more code * Remove unused function * Use spans in low level lexer char array handling code (#79232) * Remove unused functions (#79234) * Update src/Compilers/Core/Portable/InternalUtilities/StringTable.cs * Remove using * Rename FileBasedProgramsProjectFactory to MiscellaneousFilesWorkspaceProjectFactory * Don't double register for document sync * Don't check configuration if the client doesn't support it (ie, tests) * Extensions: adjust logic related to primary ctor parameter (#79056) * Address another set of functionality gaps for extension operators (#79227) Related to #78967 Related to #78968 Related to #76130 * Revert "Use spans in low level lexer char array handling code (#79232)" (#79245) This reverts commit fb38d6b. * Use spans in low level lexer char array handling code (part 2) (#79250) * Allow the Razor extension to report telemetry (and initialize) * Make the new Workspace.Register*Handler methods public We're obsoleting the old ones since the expectation going forward is most users can use the new ones only. * Do not use a constructed method symbol in a BoundMethodDefIndex (#79211) * Simplify * Address follow-up comments for extension operators (#79249) Closes #79136. * Use collection expression * Docs * ordering * Initial support for adding obsolete attributes to primary constructors * Flesh out * Use raw strings * File scoped namespaces * Simplify tests * in progress * Simpler approach * Revert * Simplify tests * Working * Delete file * Remove file * Use the miscellaneous files project name for rich misc projects (#79267) * Update tests * Inline strings in test code * Make tests non-async * Add the "experimental feature" string back Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/2453154 * Move telemetry initialization out of our UI-thread bound helper This isn't needed anymore. * Update global.json * Update nuget.config * Update Arcade * Use .NET 10 Preview 5 SDK * Add back dotnet9 feed * Add back additional feeds * Add back dotnet6 feed * The Unix CI build does not need to pack or publish * Downgrade arcade * Reapply "Move to .NET 10 Preview 5 (#78906)" This reverts commit 1ab27c2. * Use scouting queue for integration tests * Revert: Use scouting queue for integration tests * Fix Rename adornment positioning after scrolling * Fix debugging of build tasks (#78271) * Fix debugging of build tasks * Split into a new target * Fix up tests * Tweak 'add required parens' to recognize a common C# idiom * Lint * Revert compiler change (#79288) * Extensions: extension grouping type names (#79217) * Fix ref safety of user-defined increment operators (#79034) * Fix ref safety of user-defined increment operators * Update after merge * Improve code and tests * Inline test code * Make tests synchronous * Remove unnecessary await * Enable C# classification in more tests * Fix unit tests There's two issues here: 1. Some tests of the VS layer didn't have the telemetry service in the first place. 2. There was now a circularity between the VisualStudioWorkspace and VisualStudioWorkspaceTelemetryService, so a Lazy has been added. * Use collection expressions * Explicitly document that ITaggerEventSource.Changed can be on any thread All subscribers are working in thread-safe manners (they either take locks to clear caches, or use existing batching/threading primitives to queue new work). * Document that LspSolutionChanged may happen on any thread Subscribers were already thread safe. I've also removed the IMPORTANT warning since it seems quite stale -- this is already in a delayed queue so it's not really clear what it meant. * Remove subscription from WorkspaceChanged in the StackTraceExplorer I'm unable to figure out what the intent was here -- it clears the list but only on a SolutionChanged event, which is the type of event raised if multiple projects are modified at once in a single workspace change -- any other type of event would have a more specific kind. I could imagine the intent might have been for solution close, but then I can imagine scenarios where the user might have pasted a stack and now needs to switch the solution to navigate from the stack. Since this likely never ran, I'm just deleting it. * Avoid checking the CanonicalName each time for the analyzer If the user expands a node in a CPS project wanting to look at the diagnostics under an analyzer, but we haven't been told about that analyzer let, we stick a WorkspaceChanged handler there to find it once it comes back. We were hopping to the UI thread to see if the CanonicalName of the item could have changed, but that's not really going to happen ever for these items, so we can just grab it once during creation and be done with it. * Remove thread requirement from CodeAnalysisDiagnosticAnalyzerService This looks to be safely callable from any thread. * Reduce probability of stack overflow during exception handling in ModelComputation (#79292) Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/2428510 This was previously addressed by adding a Task.Yield to the TransformModelAsync execution, instead this moves that stack size mitigation to the exception handling process. Per #26567, the vast majority of stack size during stack unwinding is due to the unwinding process itself. * Remove synchronous rename (#78839) * wip * wip * wip * fix tests * wip * remove unused code * update test impl * update option test * add cancel method * some cleanup * call CommitAsync in tests instead * fix some comments and naming * rename commit to make it more clear it's an async operation * feedback * feedback * fix integration test * Update * Add test demonstrating issue * inline * fix casing mismatch when using non-unc file paths * Collection expressions * Skip failing tests * Add cookbook section for avoiding inheritance (#79276) * Add cookbook section for avoiding inheritance Adds a section to the incremental generators cookbook on why users should avoid inheritance, that goes over a few possible scenarios and how they can hurt IDE responsiveness. * Update ToC * Add a small note on preferring FAWMN. * Make 'convert to raw string' a syntax-only refactoring * Fix issue with use-raw-string and fix-all * Fix bad merge * Use raw strings in tests * [main] Source code updates from dotnet/dotnet (#79313) * [VMR] Codeflow 9eec48b-9eec48b [[ commit created by automation ]] * Update dependencies from https://github.com/dotnet/dotnet build 274568 No dependency updates to commit --------- Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> * Fix argument indentation * REvert * Simplify initialization of OpenFileTracker There's only two things here that could potentially need the UI thread: 1. The Running Document Table subscription, but OpenTextBufferProvider already abstracted that away. 2. The IEditorOptionsFactoryService usage, which being a MEF component shouldn't care, but even then we don't need it right away. It's easy to clean all this up, so just do so. * Revert * revise * NRT * Auto prop * nrt * Fix * Update src/Compilers/Core/Portable/AdditionalTextFile.cs * better fix * better fix * better fix * Set DeployExtension for all the extensions we expect to deploy * Set RoslynCompilerType=Custom in all toolset package flavors (#79327) * Fix code gen for some increment/compound assignment scenarios involving constrained calls. (#79293) Fixes #79268 * In progress * In progress * Hook up * Add work item * Cleanup * Simplify * Update to xunit.runner.visualstudio 3.1.1 Address changes from dotnet/sdk#49248. * Temporarily increase timeout of helix items to see if it resolves timeouts * Update resx generator test resources to assert new behavior, and fix generator in one instance * Revert broken raw string changes in PropertySetAnalysisTests * Fix 'use var' with spans * Skip tests affected by dotnet/runtime#117566. * Disable tests for #79351 * Disable tests for #79352. * Make tests more consistent * Fix issue offering to remove nullable cast in a ternary expression * Fix not offering to remove unnecessary nullable pragmas * Fix issue with remove unnecessary parens in vb * Fix crash in replace property with methods * Fix * Track assembly names, not counts * Disable more tests for #79352 * Add test * Add test showing issue no longer reproes * Fix issue where typeof/sizeof weren't classified properly in FindRefs * Allow user to still create a new field/prop when offering to initialize an existing prop * Ensure generated types come after top level statements * Null tolerance * Ensure we collect dumps on hangs/crashes * nrt * Update configs for snap * *** DO NOT MERGE: Revert "Remove most remaining uses of WorkspaceChanged off the UI thread (#78778)" (#79366) This reverts commit 67cce50, reversing changes made to aafd6eb. Going to run a test insertion to see if this is the cause of regressions flagged in https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/650773 * Disable Assert that is causing hangs Will follow up with an issue but disabling for now to see if this unblocks the CoreCLR tests * Add console logger to ensure helix console log has detailed info * Replace Assert.False call This `Assert.False` call was executing directly an a thread pool thread. That meant when it triggered it was an unhandled exception on a TPT which crashes the process. The calling code already fails the test when this happen hence changed this to just output the failure info to the test logs. * Fix helix timeout * Use new dll name for hooking xunit dispose --------- Co-authored-by: Cyrus Najmabadi <cyrusn@microsoft.com> Co-authored-by: David Wengier <david.wengier@microsoft.com> Co-authored-by: DoctorKrolic <mapmyp03@gmail.com> Co-authored-by: Cyrus Najmabadi <cyrus.najmabadi@gmail.com> Co-authored-by: AlekseyTs <AlekseyTs@users.noreply.github.com> Co-authored-by: Julien Couvreur <julien.couvreur@gmail.com> Co-authored-by: Ankita Khera <40616383+akhera99@users.noreply.github.com> Co-authored-by: Jan Jones <janjones@microsoft.com> Co-authored-by: gel@microsoft.com <gel@microsoft.com> Co-authored-by: Jason Malinowski <jason.malinowski@microsoft.com> Co-authored-by: Gen Lu <genlu@users.noreply.github.com> Co-authored-by: Rikki Gibson <rigibson@microsoft.com> Co-authored-by: Joey Robichaud <joseph.robichaud@microsoft.com> Co-authored-by: Joey Robichaud <jorobich@microsoft.com> Co-authored-by: Oleg Tkachenko <olegtk@microsoft.com> Co-authored-by: Todd Grunke <toddgrun@microsoft.com> Co-authored-by: David Barbet <dabarbet@microsoft.com> Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: jaredpar <jared@paranoidcoding.org>

This is a reapply of #78778 that got reverted due to a potential regression. Commit-at-a-time is recommended, since each use gets its own commit with further analysis there. The remaining uses of eventing that hit the UI thread (of any kind of workspace event) are: 1. Inline rename subscribes when there is an active rename to cancel on changes it doesn't know about. Since this is only active during the session, it doesn't seem worth it change it. 2. MiscellanousFilesWorkspace subscribes to workspace _registration_ changed, which is when a document is opened/closed, and that's not urgent to fix and restricted to just registration events. 3. XamlProjectService subscribes to document _closed_. This only happens if XAML is loaded in the first place, and then is only raised on the close path. I looked at making a small change to move it off but the code is generally quite scary and since it's document close only that's not chatty. The last commit makes a larger change for performance to avoid a regression in our internal performance tests. To explain the change a bit of explanation is needed: Our WorkspaceChanged events are raised with an batching work queue with a delay of zero. When we have events to raise on the UI thread, the batch handler first raises the background thread notifications, then jumps to the UI thread (if needed) and raises them there. This means that in this case, our events are roughly 'throttled' by the availability of the UI thread; so we're likely to end up with a flurry of background events, then foreground events, then background events, etc. Usually these event handlers are feeding into other batching work queues, including the one we use to synchronize the VS Workspace to our OOP process to keep things relatively up to date. That had a delay of 50ms to batch up a request to start syncing it again. My belief is that prior to the WorkspaceChanged change, that delay didn't matter so much -- the actual batching may have been that if (say) the UI thread was only available once every 100ms to fire events, then that was the real rate limiting. Once that is freed up, then now we were creating a lot more batches. I did some tests counting the number of batches during a Roslyn solution load and it seemed to be roughly double, which matched some of the extra allocation overhead happening in StreamJsonRpc as we were synchronizing with OOP. I have changed the batching delay for the OOP sync from 50ms to 250ms, that number chosen because that's what "Short" is in our delay choices. The excess memory allocations seen before have disappeared, making me think we're now doing fewer but larger batches. The ManagedLanguages.SolutionManagement RPS test shows a 10% reduction in ServiceHub allocations than the baseline. I don't imagine this would be particularly noticeable to a user the extra delay since that'd only impact the final sync that happens after a solution load, which isn't hugely critical. The immediate-sync that we have of a changed document is left in place.

jasonmalinowski requested a review from a team as a code owner May 31, 2025 00:24

github-actions bot added the Area-IDE label May 31, 2025

dotnet-policy-service bot added the VSCode label May 31, 2025

jasonmalinowski requested a review from ToddGrun May 31, 2025 00:25

jasonmalinowski commented May 31, 2025

View reviewed changes

src/VisualStudio/Core/Impl/SolutionExplorer/DiagnosticItem/CpsDiagnosticItemSource.cs Show resolved Hide resolved

jasonmalinowski self-assigned this May 31, 2025

ToddGrun reviewed May 31, 2025

View reviewed changes

src/Features/Core/Portable/Diagnostics/CodeAnalysisDiagnosticAnalyzerService.cs Show resolved Hide resolved

ToddGrun reviewed May 31, 2025

View reviewed changes

ToddGrun approved these changes Jun 2, 2025

View reviewed changes

CyrusNajmabadi reviewed Jun 2, 2025

View reviewed changes

CyrusNajmabadi approved these changes Jun 2, 2025

View reviewed changes

src/Features/Core/Portable/Diagnostics/CodeAnalysisDiagnosticAnalyzerService.cs Show resolved Hide resolved

CyrusNajmabadi approved these changes Jun 2, 2025

View reviewed changes

jasonmalinowski force-pushed the remove-most-remaining-workspace-changed-on-ui-thread branch from 1e91e6a to f199169 Compare June 11, 2025 23:14

jasonmalinowski added 5 commits July 8, 2025 13:35

Explicitly document that ITaggerEventSource.Changed can be on any thread

8131710

All subscribers are working in thread-safe manners (they either take locks to clear caches, or use existing batching/threading primitives to queue new work).

Document that LspSolutionChanged may happen on any thread

578c6a3

Subscribers were already thread safe. I've also removed the IMPORTANT warning since it seems quite stale -- this is already in a delayed queue so it's not really clear what it meant.

Remove thread requirement from CodeAnalysisDiagnosticAnalyzerService

c6c55d5

This looks to be safely callable from any thread.

jasonmalinowski force-pushed the remove-most-remaining-workspace-changed-on-ui-thread branch from f199169 to c6c55d5 Compare July 8, 2025 22:51

jasonmalinowski merged commit 67cce50 into dotnet:main Jul 9, 2025
25 checks passed

jasonmalinowski deleted the remove-most-remaining-workspace-changed-on-ui-thread branch July 9, 2025 01:58

dotnet-policy-service bot added this to the Next milestone Jul 9, 2025

ToddGrun added a commit to ToddGrun/roslyn that referenced this pull request Jul 14, 2025

Revert "Remove most remaining uses of WorkspaceChanged off the UI thr…

56e5de5

…ead (dotnet#78778)" This reverts commit 67cce50, reversing changes made to aafd6eb.

jasonmalinowski restored the remove-most-remaining-workspace-changed-on-ui-thread branch July 14, 2025 22:51

jasonmalinowski mentioned this pull request Jul 15, 2025

Remove most remaining workspace changed on UI thread #79391

Merged

drewnoakes mentioned this pull request Jul 17, 2025

Dependencies tree should appear immediately dotnet/project-system#9710

Open

RikkiGibson modified the milestones: Next, 18.0 P1 Aug 19, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Remove most remaining uses of WorkspaceChanged off the UI thread #78778

Remove most remaining uses of WorkspaceChanged off the UI thread #78778

Uh oh!

jasonmalinowski commented May 31, 2025

Uh oh!

Uh oh!

Uh oh!

ToddGrun May 31, 2025

Uh oh!

jasonmalinowski May 31, 2025

Uh oh!

jasonmalinowski May 31, 2025

Uh oh!

ToddGrun left a comment

Uh oh!

CyrusNajmabadi Jun 2, 2025

Uh oh!

jasonmalinowski Jun 2, 2025

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Remove most remaining uses of WorkspaceChanged off the UI thread #78778

Remove most remaining uses of WorkspaceChanged off the UI thread #78778

Uh oh!

Conversation

jasonmalinowski commented May 31, 2025

Uh oh!

Uh oh!

Uh oh!

ToddGrun May 31, 2025

Choose a reason for hiding this comment

Uh oh!

jasonmalinowski May 31, 2025

Choose a reason for hiding this comment

Uh oh!

jasonmalinowski May 31, 2025

Choose a reason for hiding this comment

Uh oh!

ToddGrun left a comment

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Jun 2, 2025

Choose a reason for hiding this comment

Uh oh!

jasonmalinowski Jun 2, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants