Skip to content

Reduce Enum.GetEnumName overheads - #76162

Merged
stephentoub merged 3 commits into
dotnet:mainfrom
stephentoub:enumgetname
Sep 29, 2022
Merged

Reduce Enum.GetEnumName overheads#76162
stephentoub merged 3 commits into
dotnet:mainfrom
stephentoub:enumgetname

Conversation

@stephentoub

@stephentoubstephentoub commented Sep 26, 2022

Copy link
Copy Markdown
Member

This also helps Enum.ToString(), Enum.IsDefined, etc. Many enums are composed of sequential values starting at 0 (half of all enums exposed from corelib, for example). Today when looking up a value, we search the array of values, but if an enum is known to have such sequential values, we can instead just index into the array at the appropriate location.

privateDayOfWeek_dow=DayOfWeek.Saturday;[Benchmark]publicstringDOWSaturday()=>Enum.GetName(_dow);
MethodToolchainMeanErrorStdDevRatioCode Size
DOWSaturday\main\corerun.exe13.271 ns0.3248 ns0.5427 ns1.001,383 B
DOWSaturday\pr\corerun.exe7.111 ns0.2060 ns0.4065 ns0.54808 B

@stephentoubstephentoub added this to the 8.0.0 milestone Sep 26, 2022
@ghost

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

Issue Details

This also helps Enum.ToString(), Enum.IsDefined, etc. Many enums are composed of sequential values starting at 0 (half of all enums exposed from corelib, for example). Today when looking up a value, we search the array of values, but if an enum is known to have such sequential values, we can instead just index into the array at the appropriate location.

privateDayOfWeek_dow=DayOfWeek.Saturday;[Benchmark]publicstringDOWSaturday()=>Enum.GetName(_dow);
MethodJobToolchainMeanErrorStdDevRatio
DOWSaturdayJob-SKXTFP\main\corerun.exe12.523 ns0.1381 ns0.1224 ns1.00
DOWSaturdayJob-QEFEOM\pr\corerun.exe8.019 ns0.1229 ns0.1150 ns0.64
Author:stephentoub
Assignees:-
Labels:

area-System.Runtime, tenet-performance

Milestone:8.0.0

Comment threadsrc/libraries/System.Private.CoreLib/src/System/Enum.EnumInfo.cs Outdated
This also helps Enum.ToString(), Enum.IsDefined, etc. Many enums are composed of sequential values starting at 0 (half of all enums in corelib, for example, meet this criteria). Today when looking up a value, we search the array of values, but if an enum is known to have such sequential values, we can instead just index into the array at the appropriate location.

@jkotasjkotas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you are looking into squeezing more cycles from Enum formatting and parsing, you can try switching on underlying enum CorElementType as the very first thing (using a cached CorElementType for non-generic methods and intrinsic method for the generic methods), and then dispatch to specialized code for each underlying CorElementType.

The benefits of this approach should be:

  • Avoid conversions to / from ulong type
  • The array of values will be 2x smaller for the most common int underlying type
  • Eliminate generic code duplication for Enum methods since the switch will completely evaporate and the rest of the code will be all shared between different Enum types

This would mimic what the high-performance Enum packages out there use, except they typically do that using generic code stamping of the whole thing. Sharing the code per CorElementType should solve that problem.

Comment threadsrc/libraries/System.Private.CoreLib/src/System/Enum.cs Outdated
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Enum.cs Outdated
Comment threadsrc/coreclr/System.Private.CoreLib/src/System/Enum.CoreCLR.cs Outdated
@stephentoub
stephentoub merged commit 4a302db into dotnet:mainSep 29, 2022
@stephentoub
stephentoub deleted the enumgetname branch September 29, 2022 11:37
@drieseng

Copy link
Copy Markdown
Contributor

@stephentoub Would it possible to also support this for "sequential" enums starting from another value (eg. one), and keep that offset around? I generally prefer to use one for the first value as I don't want a (not nullable) enum field or property to have a (valid) default value if - for some reason - it was not assigned (while it should have been). Perhaps this is not common enough for you to care - and pay the extra cost of keeping an offset around using it where applicable - though, no problem.

@stephentoub

Copy link
Copy Markdown
MemberAuthor

Perhaps this is not common enough for you to care - and pay the extra cost of keeping an offset around using it where applicable - though, no problem.

Yeah, I'm not sure it's worth it. That's not to say we'd never do it, but it would add cost for everyone, and enums with sequential values starting at 1 are a lot less common than with values starting at 0.

@danmoseley

Copy link
Copy Markdown
Contributor

Do we have test coverage for both paths?

@drieseng

Copy link
Copy Markdown
Contributor

@stephentoub, would you consider a PR where the ValuesAreSequentialFromZero bool is replaced with an ulong? (or uint?) whose value would be would be null if the values are not sequential? Not asking you to guarantee that it would be merged of course.

@stephentoub

Copy link
Copy Markdown
MemberAuthor

That's the same as "keep that offset around" that you asked about earlier :)

@stephentoub

Copy link
Copy Markdown
MemberAuthor

Do we have test coverage for both paths?

Yes

@drieseng

Copy link
Copy Markdown
Contributor

@stephentoub, yeah indeed. I just meant to say that it's not necessary to have both the offset and the boolean, I wasn't clear enough. If I find a moment, I'll prepare a PR and we'll see how it affects performance (for both cases).

@stephentoub

Copy link
Copy Markdown
MemberAuthor

just meant to say that it's not necessary to have both the offset and the boolean

A ulong? is a Nullable<ulong> which is a ulong and a bool 😄

@drieseng

Copy link
Copy Markdown
Contributor

You can indeed see it that way. As I said, I'll see if I can prepare a PR. If it doesn't meet the bar, that's ok.

@ghostghost locked as resolved and limited conversation to collaborators Nov 13, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Runtimetenet-performancePerformance related issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@stephentoub@drieseng@danmoseley@marek-safar@gfoidl@jkotas