Implement floating point conversion with ryu by 0xlwu · Pull Request #365 · haskell/bytestring (original) (raw)

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

@0xlwu

This PR implements floatDec and doubleDec using the Ryu algorithm described in Ulf Adams' paper. Compared to #222, this is a native Haskell implementation based on https://github.com/Lumaere/ryu and the performance difference is relatively minimal.

However, this does add dependencies on template-haskell (which can probably be removed but will result in the hardcoding of large tables) and array.

Relevant benchmarks for this implementation on my i7-8700k

      foldMap floatDec (10000):                               OK (0.28s)
        1.0 ms ±  42 μs
      foldMap doubleDec (10000):                              OK (0.15s)
        1.1 ms ±  91 μs

And current

      foldMap floatDec (10000):                               OK (0.15s)
        9.5 ms ± 861 μs
      foldMap doubleDec (10000):                              OK (0.12s)
         17 ms ± 1.6 ms

@Bodigrim

Wonderful!

We will drop older GHCs soon (see #265), do not worry about correspondent CI failures.

Could you please submit tests as a separate PR?

@0xlwu

Moving off of those older GHCs is great. Any thoughts on the windows build failure

C:\Users\RUNNER~1\AppData\Local\Temp\ghcCE41.o.tmp: renameFile:renamePath:MoveFileEx "\\\\?\\C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\ghcCE41.o.tmp" Just "\\\\?\\C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\ghcCE41.o": does not exist (The system cannot find the file specified.)

Some light searching found https://gitlab.haskell.org/ghc/ghc/-/issues/18274 which suggests that there is some issue with process but I'm not very familiar with windows builds

@sjakobi

Some light searching found https://gitlab.haskell.org/ghc/ghc/-/issues/18274 which suggests that there is some issue with process but I'm not very familiar with windows builds

IIUC we could work around this by using a different cabal executable. Maybe we could simply upgrade to using cabal-install-3.4?! Unfortunately I'm not very familiar with our use of GitHub Actions.

@Bodigrim

An issue with a temporary file used to happen before, but became much more prominent when Windows build switched to GHC 9.0. FIled as haskell/actions#36.

Not much can be done on our side, I believe, except just rerunning tests twice or thrice cabal test || cabal test || cabal test. I'll raise a PR.

@Bodigrim

Let's wait for haskell/actions#39 to land, maybe it will alleviate our issues with Windows build.

@Bodigrim

However, this does add dependencies on template-haskell (which can probably be removed but will result in the hardcoding of large tables) and array.

template-haskell is OK, #330 adds it to dependencies as well.
array could be OKish, but I'd prefer to avoid. Could you please point me where it is used? I wonder if we can use Addr# instead. Not only it eliminates a dependency, but also is usually significantly faster.

@0xlwu

The precomputed tables of multipliers and the formatting code both use arrays. Since they aren't exposed anywhere, I'll take a look at converting to raw address lookups

This was referenced

Feb 26, 2021

@0xlwu

@Bodigrim the array dependency has been removed but the build ran into the windows issue again.

@Bodigrim

@Lumaere I rerun, all green now :)

I'm a bit confused: f65dde3 had tests, but they disappeared after push force. Could you please submit tests as a separate PR to be merged before this one?

@0xlwu

Can do. Some of the tests are more specific to internals (checking e.g round-trip and rounding). Should I stub those to the reference implementations or bring them back to this PR?

@hsyl20

template-haskell is OK, #330 adds it to dependencies as well.

Is it OK? bytestring is built with the stage1 compiler which doesn't support template-haskell.

@Bodigrim

@hsyl20 Thanks, good to know. How does template-haskell work for other boot libraries? I see that text and exceptions depend on it.

We can review this with template-haskell, and replace it with generated splices as the very last step.

@hsyl20

@Bodigrim The issue is that we can't link/execute generated code with GHC stage1, hence no TH splice.

We can review this with template-haskell, and replace it with generated splices as the very last step.

Indeed. Or perhaps write a little utility program to generate the tables?

Also I think we could improve the generated tables by avoiding creating ByteArray at runtime. We can't create static ByteArray but we could generate primitive strings (which have type Addr#) and directly use indexWord64OffAddr#. For example double_pow5_inv_split would look like

double_pow5_inv_split :: Addr# double_pow5_inv_split = "\0\1\123\45..."#

We have to be careful about endianness though. Perhaps always generate little-endian tables and perform a byte-swap at runtime if the host is big-endian.

@Bodigrim

Can do. Some of the tests are more specific to internals (checking e.g round-trip and rounding). Should I stub those to the reference implementations or bring them back to this PR?

@Lumaere sorry, missed your question. Do you mean that some tests from your branch fail under existing, show-based implementation (because of its infelicities)? In such case still submit them, wrapped into expectFail.

I'm keen to get this merged into the next release (which is September).

