Skip to content
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

[API Proposal]: Add method TryFormat for Enum #57881

Closed
Tracked by #79053
leandromoh opened this issue Aug 21, 2021 · 18 comments · Fixed by #78580
Closed
Tracked by #79053

[API Proposal]: Add method TryFormat for Enum #57881

leandromoh opened this issue Aug 21, 2021 · 18 comments · Fixed by #78580
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime
Milestone

Comments

@leandromoh
Copy link

leandromoh commented Aug 21, 2021

Background and motivation

Since Span type was created a lot of types added TryFormat method to write the value as text into a destination span. In .NET 6 these methods were added for char and bool types, however for enum type I could not find anything. I suggest to add it.

I also would like to submit a PR for this.
My current implementation got good results:

BenchmarkDotNet=v0.13.1, OS=Windows 10.0.18363.1440 (1909/November2019Update/19H2)
Intel Core i7-8650U CPU 1.90GHz (Kaby Lake R), 1 CPU, 8 logical and 4 physical cores
.NET SDK=6.0.100-preview.6.21355.2
  [Host]     : .NET 6.0.0 (6.0.21.35212), X64 RyuJIT
  DefaultJob : .NET 6.0.0 (6.0.21.35212), X64 RyuJIT

Method N Mean Error StdDev Median Gen 0 Allocated
SPAN_Parse_text 100000 976.3 μs 36.55 μs 106.6 μs 926.1 μs - 128 B
SPAN_Parse_number 100000 1,949.5 μs 86.76 μs 255.8 μs 1,913.6 μs - 128 B
STR_Parse_text 100000 2,942.4 μs 99.17 μs 290.8 μs 2,806.8 μs 570.3125 2,400,257 B
STR_Parse_number 100000 4,418.8 μs 139.81 μs 405.6 μs 4,231.5 μs 1335.9375 5,600,602 B

API Proposal

namespace System {
    public abstract class Enum : ValueType, IComparable, IConvertible, IFormattable {

       // new methods
        public static bool TryFormat<TEnum>(TEnum value, Span<char> destination, out int charsWritten, ReadOnlySpan<char> format = default, IFormatProvider? provider = default) 
             where TEnum : struct, Enum;
     }
}

API Usage

Span<char> destination = new char[50];

var isSuccess = Enum.TryFormat(Color.LightBlue, destination, out var charsWritten);

Debug.Assert(isSuccess);
Debug.Assert(destination.Slice(0, charsWritten).SequenceEqual(nameof(Color.LightBlue)));

Risks

I dont see any risk

@leandromoh leandromoh added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Aug 21, 2021
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Aug 21, 2021
@leandromoh
Copy link
Author

@stephentoub
Copy link
Member

Since Span type was created a lot of types added TryFormat method to write the value as text into a destination span. In .NET 6 these methods were added for char and bool types, however for enum type I could not find anything. I suggest to add it.

