-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Optimize traverse #4498
Optimize traverse #4498
Changes from 18 commits
858cca4
847df54
878c4d0
9d86c98
c4bcba2
c4c64ec
1344a70
b6973dc
f0648c4
dbccc8e
fbf5ea5
9f60d2b
f4fcf57
d50bf83
5c5049f
a1c9ff5
8cc742e
e3ae584
f95b087
c5d90a6
2d5f4d7
702ab8b
b08196e
92c91e4
7d615dc
eebed5b
803107d
0ee49e6
6529be7
a62ce4c
6cb787d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,6 +23,7 @@ package cats | |
|
||
import cats.data.State | ||
import cats.data.StateT | ||
import cats.kernel.compat.scalaVersionSpecific._ | ||
|
||
/** | ||
* Traverse, also known as Traversable. | ||
|
@@ -284,4 +285,30 @@ object Traverse { | |
@deprecated("Use cats.syntax object imports", "2.2.0") | ||
object nonInheritedOps extends ToTraverseOps | ||
|
||
private[cats] def traverseDirectly[G[_], A, B]( | ||
fa: IterableOnce[A] | ||
)(f: A => G[B])(implicit G: StackSafeMonad[G]): G[List[B]] = { | ||
G.map(fa.iterator.foldLeft(G.pure(List.empty[B])) { case (accG, a) => | ||
G.flatMap(accG) { acc => | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. add a comment why not Since we know G is lazy (a StackSafeMonad has to be I think), I'm not sure the downside here. One answer would be we don't have to call By calling map2 we are at least communicating to G what we are doing, and in principle, some monads could optimize this (e.g. a Parser can make a more optimized map2 than flatMap, and it can also be StackSafe since runs lazily only when input is passed to the resulting parser). |
||
G.map(f(a)) { b => | ||
b :: acc | ||
} | ||
} | ||
})(_.reverse) | ||
} | ||
|
||
private[cats] def traverse_Directly[G[_], A, B]( | ||
fa: IterableOnce[A] | ||
)(f: A => G[B])(implicit G: StackSafeMonad[G]): G[Unit] = { | ||
val iter = fa.iterator | ||
if (iter.hasNext) { | ||
val first = iter.next() | ||
G.void(iter.foldLeft(f(first)) { case (g, a) => | ||
G.flatMap(g) { _ => | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why not There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was no good reason 😅 Thanks, I'll change it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Maybe this is something we should consider though? Edit: ahh, I see your comment on f95b087. Fair enough. |
||
f(a) | ||
} | ||
}) | ||
} else G.unit | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,7 @@ | |
package cats | ||
|
||
import cats.data.State | ||
import cats.kernel.compat.scalaVersionSpecific._ | ||
|
||
import scala.collection.immutable.{IntMap, TreeSet} | ||
|
||
|
@@ -203,4 +204,17 @@ object TraverseFilter { | |
@deprecated("Use cats.syntax object imports", "2.2.0") | ||
object nonInheritedOps extends ToTraverseFilterOps | ||
|
||
private[cats] def traverseFilterDirectly[G[_], A, B]( | ||
fa: IterableOnce[A] | ||
)(f: A => G[Option[B]])(implicit G: StackSafeMonad[G]): G[Vector[B]] = { | ||
fa.iterator.foldLeft(G.pure(Vector.empty[B])) { case (bldrG, a) => | ||
G.flatMap(bldrG) { acc => | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. add a comment on why we didn't write: G.map2(bldrG, f(a)) {
case (acc, Some(b)) => acc :+ b
case (acc, _) => acc
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, good point 👍 |
||
G.map(f(a)) { | ||
case Some(b) => acc :+ b | ||
case None => acc | ||
} | ||
} | ||
} | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1241,11 +1241,27 @@ sealed abstract private[data] class ChainInstances extends ChainInstances1 { | |
def traverse[G[_], A, B](fa: Chain[A])(f: A => G[B])(implicit G: Applicative[G]): G[Chain[B]] = | ||
if (fa.isEmpty) G.pure(Chain.nil) | ||
else | ||
traverseViaChain { | ||
val as = collection.mutable.ArrayBuffer[A]() | ||
as ++= fa.iterator | ||
KernelStaticMethods.wrapMutableIndexedSeq(as) | ||
}(f) | ||
G match { | ||
johnynek marked this conversation as resolved.
Show resolved
Hide resolved
|
||
case x: StackSafeMonad[G] => | ||
x.map(Traverse.traverseDirectly(fa.iterator)(f)(x))(Chain.fromSeq(_)) | ||
case _ => | ||
traverseViaChain { | ||
val as = collection.mutable.ArrayBuffer[A]() | ||
as ++= fa.iterator | ||
KernelStaticMethods.wrapMutableIndexedSeq(as) | ||
}(f) | ||
} | ||
|
||
override def traverse_[G[_], A, B](fa: Chain[A])(f: A => G[B])(implicit G: Applicative[G]): G[Unit] = | ||
G match { | ||
johnynek marked this conversation as resolved.
Show resolved
Hide resolved
|
||
case x: StackSafeMonad[G] => Traverse.traverse_Directly(fa.iterator)(f)(x) | ||
case _ => | ||
foldRight(fa, Always(G.pure(()))) { (a, acc) => | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: |
||
G.map2Eval(f(a), acc) { (_, _) => | ||
() | ||
} | ||
}.value | ||
} | ||
|
||
override def mapAccumulate[S, A, B](init: S, fa: Chain[A])(f: (S, A) => (S, B)): (S, Chain[B]) = | ||
StaticMethods.mapAccumulateFromStrictFunctor(init, fa, f)(this) | ||
|
@@ -1354,11 +1370,16 @@ sealed abstract private[data] class ChainInstances extends ChainInstances1 { | |
def traverseFilter[G[_], A, B](fa: Chain[A])(f: A => G[Option[B]])(implicit G: Applicative[G]): G[Chain[B]] = | ||
if (fa.isEmpty) G.pure(Chain.nil) | ||
else | ||
traverseFilterViaChain { | ||
val as = collection.mutable.ArrayBuffer[A]() | ||
as ++= fa.iterator | ||
KernelStaticMethods.wrapMutableIndexedSeq(as) | ||
}(f) | ||
G match { | ||
case x: StackSafeMonad[G] => | ||
G.map(TraverseFilter.traverseFilterDirectly(fa.iterator)(f)(x))(Chain.fromSeq(_)) | ||
case _ => | ||
johnynek marked this conversation as resolved.
Show resolved
Hide resolved
|
||
traverseFilterViaChain { | ||
val as = collection.mutable.ArrayBuffer[A]() | ||
as ++= fa.iterator | ||
KernelStaticMethods.wrapMutableIndexedSeq(as) | ||
}(f) | ||
} | ||
|
||
override def filterA[G[_], A](fa: Chain[A])(f: A => G[Boolean])(implicit G: Applicative[G]): G[Chain[A]] = | ||
traverse | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.