Skip to content

Conversation

hdgarrood
Copy link
Contributor

Change filterM to just be filterA in order to have this be a non-breaking change; I was just thinking it would be nice for all these performance improvements to be available for people without having to update to the next major version.

This removes another use of uncons, refs #71.

The new devDependency on Const is because that seemed like the best choice for testing that effects are applied in the right order; the tests were not previously testing that filterM applies effects in the right order.

Benchmarking code:

benchFilterM :: Benchmark
benchFilterM = mkBenchmark
  { slug: "filterM"
  , title: "filterM"
  , sizes: A.range 1 5 <#> (_ * 500)
  , sizeInterpretation: "Length of the array"
  , inputsPerSize: 1
  , gen: \n -> vectorOf n (arbitrary :: Gen Int)
  , functions: [ benchFn "current"   (currentFilterM pred)
               , benchFn "suggested" (suggestedFilterM pred)
               ]
  }
  where
  pred :: Int -> WriterT Unit Identity Boolean
  pred x = tell unit $> even x

I had to stop at 2500 elements since the current implementation stack overflows at around 3k, but the new implementation should work for much larger arrays, because of the stack-safety of the Traversable Array instance. I tested this implementation with up to 10k elements and it seemed to work fine.

Change filterM to just be filterA in order to have this be a
non-breaking change.

This removes another use of uncons, refs #71
@hdgarrood
Copy link
Contributor Author

Oh, nearly forgot - benchmark results!
filterm

@hdgarrood
Copy link
Contributor Author

Also I should probably check - are we ok with deprecating filterM now that we are exporting filterA?

@paf31
Copy link
Contributor

paf31 commented Nov 21, 2016

👍 LGTM, thanks (and also +1 to deprecating filterM)

@hdgarrood
Copy link
Contributor Author

Great. :)

@hdgarrood hdgarrood merged commit 40f5b0a into master Nov 21, 2016
@hdgarrood hdgarrood deleted the filterA branch November 21, 2016 18:39
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.

2 participants