Skip to content

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