Allow caller more control in stream decoding

[david-sledge]

I've been wanting this feature for a while, so I decided to prototype it and create a pull request.

[Lysxia]

Rather than STT, safer alternatives would be to specialize this to one specific monad (was state the original motivation?) or to use a different algorithm that does not involve STT.

[david-sledge]

Rather than STT, safer alternatives would be to specialize this to one specific monad (was state the original motivation?) or to use a different algorithm that does not involve STT.

State (along with reader) was secondary. My primary motivation was to multiple monads in a stack (I use IO and continuation frequently). Specializing is not ideal, since there'd have to be a different one not just for each monad, but each combination of monads.

[Lysxia]

STT is notably unsafe with continuations.

[Bodigrim]

text is a boot library, it cannot depend on STMonadTrans package, unfortunately.

I'm not very happy with the API of Data.Text.Encoding. The fact that the only way to abort decoding is to raise an error (and catch it elsewhere) is a joke. Ideally it should be possible to drive decoding from without, e. g.,

decodeUtf8With2 :: ByteString -> ByteString -> Either Int (Text, ByteString)

Then clients are free to interpret Int (the position, where decoding failed) as they like, in whatever monad.

[david-sledge]

text is a boot library, it cannot depend on STMonadTrans package, unfortunately.

Removed.

I'm not very happy with the API of Data.Text.Encoding. The fact that the only way to abort decoding is to raise an error (and catch it elsewhere) is a joke. Ideally it should be possible to drive decoding from without, e. g.,

decodeUtf8With2 :: ByteString -> ByteString -> Either Int (Text, ByteString)

Done (though not in that exact manner). Check out the test case t_decode_withM_error5 in tests/Tests/Properties/Transcoding.hs. There other examples, too, for Cont and Maybe. What's more is that the error handlers of the additional functions (the ones ending with M) take the byte position where the error occurred.

[Lysxia]

This still has the same unsafety as STT, since it's just an inlining of the STT logic. You can cause a segfault by specializing to ContT and by using a continuation twice.
Taking a callback is fundamentally flawed, so that the current implementation cannot simply be generalized to be in an arbitrary monad. Changing the API as bodigrim suggested seems like a better option (you may also remember the part that was decoded so far).

[david-sledge]

Ahhh... I see what you mean, and it'd be a problem with the list monad, too. However, I noticed that streamDecodeUtf8With returns a continuation in the Decoding data type. I looked how the risk of calling that continuation multiple times is mitigated. Essentially, the continuation a wholly separate invocation of a function. It calls runST in which the text array is created, populated, frozen, embedded in the Text data type, and returned without being modified any further.

So I applied the same concept around the call to the error handler. Before the error handler is called, it runs through the processes just before exiting; the text array is frozen, no longer modified, and the reference to last state value is discarded. Then after the error handler returns, it simulates runST with a call to runRW#, makes a copy of the text array where it left off, and continues its operations on the copy.

[Lysxia]

Changing the API to not take a callback at all and instead return an Either that tells you exactly where it stopped would still be much simpler, safer, and faster. I would prefer to consider that first.

Passing the State# token explicitly like this, while in an arbitrary monad, makes this quite difficult to review with confidence.

[david-sledge]

While I understand the desire to use Either, that doesn't really get me closer to my goal, which is why I was going for arbitrary monad which would meet my goal and allow for Either. Since insert-your-favorite-monads-here option has been removed from the table, I found myself at an impasse. So I took a step back, looked at the issue again, and reevaluated the problem.

With the requirements and constraints of

• Allow a means to abort decoding without raising an error.
• No STT.
• No arbitrary monad implementation.
• No manually passing the State# token.
• Indicate position in source ByteStream where the error occurs.

I think I found a solution that'll fit the bill.

[Lysxia]

This is actually close to what we were suggesting, and indeed much more satisfactory.

[david-sledge]

Excellent! A couple of questions:

• The byte position does not currently track across continuation invocations and instead resets back to zero. What's the opinion on whether it should track or the caller be responsible for tracking the cumulative byte position/total bytes read?
• What version should the @since annotation be for streamDecodeUtf8With'? 2.0.1, 2.1.0, or do I not need to worry about it yet?

