Enhances stack safety for Eval. #1888

non · 2017-09-02T15:22:01Z

The basic idea is to create very deep chains of operations to try to expose any
stack problems. We know that for things like map/flatMap Eval seems to work, so
let's just randomly construct deeply-nested Eval expressions to see what
happens.

This test exposed a weakness with .memoize which is fixed.

As part of this commit, I noticed that our Arbitrary[Eval[A]] instances were
somewhat weak, so I upgraded them.

I was hoping to find something involving Eval.defer to fix #1703 which @mpilquist raised, but I couldn't find any weakness besides the things here involving .memoize.

Update by Kai: It seems fixed #1703

The basic idea is to create very deep chains of operations to try to expose any stack problems. We know that for things like map/flatMap Eval seems to work, so let's just randomly construct deeply-nested Eval expressions to see what happens. This test exposed a weakness with .memoize which is fixed. As part of this commit, I noticed that our Arbitrary[Eval[A]] instances were somewhat weak, so I upgraded them.

…tests

non · 2017-09-02T17:51:43Z

Maybe my change to use "real" Arbitrary[Eval[A]] instances is killing Travis? Not sure what's going on.

johnynek · 2017-09-02T17:50:43Z

core/src/main/scala/cats/Eval.scala

+    def memoize: Eval[A] =
+      new Call[A](thunk) {
+        override def memoize: Eval[A] = this
+        override lazy val value: A = Call.loop(this).value


Won't this be lost if you flatMap? Isn't it ignore in that case. Is that okay?

johnynek · 2017-09-02T17:52:54Z

core/src/main/scala/cats/Eval.scala

+        val start: () => Eval[Start] = self.start
+        val run: Start => Eval[A] = self.run
+        override def memoize: Eval[A] = this
+        override lazy val value: A = self.value


Same, this lazy val is lost on flatMap no?
Maybe memoise only applies to an outer flatMap?

I have used it when forking into two paths so I don't evaluate something twice, but this wouldn't actually work with this change I think.

johnynek · 2017-09-02T18:43:28Z

Can we have a test something like this:

forAll { (e: Eval[Int], fn: Int => Eval[Int]) =>
  var cnt = 0

  val action = e.flatMap { i = >
    cnt += 1
    fn(i)
  }.memoize

  val res = for {
    i1 <- action
    i2 <- action
  } yield i1 == i2

  assert(res.value == true)
  assert(cnt == 1) // memoize means we don't build what is up that tree more than once.
}

non · 2017-09-02T21:04:12Z

@johnynek This is a great observation.

Much like the (now-removed) exception-handling, I think if we want to support this kind of intermediate memoization in the current approach, we have to sacrifice constant stack per intermediate memoization.

The alternative would be to use a more complicated structure instead of a queue of functions, and then to "update" the various nodes as their results are complete.

I'm happy to revert to the previous memoization implementation, especially since I don't think the code @mpilquist was worried about was using memoize. In that case I should just note that calling .memoize on internal nodes should be limited since it does consume stack in some cases.

johnynek · 2017-09-02T23:04:04Z

So, the contract on memoize then is that it only applies to a final value? If that's true, why have it at all?

you can just do lazy val myValue = eval.value and get the result right? Or Eval.lazy(eval.value) if you want to return an Eval still. Seems like it is a bit dishonest if it can be discarded.

That said, I don't quite see why it is impossible, just that this implementation does not have it. For instance, what if we add a case class Memoized[A](of: Eval[A]) extends Eval[A] node and change the evaluation loop. Maybe that hits similar issues as raise did, but I can imagine if we had a side mutable.Map[Eval[_], _] in the evaluation loop where we cached the value, it might be okay, but making sure we update the map in a safe way might be a challenge

non · 2017-09-02T23:19:38Z

@johnynek Your Memoized suggestion is what I mean -- we can totally do it but we'll need to complicate the compute logic a fair bit.

I wasn't disagreeing with you -- I actually think that we should not take the change I made, and should either stick with what we have for memoize or pursue a more radical alternative.

johnynek · 2017-09-02T23:58:55Z

Here's what I mean @non

bce5de8

non · 2017-09-03T00:16:55Z

@johnynek Nice!

I don't think you even need the mutable.Map right? Once you update var result in the continuation, that should be good enough, no? (Since unwinding these things is synchronous, there's no real opportunity for a race.)

