PERF: Reduce the number of nodes walked during import completion commit. #77305
Conversation
This file contains 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
3 tasks
|
Perf numbers look really good, going to escalate out of draft mode. |
ToddGrun
changed the title
|
@dotnet/roslyn-ide -- this is ready for review |
CyrusNajmabadi
approved these changes
Feb 25, 2025
This was referenced Mar 4, 2025
JoeRobich
added a commit
to dotnet/vscode-csharp
that referenced
this pull request
Mar 8, 2025
* Don't use an implicit-object if the lang version doesn't support it (PR: [#77437](dotnet/roslyn#77437)) * Fix issue where we were changing semantics when converting to a collection expr. (PR: [#77417](dotnet/roslyn#77417)) * Detect and emit more idiomatic null check patterns (PR: [#77412](dotnet/roslyn#77412)) * Partial events and constructors: IDE (PR: [#77337](dotnet/roslyn#77337)) * Simplify keyword recommenders. (PR: [#77396](dotnet/roslyn#77396)) * Remove async/await (PR: [#77397](dotnet/roslyn#77397)) * Preserve encoding during DocumentState updates (PR: [#77354](dotnet/roslyn#77354)) * Don't realize the SourceText in SyntaxTree.OverlapsHiddenPosition if not needed (PR: [#77334](dotnet/roslyn#77334)) * Handle ModuleCancellationTokenExpression in AbstractFlow visitor (PR: [#77310](dotnet/roslyn#77310)) * PERF: Reduce the number of nodes walked during import completion commit. (PR: [#77305](dotnet/roslyn#77305)) * Allow expression body refactorings on non empty selections (PR: [#76969](dotnet/roslyn#76969)) * Partial events and constructors: public API (PR: [#77202](dotnet/roslyn#77202)) * Refresh diagnostics when fading options change (PR: [#77322](dotnet/roslyn#77322)) * Reduce allocations during completion in FilterToVisibleAndBrowsableSymbols (PR: [#77315](dotnet/roslyn#77315)) * Change override completion to select text after updating the buffer. (PR: [#76983](dotnet/roslyn#76983)) * Fix false positive 'Unnecessary assignment of a value' (PR: [#77297](dotnet/roslyn#77297))
333fred
added a commit
that referenced
this pull request
Mar 11, 2025
* Parse partial events and constructors (#76860) * Parse partial events and constructors * Update pre-existing tests * Explain why partial ctors are sometimes disallowed * Parse `partial` constructors unconditionally * Gate parsing on LangVersion * Extend tests and docs * Fix indentation * Rename combinatorial values * Test more lang versions * Parse partial events unconditionally * Update dependencies from https://github.com/dotnet/source-build-reference-packages build 20250203.2 Microsoft.SourceBuild.Intermediate.source-build-reference-packages From Version 10.0.607901 -> To Version 10.0.610302 * Update dependencies from https://github.com/dotnet/source-build-reference-packages build 20250204.2 Microsoft.SourceBuild.Intermediate.source-build-reference-packages From Version 10.0.607901 -> To Version 10.0.610402 * Partial events and constructors: merge and check symbols (#76970) * Partial events and constructors: merge and check symbols * Improve code * Move partial availability check * Improve code * Fix duplicate definition diagnostic * Test different ordering * Avoid reporting diagnostic for escaped `partial` * Clarify naming of AccessorsHaveImplementation property * Add event definition accessor symbol * Improve code * Test sequence points * Test extern IL * Fix interface container diagnostics * Fix sequence points issue in the test utility * Ensure the new accessor owner is a definition * Update dependencies from https://github.com/dotnet/source-build-reference-packages build 20250210.2 Microsoft.SourceBuild.Intermediate.source-build-reference-packages From Version 10.0.607901 -> To Version 10.0.611002 * Remove async/await * Update dependencies from https://github.com/dotnet/arcade build 20250211.5 Microsoft.SourceBuild.Intermediate.arcade , Microsoft.DotNet.Arcade.Sdk , Microsoft.DotNet.Helix.Sdk , Microsoft.DotNet.XliffTasks From Version 9.0.0-beta.25077.4 -> To Version 9.0.0-beta.25111.5 * Partial events and constructors: check differences (#77118) * Partial events and constructors: check differences * Improve code and tests * Complete constructor implementation part * Report error for tuple differences in constructors * Update dependencies from https://github.com/dotnet/source-build-reference-packages build 20250212.3 Microsoft.SourceBuild.Intermediate.source-build-reference-packages From Version 10.0.608004 -> To Version 10.0.611203 * Add test for override completion error * Fix override completion when attribute below * Partial events and constructors: doc comments (#77200) * Ensure generated operator methods have the format annotation * Update VB tests to assert new formatting behavior * Use raw strings in override tests * Ensure diagnostics refresh when source generators run in balanced mode * Update dependencies from https://github.com/dotnet/source-build-reference-packages build 20250218.1 Microsoft.SourceBuild.Intermediate.source-build-reference-packages From Version 10.0.608004 -> To Version 10.0.611801 * Create missing options types * Fix Quick Info nullability display for backing fields (#77240) Co-authored-by: Cyrus Najmabadi <cyrus.najmabadi@gmail.com> * Mix the source generator version info into the dependent checksum we get for projects * Make diagnostic checksum an extension * Move * simplify * Fix crash in 'introduce variable' when converting an object creation expression to an implicit object creation expression * Enable NRT * Update src/Tools/SemanticSearch/ReferenceAssemblies/Apis/Microsoft.CodeAnalysis.CSharp.txt * Partial events and constructors: attributes (#77182) * Partial events and constructors: attributes * Update pre-existing tests * Fixup expected PEVerify output * Check a flag before materializing syntax nodes * Assert that the definition part of an event doesn't have accessors * Keep pre-existing event attribute target behavior * Improve code * Add WorkItem attribute * Strengthen asserts Co-authored-by: Rikki Gibson <rikkigibson@gmail.com> --------- Co-authored-by: Rikki Gibson <rikkigibson@gmail.com> * Use proj dependent checksum instead now that it contains SG info * disable realtime monitoring * Update debugger contracts to fix hot reload issue * Update dependencies from https://github.com/dotnet/source-build-reference-packages build 20250219.1 Microsoft.SourceBuild.Intermediate.source-build-reference-packages From Version 10.0.608004 -> To Version 10.0.611901 * Return stronger-ly typed results from completion * Forgot to change the file header when copying the file contents * One level deep * Add test * Add an option to ignore aliases when moving using directives outside a namespace * Add test * Add work items * Update runtime async spec (#76872) * Update runtime async spec Updates from the runtime side: dotnet/runtime#110420. * Update method signature * Add note about blocking * Fill in more examples * Remove outdated paragraph * A bit of reorganization for clarity * Collapse a number of samples to expandable section * Fix heading * Remove out of support branches from PublishData.json * Switch TextDocumentState.GetTextVersionAsync to ValueTask to reduce allocations. (#77213) The task creation from calling this method accounts for 36 MB (0.2%) of allocations in the CSharpEditingTests.Completion speedometer test. Speedometer results with this change show marked improvement in these allocations. * Fix * Update PublicAPI files for 17.13 ship (#77266) (#77295) (cherry picked from commit f8f04dd) * Expand doc comment * Update azure-pipelines-official.yml * Don't use hardcoded line endings (#77296) Was causing these files to change every build on linux. * Allow Semantic Search query function to take symbol parameter (#77272) Fix * Update to release nuget packages (#77261) (#77298) * Update to release nuget packages In order to publish 17.13 nuget packages, we need to update a few dependencies. I updated the minimum number of packages required from our public packages, then fixed up any restore issues. For each updated package, I then went and confirmed that the dll was actually present in 17.13.0, so we should be safe from dependency loading issues. This change does not need to be inserted into VS. * Skip tests for microsoft/vs-solutionpersistence#95 * Skip failing tests, see #77293. (cherry picked from commit 3350036) * Generate Documentation Auto-Insert (#76665) * wip * wip * wip * wip * wip * wip * wip * remove some commented out code * still cleaning up * revert file * clean up + option * wip * dispose of disable of intellicode line completions * comments * comments * add quota exceeded checks * fix tests * revert file * remove unused optional params * last few fixes * feedback * fix tests * small fix * fix * feedback * feedback * removed per comments * remove unused usings * feedback * feedback * move stuff around with new EA structure in roslyn * wip * feedback * feedbacl * fix * feedback * feedback * feedback * revert * remove null check * clean up * fix * fix tests * last fixes * telemetry + fixes * last comments * Move check to 'make field readonly' * Add test * Update src/Tools/SemanticSearch/ReferenceAssemblies/Apis/Microsoft.CodeAnalysis.CSharp.txt * Partial events and constructors: more tests (#77280) * Add Copilot UI prototype to Semantic Search window (#77222) * disable the WPF ComboBox Search behavior in Rename popup * Change override completion to select text after updating the buffer. (#76983) * Change override completion to select text after updating the buffer. This (along with #76969) will allow a user to commit an override completion and easily replace the default text (and still use the expression body fixers). This required the CompletionChange class to now allow the data to represent a selection instead of just a cursor position. * Explictly jump off the main thread before serializing data to call Razor * Reduce allocations during completion in FilterToVisibleAndBrowsableSymbols (#77315) * Reduce allocations during completion in FilterToVisibleAndBrowsableSymbols This method was visible in allocations in one of the completion speedometer profiles I was looking at and has a couple of easily removable allocations. Can combine two WhereAsArray calls Can switch to using WhereAsArray(ImmutableAray) instead of the IEnumerable overload Can use the pooling support in MetadataUnifyingSymbolHashSet Can avoid EditorBrowsableInfo allocation when input symbols is empty Can avoid the closure allocation that was present in RemoveOverriddenSymbolsWithinSet Even with all these changes, the net benefit didn't prove large from the speedometer run, but I've done the work, it doesn't complicate things, and every bit helps. * Fix typos (#77316) * Refresh diagnostics when fading options change * Mix options into diagnostic data checksum * Apply suggestions from code review * Support 'view call hierarchy' on primary consstructor * Fix * Arrays * cleanup tests * Add test * Docs * NRT * Primary constructor * Add nuget to documentation (#77330) * Implement TypeScript ExternalAccess wrappers for AsynchronousOperationListener (#77318) * Update configs for 17.14 P2 snap (#77329) * wip * update configs for 17.14p2 snap * Update eng/config/PublishData.json Co-authored-by: Joey Robichaud <joseph.robichaud@microsoft.com> --------- Co-authored-by: Joey Robichaud <joseph.robichaud@microsoft.com> * Simplify * Disallow initializers on partial constructor definitions (#77275) * Partial events and constructors: public API (#77202) * Partial events and constructors: public API * Simplify code * Add more `sealed` * Add more asserts * Null annotate MetadataHelpers (#77309) * Null annotate MetadataHelpers Issue #76394 indicites there is a null reference happening inside `MetadataHelpers.GetInfoForImmediateNamespaceMembers`. The symptoms indicate it's non-deterministic (deterministic ordering or race condition). Decided to null annotate the type and a few surrounding calls to see if I could get lucky and spot the source of the NRE. Was not able to do this but decided to submit the annotations. * more * more * pr feedback * pr feedback * Allow expression body refactorings on non empty selections (#76969) Currently, all expression/body refactorings only work with an empty selection. I've started looking into #29495 which requests overload completion to set the selection upon commit. Upon commit (and setting the selection), it's desirable to be able to do the expression/body refactoring. This change should allow that. * PERF: Reduce the number of nodes walked during import completion commit. (#77305) ImportAdderService.AddImportsAsync was previously doing a full tree walk (and realizing all the nodes in the tree) during override completion commit. Noticed while looking at the csharp completion speedometer profile for the completion commit of override methods. Previously, this accounted for about 28% of CPU and 23% of allocations during CommitManager.TryCommit. With these changes applied, this enumeration is basically free. See PR for details on the performance numbers * Handle ModuleCancellationTokenExpression in AbstractFlow visitor (#77310) * Fix issue in watch window completion (#77204) * Fix issue in watch window completion The issue here is when the watch window contains for (int i = 0; i < 10; i++) [Console.WriteLine(i);] where [] denotes the current statement. In that case, the adjusted context should be at the start of Console.WriteLine. Additionally, a semicolon should not be placed in the generated projection buffer before debuggerMappedSpan, as that would place debuggerMappedSpan outside the for loop. Fixes #42718 * Don't realize the SourceText in SyntaxTree.OverlapsHiddenPosition if not needed (#77334) This shows up in override completion commit in the speedometer profiles as 12.5% of allocations and 11.6% of CPU under CommitManager.TryCommit. We can avoid needing the SourceText in this method if the line mappings in the tree are empty. With this change, OverlapsHiddenRegion no longer appears in either allocations or cpu usage during override completion commit. I've also verified that these costs associated with creating this sourcetext weren't incurred elsewhere (at least under CommitManager.TryCommit) See PR for more detailed numbers * Expose some functionality from SourceGeneratedDocumentUri to Razor * Update src/Tools/ExternalAccess/Razor/RazorUri.cs * add resource to automate github action merge PRs * Update resourceManagement.yml * Preserve encoding during DocumentState updates Fixes dotnet/sdk#46780 * Fix issue where we were crashing trying to get inheritance margin information for SG docs * Docs * Generate-Documentation Check if file is excluded (#77353) * check for excluded files * remove unused usings * fix indentation * PR Validation - allow insertion to run on commit that isn't latest (#77369) * allow insertion to run on commit that isn't latest * remove comments * Allow PR Val pipeline runs to opt-out of the latest commit validation * Update CI excludes paths to include some yml files * Update setup-pr-validation.ps1 * Apply suggestions from code review --------- Co-authored-by: Joey Robichaud <joseph.robichaud@microsoft.com> * Fix yml typos * Update PublishData.json after VS Snap (#77383) * Fix formatting in tests * Collection expr * Collection expr * Make selaed * Make selaed * Primary constructors * Fix out of bounds in text extent computation * Fix out of bounds in text extent computation * Fix issue where goto-def wouldn't go to the right place after invoking twice * [main] Update dependencies from dotnet/source-build-reference-packages (#77335) * Update dependencies from https://github.com/dotnet/source-build-reference-packages build 20250224.2 Microsoft.SourceBuild.Intermediate.source-build-reference-packages From Version 10.0.611901 -> To Version 10.0.612402 * Update dependencies from https://github.com/dotnet/source-build-reference-packages build 20250225.2 Microsoft.SourceBuild.Intermediate.source-build-reference-packages From Version 10.0.611901 -> To Version 10.0.612502 * Update dependencies from https://github.com/dotnet/source-build-reference-packages build 20250227.2 Microsoft.SourceBuild.Intermediate.source-build-reference-packages From Version 10.0.611901 -> To Version 10.0.612702 * Update dependencies from https://github.com/dotnet/source-build-reference-packages build 20250228.4 Microsoft.SourceBuild.Intermediate.source-build-reference-packages From Version 10.0.611901 -> To Version 10.0.612804 * Update dependencies from https://github.com/dotnet/source-build-reference-packages build 20250303.3 Microsoft.SourceBuild.Intermediate.source-build-reference-packages From Version 10.0.611901 -> To Version 10.0.615303 --------- Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> * Partial events and constructors: IDE (#77337) * Partial events and constructors: IDE * Fix event keyword recommender * Fix symbol suggestions * Fixup failing tests * Fixup SymbolKey * Improve code * Use `Equal` instead of `Same` in SymbolKey tests * Extend a test * Use collection expressions Co-authored-by: Cyrus Najmabadi <cyrus.najmabadi@gmail.com> --------- Co-authored-by: Cyrus Najmabadi <cyrus.najmabadi@gmail.com> * Partial events and constructors: compact error codes * Detect more null check patterns * Emit call to ThrowIfNull * Add string checks * Add string checks * Add work items * Improve performance of SyntaxReplacer.ReplaceNode (#77314) Improve performance of SyntaxReplacer.ReplaceNode This should allow better performance when replacing nodes. I've noticed overrides completion commit profiles as indicating a large amount of time/memory spent in their ReplaceNode calls. (about 18% of CPU time and 20% of memory allocations spent within CommitManager.TryCommit are attributable to ReplaceNode calls) See PR for more details on the performance benefit the speedometer achieves with this change. * Fix issue where we were changing semantics when converting to a collection expr * Work item * Update test * Update formatting tests * File scoped namespace * Fix projects * Configure main-vs-deps (#77408) * Configure main-vs-deps * Run every 3 hours * Don't use an implicit-object if the lang version doesn't support it * Add workitem * Fix bot name (#77436) * Auto merge PRs instead of just applying a label to them (#77438) * Small cleanup in the packages and editor factories (#77439) * Small cleanup in the packages and editor factories This area is going to get some larger changes in Dev18. As I'm waiting for that branch to become available, do a small cleanup in the area in anticipation of those changes. These changes are mostly cosmetic. The only change that is a bit more than that is the (Un)RegisterObjectBrowserLibraryManager changes to not be sync. This was done to remove a couple JTF.Runs in the code, and was done after verifying with JeffRo that our package disposal will indeed always be done on the main thread. * Update dependencies from https://github.com/dotnet/source-build-reference-packages build 20250305.3 (#77452) Microsoft.SourceBuild.Intermediate.source-build-reference-packages From Version 10.0.615303 -> To Version 10.0.615503 Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> * Remove workaround of WPF bug This shouldn't be needed anymore, and prevents some design time build breaks if NuGet restore wasn't able to get this package. Fixes #74923 * Fix design time build error in Roslyn.VisualStudio.Setup The MSBuild task invocation here is a copy/paste from earlier in the file, but the item group renamed so it didn't conflict. The rename was in complete, so the use in the Properties property was still using the old name; this had the effect that we wouldn't set the TFM property, which would cause problems for multi-targeting projects. * [Rename flyout] Fixed cropping issue on small screens * Addressed feedback * Remove BoundConversion.OriginalUserDefinedConversionsOpt property (#77465) * Update dependencies from https://github.com/dotnet/source-build-reference-packages build 20250306.1 (#77470) Microsoft.SourceBuild.Intermediate.source-build-reference-packages From Version 10.0.615503 -> To Version 10.0.615601 Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> * Update dev18 target branch (#77443) * Remove unnecessary imports * Update dependencies from https://github.com/dotnet/source-build-reference-packages build 20250310.1 Microsoft.SourceBuild.Intermediate.source-build-reference-packages From Version 10.0.615601 -> To Version 10.0.616001 * Mark partial events and ctors as complete (#77525) --------- Co-authored-by: Jan Jones <janjones@microsoft.com> Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: Cyrus Najmabadi <cyrusn@microsoft.com> Co-authored-by: David Barbet <dabarbet@microsoft.com> Co-authored-by: Ankita Khera <40616383+akhera99@users.noreply.github.com> Co-authored-by: David Wengier <david.wengier@microsoft.com> Co-authored-by: Rikki Gibson <rigibson@microsoft.com> Co-authored-by: Cyrus Najmabadi <cyrus.najmabadi@gmail.com> Co-authored-by: Rikki Gibson <rikkigibson@gmail.com> Co-authored-by: akhera99 <ankitakhera@microsoft.com> Co-authored-by: Joey Robichaud <joseph.robichaud@microsoft.com> Co-authored-by: Todd Grunke <toddgrun@microsoft.com> Co-authored-by: Youssef1313 <youssefvictor00@gmail.com> Co-authored-by: Joey Robichaud <jorobich@microsoft.com> Co-authored-by: Tomáš Matoušek <tmat@users.noreply.github.com> Co-authored-by: Amadeusz Wieczorek <amwieczo@microsoft.com> Co-authored-by: Charles Stoner <10732005+cston@users.noreply.github.com> Co-authored-by: Jared Parsons <jared@paranoidcoding.org> Co-authored-by: Sam Harwell <Sam.Harwell@microsoft.com> Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: Jason Malinowski <jason.malinowski@microsoft.com> Co-authored-by: Etienne Baudoux <etbaudou@microsoft.com> Co-authored-by: AlekseyTs <AlekseyTs@users.noreply.github.com> Co-authored-by: Jan Jones <jan.jones.cz@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
ImportAdderService.AddImportsAsync was previously doing a full tree walk (and realizing all the nodes in the tree) during override completion commit.
Noticed while looking at the csharp completion speedometer profile for the completion commit of override methods. Previously, this accounted for about 28% of CPU and 23% of allocations during CommitManager.TryCommit. With these changes applied, this enumeration is basically free.
*** Old CPU ***
*** New CPU ***
*** Old Allocations ***
*** New Allocations ***