[david-sledge]

For the first question, I went ahead and let the library track the absolute position for the caller.

[david-sledge]

Stream decoders added for utf-16 and utf-32, and an Either decoder for ASCII. I'll work on the lazy options next.

[Bodigrim]

We are getting closer to the final cut off before GHC 9.4. @Lysxia could you please complete your review on this? I don't have much bandwidth at the moment.

[Lysxia]

@Bodigrim How much time is left for the cut off? I'll allocate more cycles to shepherd this PR (sorry for dragging this @david-sledge !) but I feel like this is going to take a couple more rounds.

[Bodigrim]

I'm not sure exactly. Given that @bgamari has not chased us yet, it would probably take at least a week or more.

[Bodigrim]

Given #453 (comment), I'll cut text-2.0.1 from the current master, and this PR will be delayed until text-2.0.2. I'm happy to release text-2.0.2 fairly soon, in time for GHC 9.4.2. Sorry @david-sledge, I hope it's not too much of delay for your purposes.

[david-sledge]

This branch no longer uses simdutf anymore. Should all of it and references to it be removed, or is it worth modifying simdutf to have a determine_utf8_prefix_length function?

[Lysxia]

simdutf is there for performance, so if it is no longer used that needs to be justified by benchmarks. I suspect it's going to be difficult to compete with a vectorized implementation.

[Lysxia]

I "accepted" my own change to remove the "requested change" flag in the Github UI. Please review :)

And sorry for the messy git history. I'm assuming this is going to be squashed.

I added some optimizations so the benchmarks run at least as fast as before. Most of the time should be spent looping in C++, so there should be no change (confirmed by getting a screenful of "same as baseline" where "baseline" is master), except for the "tiny" benchmark where the work in entering and exiting the function dominates. Somehow the "LazyText" benchmark for the "tiny" input is consistently 25% faster even on my noisy machine. (The strict "Text" benchmark for "tiny" is "same as baseline"; it used to be 3x slower, and that's what my last round of commits fixes).

  Pure
    tiny
      decode
        LazyText: OK (1.20s)
          142  ns ± 7.0 ns, 28% less than baseline
      decode'
        LazyText: OK (0.39s)
          176  ns ± 5.6 ns, 27% less than baseline

Note that the benchmarks test valid UTF-8. When there are invalid bytes, the old code performs badly (quadratically) anyway as reported in issue #495. This PR fixes #495.

[Bodigrim]

Just couple of suggestions, otherwise looks very good.

[Bodigrim]

Great job!

[Lysxia]

Thanks to @david-sledge for getting this started and your great work!

[Lysxia]

The test is due to a mismatch between Unicode versions (text = Unicode 14, GHC 8.6 = ???). We probably should disable those tests for old GHC (and remain notified when new GHC/Unicode break this test again).

[Bodigrim]

Yeah, t_toCaseFold_char fails on '\66937', which is VITHKUQI CAPITAL LETTER FE' (U+10579) added in Unicode 14.0. The test should be guarded with #if MIN_VERSION_base(4,16,0).

@Lysxia could you please rebase?

[Lysxia]

Uh, I can if that's what we want to do. But I thought it would be cleaner to squash it. In the Github UI "Rebase and merge" is grayed out but we can still just select "Squash and merge". Is that okay?

[Bodigrim]

Ah, I see. Yes, squashing is OK, please go ahead.

[414owen]

Hi @david-sledge,

I want to decode some utf8 from a socket, and abort early if I get any invalid data.
The haddock states that the second element of the tuple returned by decodeUtf8{Chunk,More} is a

undecoded remainder of the given chunk, for diagnosing errors and resuming

It looks like the suffix can also contain input that wasn't parsed because the parser needs more bytes to determine whether it has a valid code point. Is that correct?
If so, {can,how do} I distinguish between a suffix that's due to a unicode error, and one that's due to a lack of input?

[Lysxia]

@414owen You can look at the third component of the returned triple Maybe Utf8State.

In the doc of decodeUtf8More:

Just the new state, or Nothing if an invalid byte was encountered (it will be within the first 4 bytes of the undecoded remainder).