Attempt to move several data structures to be immutable #5930
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
dotnet-policy-service
bot
added
the
Community
ToddGrun
commented
Jul 22, 2024
ToddGrun
commented
Jul 22, 2024
ToddGrun
commented
Jul 22, 2024
ToddGrun
commented
Jul 22, 2024
ToddGrun
commented
Jul 22, 2024
ToddGrun
commented
Jul 22, 2024
ToddGrun
commented
Jul 22, 2024
ToddGrun
commented
Jul 22, 2024
src/NuGet.Core/NuGet.ProjectModel/PackageSpecReferenceDependencyProvider.cs
Outdated
Show resolved
Hide resolved
nkolev92
reviewed
Jul 23, 2024
src/NuGet.Core/NuGet.ProjectModel/CentralTransitiveDependencyGroup.cs
Outdated
Show resolved
Hide resolved
src/NuGet.Core/NuGet.ProjectModel/PackageSpecReferenceDependencyProvider.cs
Outdated
Show resolved
Hide resolved
src/NuGet.Core/NuGet.ProjectModel/TargetFrameworkInformation.cs
Outdated
Show resolved
Hide resolved
src/NuGet.Core/NuGet.ProjectModel/TargetFrameworkInformation.cs
Outdated
Show resolved
Hide resolved
src/NuGet.Core/NuGet.ProjectModel/TargetFrameworkInformation.cs
Outdated
Show resolved
Hide resolved
src/NuGet.Core/NuGet.ProjectModel/TargetFrameworkInformation.cs
Outdated
Show resolved
Hide resolved
src/NuGet.Core/NuGet.ProjectModel/TargetFrameworkInformation.cs
Outdated
Show resolved
Hide resolved
zivkan
reviewed
Jul 27, 2024
src/NuGet.Core/NuGet.Commands/Utility/AssetTargetFallbackUtility.cs
Outdated
Show resolved
Hide resolved
src/NuGet.Core/NuGet.Build.Tasks.Console/MSBuildStaticGraphRestore.cs
Outdated
Show resolved
Hide resolved
...uGet.CommandLine.XPlat/Commands/PackageReferenceCommands/AddPackageReferenceCommandRunner.cs
Show resolved
Hide resolved
src/NuGet.Core/NuGet.ProjectModel/CentralTransitiveDependencyGroup.cs
Outdated
Show resolved
Hide resolved
nkolev92
previously approved these changes
Aug 1, 2024
There was a problem hiding this comment.
I don't really have any feedback here.
This is a great change and something we've wanted to do for a while.
This should allow us to improve the no-op calculation eventually since we're primarily dealing with immutable objects (nearly all of them are I think).
We'll want to run some manual validation and get sign off as this is a binary breaking change.
There are ways we can reduce that footprint (by obsoleting certain method and using the new ones only), but some places, like the types becoming immutable there's simply not other way.
zivkan
force-pushed
the
SwitchToImmutable
branch
from
0fd3911
to
679d1af
Compare
kartheekp-ms
reviewed
Aug 8, 2024
|
Team Triage: Given the recent chnages in e68fb0f, we want to minimize the code churn in the restore area. We'd need to rebase closer to when we'd be ready for the next release. |
nkolev92
added
the
Merge next release
dotnet-policy-service
bot
added
the
Status:No recent activity
3 tasks
nkolev92
removed
the
Merge next release
dotnet-policy-service
bot
removed
the
Status:No recent activity
microsoft-github-policy-service
bot
added
the
Status:No recent activity
nkolev92
force-pushed
the
SwitchToImmutable
branch
from
679d1af
to
3daa8d9
Compare
microsoft-github-policy-service
bot
removed
the
Status:No recent activity
nkolev92
force-pushed
the
SwitchToImmutable
branch
from
3daa8d9
to
262f923
Compare
zivkan
reviewed
Oct 7, 2024
src/NuGet.Core/NuGet.DependencyResolver.Core/Remote/RemoteDependencyWalker.cs
Outdated
Show resolved
Hide resolved
nkolev92
force-pushed
the
SwitchToImmutable
branch
3 times, most recently
from
ebb3f9d
to
1b476ad
Compare
|
@NuGet/nuget-client This is ready for review. |
nkolev92
force-pushed
the
SwitchToImmutable
branch
from
1b476ad
to
8c7e4e5
Compare
zivkan
previously approved these changes
Oct 16, 2024
|
|
||
| for (var i = 0; i < spec.Dependencies.Count; i++) | ||
| { | ||
| // existingDependency.LibraryRange.VersionRange = range; |
There was a problem hiding this comment.
This looks like old code (line 43) that was commented out during development, rather than an intentional comment. If it's an intentional comment, it needs improvement to be more understandable.
zivkan
approved these changes
Oct 16, 2024
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.
Bug
Fixes: NuGet/Home#13647
Reduces allocations during solution load in VS
Description
Modified several data structures to be immutable, such that the PackageSpec.Clone operation can allocate significantly less. The scenario is outlined in the bug, but it's essentially cutting a 2-3% of allocations of solution load from the solutions I've tested.
This change is a bit viral, and ended up being more involved than I had originally thought it would be, but it feels like this ends up not only being more performant, but also lends these data structures to be more sanely reasoned about.
*** before changes ***
*** with changes ***
PR Checklist