@0xlwu

@Bodigrim The tests were largely ported from the original C implementation of Ryu by Ulf Adams. I made the modifications to them so that they agree with show based implementation regarding rounding, etc. However, they still look at a new internal function (exposed for testing... they don't handle precision for all formats) called formatFloat / formatDouble that is based loosely on formatRealFloat. I can make the test PR expose these as

formatFloat :: FFFormat -> Maybe Int -> Float -> Builder
formatFloat fmt prec = string7 . formatRealFloat fmt prec

formatDouble :: FFFormat -> Maybe Int -> Double -> Builder
formatDouble fmt prec = string7 . formatRealFloat fmt prec

or modify all the tests to only use floatDec / doubleDec

@Bodigrim

From my perspective, it is better to modify tests to use only floatDec / doubleDec.

@0xlwu 0xlwu mentioned this pull request

Jun 17, 2021

@0xlwu

Works for me. The PR is #402. Let me know if you wanted it in a different format. Thanks

@Bodigrim

@la-wu could you please rebase?

Bodigrim

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@la-wu thanks for your astonishing work! It will take some time to review, here are my first suggestions.

-- doubles: 1 (sign) + 17 (mantissa) + 1 (.) + 1 (e) + 4 (exponent) = 24
--
maxEncodedLength :: Int
maxEncodedLength = 32

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So why it it 32 instead of 24?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC, I liked rounding to the nearest power of 2 for whatever reason

boundString s = boundedPrim maxEncodedLength $ const (pokeAll s)
-- Sign -> Exp -> Mantissa
special :: Bool -> Bool -> Bool -> BoundedPrim ()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we possibly define data Foo = Foo { fooSign :: !Bool, fooExpr :: !Bool, fooMantissa :: !Bool }, so that the meaning of arguments is self-explanatory, and their ordering is less error-prone?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sjakobi

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First off, apologies for the late review! I hadn't realized that this PR had been waiting for a review for so long.

I've made a first pass over the code. So far I'm pretty impressed by the clean and well-documented internals! 👍

I should point out that I'm not very experienced with floating-point encodings, so I probably won't be able to comment on much of the internals. For correctness, I'll have to rely on the test suite, which I'm grateful you have improved so much! :)

I have a few requests:

@0xlwu

Thanks for the review. If you're interested in reading about the floating point string conversion, I think part 2 of the ryu paper has a good intro to it.

* [ ]  Please add module header haddocks to all the added modules.

Will do. What do you suggest I put in the fields Copyright, Maintainer, Stability?

