Skip to content

Add Data.ByteString.Short.unconsN#525

Draft
hasufell wants to merge 2 commits intohaskell:masterfrom
hasufell:unconsN-bench
Draft

Add Data.ByteString.Short.unconsN#525
hasufell wants to merge 2 commits intohaskell:masterfrom
hasufell:unconsN-bench

Conversation

@hasufell
Copy link
Copy Markdown
Member

@hasufell hasufell commented Jul 3, 2022

Fixes #524

@Bodigrim
Copy link
Copy Markdown
Contributor

Bodigrim commented Jul 3, 2022

Is your implementation of unconsN faster than split / unpack?

myUnconsN :: Int -> ShortByteString -> Maybe ([Word8], ShortByteString)
myUnconsN n x
  | S.length x < n = Nothing
  | otherwise = Just (S.unpack y, z)
  where
    (y, z) = S.splitAt n x

@hasufell
Copy link
Copy Markdown
Member Author

hasufell commented Jul 3, 2022

Is your implementation of unconsN faster than split / unpack?

It seems so:

All
  ShortByteString
    ShortByteString unpack/uncons comparison
      unpack and look at first 5 elements: OK (1.09s)
        15.3 ms ± 1.1 ms
      uncons consecutively 5 times:        OK (0.92s)
        401  μs ±  34 μs
      unconsN 5:                           OK (0.69s)
        80.4 ns ± 5.9 ns
      myUnconsN 5:                         OK (0.93s)
        49.8 μs ± 3.3 μs

@Bodigrim
Copy link
Copy Markdown
Contributor

@hasufell could you please rebase?

@sjakobi
Copy link
Copy Markdown
Member

sjakobi commented Sep 26, 2022

Is this PR still needed now that unpack performance has been improved in #526? Frankly I don't find the API particularly appealing. For example, it seems that the [Word8] returned is never empty?!

For completeness, I think we'll also end up adding unsnocN, and variants for StrictByteString and LazyByteString – a significant amount of additional code.

@Bodigrim
Copy link
Copy Markdown
Contributor

It might be that benchmarks after rebase will be more favorable to split / unpack and this PR will be redundant.

@hasufell
Copy link
Copy Markdown
Member Author

All
  ShortByteString
    ShortByteString unpack/uncons comparison
      unpack and look at first 5 elements: OK (0.31s)
        412  ns ±  35 ns
      uncons consecutively 5 times:        OK (0.94s)
        406  μs ±  38 μs
      unconsN 5:                           OK (0.17s)
        77.4 ns ± 5.8 ns
      unconsNViaSplit 5:                   OK (0.15s)
        33.8 ns ± 3.0 ns

All 4 tests passed (1.61s)

@Bodigrim
Copy link
Copy Markdown
Contributor

This suggests that unconsNViaSplit is the fastest option, right?

@hasufell
Copy link
Copy Markdown
Member Author

I guess so

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Provide unconsN/unsnocN?

3 participants