-
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
Rewrite Enum and add {ISpanFormattable}.TryFormat #78580
Conversation
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
e3a5c6e
to
55c10ae
Compare
408b87b
to
a2f5709
Compare
I have pushed a change that reduces this to 6.5kb with tiered compilation enabled and 1058 bytes with tiered compilation disabled. It can be further reduced to 894 bytes by copy&pasting TryFormatUnconstrained implementation into TryFormat instead of depending on the JIT inliner to do it for you. I have not done it in my commit, but it may be worth doing. It should not actually increase the IL size thanks to IL body folding. Both methods should have identical IL and this copy&paste would be actually a tiny IL size reduction. |
Thanks. Would you recommend looking to replace Unsafe.As in other places like this as well (separate from this PR)? Or you view this as a temporary thing until inlining can be improved? |
4bd9b27
to
108dc7c
Compare
Yes, I do not see any downsides. It is only possible when the source is stackallocated or in unmanaged memory.
It is not inlining problem. The problem is that the runtime has to materialize the generic |
Would it be worthwhile to replace usages of |
d3df156
to
4a3e3ec
Compare
4a3e3ec
to
2e5cf8e
Compare
Yes: #78741 |
re:WasmBuildTests failures, these are some Blazor AOT builds getting oomkill'ed when linking. I have seen these before, and have an open issue for similar tests. I will investigate these separately, and this PR doesn't need to be blocked. |
The iOS & tvOS legs are failing to AOT System.Net.Http.Json. Need to investigate further to determine if it's related to this PR or not.
|
@steveisok, let me know if/how I can help your investigation. |
I have opened #79279 on this. |
This reverts commit 62f3eb2.
I tried reverting this in CI, and that revert PR still hits the failures:
so it's not this PR. |
@stephentoub We noticed some enum order changes when updating ASP.NET Core to the latest runtime bits. The dependency update has been blocked for a long time, so it's hard to know precisely when the change happened. This PR might be the cause. See The enum values are fetched with reflection in Swashbuckle here - https://github.com/domaindrivendev/Swashbuckle.AspNetCore/blob/1b1d3daedea177895062780cf09f1755647775c4/src/Swashbuckle.AspNetCore.SwaggerGen/SchemaGenerator/SchemaGenerator.cs#L292 Was the order change intentional? |
What are the enum's values and what is the order difference you observed? |
Example:
Before this change: B C A Before this change, the values were always sorted as ulongs bit patterns. After this change, the values are sorted using their underlying type - signed, unsigned, floating point. |
I found it: |
Jan beat me by two minutes. |
Fixes #57881
Fixes #76157
Fixes #76398
Fixes #29266
This rewrites Enum to change how it stores the values data. Rather than having a non-generic EnumInfo that stores a ulong[] array of all values, there’s a generic EnumInfo that stores a TUnderlyingType[]. Then based on the enum’s type, every entry point maps to an underlying TUnderlyingType and invokes a generic method with that TUnderlyingType, e.g. Enum.IsDefined(…) and Enum.IsDefined(typeof(TEnum), …) will look up the TUnderlyingValue and then invoke Enum.IsDefinedPrimitive(typeof(TEnum)). In this way, a) we store an array strongly typed to the underlying value rather than storing the worst case ulong[], and b) we share implementations across generic and non-generic entrypoints while not having full generic specialization for every TEnum; worst case, we have only one generic specialization per underlying type, of which only 8 are expressible in C#. The generic entrypoints are able to do the mapping very efficiently, thanks to the recently added enum-based intrinsics. The non-generic entrypoints use the same switches on TypeCode/CorElementType they do today when doing e.g. ToUInt64.
@jkotas, I hope this meets your desires as expressed in #71590 (comment). If I've misinterpreted or you were hoping for something else, please let me know. There, you quoted the TryFormat method increasing working set by 7.5kb, which I was able to repro. With this PR, it's ~3.5kb with tiered compilation off... with tiered compilation enables, it's actually more, at ~9.5kb. Part of the issue appears to be how the repro itself is structured and how it interacts with JIT inlining, which is to say, not well; the JIT gives up inlining very early on, and that looks to contribute meaningfully to the size (plus little optimization happening in tier 0, hence the large number).
This also adds the static Enum.TryFormat as well as changing Enum to implement ISpanFormattable, with ISpanFormattable.TryFormat as an explicit implementation. These both share the same underlying TUnderlyingValue-based generic implementation, with just the entrypoint methods differing as to how the TUnderlyingValue mapping is done. And then that Enum.TryFormat is used in multiple interpolated string handlers to avoid boxing the enum; today it’ll be boxed as part of using its IFormattable implementation, and with this change, it both won’t be boxed and will call through the optimized generic path. This uses typeof(T).IsEnum, which is now a JIT intrinsic that’ll become a const true/false.
Overall, performance improves, in particular for the generic methods. Some of the non-generics get better as well. However, there is a repeatable regression here for other non-generic methods, in particular the non-generic Parse methods, and I haven’t been able to root cause that yet. @jkotas or @EgorBo, if you see anything obvious here, please let me know.
This includes the initial commit from #71590, although the primary thing still around from that are the tests that were added. Thanks, @heathbm.
@tannergooding and/or @dakersnar, if you could help look at the use of generic math, that'd be welcome.
I have a set of changes I plan to PR to dotnet/performance. Here are the results of those tests locally: