Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Enable fusion of unsafePackLen{Bytes,Chars} #494

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

Enable fusion of unsafePackLen{Bytes,Chars} #494

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
12a6069
Add packing benchmarks
noughtmare Mar 5, 2022
969ec2a
Expand packing benchmarks
noughtmare Mar 5, 2022
a04d938
Enable fusion of unsafePackLen{Bytes,Chars}
noughtmare Mar 3, 2022
2ef63df
Fix name shadowing warnings
noughtmare Mar 3, 2022
b43d295
Remove some points
noughtmare Mar 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 10 additions & 10 deletions Data/ByteString/Internal.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,18 +339,18 @@ packChars cs = unsafePackLenChars (List.length cs) cs
#-}

unsafePackLenBytes :: Int -> [Word8] -> ByteString
unsafePackLenBytes len xs0 =
unsafeCreate len $ \p -> go p xs0
where
go !_ [] = return ()
go !p (x:xs) = poke p x >> go (p `plusPtr` 1) xs
unsafePackLenBytes len =
unsafeCreate len . foldr
(\x go p -> poke p x >> go (p `plusPtr` 1))
(\_ -> return ())
{-# INLINE unsafePackLenBytes #-}
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder why these functions have not been inlined before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As the benchmarks show, if no fusion happens then there is no benefit in inlining (it even costs a few percents of performance).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I now think this is actually quite a big problem, because the only way this function is exposed in the safe API is through pack which can never fuse anyway, so is this patch really still desirable? We would need a safe packLen function to really get the benefits.

Copy link
Contributor Author

@noughtmare noughtmare Mar 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Here's a version of pack that is fusable (by using dynamic arrays doubling in size):

https://gist.github.com/noughtmare/8902ddec65fedae9fb24d3826dbe11a0

This again has the problem that it is slightly slower for short inputs if not fused (around x1.5), but a lot faster if it is fused.


unsafePackLenChars :: Int -> [Char] -> ByteString
unsafePackLenChars len cs0 =
unsafeCreate len $ \p -> go p cs0
where
go !_ [] = return ()
go !p (c:cs) = poke p (c2w c) >> go (p `plusPtr` 1) cs
unsafePackLenChars len =
unsafeCreate len . foldr
(\x go p -> poke p (c2w x) >> go (p `plusPtr` 1))
(\_ -> return ())
{-# INLINE unsafePackLenChars #-}
noughtmare marked this conversation as resolved.
Show resolved Hide resolved


-- | /O(n)/ Pack a null-terminated sequence of bytes, pointed to by an
Expand Down
13 changes: 13 additions & 0 deletions bench/BenchAll.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import Prelude hiding (words)

import qualified Data.ByteString as S
import qualified Data.ByteString.Char8 as S8
import qualified Data.ByteString.Internal as SI
import qualified Data.ByteString.Lazy as L
import qualified Data.ByteString.Lazy.Char8 as L8

Expand Down Expand Up @@ -481,6 +482,18 @@ main = do
[ bench "lazy" $ nf L8.unlines (map (L8.pack . show) intData)
, bench "strict" $ nf S8.unlines (map (S8.pack . show) intData)
]
, bgroup "pack"
[ bench "not fused" $ nf S.pack (replicate nRepl 0)
, bench "fused" $ nf (S.pack . replicate nRepl) 0
sjakobi marked this conversation as resolved.
Show resolved Hide resolved
]
, bgroup "unsafePackLenBytes"
[ bench "not fused" $ nf (SI.unsafePackLenBytes nRepl) (replicate nRepl 0)
, bench "fused" $ nf (SI.unsafePackLenBytes nRepl . replicate nRepl) 0
]
, bgroup "unsafePackLenChar"
[ bench "not fused" $ nf (SI.unsafePackLenChars nRepl) (replicate nRepl 'A')
, bench "fused" $ nf (SI.unsafePackLenChars nRepl . replicate nRepl) 'A'
]
, benchBoundsCheckFusion
, benchCount
, benchCSV
Expand Down