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: Public interface ISpanFormattable #26374

Closed
Tornhoof opened this issue Jun 3, 2018 · 17 comments · Fixed by #51086
Closed

API Proposal: Public interface ISpanFormattable #26374

Tornhoof opened this issue Jun 3, 2018 · 17 comments · Fixed by #51086
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Memory
Milestone

Comments

@Tornhoof
Copy link
Contributor

Tornhoof commented Jun 3, 2018

@davidfowl suggested in #26373 that I make a formal proposal.
Motivation
There is an internal interface ISpanFormattable
The only member is:
https://github.com/dotnet/corefx/blob/d7f120a01ac6ea09a2139c99e43d7811950d2f73/src/Common/src/CoreLib/System/ISpanFormattable.cs#L9

This interface is the Span version of IFormattable and is implemented by many of the primitive types to allow formatting into a Span. It would be useful make this interface public for users wanting to use TryFormat with these types or implement their own formatting methods for Span.

Example:
The Utf8Formatter for float currently is using ToString() to format float/double
https://github.com/dotnet/corefx/blob/d7f120a01ac6ea09a2139c99e43d7811950d2f73/src/System.Memory/src/System/Buffers/Text/Utf8Formatter/Utf8Formatter.Float.cs#L68

If the interface would be public it could replace the type constraint IFormattable in the method above and it would be possible to optimize the allocations in the method above, by using stackalloc and TryFormat. As double/float formatting is a complex process the utf8 version formats the value into a string and then copies the ascii characters into the utf8 byte array.

Alternatives
@davidfowl pointed out, that it could be possible to make the interface generic, e.g. ISpanFormattable<T> with ISpanFormattable<char> and ISpanFormattable<byte>
This would add quite a bit of additional work, e.g. moving pretty much everything from Utf8Formatter into the relevant types and/or writing new formatter methods. This idea is currently not in my scope, it might be interesting to revisit the idea after Utf8String is finished.
Having a public generic interface where char and byte (and maybe uint for UTF32) are the only valid type constraints is a bit weird imho, this might change with future constraint possibilities.

Intended Audience

  • Everyone formatting values to Span, e.g. serializers, logging libraries etc.

Open points

  • How to get the interface into System.Memory, do a conditional include for netstandard and type-forward for netcoreapp work?
@ahsonkhan
Copy link
Member

cc @KrzysztofCwalina, @atsushikan

@ahsonkhan
Copy link
Member

We have a very similar interface in corefxlab and I am wondering if there is a way to consolidate the two:
https://github.com/dotnet/corefxlab/blob/bb92f6655bf3c014a8190c5f7a4c47d4746f2d37/src/System.Text.Primitives/System/Text/IBufferFormattable.cs#L21

@KrzysztofCwalina
Copy link
Member

KrzysztofCwalina commented Sep 12, 2018

I am in favor of adding one of the interfaces to the platform. I think IBufferFormattable is more generally applicable. It can format in an arbitrary encoding.
I don't think we need a generic version. The Byte version can format any encoding and given Span<char> can be cast to Span<byte> reliably and cheaply (freely?), I am not sure we need a char version.

But, we would need to think what types would actually implement the interface.

@stephentoub
Copy link
Member

Why was this closed?

@stephentoub stephentoub reopened this Nov 16, 2018
@Tornhoof
Copy link
Contributor Author

I honestly can't remember, might have been a mistake or the assumption that IBufferFormattable would be a different proposal.

@JeremyKuhne
Copy link
Member

Having this interface would potentially make adding AppendFormat (or something similar) to ValueStringBuilder more useful/flexible.

@LYP951018
Copy link
Contributor

LYP951018 commented Nov 17, 2018

What's the value of charsWritten when Tryformat returns false? Total count of chars required or arbitrary value?
If the first case, we could optimize StringBuilder even more ... (After TryFormat returns false, make room with charsWritten size, and then call TryFormat again)

@Tornhoof
Copy link
Contributor Author

In the current implementations of the interfaces it's 0. There was a previous discussion about this, if I remember correctly, the assumption is that most of the time the buffer will be large enough and the overhead of calculating the actual necessary length isn't worth it. Calculating the length might in some cases mean, that the format operation, or a good part of it, is executed to obtain the length.
So a try/resize/repeat pattern is more performant.

@JeremyKuhne
Copy link
Member

dotnet/corefxlab#2595 has a usage for this on ValueStringBuilder as described above. We could also create constrained AppendFormat<T> type methods that would avoid boxing on all of our base types.

As part of this suggestion we would add this interface to any intrinsic types that we haven't already and additional common types such as: DateTime, DateTimeOffset, Guid, and TimeSpan.

Additionally we would add this to Enum.

@stephentoub
Copy link
Member

and additional common types such as: DateTime, DateTimeOffset, Guid, and TimeSpan

DateTime, DateTimeOffset, Guid, and TimeSpan all already implement it 😉

@JeremyKuhne
Copy link
Member

DateTime, DateTimeOffset, Guid, and TimeSpan all already implement it

Yep, just trying to set initial scoping expectations. 😁

@jkotas
Copy link
Member

jkotas commented Nov 28, 2018

From discussion at dotnet/corefxlab#2595 (comment) :

It would be useful to have the overall no-allocation high-performance formatting story figured out before we start making parts of it public. ISpanFormattable worked ok as internal implementation detail, but it may not be the right type to set in stone to support the public non-allocation high-performance string formatting story.

@KrzysztofCwalina
Copy link
Member

It would be useful to have the overall no-allocation high-performance formatting story figured out

In particular, I think we need to seriously look how this would work for UTF8 formatting.

@stephentoub
Copy link
Member

stephentoub commented Nov 28, 2018

In particular, I think we need to seriously look how this would work for UTF8 formatting.

This interface would not. We would need a separate interface for that.

@Gnbrkm41
Copy link
Contributor

I opened an issue some time ago about adding TryFormat to IFormattable as a DIM (#30547). I personally think that it wouldn't be too bad if we were to have it as part of the existing interface as behaviourally they do the same thing.

@stephentoub stephentoub modified the milestones: 5.0.0, Future Jun 17, 2020
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Apr 11, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Apr 15, 2021
@TylerBrinkley
Copy link
Contributor

Has this been API reviewed? Where are the comments regarding that and the update to the labels?

@davidfowl
Copy link
Member

#50601

@ghost ghost locked as resolved and limited conversation to collaborators May 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Memory
Projects
None yet
Development

Successfully merging a pull request may close this issue.