These were added as instance TryFormat methods, implementing the ISpanFormattable interface. Enum could do that as well, but without assistance from the JIT it'll incur some of the same overheads you're seeing in your STR_Parse_text benchmark: those allocations are for boxing the enum in order to call ToString (for the known enum values, ToString itself won't allocate).

@ghost
Copy link

ghost commented Aug 22, 2021

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

Issue Details

Background and motivation

Since Span type was created a lot of types added TryFormat method to write the value as text into a destination span. In .NET 6 these methods were added for char and bool types, however for enum type I could not find anything. I suggest to add it.

I also would like to submit a PR for this.
My current implementation got good results:

BenchmarkDotNet=v0.13.1, OS=Windows 10.0.18363.1440 (1909/November2019Update/19H2)
Intel Core i7-8650U CPU 1.90GHz (Kaby Lake R), 1 CPU, 8 logical and 4 physical cores
.NET SDK=6.0.100-preview.6.21355.2
  [Host]     : .NET 6.0.0 (6.0.21.35212), X64 RyuJIT
  DefaultJob : .NET 6.0.0 (6.0.21.35212), X64 RyuJIT

Method N Mean Error StdDev Median Gen 0 Allocated
SPAN_Parse_text 100000 976.3 μs 36.55 μs 106.6 μs 926.1 μs - 128 B
SPAN_Parse_number 100000 1,949.5 μs 86.76 μs 255.8 μs 1,913.6 μs - 128 B
STR_Parse_text 100000 2,942.4 μs 99.17 μs 290.8 μs 2,806.8 μs 570.3125 2,400,257 B
STR_Parse_number 100000 4,418.8 μs 139.81 μs 405.6 μs 4,231.5 μs 1335.9375 5,600,602 B

API Proposal

namespace System {
    public abstract class Enum : ValueType, IComparable, IConvertible, IFormattable {

       // new method
        public static bool TryFormat<TEnum>(TEnum value, Span<char> destination, out int charsWritten) 
             where TEnum : struct, Enum;
     }
}

API Usage

Span<char> destination = new char[50];

var isSuccess = Enum.TryFormat(Color.LightBlue, destination, out var charsWritten);

Debug.Assert(isSuccess);
Debug.Assert(destination.Slice(0, charsWritten).SequenceEqual(nameof(Color.LightBlue)));

Risks

I dont see any risk

Author: leandromoh
Assignees: -
Labels:

api-suggestion, area-System.Runtime, untriaged

Milestone: -

@leandromoh
Copy link
Author

leandromoh commented Aug 22, 2021

without assistance from the JIT it'll incur some of the same overheads you're seeing in your STR_Parse_text benchmark: those allocations are for boxing the enum in order to call ToString

While the JIT assistance does not happen, are you interested in a PR with an initial implementation? IMHO in this case is nice to provide the API (since it offers significante performance improvements) and improve implementation itself when possible.

@stephentoub
Copy link
Member

Thanks. A few thoughts:

  1. For new APIs, they need to be approved before we'll accept PRs. https://github.com/dotnet/runtime/blob/main/docs/project/api-review-process.md
  2. I'm not convinced a public static Enum.TryFormat is particularly helpful. I'd expect the vast majority use case for formatting enums will be as part of a composite formatting operation, in which case it'll generally be some utility code operating over arbitrary types, e.g. via ISpanFormattable, and won't know or want to or even be able to special-case enums to call the static Enum.TryFormat. Making Enum implement ISpanFormattable and exposing a public instance TryFormat does make sense to me, and is something I've wanted us to do, however without the JIT's assistance, every call to TryFormat will end up allocating, and that's a problem for code that expects an ISpanFormattable.TryFormat implementation to be cheap. It's often used in a loop with a growable destination buffer, and it'd be unfortunate if each call ended up allocating as part of that. In the majority case (where the enum value is a single, named value from the enum), it'd have been cheaper for the implementation to just call ToString.
  3. Your prototype, while neat, does code generation at run time to spit out a custom implementation for an enum that special-cases all known values. I don't believe we'd want to do such dynamic code generation for enums, as I don't believe the throughput benefits would warrant the startup and working set impact. There is a separate issue discussing this at Enum.ToString is slower than explicit implementation #57748.
  4. The prototype also uses System.Linq.Expressions. That's not something that can be used from corelib, which is where Enum is implemented.

@tannergooding tannergooding added needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration and removed untriaged New issue has not been triaged by the area owner labels Sep 7, 2021
@tannergooding tannergooding added this to the Future milestone Sep 7, 2021
@drieseng
Copy link
Contributor

@tannergooding Is assisting to implement ISpanFormattable (efficiently) for System.Enum on the radar for the JIT team?

@tannergooding
Copy link
Member

This hasn't even gone to API review yet so its not been considered. That would also be a question for the JIT/codegen owners.

As for whether this should goto API review, I'd like to hear weigh in from @jkotas since this would be exposing yet another generic overload to Enum and there may be a better alternative.

@jkotas
Copy link
Member

jkotas commented Feb 14, 2022

We have static Enum.Parse generic overloads like this. I do not see a problem with adding generic TryFormat for parity. The static TryFormat method won't automatically light-up ISpanFormattable, so it would be of a limited use.

Is assisting to implement ISpanFormattable (efficiently) for System.Enum on the radar for the JIT team?

You would want to add instance TryFormat method for that and then depend on the JIT to eliminate the boxing. It would one of those unreliable optimizations that depends on the JIT not running out of the optimization budget, etc.

@tannergooding tannergooding added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration labels Feb 14, 2022
@drieseng
Copy link
Contributor

drieseng commented Feb 14, 2022

@jkotas String interpolation with enums values would, for example, automatically benefit from an efficient implementation of ISpanFormattable

@jkotas
Copy link
Member

jkotas commented Feb 14, 2022

@jkotas String interpolation with enums values would automatically benefit from an efficient implementation of ISpanFormattable.

Right. Do you have any specific ideas for how the efficient implementation of ISpanFormattable should work?

@drieseng
Copy link
Contributor

drieseng commented Mar 1, 2022

@jkotas No, I was just echoing other comments and giving an example of an area which would benefit. I'm well aware of my limits.

@terrajobst
Copy link
Member

terrajobst commented Mar 8, 2022

Video

  • Makes sense
  • It seems also desirable to also implement ISpanFormattable, this would be useful, for example, for interpolated strings
    • This would require work in the JIT to avoid the boxing overhead
namespace System
{
    public partial class Enum : ISpanFormattable
    {
        public bool TryFormat(Span<char> destination, out int charsWritten, ReadOnlySpan<char> format, IFormatProvider? provider);

        public static bool TryFormat<TEnum>(TEnum value, Span<char> destination, out int charsWritten) 
             where TEnum : struct, Enum;
     }
}

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Mar 8, 2022
@danmoseley
Copy link
Member

@leandromoh are you interested in working on this one?

@leandromoh
Copy link
Author

leandromoh commented Mar 15, 2022

@leandromoh are you interested in working on this one?

I have no previous experience about coding runtime/CLR @danmoseley.
There is some tutorial or something similiar for starters?
But being honest, I think this task is probably beyound my current knowledge

@danmoseley
Copy link
Member

@leandromoh my bad, I missed your comment.

Many of our long time contributors started by contributing some small changes. Perhaps you would like to do that first? If you are interested, you could look through https://github.com/dotnet/runtime/issues?q=is%3Aissue+is%3Aopen+label%3Aup-for-grabs for something that seems easy (there is actually an 'easy' label, but not applied to all easy things). There are good instructions in this repo explaining how to clone and build the repo, how to make changes and test them. I am happy to help you as you go along, if the change is in my knowledge area.

@danmoseley danmoseley added the help wanted [up-for-grabs] Good issue for external contributors label Apr 13, 2022
@heathbm
Copy link
Contributor

heathbm commented May 14, 2022

@danmoseley #69343 (comment) mentions that the API proposal is incorrect as it is missing the format specifier argument.

@leandromoh
Copy link
Author

leandromoh commented May 16, 2022

@danmoseley #69343 (comment) mentions that the API proposal is incorrect as it is missing the format specifier argument.

I edited the current propostal to add format e provider parameters.

@stephentoub stephentoub self-assigned this Nov 16, 2022
@stephentoub stephentoub removed the help wanted [up-for-grabs] Good issue for external contributors label Nov 16, 2022
@stephentoub stephentoub modified the milestones: Future, 8.0.0 Nov 16, 2022
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Nov 18, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Dec 6, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jan 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants