[WIP] isValidUtf8 function, tests #423

kozross · 2021-09-15T00:42:12Z

Closes #417, using work done previously for text's UTF-8ing efforts. This is marked WIP for now until we can get comparative benches of this solution versus the simdjson-based one currently in text.

Currently, there is no Aarch64-specific solution - I will provide one in a follow-up PR. The fallback C version should be quite fast though.

Bodigrim

Thanks for nicely commented C-code!

.gitignore

Changelog.md

bytestring.cabal

tests/is-valid-utf8/Main.hs

Bodigrim · 2021-09-16T17:47:24Z

@kozross As witnessed by CI, this does not quite work at least on Mac, unfortunately.

$ cabal run is-valid-utf8
Resolving dependencies...
Build profile: -w ghc-8.10.6 -O1
In order, the following will be built (use -v for more details):
 - bytestring-0.11.2.0 (test:is-valid-utf8) (additional components to build)
Preprocessing test suite 'is-valid-utf8' for bytestring-0.11.2.0..
Building test suite 'is-valid-utf8' for bytestring-0.11.2.0..
zsh: segmentation fault  cabal run is-valid-utf8

kozross · 2021-09-16T18:23:09Z

This is difficult for me to debug, as I don't have a Mac.

Bodigrim · 2021-09-20T21:45:28Z

@kozross FreeBSD build also fails: https://cirrus-ci.com/task/4802955137253376?logs=main#L715
Does it suggest an issue with clang?

kozross · 2021-09-20T21:47:17Z

This is possible. I can test this, but this involves building a clang-only toolchain. I'll try doing that and see if it reproduces.

kozross · 2021-10-03T00:41:43Z

@Bodigrim - I have narrowed the issue down. It occurs only in the combination of Clang and tasty. GHCi works fine, a toy executable using isValidUtf8 also works fine, and both behave exactly as we would expect. As I don't know the internals of tasty, I can only assume that there's something going on there that causes this to segfault.

Interestingly, in my case (GHC 9.0.1, built with Clang), I don't even get notified of a segfault: the tests just fail outright without explanation.

Would setting up a catch of SomeException in the test bodies possibly tell us something? I genuinely do not know if segfaults in FFI code trigger the throwing of some exception (pun not intended) or not.

Bodigrim · 2021-10-03T07:40:11Z

Sorry, AFK, cannot take a closer look atm.
This is more or less typical for segmentation faults: different heap layouts lead to different outcomes and may mask an issue. I've seen this couple of times before and it rarely signals any issue with tasty itself.

tests/is-valid-utf8/Main.hs

Data/ByteString.hs

Bodigrim · 2021-10-09T16:12:06Z

$ cabal run is-valid-utf8 -- -p '/Invalid UTF-8/' --quickcheck-replay=431466
Up to date
UTF-8 validation
  Invalid UTF-8: FAIL
    *** Failed! Falsified (after 49 tests):
    InvalidUtf8 {prefix = "", invalid = "\214", suffix = "\130{", asBS = "\214\130{", length = 3}
    True /= False
    Use --quickcheck-replay=431466 to reproduce.

1 out of 1 tests failed (0.00s)

kozross · 2021-10-10T22:19:28Z

@Bodigrim That's a generator bug - that's not invalid UTF-8, since that's the sequence 0xD6, 0x82 (which is 'ւ') followed by a '{'. Will fix too.

Bodigrim · 2021-10-10T23:21:10Z

Windows failure sounds like https://gitlab.haskell.org/ghc/ghc/-/issues/19900, please try if os(windows) extra-libraries: gcc_s.

Bodigrim · 2021-10-11T18:56:07Z

@kozross I recall that there is a subtle issue with TemplateHaskell and C sources introduced in GHC 9.0.1, already fixed in GHC HEAD. Could you please manually downgrade GHC, used in Windows build, to 8.10 in your branch?

bytestring/.github/workflows/ci.yml

Lines 22 to 23 in bd3c32c

    
           - os: windows-latest 
        
             ghc: 'latest'

Bodigrim · 2021-10-11T19:36:34Z

Ah, big endian is biting again:

UTF-8 validation
  Valid UTF-8:   FAIL (0.05s)
    *** Failed! Falsified (after 40 tests):
    "\233\128\154\231\148\160\231\141\173/\235\178\151eB\221\183g\217\154\228\164\189\208\154\242\142\168\153\205\146\231\135\176\229\162\189\219\145\240\185\158\159\236\150\141\207\173B\224\175\143\243\151\164\148\225\134\178"
    False /= True
    Use --quickcheck-replay=410548 to reproduce.
    Use -p '/Valid UTF-8/' to rerun this test only.
  Invalid UTF-8: OK (16.27s)
    +++ OK, passed 100000 tests.

You can reproduce as follows:

apt install -y docker.io && \
docker run --rm --privileged multiarch/qemu-user-static --reset -p yes && \
docker run --rm -i -t s390x/ubuntu

and then inside of the virtual machine:

apt update && \
apt install -y ghc cabal-install git build-essential && \
git clone https://github.com/mlabs-haskell/bytestring && \
cd bytestring && \
git checkout master && \
cabal new-update && \
cabal -j1 new-test is-valid-utf8

kozross · 2021-10-11T20:36:15Z

@Bodigrim Sure. I'll need an 8.10 with Clang to make sure I catch breakages, but that's workable.

Endianness is... a tricky business for sure. I need to do some refactors to make my code endian-safe, which will likely cause regressions in performance. I could probably do some checks statically for this, as I don't think we need to be runtime endianness portable. This should only be an issue in the fallback though.

cbits/is-valid-utf8.c

Bodigrim · 2021-10-16T00:17:35Z

@kozross If you could possibly resolve big endian issue soon, we might potentially merge as is and leave performance polishing for later. It’s important to get API in place and make a release of bytestring, so that text can be built upon it.

kozross · 2021-10-16T00:35:50Z

@Bodigrim Will get that done tomorrow.

Bodigrim · 2021-10-18T21:57:44Z

This is blocked now by Windows build, unfortunately. I thought that we are dealing with https://gitlab.haskell.org/ghc/ghc/-/issues/19417, which is specific to 9.0 series, but apparently it is something else. @kozross if you get time to raise this build failure at GHC bug tracker, it would be much appreciated.

kozross · 2021-10-18T22:16:43Z

@Bodigrim Do we need a minimal reproducing example, or is it enough just to aim people at this PR, or specifically, the build failure?

Bodigrim · 2021-10-25T22:30:41Z

I don't really understand why 32-bit build was fine before, but now cabal-1.24 struggles to derive a build plan. Is it because there is one more build target?.. Anyways, please add --constraint 'QuickCheck +old-random' and raise a separate PR for it.

Bodigrim · 2021-10-27T18:38:05Z

@kozross please rebase atop the latest master.

Bodigrim

Thanks @kozross! I rely on tests to validate UTF-8 validation logic, so just some nitpicking. Sorry that it takes so long.

@vdukhovni would you be interested to take a look at C?

.github/workflows/ci.yml

Data/ByteString.hs

cbits/is-valid-utf8.c

tests/is-valid-utf8/Main.hs

cbits/is-valid-utf8.c

sjakobi

I did a quick, initial review. I don't understand C very well, so I can't say much about it.

bytestring.cabal

Data/ByteString.hs

vdukhovni

Some questions/comments.

vdukhovni · 2021-10-30T21:07:44Z

cbits/is-valid-utf8.c

+        uint64_t results[4] = {(*big_ptr) & high_bits_mask,
+                               (*(big_ptr + 1)) & high_bits_mask,
+                               (*(big_ptr + 2)) & high_bits_mask,
+                               (*(big_ptr + 3)) & high_bits_mask};


C is not a lazy language, I don't think it makes sense to precompute all 4 strides, when just below you don't use the remaining strides when one of the masked results is non-zero... The start pointer here is not aligned to a 32-byte multiple, so I don't think we get particular cache line efficiency here by preloading these? Am I missing something?

cbits/is-valid-utf8.c

cbits/x86/is-valid-utf8.c

sjakobi

Thanks!

Bodigrim · 2021-11-03T18:55:53Z

Thanks @kozross, this is great! Would you be able to work on aarch64 solution and restoration of happy path for ASCII any time soon?

* isValidUtf8 function, tests * Cleanup, change to forAll * Fix accidentally-correct generation of wrong test cases * Increase test count, fix segfault on Clang * Try fixing Windows * Eliminate block-checking for ASCII in fallback mode * Use GHC 8.10 for CI on Windows * Namespace externally-visible C function better * Link to libgcc on Windows * Fix error accumulation in AVX2 check * Fix use of <> on older GHCs * Repair Drone config to deal with older Cabal * Ensure non-SSE2 x86 can still build * License information and contact for C code * Fix test name typo, allow more than pre-set count of tests * Stop using MagicHash for FFI wrapper * Revert change to 8.10 for Windows CI * Use existing license verbatim for C code * Explain the use of gcc and gcc_s * More concise implementation of isValidUtf8 * Document AVX2 block-checking function

kozross · 2021-11-03T21:17:03Z

@Bodigrim I can do this within the next day or two. The Aarch64 solution using SIMD instructions is fairly easy (and the fallback is pretty fast in the meantime), whereas the 'happy path' is only disabled on the fallback, and will take a bit longer. I have the tools to test this properly now at least, so it shouldn't be very long.

* isValidUtf8 function, tests * Cleanup, change to forAll * Fix accidentally-correct generation of wrong test cases * Increase test count, fix segfault on Clang * Try fixing Windows * Eliminate block-checking for ASCII in fallback mode * Use GHC 8.10 for CI on Windows * Namespace externally-visible C function better * Link to libgcc on Windows * Fix error accumulation in AVX2 check * Fix use of <> on older GHCs * Repair Drone config to deal with older Cabal * Ensure non-SSE2 x86 can still build * License information and contact for C code * Fix test name typo, allow more than pre-set count of tests * Stop using MagicHash for FFI wrapper * Revert change to 8.10 for Windows CI * Use existing license verbatim for C code * Explain the use of gcc and gcc_s * More concise implementation of isValidUtf8 * Document AVX2 block-checking function

isValidUtf8 function, tests

b218286

Bodigrim reviewed Sep 15, 2021

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

Changelog.md Show resolved Hide resolved

bytestring.cabal Outdated Show resolved Hide resolved

tests/is-valid-utf8/Main.hs Outdated Show resolved Hide resolved

kozross added 2 commits September 16, 2021 14:51

Merge branch 'master' of github.com:haskell/bytestring

1c19b83

Cleanup, change to forAll

3b168de

Bodigrim reviewed Oct 3, 2021

View reviewed changes

tests/is-valid-utf8/Main.hs Outdated Show resolved Hide resolved

Bodigrim reviewed Oct 9, 2021

View reviewed changes

Data/ByteString.hs Show resolved Hide resolved

Bodigrim mentioned this pull request Oct 10, 2021

TH Lift #392

Merged

kozross added 2 commits October 11, 2021 11:35

Fix accidentally-correct generation of wrong test cases

800fa95

Increase test count, fix segfault on Clang

32c6048

Try fixing Windows

0f513c3

Bodigrim reviewed Oct 11, 2021

View reviewed changes

cbits/is-valid-utf8.c Show resolved Hide resolved

Bodigrim mentioned this pull request Oct 15, 2021

Make simdutf an optional component haskell/text#379

Merged

kozross added 2 commits October 17, 2021 11:45

Eliminate block-checking for ASCII in fallback mode

331c9fa

Use GHC 8.10 for CI on Windows

374caa4

Repair Drone config to deal with older Cabal

9eff4d6

kozross mentioned this pull request Oct 26, 2021

Hide dev files, modify 32-bit config not to blow up on QuickCheck #431

Merged

kozross added 2 commits October 28, 2021 15:08

Merge branch 'master' of github.com:haskell/bytestring

2584a5d

Ensure non-SSE2 x86 can still build

e3a9f21

Bodigrim reviewed Oct 28, 2021

View reviewed changes

kozross added 4 commits October 29, 2021 14:15

License information and contact for C code

786151f

Fix test name typo, allow more than pre-set count of tests

abb9a10

Stop using MagicHash for FFI wrapper

50d61e6

Revert change to 8.10 for Windows CI

df2a8b9

Bodigrim reviewed Oct 29, 2021

View reviewed changes

cbits/is-valid-utf8.c Outdated Show resolved Hide resolved

Bodigrim requested a review from sjakobi October 29, 2021 21:29

sjakobi reviewed Oct 29, 2021

View reviewed changes

bytestring.cabal Show resolved Hide resolved

Data/ByteString.hs Outdated Show resolved Hide resolved

kozross added 3 commits October 30, 2021 16:28

Use existing license verbatim for C code

073499f

Explain the use of gcc and gcc_s

d8def8c

More concise implementation of isValidUtf8

121c4fc

Bodigrim approved these changes Oct 30, 2021

View reviewed changes

vdukhovni reviewed Oct 30, 2021

View reviewed changes

cbits/x86/is-valid-utf8.c Outdated Show resolved Hide resolved

sjakobi approved these changes Nov 1, 2021

View reviewed changes

Document AVX2 block-checking function

aaeb320

Bodigrim merged commit dad6dd1 into haskell:master Nov 3, 2021

Bodigrim added this to the 0.11.2.0 milestone Nov 3, 2021

Bodigrim mentioned this pull request Dec 4, 2021

Uncomment happy path for ASCII validation and fix big-endian issues #445

Merged

sjakobi mentioned this pull request Mar 14, 2022

Remove outdated comments on Windows extra-libraries #502

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] isValidUtf8 function, tests #423

[WIP] isValidUtf8 function, tests #423

kozross commented Sep 15, 2021

Bodigrim left a comment

Bodigrim commented Sep 16, 2021

kozross commented Sep 16, 2021

Bodigrim commented Sep 20, 2021

kozross commented Sep 20, 2021

kozross commented Oct 3, 2021

Bodigrim commented Oct 3, 2021 •

edited

Loading

Bodigrim commented Oct 9, 2021

kozross commented Oct 10, 2021 •

edited

Loading

Bodigrim commented Oct 10, 2021

Bodigrim commented Oct 11, 2021

Bodigrim commented Oct 11, 2021

kozross commented Oct 11, 2021

Bodigrim commented Oct 16, 2021

kozross commented Oct 16, 2021

Bodigrim commented Oct 18, 2021

kozross commented Oct 18, 2021

Bodigrim commented Oct 25, 2021

Bodigrim commented Oct 27, 2021

Bodigrim left a comment

sjakobi left a comment

vdukhovni left a comment

vdukhovni Oct 30, 2021

sjakobi left a comment

Bodigrim commented Nov 3, 2021

kozross commented Nov 3, 2021

[WIP] isValidUtf8 function, tests #423

[WIP] isValidUtf8 function, tests #423

Conversation

kozross commented Sep 15, 2021

