Merge `shortbytestring` package back into `bytestring` wrt #444 #471

hasufell · 2022-01-21T18:24:12Z

Bodigrim · 2022-01-21T20:19:47Z

Awesome! @hasufell could you please make CI happy?

hasufell · 2022-01-21T20:32:07Z

@Bodigrim I'm actually not sure how... opt seems to barf on ARM: https://cloud.drone.io/haskell/bytestring/284/2/2

[ 8 of 29] Compiling Data.ByteString.Builder.RealFloat.Internal ( Data/ByteString/Builder/RealFloat/Internal.hs, /drone/src/dist-newstyle/build/aarch64-linux/ghc-8.6.5/bytestring-0.11.3.0/build/Data/ByteString/Builder/RealFloat/Internal.o )
[ 9 of 29] Compiling Data.ByteString.Lazy.Internal ( Data/ByteString/Lazy/Internal.hs, /drone/src/dist-newstyle/build/aarch64-linux/ghc-8.6.5/bytestring-0.11.3.0/build/Data/ByteString/Lazy/Internal.o )
[10 of 29] Compiling Data.ByteString.Short.Internal ( Data/ByteString/Short/Internal.hs, /drone/src/dist-newstyle/build/aarch64-linux/ghc-8.6.5/bytestring-0.11.3.0/build/Data/ByteString/Short/Internal.o )
opt-6.0: /tmp/ghc725_0/ghc_123.ll:42130:42: error: use of undefined value '@memcmp$def'
  %ln1EwJ = bitcast i32 (i8*, i8*, i64)* @memcmp$def to i64 (i8*, i8*, i64)*

hasufell · 2022-01-21T20:39:55Z

This appears to be a GHC bug (or similar to that): https://gitlab.haskell.org/ghc/ghc/-/issues/18857

Bodigrim · 2022-01-21T20:54:43Z

@hasufell use this instead of raw memcmp:

bytestring/Data/ByteString/Short/Internal.hs