johnynek · 2017-09-03T00:35:22Z

indeed, that's true, but using the Map means if you do:

val foo: Eval[A] = getFoo
val foo1 = foo.memoize
val foo2 = foo.memoize

and they wind up in the same Eval DAG, then you can can still only evaluate once.

Maybe that is such a corner case it isn't worth it, but it is pretty cheap to have the map (although maybe not worth it since you have to do the map updates for everything).

Would you be open to adding something like this to your change?

non · 2017-09-03T00:44:40Z

Yes! In fact, I"m already integrating it (without the Map for now). There was on bug with your thing that I think I've fixed. I'm cleaning it up now.

@johnynek

This fix was proposed by @johnynek and fixes a bug I introduced with how memoization was handled during flatMap evaluation. Our old implementation did memoize intermediate values correctly (which meant that three different evals mapped from the same source shared a memoized value). However, it was not stack-safe. My fix introduced stack-safety but broke this intermediate memoization. The new approach uses a new node (Eval.Memoize) which can be mutably-updated (much like Later). It's confirmed to be stack-safe and to handle intermediate values correctly. While it was unlikely that anyone was doing enough intermediate memoization to cause actual stack overflows, it's nice to know that this is now impossible.

codecov-io · 2017-09-03T01:19:39Z

Codecov Report

Merging #1888 into master will decrease coverage by <.01%.
The diff coverage is 97.29%.

@@            Coverage Diff             @@
##           master    #1888      +/-   ##
==========================================
- Coverage   95.17%   95.16%   -0.01%     
==========================================
  Files         248      248              
  Lines        4352     4366      +14     
  Branches      126      119       -7     
==========================================
+ Hits         4142     4155      +13     
- Misses        210      211       +1

Impacted Files	Coverage Δ
...rc/main/scala/cats/laws/discipline/Arbitrary.scala	`92.06% <100%> (ø)`	⬆️
core/src/main/scala/cats/Eval.scala	`98.75% <97.05%> (-1.25%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7e9a183...c9572b2. Read the comment docs.

johnynek · 2017-09-03T01:31:42Z

this looks great to me! Thanks for tackling this. Seems like a strictly better situation now (assuming perf didn't tank).

johnynek · 2017-09-03T01:36:22Z

minor suggestion (also made offline): what about renaming Call => Defer and Compute => FlatMap which will make it easier to read this code and recall what each are for.

johnynek · 2017-09-03T02:31:42Z

core/src/main/scala/cats/Eval.scala

          type Start = compute.Start
          val start: () => Eval[Start] = () => compute.start()
-          val run: Start => Eval[A] = s => doCall1(compute.run(s))
+          val run: Start => Eval[A] = s => advance1(compute.run(s))


Why do we need this here? Seems like it will be run later without beating the function call here.

johnynek · 2017-09-19T16:46:12Z

👍

@kailuowang can you also review?

kailuowang · 2017-09-19T21:18:27Z

core/src/main/scala/cats/Eval.scala

+   */
+  @tailrec private def advance[A](fa: Eval[A]): Eval[A] =
+    fa match {
+      case call: Eval.Defer[A] =>


totally nitpick: call can be renamed to defer and the compute below. No need to address in this PR, I can do in a separate one.

kailuowang · 2017-09-19T21:26:42Z

core/src/main/scala/cats/Eval.scala

+          m.result match {
+            case Some(a) =>
+              fs match {
+                case f :: fs => loop(f(a), fs)


This line isn't tested. I am curious how come the random stack safety stress test didn't hit it, is it expected?

kailuowang · 2017-09-25T18:00:38Z

not sure if @non has time to work on this PR. I am okay with merging it now and if necessary address the two minor issues later. WDYT @johnynek

johnynek · 2017-09-28T00:39:00Z

sounds good.

I'll merge. I think we are in a good shape.

non and others added 2 commits September 2, 2017 03:00

Merge remote-tracking branch 'upstream/master' into topic/eval-stack-…

cebeb4e

…tests

non added the in progress label Sep 2, 2017

Reduce depth to get Travis passing.

52ca2ca

johnynek reviewed Sep 2, 2017

View reviewed changes

Rename Compute to FlatMap and Call to Defer.

c9572b2

johnynek reviewed Sep 3, 2017

View reviewed changes

kailuowang self-requested a review September 19, 2017 16:50

kailuowang reviewed Sep 19, 2017

View reviewed changes

johnynek merged commit dab28d7 into typelevel:master Sep 28, 2017

stew removed the in progress label Sep 28, 2017

mpilquist mentioned this pull request Sep 28, 2017

SOE in Eval.defer #1703

Closed

kailuowang added this to the 1.0.0-RC1 milestone Oct 13, 2017

kailuowang added the enhancement label Oct 15, 2017

kailuowang changed the title ~~Add a stack safety stress test for Eval.~~ Enhances stack safety for Eval. Oct 15, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhances stack safety for Eval. #1888

Enhances stack safety for Eval. #1888

non commented Sep 2, 2017 •

edited by kailuowang

Loading

non commented Sep 2, 2017

johnynek Sep 2, 2017

johnynek Sep 2, 2017

johnynek commented Sep 2, 2017

non commented Sep 2, 2017 •

edited

Loading

johnynek commented Sep 2, 2017 •

edited

Loading

non commented Sep 2, 2017

johnynek commented Sep 2, 2017

non commented Sep 3, 2017

johnynek commented Sep 3, 2017

non commented Sep 3, 2017

codecov-io commented Sep 3, 2017 •

edited

Loading

johnynek commented Sep 3, 2017

johnynek commented Sep 3, 2017

johnynek Sep 3, 2017

johnynek commented Sep 19, 2017

kailuowang Sep 19, 2017

kailuowang Sep 19, 2017

kailuowang commented Sep 25, 2017

johnynek commented Sep 28, 2017

Enhances stack safety for Eval. #1888

Enhances stack safety for Eval. #1888

Conversation

non commented Sep 2, 2017 • edited by kailuowang Loading

non commented Sep 2, 2017

johnynek Sep 2, 2017

Choose a reason for hiding this comment

johnynek Sep 2, 2017

Choose a reason for hiding this comment

johnynek commented Sep 2, 2017

non commented Sep 2, 2017 • edited Loading

johnynek commented Sep 2, 2017 • edited Loading

non commented Sep 2, 2017

johnynek commented Sep 2, 2017

non commented Sep 3, 2017

johnynek commented Sep 3, 2017

non commented Sep 3, 2017

codecov-io commented Sep 3, 2017 • edited Loading

Codecov Report

johnynek commented Sep 3, 2017

johnynek commented Sep 3, 2017

johnynek Sep 3, 2017

Choose a reason for hiding this comment

johnynek commented Sep 19, 2017

kailuowang Sep 19, 2017

Choose a reason for hiding this comment

kailuowang Sep 19, 2017

Choose a reason for hiding this comment

kailuowang commented Sep 25, 2017

johnynek commented Sep 28, 2017

non commented Sep 2, 2017 •

edited by kailuowang

Loading

non commented Sep 2, 2017 •

edited

Loading

johnynek commented Sep 2, 2017 •

edited

Loading

codecov-io commented Sep 3, 2017 •

edited

Loading