-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Enum Improvements #20008
Comments
Updated the proposal to include some more implementation details. |
@danmosemsft I've updated the proposal to include more details about the new API defined. |
There already is a proposal for that too: dotnet/roslyn#16271. |
@svick Thanks, I've updated the proposal accordingly. |
I've updated the proposal to include generic versions of instance methods for performance reasons. |
@danmosemsft are you the CoreFX owner of this proposal. If so, could you let me know what needs to be done to get this API reviewed? |
@joperezr @AlexGhiondea are the owners of this area so one of them will either give feedback on it or flip to "api-ready-for-review". |
Thanks @danmosemsft. |
I've updated the proposal by removing the |
I see that there is already an |
If it's an interesting type that we could use as is (or modify without breaking existing users) then it's possible for us to move it down so this code could depend on i. |
Excellent, I'll update my proposal accordingly. |
@joperezr @AlexGhiondea I believe I'm done with my proposal updates. When you have some time could you please look over this proposal? |
I like the proposal a lot. public static TEnum RemoveNotValidFlags(this TEnum value) where TEnum : struct, Enum; See #18190 |
For that case I'd suggest doing this. myFlagEnumValue = myFlagEnumValue.CommonFlags(FlagEnum.AllFlags<MyFlagEnum>()); I know it's less convenient but it seems too much of a specialized case to warrant a new extension method. |
Maybe as just a static method we could have the following. public static TEnum GetValidFlags<TEnum>(TEnum value) where TEnum : struct, Enum; and in use would be myFlagEnumValue = FlagEnum.GetValidFlags(myFlagEnumValue); Thoughts, @magol? |
You right, maybe it is not a good method for a General api arter all. |
@TylerBrinkley thanks for putting this together. It is a very interesting proposal that I am trying to better understand. I have a few questions
/cc @jkotas |
@AlexGhiondea thanks for looking at the proposal.
|
It may have good enough performance for some uses, but it does not have what I would consider great performance. Great performance would mean parity when you do the same with bitmask operations. |
The JitHelpers are other tricks that @AlexGhiondea is talking about... . |
That's true for the "extension methods in non-static classes" feature, but not for " T MyMethod<T>() where T : struct => Enum.Parse<T>(GetEnumNameSomehow()); I can see two ways out of this:
Neither option sounds great, but 1 is probably more practical, assuming it does not make sense to wait until |
From my performance testing of I've added a little more details to the implementation details of the proposal regarding the use of the generic type parameter |
Runtime throughput is not the only performance metric we are concerned about. The startup and code bloat is another one - using generic helper methods for bitmasks checks is many times (100x?) worse on this performance metric. |
That's a great point. You're right, it would be a breaking change if another generic method with a In regards to the implementation of these new methods now, option 1 is not really an option as that would mean no extension methods and would mean runtime type checks which would greatly decrease performance for the bitmask operation methods which should be near native in performance. If the breaking change is not acceptable I'm in favor of option 2 if possible or else wait for C#'s
Consuming the |
Is it possible to do any optimization by roslyn som that the calls to this method is replace with native bit operations at compile time? |
@magol that should be possible similar to what's proposed for |
As a proof of concept I've started migrating my code into |
@TylerBrinkley thanks for the update! I would be interested to learn some numbers once you have your proof of concept operational. |
@AlexGhiondea Currently, |
@terrajobst and @danmosemsft would it be better for me to break this proposal up into smaller chunks so that it could be api reviewed? |
I've moved the I've moved the generic API additions that don't necessitate the performance improvements of this implementation to #2364. I've moved the flag API additions to dotnet/corefx#34079. I've moved the PrimaryAttribute addition to dotnet/corefx#34080. This ticket will remain for a few performance API's and as the basis for the improved performance implementation. |
I've implemented the generic implementation proposed here without any API changes in coreclr#22161. |
Would it be doable to have I can think of two variants, 1) one where any integral value would be invalid, because it is meaningless. 2) The other is to shoot down invalid values, such as Ad 1) in my opinion it is fairly common to define some valid input that should be constrained to known strings as an enum. All that is probably needed is to check whether the first character (if any) is a letter or underscore. Especially for technical apps it is very common to have invariant strings that need to be parsed and to define those as enums. E.g. suppose a console application that takes a For variant 2, having to parse first and then check with something like |
That would be outside the scope of this issue so perhaps you may want to open a new one? You can achieve this today with Enums.NET by specifying only |
Kudos for this for starters - I just bumped into having to do |
What's about implementing |
@TylerBrinkley I'm not sure which approach you've taken to implement this but I just posted this comment which details a way to implement these operations with full bitwise operator speed. I thought I would mention it here as it turns out this appears to be the main discussion area for these. When the methods are written like my example, they get inlined directly to bitwise operations and the branches are eliminated by the JIT so it produces very efficient code. |
I'm not sure if you're interested in additional related features but I'd love to see support added to be able to enumerate values or names in the order they are defined (likely with an optional parameter on the GetNames and GetValues methods) since the current methods only enumerate in ascending value. |
Ha! It's funny to see that almost everyone implemented their well-performing enum library when it used to be so slow in the .NET Framework era. So did I, of course. :) I'm really happy to see the improvements made in .NET Core. I would've never created my alternative version if Anyhow, if it helps anything, feel free to re-use any solutions from it. Maybe it hasn't as many features as Tyler's version and my approach was also much simpler in some aspects, but who knows... maybe you can find some useful parts in it. |
How about this api? Humanizr could use this...
|
Returning a span is never an option because of lifetime issue. Any api with result in span would require caller to allocate the span. |
As with #14084 this isn't a direction we'd like to move forward on. The idiomatic bit operations are guaranteed to be efficient where-as these named methods aren't and potentially push users towards a pit of failure. |
Enums are essential commonly used types but have several areas in need of improvement. For each non-generic
Enum
method there should be an equivalent generic version.Rationale and Usage
Enum
's static methods are non-generic leading to the following issues.IsDefined
andGetName
.ToObject
andGetValues
.With this proposal implemented what used to be this to validate a standard enum value
now becomes this
With this implemented it will address #18063.
Proposed API
API Details
This proposal makes use of a
C#
language feature that needs to be added in order for this proposal to make the most impact.This proposal specifies extension methods within
System.Enum
and as such requiresC#
to allow extension methods within non-static classes as is proposed in csharplang#301. Promoting these to extension methods later would be a breaking change due to csharplang#665 but I feel this is acceptable.Alternatively, the extension methods could be defined in a separate static
EnumExtensions
class. This is uglier but would avoid this issue and the extension methods would be available immediately instead of needing to wait for a laterC#
version to support this.Open Questions
System.Enum
extension methods later an acceptable breaking change due to csharplang#665? If not should we wait for a laterC#
version that supports extension methods inSystem.Enum
or should we introduce a separateEnumExtensions
class?Implementation Details
This proposal stems from my work on the open source library Enums.NET which addresses each of these issues and will be the basis for an implementation. For efficiency this is how
Enums.NET
is implemented.First, using
Enum
as a generic type argument causes generic code-explosion in today's runtimes so it's essential that it's included sparingly. For this reason most logic is contained in anEnumCache<TInt, TIntProvider>
object whereTInt
is the underlying type andTIntProvider
is the underlying type's operations provider which implements the interfaceINumericProvider<TInt>
which provides the bitwise operations and other needed operations. Including theTIntProvider
as a type parameter allows the runtime to inline calls to its methods so there's no interface method dispatch, a technique described in this generic calculations article.Now how can one access this
EnumCache
object when the caller only has the enum type as a generic type argument? This is solved by the bridge-like objectEnumInfo<TEnum, TInt, TIntProvider>
which implements the interfacesIEnumInfo<TEnum>
for generic methods andIEnumInfo
for non-generic methods. It simply acts as a bridge to delegate calls from the interfaces toEnumCache<TInt, TIntProvider>
. In order to do that it needs to be able to efficiently convert values betweenTEnum
andTInt
. This is performed by the conversion methodsToInt
andToEnum
which are declared asextern
methods for efficiency so that they're simply defined as casts from one type to the other. These method's functionality can be achieved with theUnsafe.As
method. ThisEnumInfo<TEnum, TInt, TIntProvider>
is created via reflection upon it's first call and is stored in the static fieldEnums<TEnum>.Info
as anIEnumInfo<TEnum>
which is then called byEnums
's generic methods.For non-generic methods the
RuntimeType.GenericCache
property will be set to theIEnumInfo
to use.Updates
System.Enums
namespace and moved its members into theSystem
namespace.PrimaryAttribute
into theSystem.ComponentModel
namespace and switched to using the existingSystem.ComponentModel.AttributeCollection
.IsValid
method as it's meaning is rather subjective and promotedIsValidFlagCombination
as an extension method.GetAllFlags
toAllFlags
as it more closely resembles a property but can't be because it is generic. Similar naming toArray.Empty
.EnumComparer
as one can just useComparer<T>.Default
andEqualityComparer<T>.Default
instead.EnumMember
support API addition.ReadOnlySpan<char>
parse overloads.FlagEnum.IsFlagEnum
intoSystem.Enum
.Enum
constraint to existing methods.EnumMember
API addition into the separate issue dotnet/corefx#34077.PrimaryAttribute
addition to separate issue dotnet/corefx#34080.Format
to API Proposal: Add a Generic version of GetValues to Enum (probably GetName/GetNames) #2364 as well.Format
as it's no different fromToString(string format)
except it throws anArgumentNullException
for anull
format
parameter.The text was updated successfully, but these errors were encountered: