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

Improve Enum.ToString performance #6645

Merged
merged 11 commits into from Aug 10, 2016
Merged

Conversation

benaadams
Copy link
Member

@benaadams benaadams commented Aug 7, 2016

Reduces allocations by 66%; increases performance by 600% (x7), over 12.1M per sec on single thread.

With these changes ToString no longer boxes the value and doesn't create an empty array for CustomAttributeRecords when the enum is not flagged; also caches whether the enum is flagged (skipping the reflection per call).

It still boxes the enum to Enum to make the ToString virtual call however.

Using the @jkotas enummark 😉

enum MyEnum { One, Two, Three, Four, Five, Six, Seven, Eight, Nine, Ten };
public static void Main(string[] args)
{
    (MyEnum.Seven).ToString();
    int start = Environment.TickCount;
    for (int i = 0; i < 10000000; i++)
    {
        (MyEnum.Seven).ToString();
    }
    int end = Environment.TickCount;
    Console.WriteLine((end - start).ToString());
}

Pre: 5828ms, 5828ms, 5829ms (1.7M/s)
Post: 828ms, 828ms, 828ms (12.1M/s)

Allocations pre

Allocations post

Counts in run are the same, just differ in images from profiling start, stop points

Resolves https://github.com/dotnet/coreclr/issues/3565

@benaadams
Copy link
Member Author

Shame you can't do something like

public static class EnumNonBoxExtensions
{
    static unsafe string ToString<TEnum>(this TEnum e) where TEnum : Enum
    {
        fixed (void* pValue = &JitHelpers.GetPinningHelper(e).m_data)
        {
            // ...
        }
    }
}


return result.ToString("X16", null);
case CorElementType.I1:
return ((byte)*(sbyte*)pValue).ToString("X2", null);
Copy link
Member

@jkotas jkotas Aug 8, 2016

Choose a reason for hiding this comment

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

It may be nice to fold to signed and unsigned cases together because of they are doing the same thing. It is fine for the unsafe low-level code like this to assume that signed and unsigned integers are stored in memory same way. There are number of similar switch statements (in VM) that make this assumption.

(It will also make the unchecked unnecessary.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Folded

@jkotas
Copy link
Member

jkotas commented Aug 8, 2016

LGTM modulo comments. Thank you for the perf improvement!

@jkotas
Copy link
Member

jkotas commented Aug 8, 2016

cc @jackfree

@benaadams
Copy link
Member Author

Build servers out of space

@dotnet-bot retest OSX x64 Checked Build and Test
@dotnet-bot retest Ubuntu x64 Checked Build and Test
@dotnet-bot retest Windows_NT x64 Release Priority 1 Build and Test
@dotnet-bot retest Windows_NT x86 ryujit Checked Build and Test

@jkotas
Copy link
Member

jkotas commented Aug 8, 2016

Could you please run the corefx tests on this? I see number of failures in my local run, like:

        System.Tests.EnumTests.Format(enumType: typeof(System.Tests.EnumTests+SimpleEnum), value: 1, format: "X", expected: "00000001") [FAIL]
           System.InvalidCastException : Unable to cast object of type 'System.Int32' to type 'System.Enum'.

@benaadams
Copy link
Member Author

Yeah, could you stick a "do not merge" on? Will dig into it.

@jkotas jkotas added the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Aug 8, 2016
@benaadams benaadams changed the title Avoid boxing in Enum.ToString [do not merge] Avoid boxing in Enum.ToString Aug 8, 2016
@benaadams
Copy link
Member Author

I'm not entirely sure how to run the corefx tests against coreclr, the instructions to override the runtime don't appear to be correct anymore https://github.com/dotnet/coreclr/blob/master/Documentation/building/testing-with-corefx.md

I'm copying the tests into a separate standalone exe and converting them to functions, I have most of them converted; though haven't finished them all yet to be sure. (found and fixed a bug in the Enum)

@jkotas
Copy link
Member

jkotas commented Aug 9, 2016

The msbuild arguments have to be passed after -- after recent changes by @maririos

build.cmd -- /p:BUILDTOOLS_OVERRIDE_RUNTIME:...

@benaadams benaadams changed the title [do not merge] Avoid boxing in Enum.ToString Avoid boxing in Enum.ToString Aug 9, 2016
@benaadams
Copy link
Member Author

benaadams commented Aug 9, 2016

Looks good; I think it tested against my build...

Was casting a non-enum, but compatible type from object in
static string Enum.Format(type, object , string) that was wrong.

@benaadams
Copy link
Member Author

Nothing went wrong so not sure.

Will back out last change; and see if it goes wrong to double check...

@benaadams
Copy link
Member Author

Still one bug

@benaadams benaadams changed the title Avoid boxing in Enum.ToString [No Merge] Avoid boxing in Enum.ToString Aug 9, 2016
@benaadams benaadams changed the title [No Merge] Avoid boxing in Enum.ToString Avoid boxing in Enum.ToString Aug 9, 2016
@benaadams
Copy link
Member Author

@jkotas Good to go!

@jkotas
Copy link
Member

jkotas commented Aug 9, 2016

I think there is a bug with negative values:

using System;

enum MyEnum {
    MyValue = -1
}

class My {
    static void Main() {
        MyEnum x = MyEnum.MyValue;
        Console.WriteLine(x.ToString());
    }
}

Actual: -1
Expected: MyValue

If there is no test coverage for this case in corefx, could you please add it?


return ((UInt32)value).ToString("X8", null); ;
case TypeCode.Int32:
return ((UInt32)(Int32)value).ToString("X8", null); ;
Copy link

Choose a reason for hiding this comment

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

Nitpick: extra semi-colon

@benaadams
Copy link
Member Author

@jackfree removed the section altogether :)