Bodigrim left a comment

Choose a reason for hiding this comment

Bodigrim commented Sep 16, 2021

kozross commented Sep 16, 2021

Bodigrim commented Sep 20, 2021

kozross commented Sep 20, 2021

kozross commented Oct 3, 2021

Bodigrim commented Oct 3, 2021 • edited Loading

Bodigrim commented Oct 9, 2021

kozross commented Oct 10, 2021 • edited Loading

Bodigrim commented Oct 10, 2021

Bodigrim commented Oct 11, 2021

Bodigrim commented Oct 11, 2021

kozross commented Oct 11, 2021

Bodigrim commented Oct 16, 2021

kozross commented Oct 16, 2021

Bodigrim commented Oct 18, 2021

kozross commented Oct 18, 2021

Bodigrim commented Oct 25, 2021

Bodigrim commented Oct 27, 2021

Bodigrim left a comment

Choose a reason for hiding this comment

sjakobi left a comment

Choose a reason for hiding this comment

vdukhovni left a comment

Choose a reason for hiding this comment

vdukhovni Oct 30, 2021

Choose a reason for hiding this comment

sjakobi left a comment

Choose a reason for hiding this comment

Bodigrim commented Nov 3, 2021

kozross commented Nov 3, 2021

Bodigrim commented Oct 3, 2021 •

edited

Loading

kozross commented Oct 10, 2021 •

edited

Loading