Merge latest bits from main into features/extensions
#79858
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…in an async method
dotnet#79507) … balanced mode Resolves dotnet#75586 Support for this was originally added in dotnet#77271 However it seems like `Project.GetDependentVersionAsync` changed to no longer increment when the sg execution version changed, and due to a test issue, the problem was not caught. Now:  clean val - https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/656041
* Don't use a pool in SemanticTokensHelper.ComputeTokens This can be just allocated directly into an array instead of fiddling with a pool. Simpler and more efficient. This shows up as 2.5% of allocations in a scenario in a scenario in RazorEditingTest.CompletionInCohosting for the Roslyn OOP. Most of that is due to the list resizing.
Add comment to improve readability.
…in an async method (dotnet#79631)
…ualChars (dotnet#79623) This method is accounting for 1.4% of allocations in the OOP process in a completion scenario in the RazorEditing.CompletionInCohosting speedometer test I am looking at. Instead of doing all the work and hitting the pools, we can just handle the normal case of there not being any surrogates or escape characters and just create a VirtualCharSequence wrapper around the existing string. Locally, I see about 75% of strings that come through hit this optimization. Going to do a test insertion with this change to verify speedometer numbers before elevating out of draft status.
Per @davkean this no longer requires the UI thread to subscribe to context change events. This lets us simplify how we subscribe to these to avoid subscribing to them in random places on the UI thread.
This is a minor change to adopt the (info level) analyzer warning coming as part of this suggestion (marked as api-approved): dotnet/runtime#117643
…ts (dotnet#79604) Part of dotnet/razor#9519 Half of the fix for dotnet/razor#12054, along with the Razor side at dotnet/razor#12055 Previously I allowed rename to work on any source generated document, but made it only opt in for when the rename request came from Razor. Now that we have a little more time for development, it's time to finish the feature off properly. This PR: * Rename will only consider source generated documents if they're from Razor * Any rename operation will automatically do so, without opt in * Before applying changes to the workspace, we call in to Razor to map edits * This matches the behaviour of non-cohosting, but that does edit mapping via a document service. Since with cohosting Razor doesn't control the document creation, there is no way to set up a document service, so there is just a workspace service that does the same thing. There will still need to be more PRs to make this work in VS Code, and to hook up the span mapping aspect of the service, but I didn't want this PR to get too big.
This is a reapply of dotnet#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.
It looks like in various refactorings we accidentally had a ConfigureAwait(false) appear in this method which could cause us to jump to a background thread when we don't need to do that. I'm not sure why we didn't just use CA(true) only here, since that seems to do what we want without any extra trickery.
Noticed this tonight. We have some pretty horrendous base class argument lists in Razor :)
…stive switch expressions (dotnet#79850)
dotnet-policy-service
bot
added
VSCode
Needs API Review
Contributor
|
This PR modifies public API files. Please follow the instructions at https://github.com/dotnet/roslyn/blob/main/docs/contributing/API%20Review%20Process.md for ensuring all public APIs are reviewed before merging. |
This was referenced Aug 19, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
18 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Aside from the usual minor conflicts (error codes, resource strings) I put the actual merge conflicts in the last commit. It's updating some tests