@jackfree
Copy link

jackfree commented Aug 9, 2016

@benaadams thank you tremendously for making these changes!

}

[System.Security.SecuritySafeCritical]
private unsafe static ulong GetValueAsULong(object value)
Copy link
Member

Choose a reason for hiding this comment

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

How is this different from static ulong ToUInt64(Object value) ?

Copy link
Member Author

Choose a reason for hiding this comment

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

That throws for negative numbers?

Copy link
Member

Choose a reason for hiding this comment

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

I do not think so ... it should work for negative numbers just fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I agree that ToUInt64(object) is not very efficient. I think it should be possible to merge ToUInt64 and GetValueAsULong(object) so that there is just one method that does it efficiently.

Copy link
Member Author

Choose a reason for hiding this comment

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

D'oh, I see what you mean

Copy link
Member Author

Choose a reason for hiding this comment

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

Renamed GetValueAsULong -> ToUInt64 also as its a better name

if (format == null || format.Length == 0)
format = "G";
formatCh = 'G';
else
Copy link
Member Author

Choose a reason for hiding this comment

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

Throw if format.Length > 1

Copy link

Choose a reason for hiding this comment

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

@benaadams, with these latest changes, has there been any impact on performance since you edited the description?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not significantly, most of the changes have been to lesser used paths (which have probably improved)

@benaadams
Copy link
Member Author

benaadams commented Aug 10, 2016

@jasonwilliams200OK rest of the time is elsewhere

enum-remain

@benaadams
Copy link
Member Author

Or perhaps better

@benaadams
Copy link
Member Author

e.g. 76.6% of the time is working out if its a flag Enum

flag-check

@benaadams
Copy link
Member Author

Might be able to mitigate that....

@benaadams
Copy link
Member Author

Added whether its Flags to the cache

@benaadams
Copy link
Member Author

benaadams commented Aug 10, 2016

@jkotas @jackfree @jasonwilliams200OK Woop! Updated the numbers, its now 7x faster than the current, rather than x1.5 before... 1.7M/s -> 12.1M/s

@benaadams benaadams changed the title Avoid boxing in Enum.ToString Improve Enum.ToString performance Aug 10, 2016
@jkotas jkotas removed the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Aug 10, 2016
@jkotas
Copy link
Member

jkotas commented Aug 10, 2016

👍

@jkotas jkotas merged commit 571b963 into dotnet:master Aug 10, 2016
@benaadams benaadams deleted the enumtostring branch August 10, 2016 15:30
@@ -324,6 +324,11 @@ internal unsafe static CustomAttributeRecord[] GetCustomAttributeRecords(Runtime
MetadataEnumResult tkCustomAttributeTokens;
scope.EnumCustomAttributes(targetToken, out tkCustomAttributeTokens);

if (tkCustomAttributeTokens.Length == 0)
{
return Array.Empty<CustomAttributeRecord>();

Choose a reason for hiding this comment

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

For my own learning: why not return static empty array here? Does Array.Empty cache?

Copy link
Member Author

@benaadams benaadams Aug 11, 2016

Choose a reason for hiding this comment

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

Array.Empty<T> is a static readonly empty array of type T shared among the entire framework, rather than each type having their own static new T[0]

@benaadams benaadams mentioned this pull request Sep 4, 2016
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
Reduces allocations by 66%; increases performance by 600% (x7)

With these changes ToString no longer boxes the value and doesn't create an empty array for `CustomAttributeRecords` when the enum is not flagged; also caches whether the enum is flagged.

It still boxes the enum to Enum to make the ToString virtual call however.

Using the @jkotas enummark 😉 

```csharp
enum MyEnum { One, Two, Three, Four, Five, Six, Seven, Eight, Nine, Ten };
public static void Main(string[] args)
{
    (MyEnum.Seven).ToString();
    int start = Environment.TickCount;
    for (int i = 0; i < 10000000; i++)
    {
        (MyEnum.Seven).ToString();
    }
    int end = Environment.TickCount;
    Console.WriteLine((end - start).ToString());
}
```
Pre: 5828ms, 5828ms, 5829ms (1.7M/s)
Post: 828ms, 828ms, 828ms (12.1M/s)

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