Implement `readNatural` plus `readInt` and `readWord` for 8, 16, 32, 64 bit and native machine bit sizes #438

vdukhovni · 2021-11-12T02:11:46Z

Some applications want to read either unsigned or explicitly 64-bit integers
(e.g. warp). Provide the missing interfaces. Based on a suggestion by @Bodigrim the code has been moved to a single module enabling simpler maintenance and reduced duplication. A very helpful nudge...

sjakobi · 2021-11-12T18:04:47Z

Some applications want to read either unsigned or explicitly 64-bit integers
(e.g. warp).

Could you check whether the warp maintainers are actually inclined to use these additions?

vdukhovni · 2021-11-12T18:10:45Z

Some applications want to read either unsigned or explicitly 64-bit integers
(e.g. warp).

Could you check whether the warp maintainers are actually inclined to use these additions?

I don't think that's necessary. It is IMHO sufficient to observe that they did something really kludgey and didn't even check for overflows. Warp maintainers are unlikely to switch quickly to relying on an as yet unreleased version of bytestring, presumably they'll want support GHC 8.8 and 8.10 for some time still.

While seeing the warp kludge inspired the change, I think the result is cleaner code. The base functionality is in readWord64, which is now cleaner than the original readInt (no need to worry about signs and separate bounds for underflow vs overflow), and the other functions are simple wrappers. It is easy to add readInt32 and word32 if that is deemed desirable (maybe even Word/Int16 and Word/Int8, though I really don't expect demand for those).

So I think the new refactored code is justified on its own merits, but of course opinions may differ, so I'm open to further discussion...

sjakobi

I'm inclined to accept these additions – it does seem likely that they will be useful.

@Bodigrim What do you think?

I haven't looked at the implementation yet.

Bodigrim · 2021-11-12T22:33:13Z

I'm on board with these changes, looks like a good idea. Have not looked at implementation details yet.

vdukhovni · 2021-11-12T22:40:03Z

I'm on board with these changes, looks like a good idea. Have not looked at implementation details yet.

Thanks! In terms of implementation, it is basically a small refactor readInt -> readWord64, which actually leads to simplification of not having to deal with signs or sign-specific overflow/underflow detection. So the code is mostly the same, but cleaner.

With that out of the way, the rest of the API is then just wrappers that read a Word64 and if it is in range return the requested data type (Int, Word, Int64), and if not then Nothing. In particular it is easy, and perhaps even warranted (opinions sought) to add readInt32, readWord32, and perhaps even smaller sizes, though I don't see much demand for 16-bit decimals in real life. But perhaps reading IP4 addresses from a ByteString could benefits from a readWord8 used to reach each quad.

Don't know whether there's demand for readHexWord64 et. al. In network protocols we'd typically expect either binary wire forms or decimals (in HTTP, SMTP, ...) Hex serialisation forms are not terribly common...

Bodigrim · 2021-11-12T23:01:23Z

Data/ByteString/Char8.hs

+        | w <= wordMaxAsWord64
+        , let !i = fromIntegral w
+        , i >= 0 = Just (i, str)


Suggested change

| w <= wordMaxAsWord64

, let !i = fromIntegral w

, i >= 0 = Just (i, str)

| w <= intMaxAsWord64 = Just (fromIntegral w, str)

Seems a bit simpler, no?

I'd also suggest to introduce monomorphic helpers word64ToInt = fromIntegral, etc.

I wanted to be sure to force fromIntegral w, so that ideally GHC would elide the thunk. Hence !i = ...
Otherwise, you're right, your bounds check is equivalent, so I can drop one conditional. So it would be:

| w <= intMaxAsWord64 = let !i = fromIntegral w in Just (i, str)

vdukhovni · 2021-11-13T00:00:00Z

I just pushed a fixup commit, that addresses the int bounds check, and tidies up some comments, cosmetic issues. Whoever decides to merge should squash first (I can do that as a force push once all is approved if you like).

Bodigrim · 2021-11-13T00:02:49Z

Data/ByteString/Char8.hs

+    cvtneg (Just (w, str))
+        | w <= wordMaxAsWord64
+        , let !i = negate $ fromIntegral w
+        , i <= 0 = Just (i, str)


This could probably also use w <= negatedIntMinAsWord64 to save a comparison.

How do you cleanly express negatedIntMinAsWord64? The naive fromIntegral (minBound :: Int) :: Word64 isn't it...

My less naive version is: fromIntegral (fromIntegral (minBound :: Int) :: Word) :: Word64

Which, withTypeApplications I guess becomes:

intMinAsWord64 = fromIntegral @Word @Word64 $ fromIntegral @Int @Word minBound

Do you want to force "neg" into the name, or is it OK to leave it implicit that this is an absolute value?

vdukhovni · 2021-11-13T00:03:20Z

Oops, forgot to add the monomorphic helpers. I guess another fixup commit?
Do such monomorphic helpers help performance, or improve type safety? Or just readability?

Bodigrim · 2021-11-13T00:06:30Z

Readability (it is easier to validate correctness of casts when types are explicit) and type-guided refactoring (it's harder to introduce a mistake after changing some types).

Since we target GHC 8.0+, alternatively you can use {-# LANGUAGE TypeApplications #-} to spell out types of fromIntegral, if you like it better.

vdukhovni · 2021-11-13T00:08:26Z

Readability (it is easier to validate correctness of casts when types are explicit) and type-guided refactoring (it's harder to introduce a mistake after changing some types).

Since we target GHC 8.0+, alternatively you can use {-# LANGUAGE TypeApplications #-} to spell out types of fromIntegral, if you like it better.

OK, we're on the same page then. Just wanted to know I wasn't missing something deeper...

vdukhovni · 2021-11-13T00:33:57Z

I think that's everything noted so far...

vdukhovni · 2021-11-13T00:36:20Z

What we don't have is a 32-bit test environment to really make sure that 32-bit Ints are handled correctly, we presently only know this by code inspection, but perhaps 32-bit systems are no longer a concern? In any case, don't know how to specify that in CI, or find GHC builds for 32-bit systems, ...

vdukhovni · 2021-11-13T00:37:12Z

I guess if we actually add the corresponding readInt32 and readWord32 wrappers, then we'd know. :-)

Bodigrim · 2021-11-13T00:37:28Z

We do have 32-bit CI, both arm and intel: https://cloud.drone.io/haskell/bytestring/132

vdukhovni · 2021-11-13T00:47:50Z

I didn't know that bytestring has 32-bit CI tests. No worries.

About minBound :: Int as Word64, another way to write it, that uses explicit negation of a Word64 vaue is:

intMinAsWord64 = negate $ fromIntegral @Int @Word64 minBound

Any preference for that over the non-negating previous version?

Bodigrim · 2021-11-13T00:57:01Z

negate is probably more illuminating, but I do not have a strong preference.

vdukhovni · 2021-11-13T00:58:51Z

negate is probably more illuminating, but I do not have a strong preference.

Perhaps so, but I do like not calling negate on unsigned quantities. I'll do whatever Simon says. :-)

sjakobi

TypeApplications is a blessing!

Data/ByteString/Char8.hs

Data/ByteString/Internal.hs

tests/Properties/ByteString.hs

Data/ByteString/Lazy/Char8.hs

vdukhovni · 2021-11-13T04:25:02Z

Two more commits (ultimately to be squashed I think). These should address all outstanding issues.

Bodigrim · 2021-11-13T13:23:59Z

Data/ByteString/Char8.hs

@@ -1,7 +1,12 @@
+{-# LANGUAGE AllowAmbiguousTypes #-}


(I'll hate myself for this suggestion, when I'll need to backport something to bytestring-0.11, but...)

Let's move read{Word,Int}-related functionality into a separate internal module, so that the main one remains unpolluted by new language extensions.

OK, works for me. I don't think it needs to be visible to users, so it should be no problem...

Let's move read{Word,Int}-related functionality into a separate internal module, so that the main one remains unpolluted by new language extensions.

I don't mind how this has played out for this PR, but what's the motivation for not using TypeApplications in D.B.Char8?

vdukhovni · 2021-11-13T19:07:19Z

@Bodigrim's suggestion to factor out the code into a separate module turned out to be a major win. This pretty much eliminated all the code duplication, and made it easier to mimic the more efficient code path from the overflow-checked functions to similarly simplify and rework readInteger also adding readNatural.

I like the result. Sorry it is basically a clean slate now, but actually should be easy to review...

Data/ByteString/Lazy/ReadInt.hs

vdukhovni · 2021-11-13T19:31:29Z

Thanks for the prompt review, pushed a fixup.

Data/ByteString/Lazy/ReadInt.hs

tests/Properties.hs

tests/Properties/ByteString.hs

vdukhovni · 2021-12-11T00:05:33Z

And we comparing both n bits by n bits against n bits by 64 bits.
In one case it is:

n of 64 x 64, then n/2 of 128 x 128, then n/4 of 256 * 256, ..., finally one (n * 32) x (n * 32)
The other case is:
64 x 64, 64 * 128, 64 x 192, 64 x 256, ..., 64 x (n-1) * 64

Either way there are n-1 products, but the size distribution of operations is different.

Bodigrim · 2021-12-11T00:13:17Z

And how big is eps in 1 + eps?

Any eps > 0 will do.

And we comparing both n bits by n bits against n bits by 64 bits.

There is no difference in native backend, because it takes O(n*m) time to multiply n bits by m bits. But in libgmp backend, which is used by default on the majority of platforms, the former approach is faster.

See https://github.com/Bodigrim/fast-digits/blob/aef0438dba67d49b814857663c24e283ea95c2ed/src/Data/FastDigits.hs#L114-L119. It deals with a reverse scenario, when you split a large number into digits, but asymptotics are the same. It is faster to divide-and-conquer than chip digits one by one. Same for reconstructing a number from digits.

vdukhovni · 2021-12-11T00:30:36Z

So it sounds like I'll be reverting the most recent fixup, but I'm not sure what to say about asymptotic performance benefits of divide-and-conquer in the code commentary. I think it is difficult to be accurate...

Bodigrim · 2021-12-11T00:33:37Z

You can refer https://gmplib.org/manual/Multiplication-Algorithms in a comment, it lists all subquadratic algorithms. For reasonably-long integers I'd quote Toom-4 estimate, O(n^1.4).

vdukhovni · 2021-12-11T02:56:41Z

Reverted to divide and conquer and addressed comment nits.

vdukhovni · 2021-12-13T08:24:35Z

This might be it? Any further nits?

Bodigrim

Overall looks good to me, these are the last nitpicks from my side.

Data/ByteString/Lazy/ReadInt.hs

Bodigrim

This looks good to me. Overall count of changes in Data is 568 insertions, 313 deletions, and the delta of 250 lines is mostly due to comments. And we provide a lot of new safe and fast (on par with bytestring-lexing) routines.

vdukhovni · 2021-12-14T12:30:03Z

Dropped leading indents from CPP guards that are no longer nested.
Negate full-width words to avoid an extra instruction:

fromIntegral @Int64 @a $ negate $ fromIntegral @Word64 @Int64 acc

rather than:

negate $ fromIntegral @Word64 @a acc

the latter looks simpler but involves more underlying conversions to actually perform the negation (NCG backend, GHC 9.2).

sjakobi

A few more comments regarding the library.

In the future I hope that we can avoid huge PRs like this one. I'd much prefer to review a series of smallish PRs that incrementally builds the same functionality.

Data/ByteString/Lazy/ReadInt.hs

Data/ByteString/Lazy/ReadNat.hs

vdukhovni · 2021-12-14T19:06:49Z

A few more comments regarding the library.

In the future I hope that we can avoid huge PRs like this one. I'd much prefer to review a series of smallish PRs that incrementally builds the same functionality.

I am surprised to see this called a large PR... It is just 100 lines of code or so in each of two modules. It is hard to see how this would happen incrementally, it is a rewrite that makes readInt polymorphic over all the fixed sizes and does more of the foreign memory ops in a single loop rather than back and forth via the various combinators.

That said, I don't have more such things in the pipeline, unless you'd welcome a follow for analogous functions for hex, perhaps even octal, ... (full range of functions from bytestring-lexing, but without uncaught overflows).

Some applications want to read either unsigned or explicitly 64-bit integers (e.g. warp). Provide all the missing overflow-checked interfaces. * readInt8, readInt16, readInt32, readInt64 * readWord, readWord8, readWord16, readWord32, readWord64 * readNatural Cleaned up the code and improved tests. Uses Word as the accumular for all types other than Int64 and Word64, which use Word64. When words are 64 bit uses base 10^19 rather than 10^9 when assembling Natural and Integer values.

sjakobi · 2021-12-16T14:50:39Z

I am surprised to see this called a large PR... It is just 100 lines of code or so in each of two modules. It is hard to see how this would happen incrementally, it is a rewrite that makes readInt polymorphic over all the fixed sizes and does more of the foreign memory ops in a single loop rather than back and forth via the various combinators.

That said, I don't have more such things in the pipeline, unless you'd welcome a follow for analogous functions for hex, perhaps even octal, ... (full range of functions from bytestring-lexing, but without uncaught overflows).

It's not just the number of lines changed. It's also the huge number of review comments and corresponding changes. I think the repeated squashing of commits also made it very hard to review this PR incrementally. IIRC the scope of the PR was also increased by adding readNatural.

This isn't meant to criticize you, @vdukhovni. I wish I had realized my problem earlier and communicated it.

In the future I'll try to request limiting the size of similar PRs at an early stage. For example, it would be possible to add naive implementations combined with tests and benchmarks in a first PR and do optimizations in a second one.

sjakobi

Thanks!

sjakobi · 2021-12-16T14:36:58Z

tests/Properties.hs

+prop_readIntBoundsCC     =     rdWordBounds @Word
+                            && rdWordBounds @Word8
+                            && rdWordBounds @Word16
+                            && rdWordBounds @Word32
+                            && rdWordBounds @Word64
+                            && rdIntBounds  @Int
+                            && rdIntBounds  @Int8
+                            && rdIntBounds  @Int16
+                            && rdIntBounds  @Int32
+                            && rdIntBounds  @Int64


It seems that combining properties with && will make them a bit hard to debug if they do end up failing. Combining them with (.&&.) should make that easier, I think.

At this stage, it's probably not worth changing though.

I did not know about ".&&.", improving the tests without touching the main code may be reasonable? Your call...

I'd prefer to get this PR finished as soon as possible. Feel free to send more improvements in follow-up PRs.

I'd prefer to get this PR finished as soon as possible. Feel free to send more improvements in follow-up PRs.

Makes sense. Thanks!

I think it is a good use case for conjoin.

Bodigrim · 2021-12-16T19:22:12Z

Thanks!

Bodigrim · 2021-12-18T13:40:11Z

I think the repeated squashing of commits also made it very hard to review this PR incrementally.

+1, please do not squash any sizeable branches during review process. We'll squash when merging. (I remember that I fat-fingered rebase instead of squash in #309, sorry for that.)

For example, it would be possible to add naive implementations combined with tests and benchmarks in a first PR and do optimizations in a second one.

+1, to avoid possible disappointment, I'd prefer a simple PR to agree on API first and elaborate implementation in subsequent PRs.

I personally do not have use cases for octal / hexadecimal parsing, but I assume their implementation could be much simpler than for decimals?.. Providing it out of the box would be nice. @sjakobi what do you think?

sjakobi · 2021-12-18T15:34:18Z

I personally do not have use cases for octal / hexadecimal parsing, but I assume their implementation could be much simpler than for decimals?.. Providing it out of the box would be nice. @sjakobi what do you think?

For reading hexadecimal numbers, I'm aware of a use case in http-client. Readers for which types did you intend to provide here, @vdukhovni?

What's the use case for reading octal numbers? If we aren't aware of one, I think it would be better to wait until one comes up – API size does affect maintainability after all.

Also, let's please move this discussion to a proper issue. A closed PR is a bad place for it.

vdukhovni force-pushed the readWord branch from 8ee27a3 to 7f54c52 Compare November 12, 2021 02:12

sjakobi reviewed Nov 12, 2021

View reviewed changes

Bodigrim reviewed Nov 12, 2021

View reviewed changes