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

Add TimeSpan Span-based APIs #22814

Closed
3 tasks
stephentoub opened this issue Jul 18, 2017 · 10 comments
Closed
3 tasks

Add TimeSpan Span-based APIs #22814

stephentoub opened this issue Jul 18, 2017 · 10 comments
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.DateTime
Milestone

Comments

@stephentoub
Copy link
Member

Separated out of https://github.com/dotnet/corefx/issues/21281 for tracking purposes.

  • Implement in System.Private.CoreLib in coreclr (corert shares the same files)
  • Expose from System.Runtime contract in corefx
  • Add tests to System.Runtime tests in corefx
namespace System
{
    public struct TimeSpan
    {
        public static TimeSpan Parse(ReadOnlySpan<char> input, IFormatProvider formatProvider = null);
        public static TimeSpan ParseExact (ReadOnlySpan<char> input, string format, IFormatProvider formatProvider, TimeSpanStyles styles = TimeSpanStyles.None);
        public static TimeSpan ParseExact (ReadOnlySpan<char> input, string[] formats, IFormatProvider formatProvider, TimeSpanStyles styles = TimeSpanStyles.None);

        public static bool TryParse(ReadOnlySpan<char> input, out TimeSpan result, IFormatProvider formatProvider = null);
        public static bool TryParseExact (ReadOnlySpan<char> input, string format, IFormatProvider formatProvider, out TimeSpan result, TimeSpanStyles styles = TimeSpanStyles.None);
        public static bool TryParseExact (ReadOnlySpan<char> input, string[] formats, IFormatProvider formatProvider, out TimeSpan result, TimeSpanStyles styles = TimeSpanStyles.None);

        public bool TryFormat(Span<char> destination, out int charsWritten, string format = null, IFormatProvider provider = null);}
}
@WinCPP
Copy link
Contributor

WinCPP commented Aug 15, 2017

@stephentoub didn't realize that dotnet/corefx#22406 was not up-for-grabs. How about this one? Quite smaller than dotnet/corefx#22403. Also it will help me get started after a gap. Thanks!

@WinCPP
Copy link
Contributor

WinCPP commented Aug 15, 2017

Ok. Analyzed what this potentially involves. I believe it goes all the way down to the tokenizer in the TimeSpanParser as well to replace string with ReadOnlySpan<char> ... and then do related changes to support both ReadOnlySpan<char> and existing string overloads at the interface? Am I on right track? Please let me know.

@stephentoub
Copy link
Member Author

Am I on right track?

That's generally the right approach, but this one is tricky as, in its current design, that would result in a span getting stored as a field in another type, and as a ref-like type, spans can only be stored as fields in other ref-like types, and at the moment there's no way to indicate that in C#. Even if the runtime doesn't currently complain, it will end up producing compilation errors later when the C# compiler verifies such things.

@WinCPP
Copy link
Contributor

WinCPP commented Aug 15, 2017

ref-like type, spans can only be stored as fields in other ref-like types ..... no way to indicate that in C# ..... it will end up producing compilation errors later when the C# compiler verifies such things.

I guess you are referring to FormatLiterals, a struct which will end up holding ReadOnlySpans...

@stephentoub
Copy link
Member Author

There are several, like TimeSpanTokenizer.

@WinCPP
Copy link
Contributor

WinCPP commented Aug 15, 2017

Ouch. Everything was compiling well 😆 and I was very happy! Hmmm...

Essentially we have to choose between these types which are structs that exist on stack and live with Strings coming and going out of existence on heap, vs., changing these structs to classes (reference type) so that ReadOnlySpan could replace all the Strings within. With latter, FormatLiterals, TimeSpanTokenizer would exist on heap and be subjected to GC but at least all those strings will go out of usage...? Latter solution adds few and removes many from GC and hence could actually help? I guess performance measurement post all changes would only be the way to decide...? I hope I am not missing some rule of thumb...

@stephentoub
Copy link
Member Author

We can't make those classes; that would result in allocations we need to avoid while parsing. It may be that we need to wait to tackle this issue until we have an updated compiler that will appropriately validate ref structs.
cc: @VSadov

@WinCPP
Copy link
Contributor

WinCPP commented Aug 15, 2017

Yup got it. We may live with TimeSpanTokenizer being allocated on heap, say, for a moment. However TimeSpanToken is surely not a candidate for becoming a class; it stores a sep, a String.

It may be that we need to wait to tackle this issue...

So should I take up something else, that I have got hang of what is to be done in such type of cases?

@stephentoub
Copy link
Member Author

So should I take up something else

Probably. How about the span-based TextReader/Writer methods, and the overrides on various types like StreamWriter?
https://github.com/dotnet/corefx/issues/22406
https://github.com/dotnet/corefx/issues/22409
etc.

You won't be able to do the buffer-based overloads yet, but you should be able to do the span-based ones.

@WinCPP
Copy link
Contributor

WinCPP commented Aug 15, 2017

Yup... was just typing 'may I' request in dotnet/corefx#22406 :-)

@stephentoub stephentoub self-assigned this Aug 25, 2017
@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 2.1.0 milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 21, 2020
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.DateTime
Projects
None yet
Development

No branches or pull requests

4 participants