-
Notifications
You must be signed in to change notification settings - Fork 73
Data.String.CodePoints.uncons probably isn't constant-time #120
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
Comments
cc @michaelficarra, I'd appreciate any input you might have here. |
@hdgarrood I don't work on any of the big JavaScript engines, but I'm pretty sure any reasonably-performant JavaScript engine will implement |
Oh of course, that’s good to hear. Thanks! |
I just tried to verify this with the following program: module Main where
import Prelude
import Data.Array as Array
import Data.Foldable (for_, foldMap)
import Data.Int as Int
import Data.Maybe (fromMaybe, maybe, Maybe(..))
import Data.String as String
import Data.Unfoldable (replicateA)
import Effect (Effect)
import Effect.Console (log, logShow)
import Effect.Random (randomInt)
import Performance.Minibench (bench)
main :: Effect Unit
main = do
log "testing (should print 15):"
logShow (digitSum "lol 1234 hahaha 5")
let inputLengths = (map (_ * 100_000) (Array.range 1 10))
for_ inputLengths \l -> do
log ""
log ("string of length " <> show l)
s <- genRandomString l
bench \_ -> digitSum s
sampler :: String
sampler = "abcdefg1234567890"
genRandomString :: Int -> Effect String
genRandomString resultLength =
let
len = String.length sampler
in
map (foldMap (maybe "" String.singleton) :: Array _ -> _)
$ replicateA resultLength do
ix <- randomInt 0 (len - 1)
pure $ Array.index (String.toCodePointArray sampler) ix
-- | Sum all digits appearing in the input string.
digitSum :: String -> Int
digitSum = go 0
where
go acc s =
case String.uncons s of
Just { head, tail } ->
let
value =
fromMaybe 0
$ Int.fromString
$ String.singleton head
in
go (value + acc) tail
Nothing ->
acc Here are the results on Node v12.4.0:
which seems close enough to linear, which is what we'd expect with a constant-time uncons. So I'm happy to close this. |
Here's the documentation for
uncons
:I would have expected this to be O(n), because you need to copy almost the entirety of the argument string into the
tail
field of the result. We should probably verify this before changing it.The text was updated successfully, but these errors were encountered: