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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve Enum.ToString performance #6645

Merged
merged 11 commits into from Aug 10, 2016
Merged

Improve Enum.ToString performance #6645

merged 11 commits into from Aug 10, 2016

Conversation

@benaadams
Copy link
Member

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 benaadams force-pushed the benaadams:enumtostring branch from 0c961f2 to 74b9d24 Aug 7, 2016
@benaadams
Copy link
Member Author

benaadams commented Aug 7, 2016

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);

This comment has been minimized.

Copy link
@jkotas

jkotas Aug 8, 2016

Member

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.)

This comment has been minimized.

Copy link
@benaadams

benaadams Aug 8, 2016

Author Member

Folded

case CorElementType.U8:
return (ulong)*(ulong*)pValue;
case CorElementType.R8:
return (ulong)*(double*)pValue;

This comment has been minimized.

Copy link
@jkotas

jkotas Aug 8, 2016

Member

Is this cast right for float/double based enums?

Float/double based enums are gray area. Some things do not quite make sense or work well for them, but they work fine for the most part.

This comment has been minimized.

Copy link
@benaadams

benaadams Aug 8, 2016

Author Member

I've folded the into the same sized type; so they will match the sorted list on (uint/ulong) bytes; but it doesn't look like float or double will produce a list?

This comment has been minimized.

Copy link
@benaadams

benaadams Aug 8, 2016

Author Member

Means won't be able to look up the name of a float or double enum?

This comment has been minimized.

Copy link
@jkotas

jkotas Aug 8, 2016

Member

I think you are right - this does not seem to be changing the behavior.

@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

@benaadams
Copy link
Member Author

benaadams commented Aug 8, 2016

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

benaadams commented Aug 8, 2016

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

@jkotas jkotas added the * NO MERGE * 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

benaadams commented Aug 9, 2016

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

benaadams commented Aug 9, 2016

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

benaadams commented Aug 9, 2016

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

benaadams commented Aug 9, 2016

@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?

@jackfree

This comment has been minimized.

Copy link

jackfree commented on src/mscorlib/src/System/Enum.cs in 42dce52 Aug 9, 2016

Why not just use "string" ?

This comment has been minimized.

Copy link
Member Author

benaadams replied Aug 9, 2016

Not sure I understand?

This comment has been minimized.

Copy link

jackfree replied Aug 9, 2016

I can see the utility in "var" if it's a complex type or one that is only used in-line in a function as a result of LINQ or something like that. Overall I despise "var" -- it makes code hard for the reader, and developers (at least on my team) use it ubiquitously because they are too lazy to use even simple primitive types.

If we know the type is "string", why not just use "string" instead of "var" ?


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

This comment has been minimized.

Copy link
@jackfree

jackfree Aug 9, 2016

Nitpick: extra semi-colon

return result.ToString("X16", null);
}

return ((UInt32)value).ToString("X8", null); ;

This comment has been minimized.

Copy link
@jackfree

jackfree Aug 9, 2016

Nitpick: extra semi-colon

@benaadams
Copy link
Member Author

benaadams commented Aug 9, 2016

@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)

This comment has been minimized.

Copy link
@jkotas

jkotas Aug 9, 2016

Member

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

This comment has been minimized.

Copy link
@benaadams

benaadams Aug 9, 2016

Author Member

That throws for negative numbers?

This comment has been minimized.

Copy link
@jkotas

jkotas Aug 9, 2016

Member

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

This comment has been minimized.

Copy link
@benaadams

This comment has been minimized.

Copy link
@jkotas

jkotas Aug 9, 2016

Member

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.

This comment has been minimized.

Copy link
@benaadams

benaadams Aug 9, 2016

Author Member

D'oh, I see what you mean

This comment has been minimized.

Copy link
@benaadams

benaadams Aug 9, 2016

Author Member

Renamed GetValueAsULong -> ToUInt64 also as its a better name

// all acceptable format string are of length 1
throw new FormatException(Environment.GetResourceString("Format_InvalidEnumFormatSpecification"));
}

char formatCh = format[0];
if (String.Compare(format, "G", StringComparison.OrdinalIgnoreCase) == 0)

This comment has been minimized.

Copy link
@jkotas

jkotas Aug 9, 2016

Member

I would think that the original code that just compared char was smaller and faster. Is there a good reason to change it to String.Compare instead?

This comment has been minimized.

Copy link
@benaadams

benaadams Aug 9, 2016

Author Member

Was copy paste from .ToString, will change .ToString to use the char instead?

This comment has been minimized.

Copy link
@jkotas

jkotas Aug 9, 2016

Member

Sounds good to me.

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

This comment has been minimized.

Copy link
@benaadams

benaadams Aug 9, 2016

Author Member

Throw if format.Length > 1

This comment has been minimized.

Copy link
@ghost

ghost Aug 9, 2016

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

This comment has been minimized.

Copy link
@benaadams

benaadams Aug 10, 2016

Author Member

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

benaadams commented Aug 10, 2016

Or perhaps better

@benaadams
Copy link
Member Author

benaadams commented Aug 10, 2016

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

flag-check

@benaadams
Copy link
Member Author

benaadams commented Aug 10, 2016

Might be able to mitigate that....

@benaadams
Copy link
Member Author

benaadams commented Aug 10, 2016

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 * label Aug 10, 2016
@jkotas
Copy link
Member

jkotas commented Aug 10, 2016

馃憤

@jkotas jkotas merged commit 571b963 into dotnet:master Aug 10, 2016
9 of 10 checks passed
9 of 10 checks passed
Linux ARM Emulator Cross Debug Build Build finished.
Details
CentOS7.1 x64 Debug Build and Test Build finished.
Details
FreeBSD x64 Checked Build Build finished.
Details
Linux ARM Emulator Cross Release Build Build finished.
Details
OSX x64 Checked Build and Test Build finished.
Details
Ubuntu x64 Checked Build and Test Build finished.
Details
Windows_NT x64 Debug Build and Test Build finished.
Details
Windows_NT x64 Release Priority 1 Build and Test Build finished.
Details
Windows_NT x86 legacy_backend Checked Build and Test Build finished.
Details
Windows_NT x86 ryujit Checked Build and Test Build finished.
Details
@benaadams benaadams deleted the benaadams:enumtostring branch Aug 10, 2016
@@ -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>();

This comment has been minimized.

Copy link
@danielmarbach

danielmarbach Aug 11, 2016

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

This comment has been minimized.

Copy link
@benaadams

benaadams Aug 11, 2016

Author Member

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]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can鈥檛 perform that action at this time.