Skip to content

Add equality override for KeyValuePairComparer #56634

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 2, 2021

Conversation

johnthcall
Copy link
Contributor

I noticed that SortedDictionary copying was allocation more memory than iterating the initial dictionary and adding items one at a time. I realized that the optimization done as part of #45659 was not completely successful and we need to override Equals for KeyValuePairComparer so that SortedSets HasEqualComparer will pass to allow efficient deep copy.

dotnet/performance@main...johnthcall:johncall/SortedSetDeepCopy

Method Toolchain N Mean Error StdDev Ratio Allocated
SortedDictionaryCopy main 1 233.25 ns 14.931 ns 17.195 ns 1.00 392 B
SortedDictionaryCopy pr 1 179.63 ns 4.817 ns 5.547 ns 0.77 344 B
SortedDictionaryCopy main 10 1,353.38 ns 55.142 ns 63.501 ns 1.00 1,136 B
SortedDictionaryCopy pr 10 600.19 ns 30.233 ns 34.817 ns 0.44 944 B
SortedDictionaryCopy main 100 16,490.14 ns 785.823 ns 904.955 ns 1.00 7,664 B
SortedDictionaryCopy pr 100 4,805.14 ns 91.329 ns 89.697 ns 0.29 6,080 B
SortedDictionaryCopy main 1000 212,509.48 ns 10,768.851 ns 11,969.549 ns 1.00 72,512 B
SortedDictionaryCopy pr 1000 48,069.80 ns 1,324.823 ns 1,525.668 ns 0.23 56,576 B

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jul 30, 2021
Copy link
Member

@eiriktsarpalis eiriktsarpalis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@eiriktsarpalis eiriktsarpalis merged commit bb7e7f9 into dotnet:main Aug 2, 2021
@johnthcall johnthcall deleted the SortedDictionaryCopyV2 branch August 2, 2021 20:54
@drieseng
Copy link
Contributor

drieseng commented Aug 3, 2021

@johnthcall Did you create a PR for the benchmark?

@johnthcall
Copy link
Contributor Author

@drieseng I did not create a PR for the benchmark as I was just sharing the branch so that the reviewer could verify the validity of the benchmark results. Should a PR be made to cover these scenarios?

@eiriktsarpalis
Copy link
Member

Should a PR be made to cover these scenarios?

Adding the benchmark to dotnet/performance would be nice to have.

@ghost ghost locked as resolved and limited conversation to collaborators Sep 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants