Skip to content
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

Rewrite Enum.HasFlags and Enum.Equals in C# #59514

Merged
merged 2 commits into from
Sep 24, 2021
Merged

Rewrite Enum.HasFlags and Enum.Equals in C# #59514

merged 2 commits into from
Sep 24, 2021

Conversation

jkotas
Copy link
Member

@jkotas jkotas commented Sep 23, 2021

No description provided.

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@marek-safar
Copy link
Contributor

What is the performance hit on Mono side? I recall we moved this to native due to bad codegen but it was long time ago.

@vargaz
Copy link
Contributor

vargaz commented Sep 23, 2021

The mono changes look ok.


TEnum[] ret = new TEnum[ulValues.Length];
for (int i = 0; i < ulValues.Length; i++)
ret[i] = (TEnum)ToObject(enumType, ulValues[i]);
Copy link
Member

Choose a reason for hiding this comment

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

We have the ToUInt64 generic helper that takes a ulong and produces a TEnum. Would it be worth adding the inverse and using it here to avoid roundtripping through a boxed object here? Or you don't think it's worthwhile since there's already the array allocation?

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason for this change is AOT friendliness. The non-generic GetValues method is not AOT friendly since it creates a new array type dynamically.

Yeah, I agree that this can be done in a better way. I am going to undo this change for this PR, and deal with it in next one.

Copy link
Member

Choose a reason for hiding this comment

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

Just linking for reference:
#44355 (comment)

Copy link
Member

@VSadov VSadov left a comment

Choose a reason for hiding this comment

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

Nice!

@jkotas
Copy link
Member Author

jkotas commented Sep 24, 2021

What is the performance hit on Mono side?

It is performance improvement for Mono.

using System;
using System.Diagnostics;

class Test
{
    static Enum e1 = StringSplitOptions.RemoveEmptyEntries;
    static Enum e2 = StringSplitOptions.TrimEntries;

    static void Main()
    {
        Stopwatch sw = new Stopwatch();
        for (;;)
        {
            sw.Restart();
            for (int i = 0; i < 100000000; i++)
                e1.HasFlag(e2);
            Console.WriteLine(sw.ElapsedMilliseconds);
        }
    }
}

Windows x64

Baseline: ~2500ms per iteration
This change: ~1600ms per iteration (35% improvement)

(For reference, CoreCLR is around 530ms per iteration.)

@jkotas
Copy link
Member Author

jkotas commented Sep 24, 2021

Same benchmark for Enum.Equals Windows x64 Mono:
Baseline: 2200ms per iteration
This change: 1400ms per iteration

@jkotas
Copy link
Member Author

jkotas commented Sep 24, 2021

The failures are #59541

@jkotas jkotas merged commit 84150d9 into dotnet:main Sep 24, 2021
@jkotas jkotas deleted the enum branch September 24, 2021 13:14
@ghost ghost locked as resolved and limited conversation to collaborators Nov 3, 2021
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