Add pure Haskell implementation #631

hsyl20 · 2023-12-11T13:01:58Z

Add pure Haskell implementation for use with the JS backend. Use same Cabal flag as text package (-fpure-haskell).

hsyl20 · 2023-12-11T15:30:26Z

CI is finally passing. I've added a new job with the -fpure-haskell flag. Reviews welcome!

clyring

more to come

.github/workflows/ci.yml

Data/ByteString/Builder/RealFloat/D2S.hs

Data/ByteString/Builder/RealFloat/F2S.hs

Data/ByteString/Internal/Pure.hs

Data/ByteString/Builder/Prim/Internal/Base16.hs

Data/ByteString/Builder/RealFloat/Internal.hs

Data/ByteString/Internal.hs

bytestring.cabal

include/bytestring-cpp-macros.h

hsyl20 · 2024-01-08T11:23:40Z

Thanks @clyring for the review! It's now ready for a second round :-)

doyougnu

Just some minor comments. Mostly around excess bangs.

Data/ByteString/Builder/RealFloat/F2S.hs

Data/ByteString/Builder/RealFloat/TableGenerator.hs

Data/ByteString/Internal/Pure.hs

bytestring.cabal

hsyl20 · 2024-01-12T09:52:38Z

Thanks @doyougnu for the review. I've fixed everything I believe. I've also added INLINABLE pragmas so that specialization kicks in for CInt and CLong variants of decode[Uns/S]ignedBlah. Now Core looks pretty good.

hsyl20 · 2024-01-16T08:54:23Z

Any chance we can get this merged and released before the GHC 9.10 fork?

clyring · 2024-01-16T12:05:26Z

Perhaps it can be merged by then but I don't think there's any reason to cut a release so soon.

Data/ByteString/Internal/Pure.hs

Data/ByteString/Internal/Type.hs

Data/ByteString/Internal/Pure.hs

bytestring.cabal

include/bytestring-cpp-macros.h

bytestring.cabal

Data/ByteString/Internal/Type.hs

Data/ByteString/Internal/Pure.hs

clyring · 2024-01-27T01:28:00Z

Data/ByteString/Internal/Pure.hs

+    -- we cannot negate directly as  0 - (minBound :: Int) = minBound
+    -- So we write the sign and the first digit.
+    pokeByteOff buf 0 '-'
+    let !(q,r) = quotRem x (-10)
+    putDigit buf 1 (fromIntegral (abs r))
+    case q of
+      0 -> pure (plusPtr buf 2)
+      _ -> decodeUnsignedDec' q (plusPtr buf 1) (plusPtr buf 2)


