Avoid unnecessary lazy init in StructuralComparisons #101344
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
ghost
added
the
This was referenced Apr 21, 2024
danmoseley
approved these changes
Apr 21, 2024
jkotas
approved these changes
Apr 21, 2024
There was a problem hiding this comment.
- Some code out there may get broken by
StructuralComparisons.StructuralComparerimplementingIEqualityComparerorStructuralComparisons.StructuralEqualityComparerimplementingIComparer. - IL trimmer may end up keeping unusued interface implementation around now that both interfaces are implemented by the same type.
Structural equality is used very rarely, so it is unlikely for these concerns to be a problem in practice. I am mentioning them out of abundance of caution. An alternative would be to keep the two types and add static readonly field in each of the types to get the lazy initialization for free.
I'll switch to that |
Move the fields to the target types and make them readonly instead of manually lazy.
stephentoub
force-pushed
the
avoidlazyinit
branch
from
3426e57 to
b7769c4
Compare
This was referenced Apr 21, 2024
jkotas
approved these changes
Apr 21, 2024
matouskozak
pushed a commit
to matouskozak/runtime
that referenced
this pull request
Apr 30, 2024
* Avoid unnecessary lazy init in StructuralComparisons Move the fields to the target types and make them readonly instead of manually lazy. * Update StructuralComparisons.cs
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
The only benefit of having these be lazy in this manner is avoiding one of the two objects being constructed. We can instead just combine them into a singleton that's initialized in a static readonly.
Best reviewed without whitespace diff: https://github.com/dotnet/runtime/pull/101344/files?w=1