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

Support span parsing #1131

Open
jskeet opened this issue May 19, 2018 · 10 comments
Open

Support span parsing #1131

jskeet opened this issue May 19, 2018 · 10 comments
Assignees

Comments

@jskeet
Copy link
Member

jskeet commented May 19, 2018

IPattern<T> should support parsing ReadOnlySpan<char> if at all possible. (It will partly depend on framework support.)

@jskeet jskeet self-assigned this May 19, 2018
@jskeet
Copy link
Member Author

jskeet commented Apr 20, 2019

We'd probably want to be able to format into a Span<char> as well, at least for known-to-be-fixed-length patterns.

@carlosschults
Copy link
Collaborator

I'd like to have a try at this.

@jskeet
Copy link
Member Author

jskeet commented May 9, 2019

You're welcome to have a go, but I should warn you that I prototyped it a while ago, and it's going to be a pretty huge piece of work. If you're up for the challenge, go for it...

@carlosschults
Copy link
Collaborator

Got it. Can you give me some general directions? Would it be possible to share the code you prototyped?

@jskeet
Copy link
Member Author

jskeet commented May 9, 2019

I hadn't even got it compiling - and it was several branches ago, before C# 8 was publicly available. I don't think I've still got it, to be honest.

I do remember having to change ValueCursor to be a ref struct, which raised some issues in itself.

It's probably worth working through the text handling code to start with to make sure you understand it before looking at doing Span work.

@jskeet
Copy link
Member Author

jskeet commented Oct 20, 2019

@carlosschults: Did you make any progress on this? I don't know when I'm going to find some concerted time to work on 3.0, but this is one of the things I'd like to make progress on. If you've made progress yourself, that's great and I don't want to reproduce it - but if you haven't been able to work on it, that's fine and I'll have another go myself.

@carlosschults
Copy link
Collaborator

@jskeet I'm sorry, I haven't really been able to get started. Fell free to go ahead. Thanks!

@jskeet
Copy link
Member Author

jskeet commented Nov 22, 2019

I now have a prototype where all the tests still pass, which is encouraging :) I need to do more work to optimize though.

@jskeet
Copy link
Member Author

jskeet commented Jan 15, 2020

This is now effectively blocking 3.0 going GA, but I'm unlikely to have enough time to work out why the span-based call is slower than the string-based one, and it's not really production ready. Options are:

  • Keep delaying 3.0 until this is done
  • Release 3.0, then introduce a separate interface (e.g. ISpanPattern<T>) either as a sibling or a parent of IPattern<T>
  • Release 3.0, then introduce methods into the interface using default interface implementations - which may or may not even be feasible for a library targeting .NET Standard 2.0
  • Release 3.0 with "we're probably going to add more members to this interface" warnings (urgh)
  • Release 3.0, then add the methods into a 4.0 release

Nothing is particularly appealing to me right now. Will prototype what adding a second interface might look like.

@jskeet
Copy link
Member Author

jskeet commented Jan 31, 2020

It looks like anything with culture-specific text parsing effectively can't be as efficient with Spans as I'd like it to be, at least when targeting netstandard2.0. I'm tempted to go with the "release 3.0 and add a new interface later" option - which definitely works, even though it's ugly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants