Add a test with a large DAG #27

johnynek · 2018-11-17T17:27:54Z

@dieu take a look...

This reproduces the stack overflow we have seen in scalding.

dieu · 2018-11-17T21:44:24Z

@johnynek nice, I have something like that, but separate for toLiteral and for Dag.

like:

def genHugeLiteral: Gen[(Literal[Box, Int], Box[Int])] =
    Gen.listOfN(100000, genConst).flatMap { literals =>
      val zeroLit = literals.head
      val zeroVal = zeroLit.evaluate

      literals.tail.foldLeft((zeroLit, zeroVal)) {
        case ((left, Box(value)), right) =>
          val rightVal = right.evaluate.get
          val fn = mk(_ + _)
          val bfn = { case (Box(l), Box(r)) => Box(fn(l, r)) }: (Box[Int], Box[Int]) => Box[Int]
          (Binary(left, right, bfn), Box(fn(value, rightVal)))
      }
    }

property("stack over flow regression check") =
    forAll(genHugeLiteral) { case (l, v) =>
      l.evaluate == v
    }

and for Dag

  test("stack over flow regression check") {
    def it(i: Int): Iterator[Int] = Iterator.single(i)

    val iters = (0 to 10000).map(it)
    val expected = iters.reduce(_ ++ _)

    val f = iters.map(Flow.apply).reduce(_ ++ _)

    val opt =
      try {
        Dag.applyRule(f, Flow.toLiteral, Flow.allRules)
      } catch {
        case e: StackOverflowError => e.printStackTrace()
      }

     opt match {
      case Flow.IteratorSource(it) =>
        assert(it.toList == expected.toList)
      case nonSrc =>
        fail(s"expected total evaluation $nonSrc")
    }
  }

johnynek · 2018-11-18T06:53:49Z

@dieu @non please take a look.

This passes (after a very long time) on my laptop. We should be able to optimize the giant dag case. Right now, we spin a long time constantly recomputing the fanOut function.

This won't throw, but it can get very slow. I think we have some quadratic algoritms in play here currently (or possibly worse). We have a number of places where we do a linear search at each step (this should be N^2 or greater if we are looking at each position on the graph to apply a rule).

We could possibly keep the memoizations across modifications to the DAG, or possibly find a way to avoid checking every node.

johnynek · 2018-11-18T16:50:06Z

three issues with this PR currently:

javascript does not throw StackError, it does another exception we would need to figure out how to catch.
scala 2.10 TailCalls does not have map/flatMap, so we can't use it for the stack safety, but I think it is fine to discard 2.10 support (2.13 is about to be released, after all).
we timeout since it is super expensive to recompute fanOut constantly.

Item 3 seems the most intractable. Knowing the fanout of a node, or at least it is used exactly once, not at all, or more than once, seems needed for many optimizations. However, I have not found a good algorithm yet to incrementally update this property. Currently, we just recompute the entire reverse dependency graph every time the graph changes.

If we could find an incremental algorithm to compute fanout (or this fanout 3 state: 0, 1, >1), I think we will have solved our problems.

johnynek · 2018-11-20T00:45:45Z

I ran this with yourkit... I think the only way we will get a lot better is maintaining data structures and not recomputing them. Right now, basically all the time is taken on getting from HashMaps.

It's pretty hard to maintain many of the views since if we change an Id from one Node to another due to a rule, that can rewrite a lot of the graph.

johnynek · 2018-11-20T01:48:10Z

Okay, this change gets the giant test passing, but still other tests fail which I need to fix.

This graph turns out to be high pathological for the current code. We were going in order from smallest ID to largest, but that tends to work on nodes from source to sink. Working from sinks to source is a better direction since the rules can see bigger subgraphs and potentially work on those.

By leveraging a recursive rule, I took the complexity from O(N^2) to O(N) I think (or maybe even N^3 to N^2). This is because we didn't modify the graph N times, and each of those modifications doing O(N) work. Instead, we modify 1 time, and do O(N) work on that modification, and then follow up with O(N) work.

This hints a better way to write rules: always apply them top down. That is a bigger change.

johnynek · 2018-11-21T20:17:41Z

@non @dieu could you review this?

I think it is mergable after we figure out the CI issues that are currently making the giant test timeout on scalajs.

I think the usual approach here is to make the test smaller on scalajs, but that seems to require some sbt hijinks, which of course will cause me to spin out on how much I hate having to deal with that crap.

actually maybe just adding a test dependency on catalysts works:
https://github.com/typelevel/catalysts/blob/master/platform/js/src/main/scala/catalysts/Platform_js.scala

johnynek · 2018-11-21T20:35:56Z

okay, this passes. @dieu can I get a review?

dieu · 2018-11-21T23:02:35Z

core/src/main/scala/com/stripe/dagon/Dag.scala

-  private val nodeToId: HCache[N, Lambda[t => Option[Id[t]]]] =
-    HCache.empty[N, Lambda[t => Option[Id[t]]]]
+  // Caches polymorphic functions of type Literal[N, T] => Option[Id[T]]
+  private val litToId: HCache[Literal[N, ?], Lambda[t => Option[Id[t]]]] =


this potentially can be a problem because Literal represents the structure of a user graph and can be as deep as a user graph. And if HCache is using hashCode and equals for lookups it can lead StackOverflow.

I don't think so, since we are not triggering it now. The trick is we memoize construction of Literal so we leverage reference equality on them for fast equals.

This works as long as the user uses Memoize as we have done here to construct the Literals.

I don't think we need to go nuts being defensive, we just need to make sure there is a usable path for users that need giant graph support.

Note, hashCode is cached for Literal nodes.

some weird stuff:

if I run the test as test:testOnly *DataFlowTest then tests are passed.

but if I run the test as test:testOnly *DataFlowTest -- -t "test a giant graph" then I got StackOverflow:

[info] at com.stripe.dagon.DataFlowTest$Flow$ComposedOM.hashCode(DataFlowTest.scala:196) [info] at scala.runtime.Statics.anyHash(Statics.java:115) [info] at scala.util.hashing.MurmurHash3.productHash(MurmurHash3.scala:64) [info] at scala.util.hashing.MurmurHash3$.productHash(MurmurHash3.scala:211) [info] at scala.runtime.ScalaRunTime$._hashCode(ScalaRunTime.scala:145) [info] at com.stripe.dagon.DataFlowTest$Flow$ComposedOM.hashCode(DataFlowTest.scala:196) [info] at scala.runtime.Statics.anyHash(Statics.java:115) [info] at scala.util.hashing.MurmurHash3.productHash(MurmurHash3.scala:64) [info] at scala.util.hashing.MurmurHash3$.productHash(MurmurHash3.scala:211) [info] at com.stripe.dagon.DataFlowTest$Flow.<init>(DataFlowTest.scala:33) [info] at com.stripe.dagon.DataFlowTest$Flow$OptionMapped.<init>(DataFlowTest.scala:108) [info] at com.stripe.dagon.DataFlowTest$Flow$composeOptionMapped$.compose(DataFlowTest.scala:279) [info] at com.stripe.dagon.DataFlowTest$Flow$composeOptionMapped$.$anonfun$apply$5(DataFlowTest.scala:285) [info] at com.stripe.dagon.Rule$$anon$1.$anonfun$apply$1(Rule.scala:27) [info] at com.stripe.dagon.Rule$$anon$1.$anonfun$apply$1(Rule.scala:27) [info] at com.stripe.dagon.Rule$$anon$1.$anonfun$apply$1(Rule.scala:27) [info] at com.stripe.dagon.Rule$$anon$1.$anonfun$apply$1(Rule.scala:27) [info] at com.stripe.dagon.Rule$$anon$1.$anonfun$apply$1(Rule.scala:27) [info] at com.stripe.dagon.Dag$$anon$2.$anonfun$toFunction$1(Dag.scala:109) [info] at com.stripe.dagon.Dag.go$1(Dag.scala:155) [info] at com.stripe.dagon.Dag.$anonfun$applyOnce$1(Dag.scala:157) [info] at scala.collection.Iterator$$anon$10.next(Iterator.scala:448) [info] at scala.collection.TraversableOnce.collectFirst(TraversableOnce.scala:145) [info] at scala.collection.TraversableOnce.collectFirst$(TraversableOnce.scala:132) [info] at scala.collection.AbstractIterator.collectFirst(Iterator.scala:1417) [info] at com.stripe.dagon.Dag.applyOnce(Dag.scala:159) [info] at com.stripe.dagon.Dag.loop$1(Dag.scala:79) [info] at com.stripe.dagon.Dag.apply(Dag.scala:84) [info] at com.stripe.dagon.DataFlowTest.$anonfun$new$87(DataFlowTest.scala:863)

That is weird. What do you propose we do about it? The test passes as part of the suite.

By the way, the stack depth could be different depending on how we run it, and it may be that this way of running increases the depth just enough... Note, that stack overflow is in the hashCode method of the test code. We know about this issue already: your AST nodes should have stack safe hashCode and equals. As mentioned before, this can be made somewhat easier in a follow up PR.

dieu · 2018-11-21T23:08:42Z

core/src/main/scala/com/stripe/dagon/Dag.scala

+    val lit = toLiteral(node)
+    try ensureFast(lit)
+    catch {
+      case _: Throwable => //StackOverflowError should work, but not on scala.js


could we find out which error is happening in scala.js?

yes, but it isn't the same error on jvm (it was in the travis CI output). I don't think it is worth trying to some special build that catches one kind of error (which does not exist on jvm) in one, and another for scalajs.

You could do this with a custom build of this method, but honestly, I don't think it is a problem. If there is a user error, they will hit it in both branches and there will be an exception in the second branch.

you right probably is not that important.

dieu · 2018-11-21T23:13:44Z

core/src/main/scala/com/stripe/dagon/Literal.scala

+    /*
+     * We *non-recursively* use either the fast approach or the slow approach
+     */
+    Memoize.functionK[Literal[N, ?], N](new Memoize.RecursiveK[Literal[N, ?], N] {


potentially same problem with caching Literal

dieu · 2018-11-21T23:17:37Z

core/src/test/scala/com/stripe/dagon/DataFlowTest.scala

+    /*
+     * You need a custom equals to avoid stack overflow
+     */
+    override def equals(that: Any) = {


We probably want to give our user some easier way to define this or avoid using equals and hashCode in our code.

I have a follow up PR to make this easier, but honestly, this library is basically for compiler-like things. It is not some general purpose thing that comes up all the time. I don't mind if those users have to think for a few minutes about best practices. Giving up performance across the board (which almost any generic solution will do) to make things easier might not be a good trade.

Note, the current changes are about making it possible to use with arbitrary graphs, i.e. fixing the internal issues IF the user makes sane equals and hashCode methods on their type.

dieu · 2018-11-21T23:20:29Z

@johnynek I need more time to play with it, I will come back to you in the next 2 days. I want to double check that's we don't have a problem with caching Literal.

johnynek · 2018-11-21T23:37:14Z

Thanks. Note we pass the tests which exercise the caching.

codecov-io · 2018-11-22T02:28:15Z

Codecov Report

Merging #27 into master will decrease coverage by 0.79%.
The diff coverage is 77.24%.

@@            Coverage Diff            @@
##           master      #27     +/-   ##
=========================================
- Coverage   85.01%   84.22%   -0.8%     
=========================================
  Files          13       13             
  Lines         267      374    +107     
  Branches       18       21      +3     
=========================================
+ Hits          227      315     +88     
- Misses         40       59     +19

Impacted Files	Coverage Δ
core/src/main/scala/com/stripe/dagon/Id.scala	`100% <ø> (ø)`	⬆️
core/src/main/scala/com/stripe/dagon/Memoize.scala	`100% <100%> (ø)`	⬆️
core/src/main/scala/com/stripe/dagon/HMap.scala	`95.65% <100%> (+0.19%)`	⬆️
core/src/main/scala/com/stripe/dagon/Literal.scala	`58.82% <57.14%> (-3.09%)`	⬇️
core/src/main/scala/com/stripe/dagon/Expr.scala	`65.51% <65%> (-4.49%)`	⬇️
core/src/main/scala/com/stripe/dagon/Dag.scala	`85.64% <79.16%> (-5.69%)`	⬇️
core/src/main/scala/com/stripe/dagon/Graphs.scala	`100% <0%> (+45.45%)`	⬆️
...re/src/main/scala/com/stripe/dagon/FunctionK.scala	`100% <0%> (+66.66%)`	⬆️

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 616eb76...a1ecfd4. Read the comment docs.

dieu · 2018-11-22T23:51:17Z

one side note, in one of my iteration to avoid recursion, I come up with something like:

  class MutableStack[A[_]](private var stack: List[A[_]]) {
    def pop[T](): A[T] = {
      val res = stack.head
      stack = stack.tail
      res.asInstanceOf[A[T]]
    }

    def pop[T](n: Int): List[A[T]] = {
      val (res, tail) = stack.splitAt(n)
      stack = tail
      res.asInstanceOf[List[A[T]]]
    }

    def push(items: List[A[_]]): Unit =
      stack = items ::: stack

    def push(item: A[_]): Unit =
      stack = item :: stack

    def at[T](index: Int): A[T] =
      stack(index).asInstanceOf[A[T]]

    def nonEmpty: Boolean = stack.nonEmpty
  }

  trait Foldable[A[_], B[_]] {
    def unfold[T](a: A[T]): List[A[_]]
    def fold[T](stack: MutableStack[B])(a: A[T]): B[T]
  }

  def functionKF[A[_], B[_]](foldable: Foldable[A, B]): FunctionK[A, B] =
    new FunctionK[A, B] {
      private val cache = HCache.empty[A, B]

      private def widen[S[_], T](s: S[_]): S[T] = s.asInstanceOf[S[T]]

      override def toFunction[U]: A[U] => B[U] = { a =>
        cache.getOrElseUpdate(a, {
          var (tree, index) = (List(a): List[A[_]], 0)

          while (index >= 0) {
            val next = foldable.unfold(widen(tree(index)))

            index -= 1

            if (next.nonEmpty) {
              index += next.length
              tree = next ::: tree
            }
          }

          val stack = new MutableStack[B](List.empty)

          tree.foreach { next =>
            val value = foldable.fold(stack)(widen(next))

            stack.push(value)

            cache.insert(widen(next), value)
          }

          cache(a)
        })
      }
    }

and usage:

def toLiteralFold: FunctionK[Flow, Literal[Flow, ?]] =
      Memoize.functionKF[Flow, Literal[Flow, ?]](new Foldable[Flow, Literal[Flow, ?]] {
        override def unfold[T](a: Flow[T]): List[Flow[_]] = a match {
          case it: IteratorSource[T] => List.empty
          case o: OptionMapped[s, T] => List(o.input)
          case c: ConcatMapped[s, T] => List(c.input)
          case t: Tagged[a, s] => List(t.input)
          case f: Fork[s] => List(f.input)
          case m: Merge[s] => List(m.left, m.right)
          case m: Merged[s] => m.inputs
        }

        override def fold[T](stack: Memoize.MutableStack[Literal[Flow, ?]])(a: Flow[T]): Literal[Flow, T] = a match {
          case it: IteratorSource[T] => Literal.Const(it)
          case o: OptionMapped[s, T] => Literal.Unary(stack.pop[s](), { f: Flow[s] => OptionMapped(f, o.fn) })
          case c: ConcatMapped[s, T] => Literal.Unary(stack.pop[s](), { f: Flow[s] => ConcatMapped(f, c.fn) })
          case t: Tagged[a, s] => Literal.Unary(stack.pop[s](), { f: Flow[s] => Tagged(f, t.tag) })
          case f: Fork[s] => Literal.Unary(stack.pop[s](), { f: Flow[s] => Fork(f) })
          case m: Merge[s] => Literal.Binary(stack.pop[s](), stack.pop[s](), { (l: Flow[s], r: Flow[s]) => Merge(l, r) })
          case m: Merged[s] =>  Literal.Variadic(stack.pop[s](m.inputs.length), { fs: List[Flow[s]] => Merged(fs) })
        }
      })

the general idea is linearizing tree into the list of items, and then process them one by one, on same way we can do equals and hashcode as well.

not sure if you like it, but for user looks like it's easy to write this kind of toLiteral.

PS: instead of stack, we can use cache directly.

dieu · 2018-11-23T00:04:52Z

core/src/main/scala/com/stripe/dagon/Dag.scala

+   * This does recursion on the stack, which is faster, but can overflow
+   */
+  protected def ensureFast[T](lit: Literal[N, T]): (Dag[N], Id[T]) =
+    findLiteral(lit, lit.evaluate) match {


should we use here saved version of Literal.evaluateMemo to maintain the cache of evaluated literals?

This is a great suggestion. Thanks.

dieu · 2018-11-23T00:10:14Z

core/src/main/scala/com/stripe/dagon/Dag.scala

+  /*
+   * This does recursion on the stack, which is faster, but can overflow
+   */
+  protected def ensureFast[T](lit: Literal[N, T]): (Dag[N], Id[T]) =


ensure basically converting Literal to Expr, right? what's do you think to remove the layer of Expr and merge Literal and Expr together?

Expr has Ids which allow us to rewrite nodes. Literal has no Ids. It’s not totally obvious to me how to merge them in a useful way. The point of adding the Ids is so we can rewrite the graph without losing the types. If the users have to construct the Expr directly they have a hard problem on their hands (basically they need to write ensure and the Is tracking in user code).

Do you have a concrete idea of how to merge them?

johnynek · 2018-11-23T17:27:31Z

@dieu I have taken your suggestion about sharing the memoization of the Literal to N mapping across the entire ensure operation.

What do you think about merging this and you can follow up with more improvements along the lines you are suggesting. I think this addresses the core problem: making it possible to use dagon with giant graphs. Note it also does so in a binary compatible way.

dieu · 2018-11-23T20:45:13Z

@dieu I have taken your suggestion about sharing the memoization of the Literal to N mapping across the entire ensure operation.

What do you think about merging this and you can follow up with more improvements along the lines you are suggesting. I think this addresses the core problem: making it possible to use dagon with giant graphs. Note it also does so in a binary compatible way.

sounds good for me.

oscar-stripe added 2 commits November 17, 2018 09:24

Add a test with a large DAG

6a83dfb

fix compilation

c6db7d8

Improve handling of giant graphs

6c95c10

some improvements

7b558dc

get the giant test passing

a6be80c

oscar-stripe added 3 commits November 21, 2018 09:54

revert some needless complexities, add rootsUp

ad2483a

get CI passing and working on scalajs

d8d8d4b

remove some unneeded code, readd a comment

12035f1

use catalysts to detect which platform we are on

7998ba1

johnynek mentioned this pull request Nov 21, 2018

Add Dagon for Real optimization stripe/rainier#269

Closed

dieu reviewed Nov 21, 2018

View reviewed changes

Make sure depth is stack safe

caa3aa5

improve test coverage

359e639

dieu reviewed Nov 23, 2018

View reviewed changes

share the Literal memoization in ensure

22d225f

remove a stray change

a1ecfd4

dieu approved these changes Nov 23, 2018

View reviewed changes

johnynek merged commit a0e248e into master Nov 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a test with a large DAG #27

Add a test with a large DAG #27

johnynek commented Nov 17, 2018

dieu commented Nov 17, 2018 •

edited

Loading

johnynek commented Nov 18, 2018

johnynek commented Nov 18, 2018

johnynek commented Nov 20, 2018

johnynek commented Nov 20, 2018

johnynek commented Nov 21, 2018

johnynek commented Nov 21, 2018

dieu Nov 21, 2018

johnynek Nov 22, 2018

johnynek Nov 22, 2018

dieu Nov 22, 2018

johnynek Nov 23, 2018

johnynek Nov 23, 2018

dieu Nov 21, 2018

johnynek Nov 22, 2018

dieu Nov 22, 2018

dieu Nov 21, 2018

dieu Nov 21, 2018

johnynek Nov 22, 2018

dieu commented Nov 21, 2018

johnynek commented Nov 21, 2018

codecov-io commented Nov 22, 2018 •

edited

Loading

dieu commented Nov 22, 2018

dieu Nov 23, 2018

johnynek Nov 23, 2018

dieu Nov 23, 2018

johnynek Nov 23, 2018

johnynek commented Nov 23, 2018

dieu commented Nov 23, 2018

Add a test with a large DAG #27

Add a test with a large DAG #27

Conversation

johnynek commented Nov 17, 2018

dieu commented Nov 17, 2018 • edited Loading

johnynek commented Nov 18, 2018

johnynek commented Nov 18, 2018

johnynek commented Nov 20, 2018

johnynek commented Nov 20, 2018

johnynek commented Nov 21, 2018

johnynek commented Nov 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dieu commented Nov 21, 2018

johnynek commented Nov 21, 2018

codecov-io commented Nov 22, 2018 • edited Loading

Codecov Report

dieu commented Nov 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnynek commented Nov 23, 2018

dieu commented Nov 23, 2018

dieu commented Nov 17, 2018 •

edited

Loading

codecov-io commented Nov 22, 2018 •

edited

Loading