Conversation
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); |
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Folded
LGTM modulo comments. Thank you for the perf improvement! |
cc @jackfree |
Build servers out of space @dotnet-bot retest OSX x64 Checked Build and Test |
Could you please run the corefx tests on this? I see number of failures in my local run, like:
|
Yeah, could you stick a "do not merge" on? Will dig into it. |
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) |
The msbuild arguments have to be passed after
|
Looks good; I think it tested against my build... Was casting a non-enum, but compatible type from object in |
Nothing went wrong so not sure. Will back out last change; and see if it goes wrong to double check... |
Still one bug |
@jkotas Good to go! |
I think there is a bug with negative values:
Actual: -1 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); ; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: extra semi-colon
@jackfree removed the section altogether :) |
@benaadams thank you tremendously for making these changes! |
} | ||
|
||
[System.Security.SecuritySafeCritical] | ||
private unsafe static ulong GetValueAsULong(object value) |
There was a problem hiding this comment.
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)
?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ends up going through these various methods? https://github.com/dotnet/coreclr/blob/master/src/mscorlib/src/System/Convert.cs#L1413-L1417
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
Might be able to mitigate that.... |
Added whether its Flags to the cache |
👍 |
@@ -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>(); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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]
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
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 😉
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