Skip to content
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.

Sign up for GitHub

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

Jump to bottom

remove casts from Eval, fix stack overflow in Eval #3519

Merged
merged 5 commits into from
Jul 16, 2020
Merged

remove casts from Eval, fix stack overflow in Eval #3519

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9a81bb7
remove casts from Eval
johnynek Jul 12, 2020
fbc4dab
move allocations down a bit
johnynek Jul 12, 2020
fef839f
fix an Eval stack overflow
johnynek Jul 12, 2020
8ce139f
add Eval.Leaf to make reasoning clearer
johnynek Jul 13, 2020
12eebe6
simplify a bit more, update some Eval.now
johnynek Jul 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 28 additions & 19 deletions core/src/main/scala/cats/Eval.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,52 +318,61 @@ object Eval extends EvalInstances {
}
}

private def evaluate[A](e: Eval[A]): A = {
type L = Eval[Any]
type M = Memoize[Any]
type C = Any => Eval[Any]
/*
* This represents the stack of flatmap functions in a series
* of Eval operations
*/
sealed abstract private class FnStack[-A, +B]
final private case class Ident[A, B](ev: A <:< B) extends FnStack[A, B]
final private case class Many[A, B, C](first: A => Eval[B], rest: FnStack[B, C]) extends FnStack[A, C]

def addToMemo(m: M): C = { (a: Any) =>
private def evaluate[A](e: Eval[A]): A = {
def addToMemo[A1](m: Memoize[A1]): A1 => Eval[A1] = { (a: A1) =>
m.result = Some(a)
Now(a)
}

@tailrec def loop(curr: L, fs: List[C]): Any =
@tailrec def loop[A1](curr: Eval[A1], fs: FnStack[A1, A]): A =
curr match {
case c: FlatMap[_] =>
case c: FlatMap[A1] =>
val nextFs: Many[c.Start, A1, A] = Many(c.run, fs)
c.start() match {
case cc: FlatMap[_] =>
loop(cc.start().asInstanceOf[L], cc.run.asInstanceOf[C] :: c.run.asInstanceOf[C] :: fs)
case cc: FlatMap[c.Start] =>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not entirely sure why we don't do:

case c: FlatMap[A1] =>
  loop(c.start(), Many(c.run, fs))

and let the rest of the code run.

That would be correct, but I assume @non inlined this because for a FlatMap we know that we have at least one item on the stack and if we do my suggestion above, we allocate a Many just to pattern match on it in many cases and discard it. So, by nesting the pattern match on the c.start() we often can often sometimes avoid the allocation and pattern match.

If @non has time to chime in, it would be appreciated, I could update a comment to make it clearer.

loop(cc.start(), Many(cc.run, nextFs))
case mm @ Memoize(eval) =>
mm.result match {
case Some(a) =>
loop(Now(a), c.run.asInstanceOf[C] :: fs)
loop(nextFs.first(a), nextFs.rest)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here is an optimization: since we know we already have a Many, we don't need to recurse.

case None =>
loop(eval, addToMemo(mm.asInstanceOf[M]) :: c.run.asInstanceOf[C] :: fs)
loop(eval, Many(addToMemo(mm), nextFs))
}
case xx =>
// note: xx could be a Defer so calling .value
// could blow the stack?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks unsafe to me. I don't see why we don't need to check for Defer here. I think this could be the cause of #2587

If this is safe, I think we need a comment as to why xx.value is okay.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@non do you know why we wouldn't do loop(xx, Many(c.run, fs)) when we have a Defer?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, so defer(e).flatMap(f) is safe because .flatMap checks for Defer and uses the thunk as start, BUT defer(defer(e)).flatMap(f) isn't safe because flatMap only unwraps one layer of Defer and then we wind up calling .value on a defer which blows the stack.

I reproed this failure mode in a test.

Should I combine with this code for the fix or make a follow up PR?

loop(c.run(xx.value), fs)
}
case call: Defer[_] =>
case call: Defer[A1] =>
loop(advance(call), fs)
case m @ Memoize(eval) =>
case m: Memoize[a] =>
// a <:< A1
m.result match {
case Some(a) =>
fs match {
case f :: fs => loop(f(a), fs)
case Nil => a
case Many(f, fs) => loop(f(a), fs)
case Ident(ev) => ev(a)
}
case None =>
loop(eval, addToMemo(m.asInstanceOf[M]) :: fs)
loop[a](m.eval, Many[a, A1, A](addToMemo[a](m), fs))
}
case x =>
// Now, Later or Always don't have recursions
fs match {
case f :: fs => loop(f(x.value), fs)
case Nil => x.value
case Many(f, fs) => loop(f(x.value), fs)
case Ident(ev) => ev(x.value)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the calls to x.value here are safe because Now, Later and Always don't do any recursion on Eval.

}
}

loop(e.asInstanceOf[L], Nil).asInstanceOf[A]
loop(e, Ident(implicitly[A <:< A]))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here we have 1 allocation more than we did before: Ident where as before we did Nil. But since this is one allocation per call to .value, I think this is well worth the cost considering how many allocations we already make (1 per depth at least to build up the user level stack).

}
}

Expand Down