Add Generic Enum.TryFormat() #71590
Add Generic Enum.TryFormat() #71590
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. |
ghost
added
the
community-contribution
|
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. |
|
Tagging subscribers to this area: @dotnet/area-system-runtime Issue DetailsPartial implementation of #57881 Benchmarksusing BenchmarkDotNet.Attributes;
using MicroBenchmarks;
namespace System.Tests
{
[MemoryDiagnoser]
[BenchmarkCategory(Categories.Libraries)]
public class Perf_Enum
{
public enum Colors
{
Red = 0x1,
Orange = 0x2,
Yellow = 0x4,
Green = 0x8,
Blue = 0x10
}
[Benchmark]
public void Format()
{
var destination = Enum.Format(typeof(Colors), Colors.Green, "F");
}
[Benchmark]
public void TryFormat()
{
Span<char> destination = stackalloc char[5];
Enum.TryFormat(Colors.Green, destination, out _, "F");
}
}
}
Profiling:Profiling code: https://gist.github.com/heathbm/079c4bea1dc541ffc12c91f283892875
Only allocations appear in the first run from:
|
|
The Enum tests are failing. |
@jkotas I have removed a Debug.Assert that was unreliable since that assert can now deal with externally provided spans, that can be larger than what was originally expected. I did not see any way to elegantly satisfy the assert without additional operations just to make the assert pass. |
|
@jkotas was your feedback addressed? |
|
It was addressed, thought there is still pretty significant code-bloat per instantiations that could be avoided by more significant refactoring. Are there places in this repo where this method can be used? |
My expectation is we'll use it at a minimum in our interpolated string handlers, to avoid the boxing and string allocations that come from falling back to using ToString today. We can start by calling this explicitly after doing some type testing (e.g. |
dakersnar
added
the
needs-author-action
|
@jkotas I added the "needs-author-action" label until your code-bloat comment is addressed. |
|
This pull request has been automatically marked |
|
This pull request will now be closed since it had been marked |
ghost
closed this
|
Discussed this with @jkotas, paraphrasing our conversation here to let others weigh in. Reopening for now. To address his code bloat comment, we would need something like #76398. He does not have a strong opinion on whether or not we should merge this in its current state, but we will likely have some perf regressions in existing APIs if we do. |
dakersnar
added
blocked
What kind of regressions, and do you have an example of where we'd expect this? |
There seems to be some extra work done on paths through the existing APIs. We should run enum microbenchmarks to see whether it is going to show up. |
I see. It seems like that was done in response to feedback that there was logical code duplication, and the changes are due to consolidating it? It'd be really nice to get TryFormat added. Would you prefer it be entirely new code, without touching existing code paths, and then subsequently we can look at how to unify as much as possible without regression?
👍 |
Ok with me. I am not happy about the amount of code bloat generated for each use of the generic enums parse and format methods. It is a pre-existing problem. |
|
I've assigned this to myself to land it. |
stephentoub
removed
blocked
stephentoub
force-pushed
the
implement-enum-tryformat
branch
from
bc25940 to
8299b28
Compare
- Add TryFormat XML docs - Make TryFormat's format parameter optional and allow an empty specifier - Make it an exception to specify invalid format specifier in TryFormat - Streamline TryFormat's default specifier case - Add missing cases where feasible for char/bool/nint/nuint underlying types - Reduce duplicate branches in hex formatting methods where e.g. int/uint cases can be shared - Refactor method implementations to better share return blocks - Add AggressiveInlining to places where call sites are finite / overhead is measurable - Remove checked multiplication from length calculation - Remove some branching from code to find name(s) in flags names - Revise multi-flag name writing to reduce bounds checking - Change all format specifier switches to use ASCII casing trick - Outline exception creation from generic methods to reduce specialized code bloat - Rename helper methods to make their functionality clearer - Tweak formatting for consistency within the file
- Use Enum.GetUnderlyingType instead of Type.GetTypeCode so that the intrinsic enables optimization at the call site - Add a generic cache with readonly statics that enables more generic specialization and branch elimination at call sites - Add more missing nint/nuint/char/etc. cases - Add a generic GetEnumName that can trim away half based on ValuesAreSequentialFromZero, that can get a hardcoded names address (once the frozen work is complete), and that can optimize out a branch in the inlined FindDefinedIndex - Remove a bounds check from GetEnumName on the find path - Expose an internal TryFormatUnconstrained without Enum/struct constraints for corelib interpolated string handlers to use
stephentoub
force-pushed
the
implement-enum-tryformat
branch
from
8299b28 to
f993036
Compare
|
I've made a bunch of changes here, in additional commits (I squashed all the existing commits down to one). As part of validating it, I fixed #76157. This also contributes to #76398, the main missing piece there being different types for the values array. And I realize the I have a change to push up to dotnet/performance to add more tests. The existing non-generic methods effectively remain unchanged throughput-wise, subject to noise, or get a bit better. The tests: [Benchmark]
[Arguments(Colors.Yellow)]
[Arguments(Colors.Yellow|Colors.Blue)]
[Arguments(Colors.Red|Colors.Orange|Colors.Yellow|Colors.Green|Colors.Blue)]
[Arguments(Colors.Yellow|(Colors)0x20)]
[Arguments(0x20)]
public string EnumToString(Colors value) => value.ToString();
[Benchmark]
[Arguments(SearchOption.TopDirectoryOnly)]
[Arguments(SearchOption.AllDirectories)]
[Arguments((SearchOption)(-1))]
public string EnumToString_SmallNonFlagsEnum(SearchOption value) => value.ToString();
[Benchmark]
[Arguments(UnicodeCategory.UppercaseLetter)]
[Arguments(UnicodeCategory.Control)]
[Arguments(UnicodeCategory.Format)]
[Arguments(UnicodeCategory.OtherNotAssigned)]
[Arguments((UnicodeCategory)42)]
public string EnumToString_LargeNonFlagsEnum(UnicodeCategory value) => value.ToString();
[Benchmark]
[Arguments(DayOfWeek.Sunday, "")]
[Arguments(DayOfWeek.Monday, "g")]
[Arguments(DayOfWeek.Tuesday, "d")]
[Arguments(DayOfWeek.Wednesday, "x")]
[Arguments(DayOfWeek.Thursday, "f")]
[Arguments(DayOfWeek.Friday, "X")]
[Arguments(DayOfWeek.Saturday, "D")]
[Arguments((DayOfWeek)7, "G")]
[Arguments((DayOfWeek)8, "F")]
public string EnumToStringWithFormat(DayOfWeek value, string format) => value.ToString(format);on my machine result in:
For TryFormat perf, the impact is directly visible via interpolated strings, which now use Enum.TryFormat: [Benchmark]
[Arguments(Colors.Red | Colors.Green)]
[Arguments(0x20)]
public bool InterpolateEnum(Colors value) => MemoryExtensions.TryWrite(s_scratch, $"{value} {value:g} {value:d} {value:x} {value:f}", out _);results in:
Other generic Enum methods also improve, e.g. private Colors _colorValue = Colors.Blue;
private DayOfWeek _dayOfWeekValue = DayOfWeek.Saturday;
[Benchmark]
public bool IsDefined() => Enum.IsDefined(_colorValue);
[Benchmark]
public bool IsDefined_NonFlags() => Enum.IsDefined(_dayOfWeekValue);
[Benchmark]
public string GetName() => Enum.GetName(_colorValue);
[Benchmark]
public string GetName_NonFlags() => Enum.GetName(_dayOfWeekValue);
[Benchmark]
public string[] GetNames() => Enum.GetNames<Colors>();results in:
|
| @@ -24,7 +24,7 @@ | |||
| namespace System | |||
| { | |||
| [Serializable] | |||
| [System.Runtime.CompilerServices.TypeForwardedFrom("mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089")] | |||
| [TypeForwardedFrom("mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089")] | |||
| public abstract partial class Enum : ValueType, IComparable, IFormattable, IConvertible | |||
There was a problem hiding this comment.
I'm sure this is deliberate, but why not implement ISpanFormattable?
There was a problem hiding this comment.
Every call to TryFormat would box the enum, just as every call to ToString() today boxes the enum. To avoid that would require non-trivial work in the JIT to rewrite calls from the instance method to something else. Unlike GetHashCode and HasFlags, which are special-cased, the implementation of formatting is complicated and not something we'd want to replicate in the JIT itself, so most likely we'd need a static generic TryFormat anyway the JIT could rewrite the calls to target. And we don't want to expose an ISpanFormattable implementation that's going to implicitly have such expense unless/until it can be eliminated.
There was a problem hiding this comment.
@stephentoub thanks for taking the time to respond, I really appreciate it! Why can't you use constrained calls with enums to avoid boxing (like what's done here, for example)?
PS. Last question, I promise :p
There was a problem hiding this comment.
No. Try just running:
for (int i = 0; i < 1000; i++) DayOfWeek.Monday.ToString();under an allocation profiler. You'll see it allocates 1000 enum objects. The implementation of ToString itself is defined on the System.Enum reference type.
There is still a very non-trivial code duplication per enum specialization. Here are some numbers for a simple enum with 3 values. The code that I have used to get these numbers is here: https://gist.github.com/jkotas/62f1680635e230a44bad2027ace5effa
To get the per-enum instantiation overhead down, I think the implementation would need to look like this: // This is the only method that is duplicated per enum instantiation
[MethodImpl(MethodImplOptions.AggressiveInlining)]
static bool TryFormat<TEnum>(TEnum value, Span<char> destination, out int charsWritten, [StringSyntax(StringSyntaxAttribute.EnumFormat)] ReadOnlySpan<char> format = default) where TEnum : struct, Enum
{
EnumInfo enumInfo = GetEnumInfo(typeof(TEnum));
// We may want to tweak the JIT so that it dead-code eliminates unreachable branches even in tier 0
Type underlyingType = typeof(TEnum).GetEnumUnderlyingType();
if (underlyingType == typeof(int)) return TryFormatPrimitive<int>(Unsafe.As<TEnum, int>(ref value), destination, charsWritten, format, enumInfo);
if (underlyingType == typeof(long)) return TryFormatPrimitive<long>(Unsafe.As<TEnum, long>(ref value), destination, charsWritten, format, enumInfo);
...
}
// This method is only instantiated per finite set of primitive types.
static bool TryFormatPrimitive<TUnderlyingType>(TUnderlyingType value, Span<char> destination, out int charsWritten, ReadOnlySpan<char> format, EnumInfo enumInfo) where TUnderlyingType: ISpanFormattable
{
...
} |
|
Thanks, will add it to the list of potential improvements for tier0 |
ghost
locked as resolved and limited conversation to collaborators
Partial implementation of #57881
Benchmarks
Code: https://gist.github.com/heathbm/59783b3d9e08d9556d7a86fa6ee2cd20
Before implementation:
After implementation:
No visible regression.
Profiling:
Profiling code: https://gist.github.com/heathbm/079c4bea1dc541ffc12c91f283892875
Tested with:
Only allocations appear in the first run from:
I believe there is nothing I can do to prevent these allocations on the first run as they generate cache.