Content-Length: 417287 | pFad | https://github.com/dotnet/runtime/pull/76162

35 Reduce Enum.GetEnumName overheads by stephentoub · Pull Request #76162 · dotnet/runtime · GitHub
Skip to content

Reduce Enum.GetEnumName overheads #76162

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

Merged
merged 3 commits into from
Sep 29, 2022
Merged

Conversation

stephentoub
Copy link
Member

@stephentoub stephentoub commented Sep 26, 2022

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.

private DayOfWeek _dow = DayOfWeek.Saturday;

[Benchmark]
public string DOWSaturday() => Enum.GetName(_dow);
Method Toolchain Mean Error StdDev Ratio Code Size
DOWSaturday \main\corerun.exe 13.271 ns 0.3248 ns 0.5427 ns 1.00 1,383 B
DOWSaturday \pr\corerun.exe 7.111 ns 0.2060 ns 0.4065 ns 0.54 808 B

@stephentoub stephentoub added this to the 8.0.0 milestone Sep 26, 2022
@ghost
Copy link

ghost commented Sep 26, 2022

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.

private DayOfWeek _dow = DayOfWeek.Saturday;

[Benchmark]
public string DOWSaturday() => Enum.GetName(_dow);
Method Job Toolchain Mean Error StdDev Ratio
DOWSaturday Job-SKXTFP \main\corerun.exe 12.523 ns 0.1381 ns 0.1224 ns 1.00
DOWSaturday Job-QEFEOM \pr\corerun.exe 8.019 ns 0.1229 ns 0.1150 ns 0.64
Author: stephentoub
Assignees: -
Labels:

area-System.Runtime, tenet-performance

Milestone: 8.0.0

@ghost ghost assigned stephentoub Sep 26, 2022
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.
Copy link
Member

@jkotas jkotas left a comment

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.

@stephentoub stephentoub merged commit 4a302db into dotnet:main Sep 29, 2022
@stephentoub stephentoub deleted the enumgetname branch September 29, 2022 11:37
@drieseng
Copy link
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
Member Author

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
Member

Do we have test coverage for both paths?

@drieseng
Copy link
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
Member Author

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

@stephentoub
Copy link
Member Author

Do we have test coverage for both paths?

Yes

@drieseng
Copy link
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
Member Author

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

@ghost ghost locked as resolved and limited conversation to collaborators Nov 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants








ApplySandwichStrip

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier!      Saves Data!


--- a PPN by Garber Painting Akron. With Image Size Reduction included!

Fetched URL: https://github.com/dotnet/runtime/pull/76162

Alternative Proxies:

Alternative Proxy

pFad Proxy

pFad v3 Proxy

pFad v4 Proxy