Skip to content

Commit

Permalink
Fix performance regression and more precise tests.
Browse files Browse the repository at this point in the history
Replacing the `acc` argument of `overflow` with `0` mysteriously
(something in the strictness analyser at a wild guess) made the
code noticeably slower even when processing input with no overflows
at all.  The generated code was less optimal for the expected case!

So the `acc` -> `0` changes are now reverted.  Code coverage
improvements are not always performance improvements... :-(

Meanwhile, made the overflow detection tests more precise by
testing `minBound - 10` and `maxBound + 10`, which ensure that
the maximum quotient used is not off by 1 (or more).
  • Loading branch information
hs-viktor committed Oct 1, 2020
1 parent f834a76 commit 8e9953c
Show file tree
Hide file tree
Showing 2 changed files with 37 additions and 7 deletions.
4 changes: 2 additions & 2 deletions Data/ByteString/Streaming/Char8.hs
Original file line number Diff line number Diff line change
Expand Up @@ -836,7 +836,7 @@ readInt = start
-> loop (nbytes + n) a cs
| otherwise
-- too many zeros, bail out with sign
-> overflow nbytes 0 str
-> overflow nbytes acc str
| otherwise
-- skip empty segment
-> loop nbytes acc cs
Expand Down Expand Up @@ -882,7 +882,7 @@ readInt = start
-- | Plausible success, provided we got at least one digit!
result !nbytes !acc str
| nbytes > 0, !i <- w2int acc = return $! Compose $ Just i :> str
| otherwise = overflow 0 0 str -- just the sign perhaps?
| otherwise = overflow nbytes acc str -- just the sign perhaps?

-- This assumes that @negate . fromIntegral@ correctly produces
-- @minBound :: Int@ when given its positive 'Word' value as an
Expand Down
40 changes: 35 additions & 5 deletions tests/Test.hs
Original file line number Diff line number Diff line change
Expand Up @@ -123,11 +123,15 @@ readIntCases = do
let imax = maxBound :: Int
imin = minBound :: Int
imax1 = fromIntegral imax + 1 :: Integer
imin1k = fromIntegral imin - 1000 :: Integer
imax10 = fromIntegral imax + 10 :: Integer
imin1 = fromIntegral imin - 1 :: Integer
imin10 = fromIntegral imin - 10 :: Integer
smax = B.pack $ show imax
smin = B.pack $ show imin
smax1 = B.pack $ show imax1
smin1k = B.pack $ show imin1k
smax10 = B.pack $ show imax10
smin1 = B.pack $ show imin1
smin10 = B.pack $ show imin10
maxfill = QI.defaultChunkSize
cnt <- IOR.newIORef 0 -- number of effects in stream.
-- Empty input
Expand Down Expand Up @@ -185,14 +189,14 @@ readIntCases = do
res <- readIntSkip
$ QI.Chunk " \t\n\v\f\r\xa0"
$ addEffect cnt
$ QI.Chunk (B.take 4 smin1k)
$ QI.Chunk (B.take 4 smin1)
$ addEffect cnt
$ QI.Chunk (B.drop 4 smin1k)
$ QI.Chunk (B.drop 4 smin1)
$ addEffect cnt
$ QI.Chunk ""
$ addEffect cnt
$ QI.Empty 5
check cnt res Nothing (smin1k :> 5)
check cnt res Nothing (smin1 :> 5)
-- maxbound+1 with whitespace
IOR.writeIORef cnt 4
res <- readIntSkip
Expand Down Expand Up @@ -284,6 +288,32 @@ readIntCases = do
$ QI.Chunk "\n"
$ QI.Empty 14
check cnt res Nothing ("" :> 14)
-- maxbound+10 with whitespace
IOR.writeIORef cnt 4
res <- readIntSkip
$ QI.Chunk " \t\n\v\f\r\xa0"
$ addEffect cnt
$ QI.Chunk (B.take 4 smax10)
$ addEffect cnt
$ QI.Chunk (B.drop 4 smax10)
$ addEffect cnt
$ QI.Chunk ""
$ addEffect cnt
$ QI.Empty 15
check cnt res Nothing (smax10 :> 15)
-- minbound-10 with whitespace
IOR.writeIORef cnt 4
res <- readIntSkip
$ QI.Chunk " \t\n\v\f\r\xa0"
$ addEffect cnt
$ QI.Chunk (B.take 4 smin10)
$ addEffect cnt
$ QI.Chunk (B.drop 4 smin10)
$ addEffect cnt
$ QI.Chunk ""
$ addEffect cnt
$ QI.Empty 16
check cnt res Nothing (smin10 :> 16)
where
-- Count down to zero from initial value
readIntSkip = Q8.readInt . Q8.skipSomeWS
Expand Down

0 comments on commit 8e9953c

Please sign in to comment.