Reduce allocations in RuntimeDescription and RuntimeDependencySet #5248
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
These two classes follow a similar construction and clone pattern, and the changes to each are similar. Previously each expected to receive non-null enumerables with input values, which would then be enumerated to materialise copies. In cases where these collections were empty, the allocations are avoidable. This changes the constructors to receive nullable values, allowing singleton collections to be used in such cases. Similarly, when cloning, if the collections are empty, nulls are passed so that these singletons are applied. Also when cloning, the known collection type is cast to to enable allocation-free enumeration of the source when constructing the clone object.
nkolev92
previously approved these changes
Jun 21, 2023
|
After a few of the other PRs were merged, there's conflicts @drewnoakes |
…-runtime-class-allocations
|
Merged in |
zivkan
approved these changes
Jun 29, 2023
sbomer
reviewed
Jul 19, 2023
| left.RuntimeIdentifier, | ||
| // If collections are empty, pass null to avoid allocations. | ||
| inheritedRuntimes.Count == 0 ? null : inheritedRuntimes, | ||
| newSets.Count == 0 ? null : newSets); |
There was a problem hiding this comment.
newSets passed to the new overload will no longer be case-insensitive.
There was a problem hiding this comment.
Indeed! Fixed in #5321
drewnoakes
added a commit
to drewnoakes/NuGet.Client
that referenced
this pull request
Jul 20, 2023
Fixes #12757 PR NuGet#5248 reworked how construction of `RuntimeDescription` instances happens, partly to reduce the number of redundant collection copies that occurred. As an example of that innefficiency, the prior `RuntimeDesciption.Merge` method would build a dictionary of merged `RuntimeDependencySet` instances, then call a `RuntimeDescription` constructor that would copy those into a new dictionary. The PR added a new private constructor that allowed the copy to be avoided. That change introduced a bug however. It turns out that during the redundant copy a case-insensitive key comparer was specified, while the `Merge` method's dictionary was case-sensitive. So if two different `RuntimeDependencySet` identifiers differed only by case, then the collection created internally within the `Merge` method would contain both instances, then the constructor call would merge them during the subsequent `ToDictionary` call. The fix is to ensure that the `Merge` method uses the correct `StringComparer` for `RuntimeDependencySet` identifiers, to preserve the prior behaviour. Several unit tests have been added to validate the behaviour of `RuntimeDescription.Merge`.
drewnoakes
added a commit
to drewnoakes/NuGet.Client
that referenced
this pull request
Jul 20, 2023
Fixes #12757 PR NuGet#5248 reworked how construction of `RuntimeDescription` instances happens, partly to reduce the number of redundant collection copies that occurred. As an example of that innefficiency, the prior `RuntimeDesciption.Merge` method would build a dictionary of merged `RuntimeDependencySet` instances, then call a `RuntimeDescription` constructor that would copy those into a new dictionary. The PR added a new private constructor that allowed the copy to be avoided. That change introduced a bug however. It turns out that during the redundant copy a case-insensitive key comparer was specified, while the `Merge` method's dictionary was case-sensitive. So if two different `RuntimeDependencySet` identifiers differed only by case, then the collection created internally within the `Merge` method would contain both instances, then the constructor call would merge them during the subsequent `ToDictionary` call. The fix is to ensure that the `Merge` method uses the correct `StringComparer` for `RuntimeDependencySet` identifiers, to preserve the prior behaviour. Several unit tests have been added to validate the behaviour of `RuntimeDescription.Merge`.
8 tasks
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: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1837525
Fixes: NuGet/Home#12714
Regression? Last working version:
Description
The
RuntimeDescriptionandRuntimeDependencySetclasses follow similar construction and clone patterns, and the changes to each class here are similar.Previously each expected to receive non-null enumerables with input values, which would then be enumerated to materialise copies.
In cases where these collections were empty, the allocations are avoidable.
This changes the constructors to receive nullable values, allowing singleton collections to be used in such cases.
Similarly, when cloning, if the collections are empty, nulls are passed so that these singletons are applied. Also when cloning, the known collection type is cast to to enable allocation-free enumeration of the source when constructing the clone object.
PR Checklist
PR has a meaningful title
PR has a linked issue.
Described changes
Tests
Documentation