Lines 529 to 540 in 6ff6ed4

    
           compareByteArrays :: BA -> BA -> Int -> Int 
        
           #if MIN_VERSION_base(4,11,0) 
        
           compareByteArrays (BA# ba1#) (BA# ba2#) (I# len#) = 
        
             I# (compareByteArrays#  ba1# 0# ba2# 0# len#) 
        
           #else 
        
           compareByteArrays (BA# ba1#) (BA# ba2#) len = 
        
             fromIntegral $ accursedUnutterablePerformIO $ 
        
               c_memcmp_ByteArray ba1# ba2# (fromIntegral len) 
        
           foreign import ccall unsafe "string.h memcmp" 
        
             c_memcmp_ByteArray :: ByteArray# -> ByteArray# -> CSize -> IO CInt 
        
           #endif

hasufell · 2022-01-21T21:03:36Z

@Bodigrim

Couldn't match expected type ‘BA’ with actual type ‘Ptr b1’

Data/ByteString/Short/Internal.hs

hasufell · 2022-01-22T15:24:06Z

I'll try to run the test suite with this patch https://gitlab.haskell.org/ghc/ghc/-/merge_requests/7133

Reusing compareByteArrays and avoiding excessive pointer arithmetic.

hasufell · 2022-01-22T19:23:26Z

I'll try to run the test suite with this patch https://gitlab.haskell.org/ghc/ghc/-/merge_requests/7133

The following tests cause out of bounds errors:

  --, testProperty "read . show" $
  --  \x -> (x :: ShortByteString) === read (show x)

 --, testProperty "replicate" $
 --  \n (toElem -> c) -> B.unpack (B.replicate (fromIntegral n) c) === replicate n c

  --, testProperty "unfoldrN replicate" $
  --  \n (toElem -> c) -> fst (B.unfoldrN n (\t -> Just (t, t)) c) === B.replicate n c

So:

there's a bug in replicate after switching to setByteArray
there seems to be a problem with read/show... I've no idea how that happened, because I didn't even touch that code

hasufell · 2022-01-22T19:59:03Z

there seems to be a problem with read/show... I've no idea how that happened, because I didn't even touch that code

I was able to fix this with the following patch:

--- a/Data/ByteString/Short/Internal.hs
+++ b/Data/ByteString/Short/Internal.hs
@@ -442,7 +444,7 @@ packLenChars len cs0 =
     go :: MBA s -> Int -> [Char] -> ST s ()
     go !_   !_ []     = return ()
     go !mba !i (c:cs) = do
-      writeCharArray mba i c
+      writeWord8Array mba i (BS.c2w c)
       go mba (i+1) cs

Suggesting there's a problem with writeCharArray.

hasufell · 2022-01-22T20:48:37Z

Another point: I was a little lax about the complexity documentation. E.g. I'm not sure if tail is considered O(n) (since copyByteArray is a primops... it should probably be O(1), since we're documenting time not space complexity?).

Bodigrim · 2022-01-22T20:52:38Z

copyByteArray is still O(n), even if it is n/512.

hasufell · 2022-01-22T21:04:11Z

Any idea how this would be out-of-bounds?

replicate :: Int -> Word8 -> ShortByteString
replicate w c
    | w <= 0    = empty
    | otherwise = create w (\mba -> setByteArray mba 0 w (fromIntegral c))

setByteArray :: MBA s -> Int -> Int -> Int -> ST s ()
setByteArray (MBA# dst#) (I# off#) (I# len#) (I# c#) =
    ST $ \s -> case setByteArray# dst# off# len# c# s of
                 s -> (# s, () #)

Bodigrim · 2022-01-22T21:16:34Z

@hasufell see https://gitlab.haskell.org/ghc/ghc/-/merge_requests/7133#note_403462

sjakobi

Good progress.

Data/ByteString/Short/Internal.hs

sjakobi

Last comments, nearly done.

bytestring.cabal

Data/ByteString/Short/Internal.hs

sjakobi · 2022-02-08T10:21:02Z

Data/ByteString/Short/Internal.hs

+partition f = \sbs -> if
+    | null sbs  -> (sbs, sbs)
+    | otherwise -> bimap pack pack . List.partition f . unpack $ sbs


If you don't want to do this now, please record this task on the issue tracker.

sjakobi

One minor perf puzzle left. Cheers! :)

sjakobi · 2022-02-08T11:27:23Z

Data/ByteString/Short/Internal.hs

+            -> Int -- bytes written to b1
+            -> Int -- bytes written to b2
+            -> ST s (Int, Int)
+        go' !br !bw1 !bw2


One of these counters can be computed from the other two, e.g. bw2 = br - bw1. How does this affect the benchmarks?

I can't do benchmarking reliably on my thinkpad. It's full of CPU throttling and whatnot.

Alright. I have added this task to #350.

Orig:

mostlyTrueFast: OK (0.35s) 78.3 μs ± 5.0 μs mostlyFalseFast: OK (0.28s) 63.8 μs ± 4.9 μs balancedFast: OK (0.25s) 119 μs ± 6.8 μs mostlyTrueSlow: OK (0.28s) 2.18 ms ± 104 μs mostlyFalseSlow: OK (0.14s) 2.14 ms ± 193 μs balancedSlow: OK (0.14s) 2.21 ms ± 183 μs

With optimization:

mostlyTrueFast: OK (0.36s) 73.7 μs ± 5.4 μs mostlyFalseFast: OK (0.26s) 61.6 μs ± 2.9 μs balancedFast: OK (0.23s) 110 μs ± 8.7 μs mostlyTrueSlow: OK (0.14s) 2.16 ms ± 181 μs mostlyFalseSlow: OK (0.14s) 2.15 ms ± 192 μs balancedSlow: OK (0.14s) 2.20 ms ± 206 μs

I think the only one that's noticable is balancedFast and mostlyTrueFast and that seems to be somewhat consistent across many reruns.

hasufell · 2022-02-12T16:44:43Z

Can this be merged?

sjakobi · 2022-02-15T22:01:41Z

Thanks, @hasufell!

* Merge `shortbytestring` package back into `bytestring` wrt #444 * Fix build on ARM Reusing compareByteArrays and avoiding excessive pointer arithmetic. * Speed up reverse by using byteSwap64 tricks * Remove phase control from inlines * Improve performance of elemIndex * Use setByteArray in replicate * Implement intercalate manually * Annotate partial functions with HasCallStack * Fix build on base < 4.12.0.0 * Add uncons/unsnoc * Correct complexities * Exclude reverse optimization path from ARM It seems to cause segfaults on armv7, suggesting there are issues with 'indexWord8ArrayAsWord64#'. All other platforms are fine and tests pass. * Add benchmarks for ShortByteString * Improve inlining * Adjust haddock identifiers * Get rid of writeCharArray# * Haddock fixes * Clean up tests * Use -fexpose-all-unfoldings * Improve reverse * Cleanup 'reverse' * Fix possible GC race with foreign imports For more information, see #471 (comment) * Disable asserts in shortbytestring.c * Remove redundant import * Add documentation about partial functions * Fold ShortByteString prop tests into ByteString * Restore previous INLINEs * Improve naming of bindings * Consolidate error handling functions * Remove trailing whitespace * Fix uncons in documentation * Rename indexWord64Array to indexWord8ArrayAsWord64 * Improve error message * Clean up incorrect documentation * Use div/mod instead of quot/rem * Simplify branching in reverse * Move asserts to Haskell * Prefix C functions * Fix return type of c_elem_index * Fix documentation in unfoldrN * Make unfoldrN more efficient * Fix maintainer field * Fix formatting * Implement takeEnd, dropeEnd and splitAt manually * Fix some haddock identifiers * Fix unfoldrN doc * Add a primops bounds-checking job to CI * Document and clean up createAndTrim * Rename errorEmptyList to errorEmptySBS * Improve documentation for findFromEndUntil * Improve documentation and naming * Optimize out quotRem * Document compareByteArraysOff * Simplify findIndexOrLength and findFromEndUntil * Use c_count for count * Simplify elemIndex * Remove use of 'mempty' * Make sure breakSubstring is inlined into isInfixOf * Simplify stripSuffix and stripPrefix * Fix redundant import warnings * Improve 'take' * Use existing bounnds check in 'drop' * Avoid 'create' when bytestring is empty * Optimize filter * Remove redundant INLINABLE * Use shorter 'createAndTrim' in 'filter' * Simplify 'take' * Simplify 'drop' * Better formatting * Add comment to explain DNDEBUG * Refactor elemIndex * Optimize 'partition' * Optimize hot loop in 'partition' (cherry picked from commit 731caea)

hasufell force-pushed the hasufell/PR/merge-shortbytestring branch from 036b17c to 5b22feb Compare January 21, 2022 18:24

hasufell mentioned this pull request Jan 21, 2022

RFC: consider splitting out shortbytestring #444

Closed

hasufell force-pushed the hasufell/PR/merge-shortbytestring branch from 5b22feb to b87de6c Compare January 21, 2022 18:57

Bodigrim reviewed Jan 21, 2022

View reviewed changes

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

hasufell force-pushed the hasufell/PR/merge-shortbytestring branch from cbb1651 to 0528711 Compare January 21, 2022 21:58

Bodigrim reviewed Jan 21, 2022

View reviewed changes

hasufell added 9 commits January 22, 2022 20:03

Merge shortbytestring package back into bytestring wrt haskell#444

2125e9d

Fix build on ARM

46b128a

Reusing compareByteArrays and avoiding excessive pointer arithmetic.

Speed up reverse by using byteSwap64 tricks

09a8916

Remove phase control from inlines

d69c427

Improve performance of elemIndex

cb349be

Use setByteArray in replicate

e29f102

Implement intercalate manually

fcb3170

Annotate partial functions with HasCallStack

5975e88

Fix build on base < 4.12.0.0

f78661a

hasufell force-pushed the hasufell/PR/merge-shortbytestring branch from c3f4bbb to 9bce444 Compare January 22, 2022 21:06

hasufell added 2 commits January 22, 2022 22:36

Add uncons/unsnoc

b88e4fe

Correct complexities

c2b2bfd

Simplify stripSuffix and stripPrefix

08c8b5d

sjakobi reviewed Feb 7, 2022

View reviewed changes

hasufell added 6 commits February 7, 2022 18:42

Fix redundant import warnings

3d0c977

Improve 'take'

e05f05d

Use existing bounnds check in 'drop'

659edd9

Avoid 'create' when bytestring is empty

fd83ec2

Optimize filter

98450fc

Remove redundant INLINABLE

b647760

hasufell force-pushed the hasufell/PR/merge-shortbytestring branch from 3834ed8 to b647760 Compare February 7, 2022 18:32

hasufell commented Feb 7, 2022

View reviewed changes

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

sjakobi reviewed Feb 7, 2022

View reviewed changes

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

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

hasufell added 4 commits February 7, 2022 21:48

Use shorter 'createAndTrim' in 'filter'

6289d9f

Simplify 'take'

28b8686

Simplify 'drop'

bc90b58

Better formatting

fed9a73

sjakobi reviewed Feb 8, 2022

View reviewed changes

hasufell added 3 commits February 8, 2022 11:47

Add comment to explain DNDEBUG

213218c

Refactor elemIndex

04e2392

Optimize 'partition'

3d6722b

sjakobi approved these changes Feb 8, 2022

View reviewed changes

Optimize hot loop in 'partition'

d12ad66

sjakobi merged commit 731caea into haskell:master Feb 15, 2022

Bodigrim added this to the 0.11.3.0 milestone Feb 15, 2022

Bodigrim linked an issue Feb 15, 2022 that may be closed by this pull request

RFC: consider splitting out shortbytestring #444

Closed

Bodigrim linked an issue Feb 15, 2022 that may be closed by this pull request

Expand ShortByteStrings API? #83

Closed

hasufell mentioned this pull request Jun 19, 2022

Abstract filepath proposal (request for help) haskellfoundation/tech-proposals#35

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge `shortbytestring` package back into `bytestring` wrt #444 #471

Merge `shortbytestring` package back into `bytestring` wrt #444 #471

hasufell commented Jan 21, 2022

Bodigrim commented Jan 21, 2022

hasufell commented Jan 21, 2022 •

edited

Loading

hasufell commented Jan 21, 2022 •

edited

Loading

Bodigrim commented Jan 21, 2022

hasufell commented Jan 21, 2022

hasufell commented Jan 22, 2022

hasufell commented Jan 22, 2022

hasufell commented Jan 22, 2022

hasufell commented Jan 22, 2022 •

edited

Loading

Bodigrim commented Jan 22, 2022

hasufell commented Jan 22, 2022

Bodigrim commented Jan 22, 2022

sjakobi left a comment

sjakobi left a comment

sjakobi Feb 8, 2022

sjakobi left a comment

sjakobi Feb 8, 2022

hasufell Feb 8, 2022

sjakobi Feb 8, 2022

hasufell Feb 8, 2022

hasufell commented Feb 12, 2022

sjakobi commented Feb 15, 2022

Merge shortbytestring package back into bytestring wrt #444 #471

Merge shortbytestring package back into bytestring wrt #444 #471

Conversation

hasufell commented Jan 21, 2022

Bodigrim commented Jan 21, 2022

hasufell commented Jan 21, 2022 • edited Loading

hasufell commented Jan 21, 2022 • edited Loading

Bodigrim commented Jan 21, 2022

hasufell commented Jan 21, 2022

hasufell commented Jan 22, 2022

hasufell commented Jan 22, 2022

hasufell commented Jan 22, 2022

hasufell commented Jan 22, 2022 • edited Loading

Bodigrim commented Jan 22, 2022

hasufell commented Jan 22, 2022

Bodigrim commented Jan 22, 2022

sjakobi left a comment

Choose a reason for hiding this comment

sjakobi left a comment

Choose a reason for hiding this comment

sjakobi Feb 8, 2022

Choose a reason for hiding this comment

sjakobi left a comment

Choose a reason for hiding this comment

sjakobi Feb 8, 2022

Choose a reason for hiding this comment

hasufell Feb 8, 2022

Choose a reason for hiding this comment

sjakobi Feb 8, 2022

Choose a reason for hiding this comment

hasufell Feb 8, 2022

Choose a reason for hiding this comment

hasufell commented Feb 12, 2022

sjakobi commented Feb 15, 2022

Merge `shortbytestring` package back into `bytestring` wrt #444 #471

Merge `shortbytestring` package back into `bytestring` wrt #444 #471

hasufell commented Jan 21, 2022 •

edited

Loading

hasufell commented Jan 21, 2022 •

edited

Loading

hasufell commented Jan 22, 2022 •

edited

Loading