Divide and conquer array traverse

[S11001001]

Port of scalaz/scalaz#1023, as explained there.

Includes test that fails the stack-safety check with previous traverseArrayImpl, but passes with this one.

Original discussion from IRC:

[2016-03-31 16:57:54] <S11001001> (I mean [Array] should probably be divide-and-conquer but what do I know about JS allocation efficiency) <hdgarrood> the array one used to be quadratic, even, I fixed that a while ago <hdgarrood> oh in stack, right, sorry <hdgarrood> S11001001: what do you mean by divide-and-conquer? <S11001001> hdgarrood: if you traverse the halves, then liftA2 (<>) <S11001001> hdgarrood: whether that's more efficient than going via a cons list I'm not sure; in some VMs it certainly is <thimoteus> https://en.wikipedia.org/wiki/Divide_and_conquer_algorithms <hdgarrood> Ah, I see. Do you still traverse in the right order though? <S11001001> hdgarrood: yeah, you end up with equal results because <*> is associative <hdgarrood> ah of course <hdgarrood> if you do an implementation I'd happily set a benchmark up <S11001001> hdgarrood: k <hdgarrood> so that's logarithmic stack then? <S11001001> hdgarrood: yep [2016-03-31 17:01:30] 

[garyb]

This looks pretty great to me! @hdgarrood did you want to try this with benchotron or something?

[hdgarrood]

That would be nice, although I am struggling to find the time right now. 👍 lgtm, especially considering that this helps avoid stack overflows.

[garyb]

@paf31 any comments here either?

[syaiful6]

@S11001001 is it work for array that have length odd number, eg 3, 5 etc. Pardon me, if i wrong, It look like your implementation will fail if the array length is odd. for example:

// subtitute top to odd number and bottom is 0 (because we start with 0), // let pick 3 for the array.length var pivot = 0 + Math.floor((3 - 0) / 4) * 2 // pivot is 0 

and you divide it to go(bot, pivot) and go(pivot, top), assuming the array length is 3, then you will divide it to 1. go(0, 0), and 2. go(0, 3), the 2 part will fail because it same value as starting value, and because 3 - 0 is 3, it will continue the recursion.

[syaiful6]

Sorry, It look like it will only fail if the array length is 3, you maybe only need Math.floor((top - bot) / 2)

[hdgarrood]

@syaiful6 yep you're right, thanks for the heads up. I'll see if I can fix this.

[S11001001]

@syaiful6 Thanks for finding this 3 problem; for posterity, further discussion in #54.

[paf31]

Now that List has a stack-safe traverse, that gives another way to make this traverse stack-safe, which seems simpler. Any thoughts?

[hdgarrood]

The current Traversable Array and Traversable List are stack-safe in one sense, but this PR would make Traversable Array safer than both, in that it additionally helps prevent stack overflows arising from large numbers of chained applys. For example, this will stack overflow with the current Traversable Array and Traversable List instances, but will work with the code in this PR:

import Prelude import Data.Traversable import Data.List as L import Test.Assert (assert) import Control.Monad.Eff.Console (log) main = do let xs = L.range 1 25000 assert $ (traverse pure xs :: Unit -> L.List Int) unit == xs log "done"

[paf31]

Oh, I see now, thanks.

[hdgarrood]

Also, I benchmarked this locally and traverse Just seems to perform just slightly slower with the divide-and-conquer approach than with the current one - i.e. not enough to worry about. However, Maybe is going to be one of the cheapest Applicatives, and I would guess this approach is going to really shine when the apply of the underlying Applicative is more expensive.

[hdgarrood]

Ok, this is not really what I would have expected, since I would have expected all the apply calls to dominate the running time here:

benchTraverse :: Benchmark benchTraverse = mkBenchmark { slug: "traverse" , title: "Traversable Array" , sizes: A.range 1 5 <#> (_ * 1000) , sizeInterpretation: "Length of the array" , inputsPerSize: 1 , gen: \n -> vectorOf n (arbitrary :: Gen Int) , functions: [ benchFn "current" (traverse f) , benchFn "suggested" (traverse f <<< DivideConquer) ] }  where f :: forall e a. a -> MaybeT (WriterT Unit (ExceptT Unit (Eff e))) a f x = do tell unit pure x

Produced this:

Maybe we'd only see the effect I expected with an even more expensive apply, or with a larger array. Even so, the divide-and-conquer approach still seems like the right choice because of the logarithmic number of nested apply calls, I think.

[syaiful6]

@S11001001 ok, it look like we can fix it by make sure the pivot never equals to zero, so we make progress in each recursion. We can update our pivot computation to:

// pick 1 in case the computation return zero var pivot = (bottom + Math.floor((top - bottom) / 4) * 2) || 1 

[hdgarrood]

#54 is a slightly further along version of this, so shall we close this?

[S11001001]

@hdgarrood sure.