On stability (and wrt some of your other comments that I'll get to individually as I start making edits), perhaps separating the current RealFloat.hs into a stable portion for just floatDec / doubleDec and a internal/experimental portion that has the formatFloat / formatDouble (which are also new to bytestring) could be clearer. What are your thoughts?

* [ ]  Since I have encountered several bugs that involved `fromIntegral`, I have a somewhat irrational fear of it. (I also dislike that I often can't tell the types involved just from looking at it.) Would it be possible to replace its use with specialized `intToFloat`-style helpers? Alternatively, we could consider using `-XTypeApplications`, but I think that would require dropping support for GHC < 8 a bit earlier than planned so far.

* [ ]  I would also prefer if the types `(.>>)` and `(.<<)` were constrained a bit, if that can be done ergonomically.

I think these can both be at least partially achieved. Most (all?) of the fromIntegral usages are actually just signed-to-unsigned or 32-to-64-bit conversion (and vice versa). Some of it can be elided by a more careful handling of integral types and others can get explicit annotations of the whole expression (like in Builder/Prim/Binary.hs)

@sjakobi

Will do. What do you suggest I put in the fields Copyright, Maintainer, Stability?

Oh, I was mostly looking for brief module descriptions. I'm not sure we need all that other data. I usually skip it in other packages that I maintain. (CC @Bodigrim)

Do feel free to add your copyright in the modules that you've written of course!

On stability (and wrt some of your other comments that I'll get to individually as I start making edits), perhaps separating the current RealFloat.hs into a stable portion for just floatDec / doubleDec and a internal/experimental portion that has the formatFloat / formatDouble (which are also new to bytestring) could be clearer. What are your thoughts?

I do think that formatFloat / formatDouble are very desirable additions. If there is particular reason to believe that these functions will change in the near future, it might be better to to export them from an Internal module, so we don't have to do a major version bump. D.B.B.RealFloat.Internal would be the obvious candidate, but then you'd probably want to move the other contents of that module elsewhere…

I'm not overly concerned about breaking changes though. As long as there's a general sense of "improvement", I think we can do a major version bump every 2 or 3 years.

sjakobi

@0xlwu

@0xlwu

sjakobi

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @la-wu. I like this direction! Needs just a bit more polishing.

-- "12.345"
{-# INLINABLE formatFloat #-}
formatFloat :: FloatFormat -> Float -> Builder
formatFloat (MkFloatFormat fmt prec) f =

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we might want this to inline when applied to only a single argument, roughly like this:

formatFloat (MkFloatFormat fmt prec) f =
formatFloat (MkFloatFormat fmt prec) = \f ->

@Bodigrim, what do you think?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it will be better for inlining this way.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you clarify what kind of problems you're concerned about? The easiest thing might be to take a look at the Core…

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just that running f2Intermediate is unnecessary work in case of FScientific. I took a look at the dump-simpl output but it wasn't obvious to me what I should be looking for...

sjakobi

@0xlwu

@0xlwu

sjakobi

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few more documentation tweaks.

Could you possibly generate the haddocks and check whether the markup comes out right? Especially that identifiers are hyperlinked and non-identifiers aren't accidentally rendered in mono-space font?

Comment on lines 15 to 18

-- NB: this implementation matches `show`'s output (specifically with respect
-- to default rounding and length). In particular, there are boundary cases
-- where the closest and 'shortest' string representations are not used.
-- Mentions of 'shortest' in the docs below are with this caveat.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This paragraph is a bit hard to understand I think. An example would be very helpful. Please also clarify what "this implementation" refers to. Maybe create a named chunk and refer to it from the relevant places.

Also note that single quotes (') are used for marking identifiers in Haddock. Escaping the quotes might work?! E.g. \'shortest\'.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added an example / handwavy explanation about the rounding / boundaries and pointed to the paper for details. TBH at this point I am far enough from the writing this code that I would need to redigest the exact semantics.

WRT quotes, thanks for pointing that out. I'm used to normal MD.

standard :: Int -> FloatFormat
standard n = MkFloatFormat FStandard (Just n)
-- | Standard notation with the default precision (number of decimal places)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"default precision" seems like another term that could be explained in a named chunk that can be referenced from here.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It just means matching show / formatRealFloat. LMK if I should elaborate further...

Bodigrim

in mk0 ls `mappend` mkDot rs
| otherwise ->
let (ei, is') = roundTo 10 p' (replicate (-e) 0 ++ ds)
(b:bs) = digitsToBuilder (if ei > 0 then is' else 0:is')

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This raises Pattern match(es) are non-exhaustive with GHC 9.2.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm is there a way to convince GHC that the list is non-empty? Looking at the implementation of roundTo, the non-throwing return values are (0, _) and (1, 1:_). Before the call to digitsToZero, we prepend 0 to the former case's snd value so AFIACT this is 'correct'.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also just mimic the case above or add some redundant matches e.g

diff --git a/Data/ByteString/Builder/RealFloat.hs b/Data/ByteString/Builder/RealFloat.hs
index cd0d39d..0dadf45 100644
--- a/Data/ByteString/Builder/RealFloat.hs
+++ b/Data/ByteString/Builder/RealFloat.hs
@@ -258,8 +258,8 @@ showStandard m e prec =
            in mk0 ls `mappend` mkDot rs
       | otherwise ->
           let (ei, is') = roundTo 10 p' (replicate (-e) 0 ++ ds)
-              (b:bs) = digitsToBuilder (if ei > 0 then is' else 0:is')
-           in b `mappend` mkDot bs
+              (ls, rs) = splitAt 1 $ digitsToBuilder (if ei > 0 then is' else 0:is')
+           in mk0 ls `mappend` mkDot rs
           where p' = max p 0
   where
     mk0 ls = case ls of [] -> char7 '0'; _ -> mconcat ls

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not saying that your code is wrong, I'm saying that we need to get rid of a warning. Boot packages cannot have warnings. Throwing an error for an empty list would be fine, for instance.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that this warning needs to be silenced. Maybe this would be a good fit for Data.List.NonEmpty?!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the snippet above. A lot of the code in GHC.Float makes the same assumption though so do they just turn the warning off?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. That module uses the pragma

{-# OPTIONS_GHC -Wno-incomplete-uni-patterns #-}

@Bodigrim

@sjakobi sorry, I lost track of the discussion. Is there anything left unresolved?

sjakobi

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy with the API at this stage.

The haddocks could use some editing and more examples, but I don't think this needs to happen within this PR.

The GHC warning should be addressed before merging though.

-- "9.999999999999999e22"
--
-- Simplifying, we can build a shorter, lossless representation by just using
-- @"1.0e23"@ since the floating point values that are 1 ULP away are

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is "ULP"?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to know! Could you link to the article?

Note BTW that haddock supports Markdown-style hyperlinks, e.g. [ULP](https://en.wikipedia.org/wiki/Unit_in_the_last_place).

Comment on lines +15 to +55

-- NB: The float-to-string conversions exposed by this module match `show`'s
-- output (specifically with respect to default rounding and length). In
-- particular, there are boundary cases where the closest and \'shortest\'
-- string representations are not used. Mentions of \'shortest\' in the docs
-- below are with this caveat.
--
-- For example, for fidelity, we match `show` on the output below.
--
-- >>> show (1.0e23 :: Float)
-- "1.0e23"
-- >>> show (1.0e23 :: Double)
-- "9.999999999999999e22"
-- >>> floatDec 1.0e23
-- "1.0e23"
-- >>> doubleDec 1.0e23
-- "9.999999999999999e22"
--
-- Simplifying, we can build a shorter, lossless representation by just using
-- @"1.0e23"@ since the floating point values that are 1 ULP away are
--
-- >>> showHex (castDoubleToWord64 1.0e23) []
-- "44b52d02c7e14af6"
-- >>> castWord64ToDouble 0x44b52d02c7e14af5
-- 9.999999999999997e22
-- >>> castWord64ToDouble 0x44b52d02c7e14af6
-- 9.999999999999999e22
-- >>> castWord64ToDouble 0x44b52d02c7e14af7
-- 1.0000000000000001e23
--
-- In particular, we could use the exact boundary if it is the shortest
-- representation and the original floating number is even. To experiment with
-- the shorter rounding, refer to
-- `Data.ByteString.Builder.RealFloat.Internal.acceptBounds`. This will give us
--
-- >>> floatDec 1.0e23
-- "1.0e23"
-- >>> doubleDec 1.0e23
-- "1.0e23"
--
-- For more details, please refer to the
-- <https://dl.acm.org/doi/10.1145/3192366.3192369 Ryu paper>.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this section is a bit too technical for most users. If we want to keep it, we should probably move it out of the module header and to the bottom of the page.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there such a thing as a footer? This was added per one of our discussions above about clarifying 'shortest'.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can insert chunks of documentation at selected places in the export list. See e.g. Data.ByteString.Lazy.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in mk0 ls `mappend` mkDot rs
| otherwise ->
let (ei, is') = roundTo 10 p' (replicate (-e) 0 ++ ds)
(b:bs) = digitsToBuilder (if ei > 0 then is' else 0:is')

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that this warning needs to be silenced. Maybe this would be a good fit for Data.List.NonEmpty?!

@0xlwu

sjakobi

@Bodigrim

@sjakobi thanks for extensive reviews!

@la-wu if you get inspiration to fix documentation suggestions in a separate PR, it would be awesome. Otherwise thank you very much for all your heroic efforts to make this state-of-the-art algorithm available to Haskell users. This is a long-awaited improvement.

Bodigrim pushed a commit to Bodigrim/bytestring that referenced this pull request

Dec 4, 2021

@0xlwu @Bodigrim

This reverts commit 10809d3.

@sjakobi

noughtmare pushed a commit to noughtmare/bytestring that referenced this pull request

Dec 12, 2021

@0xlwu

This reverts commit 10809d3.

@hsyl20

@la-wu Out of curiosity, did you make benchmarks against the double-conversion package? [We have had troubles using this package with GHCJS recently (it needs to be linked with libstdc++...) so a pure Haskell solution is very appealing if performance is comparable]

@Bodigrim Bodigrim linked an issue

Feb 16, 2022

that may beclosed by this pull request

@0xlwu

@la-wu Out of curiosity, did you make benchmarks against the double-conversion package? [We have had troubles using this package with GHCJS recently (it needs to be linked with libstdc++...) so a pure Haskell solution is very appealing if performance is comparable]

Hey @hsyl20, not sure if this is still relevant but I just ran a quick bench for this and I got

foldMap floatDec (10000):                               OK (0.15s)
  1.17 ms ± 114 μs
foldMap doubleDec (10000):                              OK (0.37s)
  1.42 ms ±  45 μs
foldMap floatShow (10000):                              OK (0.33s)
  10.5 ms ± 541 μs
foldMap doubleShow (10000):                             OK (0.27s)
  18.0 ms ± 1.0 ms

And with double-conversion,

foldMap floatDC (10000):    OK (0.16s)
  5.17 ms ± 450 μs
foldMap doubleDC (10000):   OK (0.21s)
  6.55 ms ± 494 μs

With a setup of

import qualified Data.Double.Conversion.Convertable    as DC

...
, benchB "foldMap floatShow"       floatData          $ foldMap (string7 . show)
, benchB "foldMap doubleShow"      doubleData         $ foldMap (string7 . show)
, benchB "foldMap floatDC"         floatData          $ foldMap DC.toShortest
, benchB "foldMap doubleDC"        doubleData         $ foldMap DC.toShortest

Not sure if I was using the library correctly... but directly converting with double-conversion to Bytestring (instead of builder) was much slower fwiw.

@hsyl20

@la-wu Thanks a lot for this report! We should definitely use your code instead of having to deal with double-conversion. I've added it to our task list.