Core compiler optimization: fix MASSIVE performance bugs (#404)

... SMU example compile time (core compile time only, with hot cache
including generator results) goes from ~minute to a bit under a second.

Fixes two major performance bugs:
- ConstProp used prevent parameters without types declared from
propagating by rejecting it from the ready queue, repeatedly. This now
adds the parameter declaration as a dependency.
- (probably) DependencyGraph used to subtract the entire values keySet,
which would get massive, this now does a filterInPlace on incoming
dependencies which should be much smaller

Also changes a few things from Set to Iterable, which may marginally
improve performance

Author: ducky64

compiler/src/main/scala/edg/compiler/Compiler.scala

@@ -258,7 +258,7 @@ class Compiler private (
   // Returns all errors, by scanning the design tree for errors and adding errors accumulated through the compile
   // process
   def getErrors(): Seq[CompilerError] = {
-    val pendingErrors = elaboratePending.getMissingValue.map { missingNode =>
+    val pendingErrors = elaboratePending.getMissingValues.map { missingNode =>
       CompilerError.Unelaborated(missingNode, elaboratePending.nodeMissing(missingNode))
     }.toSeq
 
@@ -1628,15 +1628,18 @@ class Compiler private (
     val partialCompileIgnoredRecords = mutable.Set[ElaborateRecord]()
 
     // repeat as long as there is work ready, and all the ready work isn't marked to be ignored
-    var readyList = Set[ElaborateRecord]()
+    var readyList = Iterable[ElaborateRecord]()
     do {
-      readyList = elaboratePending.getReady -- partialCompileIgnoredRecords
+      // TODO this is kind of ugly and expensive in that it keeps filtering the ignored records
+      // ideally this should be done at the getReady side, but these also need to be restored
+      // when the compiler forks
+      readyList = elaboratePending.getReady.filter(!partialCompileIgnoredRecords.contains(_))
       readyList.foreach { elaborateRecord =>
         try {
           elaborateRecord match {
             case elaborateRecord @ ElaborateRecord.ExpandBlock(blockPath, blockClass, blockProgress) =>
               if (partial.blocks.contains(blockPath) || partial.classes.contains(blockClass)) {
-                partialCompileIgnoredRecords.add(elaborateRecord)
+                partialCompileIgnoredRecords += elaborateRecord
               } else {
                 expandBlock(blockPath, blockProgress)
                 elaboratePending.setValue(elaborateRecord, None)
@@ -1653,7 +1656,7 @@ class Compiler private (
             case elaborateRecord @ ElaborateRecord.Parameter(root, rootClasses, postfix, param) =>
               val container = resolveBlock(root).asInstanceOf[wir.HasParams]
               if (paramMatchesPartial(root, rootClasses, postfix)) {
-                partialCompileIgnoredRecords.add(elaborateRecord)
+                partialCompileIgnoredRecords += elaborateRecord
               } else {
                 constProp.addDeclaration(root ++ postfix, param)
                 elaboratePending.setValue(elaborateRecord, None)

compiler/src/main/scala/edg/compiler/CompilerError.scala

@@ -13,7 +13,7 @@ sealed trait CompilerError {
 }
 
 object CompilerError {
-  case class Unelaborated(record: ElaborateRecord, missing: Set[ElaborateRecord]) extends CompilerError {
+  case class Unelaborated(record: ElaborateRecord, missing: Iterable[ElaborateRecord]) extends CompilerError {
     // These errors may be redundant with below, but provides dependency data
     override def toString: String = s"Unelaborated missing dependencies $record:\n" +
       s"${missing.map(x => s"- $x").mkString("\n")}"
@@ -165,7 +165,7 @@ object CompilerError {
       root: DesignPath,
       constrName: String,
       value: expr.ValueExpr,
-      missing: Set[IndirectDesignPath]
+      missing: Iterable[IndirectDesignPath]
   ) extends AssertionError {
     override def toString: String =
       s"Unevaluated assertion: $root.$constrName: missing ${missing.mkString(", ")} in ${ExprToString.apply(value)}"

compiler/src/main/scala/edg/compiler/ConstProp.scala

@@ -42,6 +42,7 @@ class ConstProp() {
   // Assign statements are added to the dependency graph only when arrays are ready
   // This is the authoritative source for the state of any param - in the graph (and its dependencies), or value solved
   // CONNECTED_LINK has an empty value but indicates that the path was resolved in that data structure
+  // NAME has an empty value but indicates declaration (existence in paramTypes)
   private val params = DependencyGraph[IndirectDesignPath, ExprValue]()
   // Parameter types are used to track declared parameters
   // Undeclared parameters cannot have values set, but can be forced (though the value is not effective until declared)
@@ -106,16 +107,9 @@ class ConstProp() {
       connectedLink.setValue(ready, DesignPath())
     }
 
-    var readyList = Set[IndirectDesignPath]()
+    var readyList = Iterable[IndirectDesignPath]()
     do {
-      // ignore params where we haven't seen the decl yet, to allow forced-assign when the block is expanded
-      // TODO support this for all params, including indirect ones (eg, name)
-      readyList = params.getReady.filter { elt =>
-        DesignPath.fromIndirectOption(elt) match {
-          case Some(elt) => paramTypes.keySet.contains(elt.asIndirect)
-          case None => true
-        }
-      }
+      readyList = params.getReady
       readyList.foreach { constrTarget =>
         val assign = paramAssign(constrTarget)
         new ExprEvaluatePartial(getValue, assign.root).map(assign.value) match {
@@ -165,6 +159,7 @@ class ConstProp() {
       case _ => throw new NotImplementedError(s"Unknown param declaration / init $decl")
     }
     paramTypes.put(target.asIndirect, paramType)
+    params.setValue(target.asIndirect + IndirectStep.Name, BooleanValue(false)) // dummy value
     update()
   }
 
@@ -196,14 +191,19 @@ class ConstProp() {
     require(target.splitConnectedLink.isEmpty, "cannot set CONNECTED_LINK")
     val paramSourceRecord = (root, constrName, targetExpr)
 
+    // ignore params where we haven't seen the decl yet, to allow forced-assign when the block is expanded
+    val paramTypesDep = DesignPath.fromIndirectOption(target) match {
+      case Some(path) => Seq(path.asIndirect + IndirectStep.Name)
+      case None => Seq() // has indirect step, no direct decl
+    }
     if (forced) {
       require(!forcedParams.contains(target), s"attempt to re-force $target")
       forcedParams.add(target)
       require(
         !params.valueDefinedAt(target),
         s"forced value must be set before value is resolved, prior ${paramSource(target)}"
       )
-      params.addNode(target, Seq(), overwrite = true) // forced can overwrite other records
+      params.addNode(target, paramTypesDep, overwrite = true) // forced can overwrite other records
     } else {
       if (!forcedParams.contains(target)) {
         if (params.nodeDefinedAt(target)) { // TODO add propagated assign
@@ -213,7 +213,7 @@ class ConstProp() {
           )
           return // first set "wins"
         }
-        params.addNode(target, Seq())
+        params.addNode(target, paramTypesDep)
       } else {
         return // ignored - param was forced, discard the new assign
       }

compiler/src/main/scala/edg/util/DependencyGraph.scala

@@ -8,7 +8,7 @@ import scala.collection.mutable
   */
 class DependencyGraph[KeyType, ValueType] {
   private val values = mutable.HashMap[KeyType, ValueType]()
-  private val inverseDeps = mutable.HashMap[KeyType, mutable.Set[KeyType]]()
+  private val inverseDeps = mutable.HashMap[KeyType, mutable.ArrayBuffer[KeyType]]()
   private val deps = mutable.HashMap[KeyType, mutable.Set[KeyType]]() // cache structure tracking undefined deps
   private val ready = mutable.Set[KeyType]()
 
@@ -27,22 +27,23 @@ class DependencyGraph[KeyType, ValueType] {
 
   // Adds a node in the graph. May only be called once per node.
   def addNode(node: KeyType, dependencies: Seq[KeyType], overwrite: Boolean = false): Unit = {
+    val dependenciesSet = dependencies.to(mutable.Set)
     deps.get(node) match {
       case Some(prevDeps) =>
         require(overwrite, s"reinsertion of dependency for node $node <- $dependencies without overwrite=true")
         // TODO can this requirement be eliminated?
-        require(prevDeps.subsetOf(dependencies.toSet), "update of dependencies without being a superset of prior")
+        require(prevDeps.forall(dependencies.contains(_)), "update of dependencies without being a superset of prior")
       case None => // nothing if no previous dependencies
     }
     require(
       !values.isDefinedAt(node),
       s"reinsertion of dependency for node with value $node = ${values(node)} <- $dependencies"
     )
-    val remainingDeps = (dependencies.toSet -- values.keySet).to(mutable.Set)
+    val remainingDeps = dependenciesSet.filterInPlace(!values.contains(_))
 
     deps.put(node, remainingDeps)
     for (dependency <- remainingDeps) {
-      inverseDeps.getOrElseUpdate(dependency, mutable.Set()) += node
+      inverseDeps.getOrElseUpdate(dependency, mutable.ArrayBuffer()) += node
     }
 
     if (overwrite && ready.contains(node)) {
@@ -60,7 +61,7 @@ class DependencyGraph[KeyType, ValueType] {
 
   // Returns missing dependencies for a node, or empty if the node is ready or has a value assigned
   // Node must exist, or this will exception out
-  def nodeMissing(node: KeyType): Set[KeyType] = deps(node).toSet
+  def nodeMissing(node: KeyType): Iterable[KeyType] = deps(node)
 
   // Clears a node from ready without setting a value in the graph.
   // Useful to stop propagation at some point, but without crashing.
@@ -77,13 +78,12 @@ class DependencyGraph[KeyType, ValueType] {
     require(!values.isDefinedAt(node), s"redefinition of $node (prior value ${values(node)}, new value $value)")
     deps.put(node, mutable.Set())
     values.put(node, value)
-    if (ready.contains(node)) {
-      ready -= node
-    }
+    ready -= node
 
     // See if the update caused anything else to be ready
-    for (inverseDep <- inverseDeps.getOrElse(node, mutable.Set())) {
-      val remainingDeps = deps(inverseDep) -= node
+    for (inverseDep <- inverseDeps.getOrElse(node, mutable.ArrayBuffer())) {
+      val remainingDeps = deps(inverseDep)
+      remainingDeps -= node
       if (remainingDeps.isEmpty && !values.isDefinedAt(inverseDep)) {
         ready += inverseDep
       }
@@ -95,13 +95,13 @@ class DependencyGraph[KeyType, ValueType] {
   }
 
   // Returns all the KeyTypes that don't have values and have satisfied dependencies.
-  def getReady: Set[KeyType] = {
-    ready.toSet
+  def getReady: Iterable[KeyType] = {
+    ready
   }
 
   // Returns all the KeyTypes that have no values. NOT a fast operation. Includes items in the ready list.
-  def getMissingValue: Set[KeyType] = {
-    deps.keySet.toSet -- values.keySet
+  def getMissingValues: Iterable[KeyType] = {
+    deps.keys.toSet -- values.keys
   }
 
   def knownValueKeys: Iterable[KeyType] = {

compiler/src/test/scala/edg/util/DependencyGraphTest.scala

@@ -41,7 +41,7 @@ class DependencyGraphTest extends AnyFlatSpec {
     dep.nodeMissing(1) should equal(Set(0))
     dep.setValue(0, 0)
     dep.getReady should equal(Set(1))
-    dep.nodeMissing(1) should equal(Set())
+    dep.nodeMissing(1) shouldBe empty
   }
 
   it should "track multiple dependencies, and add to ready when all set" in {
@@ -57,7 +57,7 @@ class DependencyGraphTest extends AnyFlatSpec {
     dep.nodeMissing(3) should equal(Set(2))
     dep.setValue(2, 0)
     dep.getReady should equal(Set(3))
-    dep.nodeMissing(3) should equal(Set())
+    dep.nodeMissing(3) shouldBe empty
   }
 
   it should "track a chain of dependencies" in {
@@ -105,22 +105,22 @@ class DependencyGraphTest extends AnyFlatSpec {
 
   it should "return getMissing" in {
     val dep = DependencyGraph[Int, Int]()
-    dep.getMissingValue shouldBe empty
+    dep.getMissingValues shouldBe empty
     dep.addNode(1, Seq(0))
-    dep.getMissingValue should equal(Set(1))
+    dep.getMissingValues should equal(Set(1))
     dep.setValue(1, 1)
-    dep.getMissingValue shouldBe empty
+    dep.getMissingValues shouldBe empty
   }
 
   it should "return getMissing including ready nodes" in {
     val dep = DependencyGraph[Int, Int]()
-    dep.getMissingValue shouldBe empty
+    dep.getMissingValues shouldBe empty
     dep.addNode(1, Seq(0))
-    dep.getMissingValue should equal(Set(1))
-    dep.getReady should equal(Set())
+    dep.getMissingValues should equal(Set(1))
+    dep.getReady shouldBe empty
     dep.setValue(0, 0)
     dep.getReady should equal(Set(1))
-    dep.getMissingValue should equal(Set(1)) // test ready and missing
+    dep.getMissingValues should equal(Set(1)) // test ready and missing
   }
 
   it should "prevent reinsertion of a node" in {
@@ -154,20 +154,20 @@ class DependencyGraphTest extends AnyFlatSpec {
     dep.addNode(10, Seq(0))
     dep.addNode(10, Seq(0, 1), overwrite = true)
     dep.setValue(0, 0)
-    dep.getReady should equal(Set()) // should still be blocked on 1
+    dep.getReady shouldBe empty // should still be blocked on 1
 
     dep.addNode(10, Seq(1, 2), overwrite = true) // 0 should no longer be required
 
     dep.setValue(1, 1)
-    dep.getReady should equal(Set())
+    dep.getReady shouldBe empty
     dep.setValue(2, 2)
     dep.getReady should equal(Set(10))
 
     dep.addNode(10, Seq(1, 2), overwrite = true) // should be a nop
     dep.getReady should equal(Set(10))
 
     dep.addNode(10, Seq(3), overwrite = true)
-    dep.getReady should equal(Set()) // should no longer be ready
+    dep.getReady shouldBe empty // should no longer be ready
   }
 
   it should "return nodeDefinedAt and valueDefinedAt for dependencies" in {
@@ -197,12 +197,12 @@ class DependencyGraphTest extends AnyFlatSpec {
 
     dep1.setValue(0, 0)
     dep1.getReady should equal(Set(1))
-    dep1.nodeMissing(1) should equal(Set())
+    dep1.nodeMissing(1) shouldBe empty
     dep2.getReady shouldBe empty
     dep2.nodeMissing(1) should equal(Set(0))
 
     dep2.setValue(0, 0)
     dep2.getReady should equal(Set(1))
-    dep2.nodeMissing(1) should equal(Set())
+    dep2.nodeMissing(1) shouldBe empty
   }
 }