From 5f041aeb02ee976e8f766d17f56bedf3688bf7c8 Mon Sep 17 00:00:00 2001 From: Patrick Oscar Boykin Date: Sun, 8 Aug 2021 17:37:58 -1000 Subject: [PATCH 1/7] Improve mapEval somewhat in relation to #3947 --- core/src/main/scala/cats/data/Kleisli.scala | 15 +++++++++++++-- .../src/test/scala/cats/tests/KleisliSuite.scala | 13 +++++++++++++ 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/core/src/main/scala/cats/data/Kleisli.scala b/core/src/main/scala/cats/data/Kleisli.scala index e0d75bc885..27ef35a7e9 100644 --- a/core/src/main/scala/cats/data/Kleisli.scala +++ b/core/src/main/scala/cats/data/Kleisli.scala @@ -656,8 +656,19 @@ private[data] trait KleisliApply[F[_], A] extends Apply[Kleisli[F, A, *]] with K override def map2Eval[B, C, Z](fa: Kleisli[F, A, B], fb: Eval[Kleisli[F, A, C]])( f: (B, C) => Z - ): Eval[Kleisli[F, A, Z]] = - Eval.now(Kleisli(a => F.map2Eval(fa.run(a), fb.map(_.run(a)))(f).value)) + ): Eval[Kleisli[F, A, Z]] = { + // We should only evaluate fb once + val memoFb = fb.memoize + + Eval.now(Kleisli { a => + val fb = fa.run(a) + val efc = memoFb.map(_.run(a)) + val efz: Eval[F[Z]] = F.map2Eval(fb, efc)(f) + // This is not safe and results in stack overflows: + // see: https://github.com/typelevel/cats/issues/3947 + efz.value + }) + } override def product[B, C](fb: Kleisli[F, A, B], fc: Kleisli[F, A, C]): Kleisli[F, A, (B, C)] = Kleisli(a => F.product(fb.run(a), fc.run(a))) diff --git a/tests/src/test/scala/cats/tests/KleisliSuite.scala b/tests/src/test/scala/cats/tests/KleisliSuite.scala index 0879b4a02c..14e376233f 100644 --- a/tests/src/test/scala/cats/tests/KleisliSuite.scala +++ b/tests/src/test/scala/cats/tests/KleisliSuite.scala @@ -354,6 +354,19 @@ class KleisliSuite extends CatsSuite { assertEquals(program.run(A123), List((1, "2", true))) } + /* + test("traverse_ doesn't stack overflow") { + // see: https://github.com/typelevel/cats/issues/3947 + val res2 = (1 to 10000).toList.traverse_(_ => Kleisli.liftF[Id, String, Unit](())).run("") // fails with SO + assert(res2 == ()) + } + */ + + test("traverse_ doesn't stack overflow with List + Eval") { + // see: https://github.com/typelevel/cats/issues/3947 + (1 to 10000).toList.traverse_(_ => Kleisli.liftF[Eval, String, Unit](Eval.Unit)).run("").value + } + /** * Testing that implicit resolution works. If it compiles, the "test" passes. */ From f8842eb55c0b440a8056acb8fb1661d254bf70d9 Mon Sep 17 00:00:00 2001 From: Patrick Oscar Boykin Date: Sun, 8 Aug 2021 17:39:34 -1000 Subject: [PATCH 2/7] scalafmt --- tests/src/test/scala/cats/tests/KleisliSuite.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/src/test/scala/cats/tests/KleisliSuite.scala b/tests/src/test/scala/cats/tests/KleisliSuite.scala index 14e376233f..2941b411ac 100644 --- a/tests/src/test/scala/cats/tests/KleisliSuite.scala +++ b/tests/src/test/scala/cats/tests/KleisliSuite.scala @@ -360,7 +360,7 @@ class KleisliSuite extends CatsSuite { val res2 = (1 to 10000).toList.traverse_(_ => Kleisli.liftF[Id, String, Unit](())).run("") // fails with SO assert(res2 == ()) } - */ + */ test("traverse_ doesn't stack overflow with List + Eval") { // see: https://github.com/typelevel/cats/issues/3947 From f7401fdd5eaba243940fdadb32ab34ad08b21fc1 Mon Sep 17 00:00:00 2001 From: Patrick Oscar Boykin Date: Sun, 8 Aug 2021 18:48:57 -1000 Subject: [PATCH 3/7] fix traverse_ for List and Vector --- core/src/main/scala/cats/instances/list.scala | 24 +++++++++++++++++ .../main/scala/cats/instances/vector.scala | 26 +++++++++++++++++++ core/src/main/scala/cats/package.scala | 1 + .../test/scala/cats/tests/KleisliSuite.scala | 7 +++-- 4 files changed, 54 insertions(+), 4 deletions(-) diff --git a/core/src/main/scala/cats/instances/list.scala b/core/src/main/scala/cats/instances/list.scala index de746232dc..58faf46f1c 100644 --- a/core/src/main/scala/cats/instances/list.scala +++ b/core/src/main/scala/cats/instances/list.scala @@ -95,6 +95,30 @@ trait ListInstances extends cats.kernel.instances.ListInstances { wrapMutableIndexedSeq(as) }(f))(_.toList) + /** + * This avoids making a very deep stack by building a tree instead + */ + override def traverse_[G[_], A, B](fa: List[A])(f: A => G[B])(implicit G: Applicative[G]): G[Unit] = { + val empty = Eval.now(G.unit) + // the cost of this is O(size) + // c(n) = n/2 + c(n/2) = n/2 + n/4 + c(n/4) = ... 2 * n + def runHalf(size: Int, fa: List[A]): Eval[G[Unit]] = + fa match { + case Nil => empty + case h :: Nil => Eval.later(G.void(f(h))) + case _ => + val leftSize = size / 2 + val rightSize = size - leftSize + runHalf(leftSize, fa.take(leftSize)) + .flatMap { left => + val right = runHalf(rightSize, fa.drop(leftSize)) + G.map2Eval(left, right) { (_, _) => () } + } + } + + runHalf(fa.length, fa).value + } + def functor: Functor[List] = this def align[A, B](fa: List[A], fb: List[B]): List[A Ior B] = diff --git a/core/src/main/scala/cats/instances/vector.scala b/core/src/main/scala/cats/instances/vector.scala index 7362314834..fe33f44b89 100644 --- a/core/src/main/scala/cats/instances/vector.scala +++ b/core/src/main/scala/cats/instances/vector.scala @@ -95,6 +95,32 @@ trait VectorInstances extends cats.kernel.instances.VectorInstances { final override def traverse[G[_], A, B](fa: Vector[A])(f: A => G[B])(implicit G: Applicative[G]): G[Vector[B]] = G.map(Chain.traverseViaChain(fa)(f))(_.toVector) + /** + * This avoids making a very deep stack by building a tree instead + */ + override def traverse_[G[_], A, B](fa: Vector[A])(f: A => G[B])(implicit G: Applicative[G]): G[Unit] = { + val empty = Eval.now(G.unit) + // the cost of this is O(size) + // c(n) = n/2 + c(n/2) = n/2 + n/4 + c(n/4) = ... 2 * n + def runHalf(size: Int, idx: Int): Eval[G[Unit]] = + if (size > 1) { + val leftSize = size / 2 + val rightSize = size - leftSize + runHalf(leftSize, idx) + .flatMap { left => + val right = runHalf(rightSize, idx + leftSize) + G.map2Eval(left, right) { (_, _) => () } + } + } else if (size == 1) { + val a = fa(idx) + Eval.later { + val gb = f(a) + G.void(gb) + } + } else empty + + runHalf(fa.length, 0).value + } override def mapWithIndex[A, B](fa: Vector[A])(f: (A, Int) => B): Vector[B] = fa.iterator.zipWithIndex.map(ai => f(ai._1, ai._2)).toVector diff --git a/core/src/main/scala/cats/package.scala b/core/src/main/scala/cats/package.scala index 804cb8acec..6dc866bd8d 100644 --- a/core/src/main/scala/cats/package.scala +++ b/core/src/main/scala/cats/package.scala @@ -82,6 +82,7 @@ package object cats { override def ap[A, B](ff: A => B)(fa: A): B = ff(fa) override def flatten[A](ffa: A): A = ffa override def map2[A, B, Z](fa: A, fb: B)(f: (A, B) => Z): Z = f(fa, fb) + override def map2Eval[A, B, Z](fa: A, fb: Eval[B])(f: (A, B) => Z): Eval[Z] = fb.map(f(fa, _)) override def lift[A, B](f: A => B): A => B = f override def imap[A, B](fa: A)(f: A => B)(fi: B => A): B = f(fa) def foldLeft[A, B](a: A, b: B)(f: (B, A) => B) = f(b, a) diff --git a/tests/src/test/scala/cats/tests/KleisliSuite.scala b/tests/src/test/scala/cats/tests/KleisliSuite.scala index 2941b411ac..fc618b6467 100644 --- a/tests/src/test/scala/cats/tests/KleisliSuite.scala +++ b/tests/src/test/scala/cats/tests/KleisliSuite.scala @@ -354,13 +354,12 @@ class KleisliSuite extends CatsSuite { assertEquals(program.run(A123), List((1, "2", true))) } - /* test("traverse_ doesn't stack overflow") { // see: https://github.com/typelevel/cats/issues/3947 - val res2 = (1 to 10000).toList.traverse_(_ => Kleisli.liftF[Id, String, Unit](())).run("") // fails with SO - assert(res2 == ()) + val resL = (1 to 10000).toList.traverse_(_ => Kleisli.liftF[Id, String, Unit](())).run("") + val resV = (1 to 10000).toVector.traverse_(_ => Kleisli.liftF[Id, String, Unit](())).run("") + assert(resL == resV) } - */ test("traverse_ doesn't stack overflow with List + Eval") { // see: https://github.com/typelevel/cats/issues/3947 From 17be65fcee0b3185e9e1d2bb7e2ea94953dd51dc Mon Sep 17 00:00:00 2001 From: Patrick Oscar Boykin Date: Sun, 8 Aug 2021 19:12:43 -1000 Subject: [PATCH 4/7] optimize a bit --- core/src/main/scala/cats/instances/list.scala | 32 +++++++++++-------- .../main/scala/cats/instances/vector.scala | 2 +- 2 files changed, 20 insertions(+), 14 deletions(-) diff --git a/core/src/main/scala/cats/instances/list.scala b/core/src/main/scala/cats/instances/list.scala index 58faf46f1c..a0a9be987c 100644 --- a/core/src/main/scala/cats/instances/list.scala +++ b/core/src/main/scala/cats/instances/list.scala @@ -101,20 +101,26 @@ trait ListInstances extends cats.kernel.instances.ListInstances { override def traverse_[G[_], A, B](fa: List[A])(f: A => G[B])(implicit G: Applicative[G]): G[Unit] = { val empty = Eval.now(G.unit) // the cost of this is O(size) - // c(n) = n/2 + c(n/2) = n/2 + n/4 + c(n/4) = ... 2 * n + // c(n) = n + 2 * c(n/2) def runHalf(size: Int, fa: List[A]): Eval[G[Unit]] = - fa match { - case Nil => empty - case h :: Nil => Eval.later(G.void(f(h))) - case _ => - val leftSize = size / 2 - val rightSize = size - leftSize - runHalf(leftSize, fa.take(leftSize)) - .flatMap { left => - val right = runHalf(rightSize, fa.drop(leftSize)) - G.map2Eval(left, right) { (_, _) => () } - } - } + if (size > 2) { + val leftSize = size / 2 + val rightSize = size - leftSize + runHalf(leftSize, fa.take(leftSize)) + .flatMap { left => + // we are defering here to potentially skip the fa.drop + // work which we may not need if left is already a failure + val right = Eval.defer(runHalf(rightSize, fa.drop(leftSize))) + G.map2Eval(left, right) { (_, _) => () } + } + } else if (size == 1) { + // avoid pattern matching when we know that there is only one element + val a = fa.head + Eval.later { + val gb = f(a) + G.void(gb) + } + } else empty runHalf(fa.length, fa).value } diff --git a/core/src/main/scala/cats/instances/vector.scala b/core/src/main/scala/cats/instances/vector.scala index fe33f44b89..38e70643d6 100644 --- a/core/src/main/scala/cats/instances/vector.scala +++ b/core/src/main/scala/cats/instances/vector.scala @@ -101,7 +101,7 @@ trait VectorInstances extends cats.kernel.instances.VectorInstances { override def traverse_[G[_], A, B](fa: Vector[A])(f: A => G[B])(implicit G: Applicative[G]): G[Unit] = { val empty = Eval.now(G.unit) // the cost of this is O(size) - // c(n) = n/2 + c(n/2) = n/2 + n/4 + c(n/4) = ... 2 * n + // c(n) = n + 2 * c(n/2) def runHalf(size: Int, idx: Int): Eval[G[Unit]] = if (size > 1) { val leftSize = size / 2 From e6b9cd7a9f744e8d9e06696f7099452e7eb67a2c Mon Sep 17 00:00:00 2001 From: Patrick Oscar Boykin Date: Sun, 8 Aug 2021 19:32:46 -1000 Subject: [PATCH 5/7] fix a bug, add a test to check for it --- core/src/main/scala/cats/instances/list.scala | 2 +- laws/src/main/scala/cats/laws/TraverseLaws.scala | 1 + tests/src/test/scala/cats/tests/TraverseSuite.scala | 6 ++++++ 3 files changed, 8 insertions(+), 1 deletion(-) diff --git a/core/src/main/scala/cats/instances/list.scala b/core/src/main/scala/cats/instances/list.scala index a0a9be987c..62d2d54cb4 100644 --- a/core/src/main/scala/cats/instances/list.scala +++ b/core/src/main/scala/cats/instances/list.scala @@ -103,7 +103,7 @@ trait ListInstances extends cats.kernel.instances.ListInstances { // the cost of this is O(size) // c(n) = n + 2 * c(n/2) def runHalf(size: Int, fa: List[A]): Eval[G[Unit]] = - if (size > 2) { + if (size > 1) { val leftSize = size / 2 val rightSize = size - leftSize runHalf(leftSize, fa.take(leftSize)) diff --git a/laws/src/main/scala/cats/laws/TraverseLaws.scala b/laws/src/main/scala/cats/laws/TraverseLaws.scala index 8d90825c7c..856e212392 100644 --- a/laws/src/main/scala/cats/laws/TraverseLaws.scala +++ b/laws/src/main/scala/cats/laws/TraverseLaws.scala @@ -84,6 +84,7 @@ trait TraverseLaws[F[_]] extends FunctorLaws[F] with FoldableLaws[F] with Unorde first <-> traverseFirst } + def mapWithIndexRef[A, B](fa: F[A], f: (A, Int) => B): IsEq[F[B]] = { val lhs = F.mapWithIndex(fa)(f) val rhs = F.traverse(fa)(a => State((s: Int) => (s + 1, f(a, s)))).runA(0).value diff --git a/tests/src/test/scala/cats/tests/TraverseSuite.scala b/tests/src/test/scala/cats/tests/TraverseSuite.scala index 1138952be1..3d772a5e0e 100644 --- a/tests/src/test/scala/cats/tests/TraverseSuite.scala +++ b/tests/src/test/scala/cats/tests/TraverseSuite.scala @@ -32,6 +32,12 @@ abstract class TraverseSuite[F[_]: Traverse](name: String)(implicit ArbFInt: Arb } } + test(s"Traverse[$name].traverseMatches_ with Option") { + forAll { (fa: F[Int], fn: Int => Option[Int]) => + assert(Applicative[Option].void(fa.traverse(fn)) == fa.traverse_(fn)) + } + } + } object TraverseSuite { From 2e2060f743d98d33316c069fe2b37e890a727b45 Mon Sep 17 00:00:00 2001 From: Patrick Oscar Boykin Date: Mon, 9 Aug 2021 07:51:08 -1000 Subject: [PATCH 6/7] fix some issues, respond to reviews --- core/src/main/scala/cats/instances/list.scala | 14 ++++++++------ core/src/main/scala/cats/instances/vector.scala | 12 +++++++----- .../src/test/scala/cats/tests/TraverseSuite.scala | 2 +- 3 files changed, 16 insertions(+), 12 deletions(-) diff --git a/core/src/main/scala/cats/instances/list.scala b/core/src/main/scala/cats/instances/list.scala index 62d2d54cb4..7e7fa047e0 100644 --- a/core/src/main/scala/cats/instances/list.scala +++ b/core/src/main/scala/cats/instances/list.scala @@ -99,9 +99,9 @@ trait ListInstances extends cats.kernel.instances.ListInstances { * This avoids making a very deep stack by building a tree instead */ override def traverse_[G[_], A, B](fa: List[A])(f: A => G[B])(implicit G: Applicative[G]): G[Unit] = { - val empty = Eval.now(G.unit) - // the cost of this is O(size) - // c(n) = n + 2 * c(n/2) + // the cost of this is O(size log size) + // c(n) = n + 2 * c(n/2) = n + 2(n/2 log (n/2)) = n + n (logn - 1) = n log n + // invariant: size >= 1 def runHalf(size: Int, fa: List[A]): Eval[G[Unit]] = if (size > 1) { val leftSize = size / 2 @@ -113,16 +113,18 @@ trait ListInstances extends cats.kernel.instances.ListInstances { val right = Eval.defer(runHalf(rightSize, fa.drop(leftSize))) G.map2Eval(left, right) { (_, _) => () } } - } else if (size == 1) { + } else { // avoid pattern matching when we know that there is only one element val a = fa.head Eval.later { val gb = f(a) G.void(gb) } - } else empty + } - runHalf(fa.length, fa).value + val len = fa.length + if (len == 0) G.unit + else runHalf(len, fa).value } def functor: Functor[List] = this diff --git a/core/src/main/scala/cats/instances/vector.scala b/core/src/main/scala/cats/instances/vector.scala index 38e70643d6..6386451120 100644 --- a/core/src/main/scala/cats/instances/vector.scala +++ b/core/src/main/scala/cats/instances/vector.scala @@ -99,9 +99,9 @@ trait VectorInstances extends cats.kernel.instances.VectorInstances { * This avoids making a very deep stack by building a tree instead */ override def traverse_[G[_], A, B](fa: Vector[A])(f: A => G[B])(implicit G: Applicative[G]): G[Unit] = { - val empty = Eval.now(G.unit) // the cost of this is O(size) - // c(n) = n + 2 * c(n/2) + // c(n) = 1 + 2 * c(n/2) + // invariant: size >= 1 def runHalf(size: Int, idx: Int): Eval[G[Unit]] = if (size > 1) { val leftSize = size / 2 @@ -111,15 +111,17 @@ trait VectorInstances extends cats.kernel.instances.VectorInstances { val right = runHalf(rightSize, idx + leftSize) G.map2Eval(left, right) { (_, _) => () } } - } else if (size == 1) { + } else { val a = fa(idx) Eval.later { val gb = f(a) G.void(gb) } - } else empty + } - runHalf(fa.length, 0).value + val len = fa.length + if (len == 0) G.unit + else runHalf(len, 0).value } override def mapWithIndex[A, B](fa: Vector[A])(f: (A, Int) => B): Vector[B] = fa.iterator.zipWithIndex.map(ai => f(ai._1, ai._2)).toVector diff --git a/tests/src/test/scala/cats/tests/TraverseSuite.scala b/tests/src/test/scala/cats/tests/TraverseSuite.scala index 3d772a5e0e..21ea7bad5f 100644 --- a/tests/src/test/scala/cats/tests/TraverseSuite.scala +++ b/tests/src/test/scala/cats/tests/TraverseSuite.scala @@ -32,7 +32,7 @@ abstract class TraverseSuite[F[_]: Traverse](name: String)(implicit ArbFInt: Arb } } - test(s"Traverse[$name].traverseMatches_ with Option") { + test(s"Traverse[$name].traverse matches traverse_ with Option") { forAll { (fa: F[Int], fn: Int => Option[Int]) => assert(Applicative[Option].void(fa.traverse(fn)) == fa.traverse_(fn)) } From ba7afd2d43647d475e1aade86ca398f0376f4ce7 Mon Sep 17 00:00:00 2001 From: Patrick Oscar Boykin Date: Mon, 9 Aug 2021 09:08:37 -1000 Subject: [PATCH 7/7] add docs, use always --- core/src/main/scala/cats/instances/list.scala | 18 +++++++++++++----- .../src/main/scala/cats/instances/vector.scala | 11 ++++++++++- 2 files changed, 23 insertions(+), 6 deletions(-) diff --git a/core/src/main/scala/cats/instances/list.scala b/core/src/main/scala/cats/instances/list.scala index 7e7fa047e0..36f741dd98 100644 --- a/core/src/main/scala/cats/instances/list.scala +++ b/core/src/main/scala/cats/instances/list.scala @@ -106,17 +106,25 @@ trait ListInstances extends cats.kernel.instances.ListInstances { if (size > 1) { val leftSize = size / 2 val rightSize = size - leftSize - runHalf(leftSize, fa.take(leftSize)) + val (leftL, rightL) = fa.splitAt(leftSize) + runHalf(leftSize, leftL) .flatMap { left => - // we are defering here to potentially skip the fa.drop - // work which we may not need if left is already a failure - val right = Eval.defer(runHalf(rightSize, fa.drop(leftSize))) + val right = runHalf(rightSize, rightL) G.map2Eval(left, right) { (_, _) => () } } } else { // avoid pattern matching when we know that there is only one element val a = fa.head - Eval.later { + // we evaluate this at most one time, + // always is a bit cheaper in such cases + // + // Here is the point of the laziness using Eval: + // we avoid calling f(a) or G.void in the + // event that the computation has already + // failed. We do not use laziness to avoid + // traversing fa, which we will do fully + // in all cases. + Eval.always { val gb = f(a) G.void(gb) } diff --git a/core/src/main/scala/cats/instances/vector.scala b/core/src/main/scala/cats/instances/vector.scala index 6386451120..a9e53a9a0c 100644 --- a/core/src/main/scala/cats/instances/vector.scala +++ b/core/src/main/scala/cats/instances/vector.scala @@ -113,7 +113,16 @@ trait VectorInstances extends cats.kernel.instances.VectorInstances { } } else { val a = fa(idx) - Eval.later { + // we evaluate this at most one time, + // always is a bit cheaper in such cases + // + // Here is the point of the laziness using Eval: + // we avoid calling f(a) or G.void in the + // event that the computation has already + // failed. We do not use laziness to avoid + // traversing fa, which we will do fully + // in all cases. + Eval.always { val gb = f(a) G.void(gb) }