Avoid acquiring the workspace lock on the UI thread if unneeded #79047
Conversation
| // 1. We have multiple documents with the same file path, so we need to ensure the context is actually correct. | ||
| // 2. We haven't enabled the suggested actions source provider, which needs to be done once. | ||
| var needsContextUpdate = w.CurrentSolution.GetDocumentIdsWithFilePath(newFileName).Length >= 2; | ||
| if (needsContextUpdate || !_suggestedActionSourceProviderEnabled) |
There was a problem hiding this comment.
I'm still curious whether it would make sense to only call UpdateContextAfterOpenAndEnableSuggestedActionsSourceProviderAsync once instead of inside the loop.
That would reduce the potential number of UI switches. The upper bound of switches though here are files that are open and loaded prior to the solution loading, which isn't going to be that many. This fix is also being applied simultaneously to 17.14 and main so I'm keeping the change as small as reasonably possible since there's a desire for quick servicing.
There was a problem hiding this comment.
Is it ok for this to change from false=>true during the loop?
Yep, the only concern is the UI thread gets that call at least once. Beyond that, doesn't matter.
If the project system tells us about a file that's open and we are told about it on a background thread, we connect the file to the workspace on that background thread but then schedule work to the UI thread to ensure we correctly connect it to the right context (linked files, shared projects, multitargeting, etc.) That code on the UI thread then acquires the workspace lock, and we're seeing cases in the wild where that lock acquisition is taking longer than we'd like. Although there are deeper fixes we can do there, there's one trivial fix we can make: don't schedule the work to the UI thread unless the file is in at least two projects. Otherwise, we don't actually need to figure out the context in the first place and it's just wasting time. An entirely unscientific analysis of a handful of traces from users hitting this, there weren't any cases where the file was actually in more than one project, so this should have a nice improvement overall. Closes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/2471561
jasonmalinowski
force-pushed
the
avoid-unnecessary-ui-transition
branch
from
add1f95 to
83fba2a
Compare
4 participants
If the project system tells us about a file that's open and we are told about it on a background thread, we connect the file to the workspace on that background thread but then schedule work to the UI thread to ensure we correctly connect it to the right context (linked files, shared projects, multitargeting, etc.) That code on the UI thread then acquires the workspace lock, and we're seeing cases in the wild where that lock acquisition is taking longer than we'd like. Although there are deeper fixes we can do there, there's one trivial fix we can make: don't schedule the work to the UI thread unless the file is in at least two projects. Otherwise, we don't actually need to figure out the context in the first place and it's just wasting time.
An entirely unscientific analysis of a handful of traces from users hitting this, there weren't any cases where the file was actually in more than one project, so this should have a nice improvement overall.
Closes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/2471561