Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Streamline default EqualityComparer and Comparer for Enums #21604

Merged
merged 1 commit into from
Dec 21, 2018

Conversation

jkotas
Copy link
Member

@jkotas jkotas commented Dec 19, 2018

This borrows the implementation strategy for these from CoreRT. It makes it both simpler (fewer types and lines of code) and faster in some cases since we always use the exact right underlying type.

E.g. The following micro-benchmark is 25% faster with this change:

enum MyEnum : byte { x, y };

var comparer = Comparer<MyEnum>.Default;

for (int i = 0; i < 100000000; i++)
{
    comparer.Compare(MyEnum.x, MyEnum.y);
    comparer.Compare(MyEnum.y, MyEnum.x);
}

@jkotas
Copy link
Member Author

jkotas commented Dec 19, 2018

cc @marek-safar

@Suchiman
Copy link

Just a nitpick but where T : Enum does not preclude classes so it would be possible to pass boxed enums, e.g. EnumCompareTo<Enum>(null, null). To avoid this, where T : struct, Enum should be used.

@jkotas
Copy link
Member Author

jkotas commented Dec 20, 2018

@Suchiman Thanks for taking a look

Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

Simpler and faster is a nice combination :)

@marek-safar
Copy link

Fabulous!

Is breaking serialization compatibility between 2.x and master concern?

@stephentoub
Copy link
Member

This shouldn't break compatibility because of the SetType that's used. @ViktorHofer can confirm, but I believe we have tests for this specifically.

@marek-safar
Copy link

Right, but how do you hit it if the type to deserialize is e.g. UInt64EnumComparer ?

@ViktorHofer
Copy link
Member

ViktorHofer commented Dec 20, 2018

A serialization payload will never contain the type UInt64EnumComparer because when such an object is serialized, it masks itself as an ObjectComparer<>. That also implies that if serialization is involved you won't benefit from these gains.

This borrows the implementation strategy for these from CoreRT. It makes it both simpler (fewer types and lines of code) and faster in some cases since we always use the exact right underlying type.

E.g. The following micro-benchmark is 25% faster with this change:

```
enum MyEnum : byte { x, y };

var comparer = Comparer<MyEnum>.Default;

for (int i = 0; i < 100000000; i++)
{
    comparer.Compare(MyEnum.x, MyEnum.y);
    comparer.Compare(MyEnum.y, MyEnum.x);
}
```

Undo accidental change

CR feedback
@jkotas jkotas merged commit 82e02f3 into dotnet:master Dec 21, 2018
@jkotas jkotas deleted the enum-comp branch December 21, 2018 17:48
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
…reclr#21604)

This borrows the implementation strategy for these from CoreRT. It makes it both simpler (fewer types and lines of code) and faster in some cases since we always use the exact right underlying type.

E.g. The following micro-benchmark is 25% faster with this change:

```
enum MyEnum : byte { x, y };

var comparer = Comparer<MyEnum>.Default;

for (int i = 0; i < 100000000; i++)
{
    comparer.Compare(MyEnum.x, MyEnum.y);
    comparer.Compare(MyEnum.y, MyEnum.x);
}
```

Commit migrated from dotnet/coreclr@82e02f3
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants