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.

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

Add Data.ByteString.Short.unconsN #525

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

Conversation

hasufell
Copy link
Member

@hasufell hasufell commented Jul 3, 2022

Fixes #524

@Bodigrim
Copy link
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
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
Contributor

@hasufell could you please rebase?

@sjakobi
Copy link
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
Contributor

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

@hasufell
Copy link
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
Contributor

This suggests that unconsNViaSplit is the fastest option, right?

@hasufell
Copy link
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