Implement PDG.WithProjectsRemoved #78428
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
This allows batch removal of projects from the ProjectDependencyGraph. This *should* eventually give us a small perf benefit, but only at the point where our projects can be disposed (or processed) in batches, which is not something we currently do.
ghost
added
Area-IDE
untriaged
|
@jasonmalinowski -- have you had a chance to take a look yet? |
src/Workspaces/Core/Portable/Workspace/Solution/ProjectDependencyGraph_RemoveProject.cs
Show resolved
Hide resolved
| // We know all the projects directly referencing 'projectId', so remove 'projectId' from the set of | ||
| // references in each of those cases directly. | ||
| if (existingReverseReferencesMap.TryGetValue(removedProjectId, out var referencingProjects)) | ||
| foreach (var removedProjectId in removedProjectIds) |
There was a problem hiding this comment.
I worry this might be potentially a bit slow if we're removing a lot of projects at once if the maps were to be full. The extreme case being if we're removing all projects, we'll call builder.MultiRemove n^2 times. There might be an improvement here just to build a list of all the projects that need to be updated, and then have MultiRemove take a hashset so we're creating fewer intermediate sets.
I'm 100% OK deferring this until we actually see a perf problem, or track it for futher investigation if it is one.
There was a problem hiding this comment.
As mentioned in the description, currently these arrays are size 1. If we are able to batch these removals, then it will definitely be a process of determining what the costs are in the batch and seeing how that can be addressed.
There was a problem hiding this comment.
As mentioned in the description, currently these arrays are size 1. If we are able to batch these removals, then it will definitely be a process of determining what the costs are in the batch and seeing how that can be addressed.
My assumption is by merging this work we're considering that optimization a foregone conclusion, otherwise why merge this?
There was a problem hiding this comment.
But if the answer is "we want to make this work, then we'll do batching, then we'll see how to optimize this more in real life" I'm OK with that. Maybe leave a TODO comment so it's clear that was the intent.
| foreach (var (id, _) in existingForwardReferencesMap) | ||
| { | ||
| builder.MultiRemove(id, removedProjectId); | ||
| foreach (var removedProjectId in removedProjectIds) |
There was a problem hiding this comment.
In this case a MultiRemove that takes multiple would be great.
There was a problem hiding this comment.
Can you elaborate? Are you expecting it would be more performant, or just slightly easier from a perf perspective.
There was a problem hiding this comment.
I'd expect it to be more performant, at least when removedProjectIds has mutliple things in it, since we'll only do one creation of a new ImmutableHashSet and one update in the ImmutableDictionary.Builder rather than n.
| // Finally, remove 'projectId' itself. | ||
| builder.Remove(removedProjectId); | ||
| foreach (var removedProjectId in removedProjectIds) | ||
| builder.Remove(removedProjectId); |
There was a problem hiding this comment.
You could call RemoveRange.
There was a problem hiding this comment.
ImmutableDictionary's AddRange takes in an IEnumerable, and thus allocates the enumerator. I figured it was pretty trivial to just do the loop at this level.
There was a problem hiding this comment.
My assumption was ImmutableDictionary could optimize further and avoid extra allocations than doing it one at a time to offset the enumerator allocations, but I see if this is a builder and not the ImmutableDictionary itself then:
Maybe then add a comment at each site that we're not using RemoveRange to avoid the enumerator allocation?
src/Workspaces/Core/Portable/Workspace/Solution/ProjectDependencyGraph_RemoveProject.cs
Show resolved
Hide resolved
| { | ||
| builder.MultiRemove(referencedProjectId, removedProjectId); | ||
| // If the removed project did not reference any other projects, nothing to do for this project. | ||
| if (!existingForwardReferencesMap.TryGetValue(removedProjectId, out var forwardReferences)) |
There was a problem hiding this comment.
Similar potential concerns on perf here, but we can wait and see if it's a problem.
src/Workspaces/Core/Portable/Workspace/Solution/ProjectDependencyGraph_RemoveProject.cs
Show resolved
Hide resolved
src/Workspaces/Core/Portable/Workspace/Solution/ProjectDependencyGraph_RemoveProject.cs
Show resolved
Hide resolved
src/Workspaces/Core/Portable/Workspace/Solution/ProjectDependencyGraph_RemoveProject.cs
Outdated
Show resolved
Hide resolved
jasonmalinowski
approved these changes
May 12, 2025
This was referenced May 15, 2025
333fred
added a commit
that referenced
this pull request
May 29, 2025
* Add XML Solution (SLNX) support to MSBuildWorkspace * Update `AbstractImplementAbstractClassCodeFixProvider.cs` source Signed-off-by: Emmanuel Ferdman <emmanuelferdman@gmail.com> * IN progress * IN progress * in progress * in progress * Simplify * Simplify * Cleanup * Rename * Push through initial description * Rename * Removed * Add back disposal * Linked source * Rename * Update docs * Remove * Add comment * Add comment * Fix * Delay loading of CodeRefactoringProvider's till absolutely needed * CR feedback and tests. * Fix comment * CR feedback * Fix crash in 'introduce variable' on top-level statements * CR feedback * Seal certain types in LSP layer * Seal certain types in LSP layer * Publish PR validation to internal feeds * PooledObjects cleanup (#78382) * moved condition above top-level statements * REvert * Seal * Gracefully handle span mapping failing * Fix information logs getting logged as debug in VSCode * Don't unnecessarily create a document when only the document state is needed. (#78463) Cyrus noticed this during a chat where I was considering a larger change to this code. Might as well make the simple/safe change while I consider the larger change too. Other small changes: 1) Early exit if no changed documents 2) Unrelated change adding Checksum overload as I noticed a high frequency caller during solution load (ProjectSystemProjectOptionsProcessor.ReparseCommandLineIfChanged_NoLock) had an ImmutableArray and it was getting boxed and enumerator alloced. * Ensure loghub collects the now verbose level logs * Adjust some more logging messages * Cancel running requests when the connection terminates * Shorten log category name * Extensions: handle extensions in VB SymbolDisplay (#78512) * Switch to non-scouting queue to unblock CI (#78521) * Fix test helper message to avoid NRE (#78532) * Collect data about which code fixes end up making changes that conflict with other code fixes * docs * Revert * Source package fixes (#78534) * Extract helper to make it easier for other features to request copilot analysis * lint * lint * Extensions: only count extensions for determining identifier (#78523) * Cache extension method import info per project ID * Remove telemetry * Use pattern matching * Lint * Add VB ref assembly to semantic search (#78537) * Add support for FieldRva to EnC delta (#78033) * [main] Source code updates from dotnet/dotnet (#78527) * [VMR] Codeflow c53bdd8-c53bdd8 [[ commit created by automation ]] * Update dependencies from https://github.com/dotnet/dotnet build 267468 * [VMR] Codeflow b422f78-b422f78 [[ commit created by automation ]] * Update dependencies from https://github.com/dotnet/dotnet build 267725 * Update dependencies from https://github.com/dotnet/dotnet build 267776 --------- Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> * Introduce -productBuild and -sourceBuild switches (#78519) * Introduce -productBuild and -sourceBuild switches * Update eng/build.sh Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com> * Update build.sh * Add new options for SB and PB switches * Remove leftover SB arg --------- Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com> * Document more steps in our C# release process (#78539) * Extensions: analyzer actions (#78319) * Update XAML EA to use DocumentUri instead of System.Uri * Update field value for NavBar Integration Tests (#78553) * fix * fix * Update GetFirstRelatedDocumentId to not return documents with the same path within the same project (#78475) GetFirstRelatedDocumentId had a mismatch in behavior for files within the same project that have the same path (shouldn't really happen, would cause compile errors). Previously, the code would return files from the same proejct when searching the cache, but not when doing the linear walk. Talked with Jason, and he indicated that it was better to have the code not return files from the same project, as callers wouldn't expect that. * Implement PDG.WithProjectsRemoved (#78428) This allows batch removal of projects from the ProjectDependencyGraph. This should eventually give us a small perf benefit, but only at the point where our projects can be disposed (or processed) in batches, which is not something we currently do. * Selectively persist the commandline to temporary storage (#78441) Only write command lines to temporary storage when they may later be needed. Persisting project command line arguments is surprisingly costly, as evidenced by the profile image below. As this storage is only needed when there is an effective ruleset path, we can limit when we persist to storage to be limited to that scenario. See PR for detailed performance information. * File based programs IDE support (#78488) Co-authored-by: Cyrus Najmabadi <cyrus.najmabadi@gmail.com> Co-authored-by: Jason Malinowski <jason@jason-m.com> * Update dependencies from https://github.com/dotnet/arcade build 20250513.2 Microsoft.DotNet.Arcade.Sdk , Microsoft.DotNet.Helix.Sdk , Microsoft.DotNet.XliffTasks From Version 9.0.0-beta.25255.5 -> To Version 9.0.0-beta.25263.2 * Extensions: pattern-based constructs (#78480) * Unset CompilerType from Csc (#78483) * Unset CompilerType from Csc * Remove CompilerType logic * Add Microsoft.CodeAnalysis.Extensions package (#78388) * Fix MoveType to create debuggable documents (#78554) * Move SpecializedCollections to Microsoft.CodeAnalysis.Collections namespace (#78466) * Add a reference to the tracking bug for a workaround * Fix embedded language classification inside multi-line string * Remove 'Opt' suffixes * Unify resource strings for special case checks * NRT * mIn progress * Simplify * Simplify * Remove localized strings from Collections source package (#78576) * Revert "Update XAML EA to use DocumentUri instead of System.Uri (#78555)" This reverts commit 2611c9a, reversing changes made to f4e6964. * Implement `Enum.IsDefined` check refactoring * Rename LSP messages for VisualStudio.Extensibility integration (#78598) Co-authored-by: Matteo Prosperi <maprospe@microsoft.com> * More improvements to source packages (#78587) * Editor configs for source packages * Add nullable enable * Include linked files in source packages * Suppress RS1024 * Remove duplicate nullability directives * Remove unnecessary extensions * Move FatalError and FailFast to Contracts package * Add disclaimer * Ensure we pass unique binlog paths to each BuildHost Otherwise if we launch multiple processes, they might step atop each other and cause locking issues. * Clean up our SpellingExclusions Somehow almost every line started with a UTF-8 BOM. This fixes it. * Track used assemblies of data section string literals (#78552) * Track used assemblies of data section string literals * Move reporting to module * Fix a failing test * Improve test * Add a comment * Support local functions in breadcrumbs * hotfix to fix restore and stop including bin/obj artifacts in directory with loose files (#78615) * Fix angle brackets in generics in hover * more directly walk the tree for local functions and add tests * Give .NET Framework build task in sdk different identity (#78584) closes #78565 * [VMR] Codeflow 015a854-015a854 [[ commit created by automation ]] * Update dependencies from https://github.com/dotnet/dotnet build 268651 No dependency updates to commit * [VMR] Codeflow 604dfc7-170498a [[ commit created by automation ]] * Update dependencies from https://github.com/dotnet/dotnet build 268722 No dependency updates to commit * review feedback * Fix option reading * Simplify equivalence keys * Make refactoring resilient against absence of `InvalidEnumArgumentException` type * Fix formatting * Update Workspace.MSBuild to reference Microsoft.Build.Framework. * LSP: Fix batch builds for file-based programs and fix `"dotnet.projects.binaryLogPath"` throwing an exception (#78644) Co-authored-by: Joey Robichaud <joseph.robichaud@microsoft.com> * Convert operand of `true`/`false` operator invoked as part of `&&`/`||`operator evaluation (#78629) Fixes #78609. * Extensions: rename syntax node (#78625) * Simplify * Use SolutionFileReader instead of calling out to BuildHost Removes the GetProjectsInSolution BuildHost API. Instead we can use the SolutionPersister library in processes to parse the projects from a solution file. * StaticLogMessage isn't pooling correctly Noticed this when looking at a profile and was curious why there were 3 MB of allocations of the StaticLogMessage type. LogMessage.Free sets _message to null before calling FreeCore, thus StaticLogMessage.FreeCore was never adding items back to it's pool. The ordering of LogMessage setting _message to null and calling FreeCore is important, so that can't be changed. Instead, add a flag to StaticLogMessage to indicate whether it's in a constructed state or not. * Fixing symbol publishing (#78650) These need to be manually listed given that they aren't published directly as a NuPkg * Compare symbols by metadata name when searching for eqivalent symbols in FAR engine * Extensions: disallow indexers (#78626) * Add untriaged label to all new issues (#78658) * Update resourceManagement.yml * Update resourceManagement.yml * Fixed crash in CSharpConvertToRecordRefactoringProvider. * Return arity check for method symbols * Fix doc comment * Update src/Features/Core/Portable/SymbolSearch/SymbolSearchOptions.cs * Fix * Make solution file reading async * Fix storage * Hot Reload: Handle document deletes (#78516) * Use SBRP version of M.CA for analyzers in the context of source build (#78668) * fix symbol publishing? (#78671) * Cleanup * Use ThrowIfFalse when resolving paths * Add test assertions * Applied code review suggestions. * Update Microsoft.Build.Tasks.CodeAnalysis.Sdk.csproj (#78681) * Fix nullable crash for field keyword in partial property (#78672) * Use VerifyDiagnostics format to report CompileAndVerify diagnostics (#78666) * Revert "Update Microsoft.Build.Tasks.CodeAnalysis.Sdk.csproj (#78681)" (#78687) This reverts commit 6d490de. * Revert "fix symbol publishing? (#78671)" (#78686) This reverts commit 2b6024e. * Add comment * [main] Source code updates from dotnet/dotnet (#78663) * [VMR] Codeflow c3c7ad6-c3c7ad6 [[ commit created by automation ]] * Update dependencies from https://github.com/dotnet/dotnet build 268973 No dependency updates to commit * [VMR] Codeflow d219c4f-d219c4f [[ commit created by automation ]] * Update dependencies from https://github.com/dotnet/dotnet build 269082 No dependency updates to commit --------- Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> * Add a test to verify that behavior is expected and bug no longer occurs * [main] Source code updates from dotnet/dotnet (#78692) * [VMR] Codeflow 2b6024e-2b6024e [[ commit created by automation ]] * Update dependencies from https://github.com/dotnet/dotnet build 269352 No dependency updates to commit --------- Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> * Modify ABWQ.AddWork to take in a ROS instead of an IEnumerable (#78691) I'm seeing this enumerator allocation as account for 50 MB (0.9%) in a customer trace for feedback ticket https://developercommunity.visualstudio.com/t/Razor-Development:-30-Second-CPU-Churn-A/10904811 This shows up a bit smaller in the C# editing speedometer test, but does show as 2.3 MB. Test insertion showed allocations were removed. See PR for more numbers and speedometer test results. * Add assert * Add test * Add support for 'fixed' * Fix crash with introduce local within a compilation unit * Sort * Handle error case: static ref field (No NullReferenceException) (#78707) * Organize * Run on UI thread * fix: RS2007 table borders * [main] Source code updates from dotnet/dotnet (#78710) * [VMR] Codeflow 533fde8-533fde8 [[ commit created by automation ]] * Update dependencies from https://github.com/dotnet/dotnet build 269499 No dependency updates to commit * Update dependencies from https://github.com/dotnet/dotnet build 269610 No dependency updates to commit --------- Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> * Make operator glyph consistent witht eh rest of the members * Fix * avoid not needed compilationUnit clone (#78676) * Always log PID * Update MicrosoftVisualStudioExtensibilityTestingVersion (#78661) * update extensibility version * remove added package? * update minversion * test * update vswhere version * update versions.props vswhere * update min version * update coreeditor version ranges * Add IsNull property to IAsyncToken (#78718) * Extensions: nullability analysis for property access (#78646) * Add helper for mapping between documents and hierarchy+itemids * Updat epersistence version * Do not convert tuple expression with error to a bad expression too early (#78645) * [main] Source code updates from dotnet/dotnet (#78724) * [VMR] Codeflow f5705c8-57b0396 [[ commit created by automation ]] * Update dependencies from https://github.com/dotnet/dotnet build 269724 No dependency updates to commit --------- Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> * [main] Update dependencies from dotnet/arcade (#78578) * Update dependencies from https://github.com/dotnet/arcade build 20250513.5 Microsoft.DotNet.Arcade.Sdk , Microsoft.DotNet.Helix.Sdk , Microsoft.DotNet.XliffTasks From Version 9.0.0-beta.25263.2 -> To Version 9.0.0-beta.25263.5 * Update dependencies from https://github.com/dotnet/arcade build 20250516.2 Microsoft.DotNet.Arcade.Sdk , Microsoft.DotNet.Helix.Sdk , Microsoft.DotNet.XliffTasks From Version 9.0.0-beta.25263.2 -> To Version 9.0.0-beta.25266.2 * Update dependencies from https://github.com/dotnet/arcade build 2025052.1 Microsoft.DotNet.Arcade.Sdk , Microsoft.DotNet.Helix.Sdk , Microsoft.DotNet.XliffTasks From Version 9.0.0-beta.25263.2 -> To Version 9.0.0-beta.25271.1 --------- Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> * Invoke `dotnet run-api` to obtain virtual project (#78648) Co-authored-by: David Barbet <dibarbet@gmail.com> --------- Signed-off-by: Emmanuel Ferdman <emmanuelferdman@gmail.com> Co-authored-by: Deepak Rathore (ALLYIS INC) <v-deerathore@microsoft.com> Co-authored-by: Emmanuel Ferdman <emmanuelferdman@gmail.com> Co-authored-by: Cyrus Najmabadi <cyrus.najmabadi@gmail.com> Co-authored-by: Cyrus Najmabadi <cyrusn@microsoft.com> Co-authored-by: Evgeny Tvorun <evgenyt@microsoft.com> Co-authored-by: Victor Pogor <victor.pogor@outlook.com> Co-authored-by: David Barbet <dabarbet@microsoft.com> Co-authored-by: Tomáš Matoušek <tmat@users.noreply.github.com> Co-authored-by: Jason Malinowski <jason.malinowski@microsoft.com> Co-authored-by: Andrew Hall <andrha@microsoft.com> Co-authored-by: Todd Grunke <toddgrun@microsoft.com> Co-authored-by: Julien Couvreur <julien.couvreur@gmail.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: Joey Robichaud <jorobich@microsoft.com> Co-authored-by: Ella Hathaway <67609881+ellahathaway@users.noreply.github.com> Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com> Co-authored-by: Ankita Khera <40616383+akhera99@users.noreply.github.com> Co-authored-by: Rikki Gibson <rigibson@microsoft.com> Co-authored-by: Jason Malinowski <jason@jason-m.com> Co-authored-by: Jan Jones <janjones@microsoft.com> Co-authored-by: DoctorKrolic <mapmyp03@gmail.com> Co-authored-by: Matteo Prosperi <41970398+matteo-prosperi@users.noreply.github.com> Co-authored-by: Matteo Prosperi <maprospe@microsoft.com> Co-authored-by: Jared Parsons <jared@paranoidcoding.org> Co-authored-by: Joey Robichaud <joseph.robichaud@microsoft.com> Co-authored-by: AlekseyTs <AlekseyTs@users.noreply.github.com> Co-authored-by: John Douglas Leitch <JohnLeitch@outlook.com> Co-authored-by: Matt Thalman <mthalman@microsoft.com> Co-authored-by: Bernd Baumanns <baumannsbernd@gmail.com> Co-authored-by: Thomas Shephard <thomas@thomas-shephard.com> Co-authored-by: DoctorKrolic <70431552+DoctorKrolic@users.noreply.github.com> Co-authored-by: David Barbet <dibarbet@gmail.com>
This was referenced May 31, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
This allows batch removal of projects from the ProjectDependencyGraph. This should eventually give us a small perf benefit, but only at the point where our projects can be disposed (or processed) in batches, which is not something we currently do.