Skip to content

Conversation

@AaronRobinsonMSFT
Copy link
Member

Summary

Remove language about possible optimizations in ValueType.Equals implementation.

Fixes https://github.com/dotnet/coreclr/issues/26833

/cc @jkotas @tannergooding

@AaronRobinsonMSFT
Copy link
Member Author

/cc @mairaw @rpetrusha

@mairaw mairaw added this to the September 2019 milestone Sep 25, 2019
@mairaw mairaw added the doc-bug Problem with the content; needs to be fixed label Sep 25, 2019
@mairaw
Copy link
Contributor

mairaw commented Sep 25, 2019

Changes look good @AaronRobinsonMSFT. Is this behavior true for both .NET Core and .NET Framework?

@AaronRobinsonMSFT
Copy link
Member Author

@mairaw Yes.

Copy link

@rpetrusha rpetrusha left a comment

Choose a reason for hiding this comment

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

Thanks, @AaronRobinsonMSFT. I'll merge this PR now.

@rpetrusha rpetrusha merged commit 02f8401 into dotnet:master Oct 4, 2019
@AaronRobinsonMSFT AaronRobinsonMSFT deleted the patch-5 branch October 7, 2019 01:03
@Melchy
Copy link

Melchy commented Oct 7, 2019

Is this correct? I'm probably wrong but the documentation now says that:
The default implementation calls System.Object.Equals on each field of the current instance and obj and returns true if all fields are equal.

But according to #13164 .Net core implementation of ValueType.Equals calls Equals on all fields only if the Type contains Reference type or Float or Double or user defined ValueType that overrides Equals method. If Type does not contain any of these Bitwise check is performed.

Also this beahavior doesn't seem to be true for both .NET Core and .NET Framework.
I tested this on .Net Framework 4.7.2 and it works the old way (bitwise check if type does not contain reference type). According to my informations .Net Framework 4.8 didn't change anything about ValueType.Equals so the algoritm should be diffrent in .Net Framework and .Net Core.

@jkotas
Copy link
Member

jkotas commented Oct 7, 2019

But according to #13164 .Net core implementation of ValueType.Equals calls Equals on all fields only if the Type contains

This is an performance optimization. It does not affect the observable behavior. We typically do not document performance optimizations.

.Net Framework

You are right that some versions of .NET Framework and early versions of .NET Core had a bug in this performance optimization that affected observable behaviors. Do you think it is important to document this bug?

@Melchy
Copy link

Melchy commented Oct 7, 2019

Do you think it is important to document this bug?

It may introduce breaking changes for people migrating from .NET Framework to .NET Core.

This is an performance optimization. It does not affect the observable behavior.

If I read this documentation I would immediately assume that this algorithm is very slow because it always uses reflection and therefore I should never use it. But it is actualy a viable choise in many situations.

But I must agree that this probably isn't good enough reason for adding it to documentation. I can imagine that .NET makes many undocumented optimizations such as this one. And I don't see why this specific one should be documented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc-bug Problem with the content; needs to be fixed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ValueType.Equals doesn't use bit comparsion

6 participants