Gross! We can negate directly, so long as we cast to the appropriate unsigned type. (I know you got this logic directly from the C code, and it's even worse there because we can expect the C compiler to produce better code for unsigned-known-divisor quotRem anyway. ...follow-up work?)

Data/ByteString/Internal/Pure.hs

.github/workflows/ci.yml

clyring · 2024-02-01T16:01:57Z

Go ahead and just add {-# LANGUAGE TypeApplications #-} where-ever it's convenient rather than trying to avoid them. Also, if you pull in the latest master that will fix the FreeBSD CI job.

...mostly so that 16d6b7e835ffdcf9b894e79f933dd52348dedd0c (which reworks unaligned writes in Builder) and the stuff in haskell/bytestring#631 can see wider testing. The less-terrible code for unaligned writes used in Builder on hosts not known to be ulaigned-friendly also takes less effort for GHC to compile, resulting in a metric decrease for T21839c on some platforms. The metric increase on T21839r is caused by the unrelated commit 750dac33465e7b59100698a330b44de7049a345c. It perhaps warrants further analysis and discussion (see #23822) but is not critical. Metric Decrease: T21839c Metric Increase: T21839r

clyring · 2024-02-01T17:56:12Z

I plan to give this another round of review in a day or two.

...mostly so that 16d6b7e835ffdcf9b894e79f933dd52348dedd0c (which reworks unaligned writes in Builder) and the stuff in haskell/bytestring#631 can see wider testing. The less-terrible code for unaligned writes used in Builder on hosts not known to be ulaigned-friendly also takes less effort for GHC to compile, resulting in a metric decrease for T21839c on some platforms. The metric increase on T21839r is caused by the unrelated commit 750dac33465e7b59100698a330b44de7049a345c. It perhaps warrants further analysis and discussion (see #23822) but is not critical. Metric Decrease: T21839c Metric Increase: T21839r

Data/ByteString/Builder/RealFloat/D2S.hs

Data/ByteString/Builder/RealFloat/F2S.hs

Data/ByteString/Internal/Type.hs

Data/ByteString/Short/Internal.hs

Data/ByteString/Internal/Pure.hs

...mostly so that 16d6b7e835ffdcf9b894e79f933dd52348dedd0c (which reworks unaligned writes in Builder) and the stuff in haskell/bytestring#631 can see wider testing. The less-terrible code for unaligned writes used in Builder on hosts not known to be ulaigned-friendly also takes less effort for GHC to compile, resulting in a metric decrease for T21839c on some platforms. The metric increase on T21839r is caused by the unrelated commit 750dac33465e7b59100698a330b44de7049a345c. It perhaps warrants further analysis and discussion (see #23822) but is not critical. Metric Decrease: T21839c Metric Increase: T21839r

hsyl20 · 2024-02-06T10:52:03Z

I've rebased to fix the conflict.

clyring

Nearly there!

Data/ByteString/Internal/Type.hs

Data/ByteString/Internal/Pure.hs

bytestring used to rely on C functions. This patch adds equivalent functions implemented in Haskell. The main purpose is for the JavaScript backend to fully support bytestring. Pure Haskell implementation can be enabled explicitly with a cabal flag. It's automatically enabled for the JavaScript platform. Thanks to Matthew Craven for the thorough review and the many suggestions. Co-authored-by: Matthew Craven <clyring@gmail.com>

clyring · 2024-02-07T13:26:44Z

@Bodigrim Did you also want to review?

clyring · 2024-02-07T13:30:07Z

@hasufell @angerman Do you know what's going on with the arm64v8 CI job? (And are you the right people to ping about this?) It's been consistently failing at the very end with:

A job completed hook has been configured by the self-hosted runner administrator
Error: /nix/store/42qhgz20lraczal3m9iwakwnmzrh67yc-hook-job-completed/bin/hook-job-completed is not a valid path to a script. Make sure it ends in '.sh', '.ps1' or '.js'.

hasufell · 2024-02-07T14:32:46Z

Try to temporarily add the tag maerwald to the runner (in runs-on: [...].

clyring · 2024-02-07T17:14:07Z

I've done so at #658. The problem isn't specific to either PR; feel free to open another for experimentation.

bytestring used to rely on C functions. This patch adds equivalent functions implemented in Haskell. The main purpose is for the JavaScript backend to fully support bytestring. Pure Haskell implementation can be enabled explicitly with a cabal flag. It's automatically enabled for the JavaScript platform. Thanks to Matthew Craven for the thorough review and the many suggestions. Co-authored-by: Matthew Craven <clyring@gmail.com> (cherry picked from commit d497f39)

angerman · 2024-02-07T23:33:07Z

@hasufell @angerman Do you know what's going on with the arm64v8 CI job? (And are you the right people to ping about this?) It's been consistently failing at the very end with:
A job completed hook has been configured by the self-hosted runner administrator
Error: /nix/store/42qhgz20lraczal3m9iwakwnmzrh67yc-hook-job-completed/bin/hook-job-completed is not a valid path to a script. Make sure it ends in '.sh', '.ps1' or '.js'.

OMFG GitHub...

$ file /nix/store/42qhgz20lraczal3m9iwakwnmzrh67yc-hook-job-completed/bin/hook-job-completed
/nix/store/42qhgz20lraczal3m9iwakwnmzrh67yc-hook-job-completed/bin/hook-job-completed: a /nix/store/2w9vwv156cx1pi3dhg8yib5y8dan0xp2-bash-5.2-p15/bin/bash script, ASCII text executable

Alright, I'll give it a .sh suffix.

...mostly so that 16d6b7e835ffdcf9b894e79f933dd52348dedd0c (which reworks unaligned writes in Builder) and the stuff in haskell/bytestring#631 can see wider testing. The less-terrible code for unaligned writes used in Builder on hosts not known to be ulaigned-friendly also takes less effort for GHC to compile, resulting in a metric decrease for T21839c on some platforms. The metric increase on T21839r is caused by the unrelated commit 750dac33465e7b59100698a330b44de7049a345c. It perhaps warrants further analysis and discussion (see #23822) but is not critical. Metric Decrease: T21839c Metric Increase: T21839r (cherry picked from commit 2702045)

clyring reviewed Dec 12, 2023

View reviewed changes

clyring linked an issue Dec 12, 2023 that may be closed by this pull request

Compatibility with GHC's JavaScript backend #625

Closed

doyougnu reviewed Jan 10, 2024

View reviewed changes

clyring reviewed Jan 16, 2024

View reviewed changes

bytestring.cabal Outdated Show resolved Hide resolved

bytestring.cabal Show resolved Hide resolved

include/bytestring-cpp-macros.h Outdated Show resolved Hide resolved

clyring reviewed Jan 27, 2024

View reviewed changes

clyring added this to the 0.12.1.0 milestone Jan 30, 2024

hsyl20 force-pushed the hsyl20/pure-bytestring branch from 6dfb449 to 48e336b Compare February 1, 2024 16:54

clyring mentioned this pull request Feb 1, 2024

Audit foreign import ccall types #653

Open

clyring reviewed Feb 3, 2024

View reviewed changes

hsyl20 force-pushed the hsyl20/pure-bytestring branch from 311d539 to bc9aed6 Compare February 6, 2024 10:51

hsyl20 force-pushed the hsyl20/pure-bytestring branch from bc9aed6 to 839f1a3 Compare February 6, 2024 10:57

clyring reviewed Feb 6, 2024

View reviewed changes

Data/ByteString/Internal/Type.hs Outdated Show resolved Hide resolved

Data/ByteString/Internal/Pure.hs Outdated Show resolved Hide resolved

hsyl20 force-pushed the hsyl20/pure-bytestring branch from 839f1a3 to 9a2b433 Compare February 7, 2024 09:21

clyring approved these changes Feb 7, 2024

View reviewed changes

Bodigrim approved these changes Feb 7, 2024

View reviewed changes

clyring merged commit d497f39 into haskell:master Feb 7, 2024
25 of 26 checks passed

hsyl20 mentioned this pull request Feb 8, 2024

Make cast between words and floats real primops haskell/core-libraries-committee#253

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pure Haskell implementation #631

Add pure Haskell implementation #631

hsyl20 commented Dec 11, 2023 •

edited

Loading

hsyl20 commented Dec 11, 2023

clyring left a comment

hsyl20 commented Jan 8, 2024

doyougnu left a comment

hsyl20 commented Jan 12, 2024

hsyl20 commented Jan 16, 2024

clyring commented Jan 16, 2024

clyring Jan 27, 2024

clyring commented Feb 1, 2024

clyring commented Feb 1, 2024

hsyl20 commented Feb 6, 2024

clyring left a comment

clyring commented Feb 7, 2024

clyring commented Feb 7, 2024

hasufell commented Feb 7, 2024

clyring commented Feb 7, 2024

angerman commented Feb 7, 2024

Add pure Haskell implementation #631

Add pure Haskell implementation #631

Conversation

hsyl20 commented Dec 11, 2023 • edited Loading

hsyl20 commented Dec 11, 2023

clyring left a comment

Choose a reason for hiding this comment

hsyl20 commented Jan 8, 2024

doyougnu left a comment

Choose a reason for hiding this comment

hsyl20 commented Jan 12, 2024

hsyl20 commented Jan 16, 2024

clyring commented Jan 16, 2024

clyring Jan 27, 2024

Choose a reason for hiding this comment

clyring commented Feb 1, 2024

clyring commented Feb 1, 2024

hsyl20 commented Feb 6, 2024

clyring left a comment

Choose a reason for hiding this comment

clyring commented Feb 7, 2024

clyring commented Feb 7, 2024

hasufell commented Feb 7, 2024

clyring commented Feb 7, 2024

angerman commented Feb 7, 2024

hsyl20 commented Dec 11, 2023 •

edited

Loading