From 5d2f04f2d067dd557cf91c2681ea0d8faedf2337 Mon Sep 17 00:00:00 2001 From: Jason Zaugg Date: Wed, 4 Sep 2019 15:23:04 +1000 Subject: [PATCH 1/2] [backport] Revert buggy phase assembly optimizations Revert "Fix regression in phase assembly with consecutive runsRightAfter" This reverts commit 4d241c7bd10f767de3948179d3c0d50437b307a4. Revert "Log phase levels under scalac -Xshow-phases -Ydebug -Ylog:all" This reverts commit 759b9795e908a0098b18abc25a88c32d625304bd. Revert "A faster compiler phase assembly algorithm" This reverts commit e83ea4415bc42b02b3f3042563fd970967032458. --- .../scala/tools/nsc/PhaseAssembly.scala | 93 +++++-------------- .../tools/nsc/PhaseAssemblyBenchmark.scala | 5 +- test/files/neg/t7622-cyclic-dependency.check | 2 +- .../scala/tools/nsc/PhaseAssemblyTest.scala | 44 --------- 4 files changed, 28 insertions(+), 116 deletions(-) delete mode 100644 test/junit/scala/tools/nsc/PhaseAssemblyTest.scala diff --git a/src/compiler/scala/tools/nsc/PhaseAssembly.scala b/src/compiler/scala/tools/nsc/PhaseAssembly.scala index 1c4eea2ec9fc..496c96a68409 100644 --- a/src/compiler/scala/tools/nsc/PhaseAssembly.scala +++ b/src/compiler/scala/tools/nsc/PhaseAssembly.scala @@ -40,7 +40,7 @@ trait PhaseAssembly { var phaseobj: Option[List[SubComponent]] = None val after = new mutable.HashSet[Edge]() var before = new mutable.HashSet[Edge]() - var visited: VisitStatus = NotVisited + var visited = false var level = 0 def allPhaseNames(): String = phaseobj match { @@ -48,10 +48,6 @@ trait PhaseAssembly { case Some(lst) => lst.map(_.phaseName).reduceLeft(_+","+_) } } - sealed abstract class VisitStatus - case object NotVisited extends VisitStatus - case object Visiting extends VisitStatus - case object Visited extends VisitStatus val nodes = new mutable.HashMap[String,Node]() val edges = new mutable.HashSet[Edge]() @@ -108,69 +104,35 @@ trait PhaseAssembly { /* Test if there are cycles in the graph, assign levels to the nodes * and collapse hard links into nodes */ - def collapseHardLinks() { - for (node <- nodes.valuesIterator.toList) { - val hardBefores = node.before.iterator.filter(_.hard).toList - for (hl <- hardBefores) { - val effectiveNode: Node = if (nodes.contains(node.name)) node else { - nodes.find(_._2.phaseobj.exists(_.exists(_.phaseName == node.name))).get._2 - } - effectiveNode.phaseobj = Some(effectiveNode.phaseobj.get ++ hl.frm.phaseobj.get) - effectiveNode.before = hl.frm.before - nodes -= hl.frm.phasename - edges -= hl - } + def collapseHardLinksAndLevels(node: Node, lvl: Int) { + if (node.visited) { + dump("phase-cycle") + throw new FatalError(s"Cycle in phase dependencies detected at ${node.phasename}, created phase-cycle.dot") } - } - /* Test if there are cycles in the graph, assign levels to the nodes - * and collapse hard links into nodes - */ - def assignLevelsAndDetectCycles(node: Node) { - val stack = mutable.ArrayStack[Node]() - def visitNode(node: Node): Unit = { - node.visited = Visiting - for (edge <- node.before) { - val from = edge.frm - from.visited match { - case NotVisited => - visitNode(edge.frm) - case Visiting => - dump("phase-cycle") - throw new FatalError(s"Cycle in phase dependencies detected at ${node.phasename}, created phase-cycle.dot") - case Visited => + if (node.level < lvl) node.level = lvl + + var befores = node.before + def hasHardLinks() = befores.exists(_.hard) + while (hasHardLinks()) { + for (hl <- befores) { + if (hl.hard) { + node.phaseobj = Some(node.phaseobj.get ++ hl.frm.phaseobj.get) + node.before = hl.frm.before + nodes -= hl.frm.phasename + edges -= hl + for (edge <- node.before) edge.to = node } } - node.visited = Visited - stack.push(node) - } - try { - visitNode(node) - } finally { - nodes.values.foreach(_.visited = NotVisited) + befores = node.before } + node.visited = true - val topoSort: Map[Node, Int] = stack.zipWithIndex.toMap - val root = node - assert(stack.head == root, stack) - root.level = 1 - - // Nodes that have been collapsed into their hard-linked predecessor - val collapsed: Map[String, Node] = stack.iterator.flatMap(p => p.phaseobj.toList.flatMap(_.map(x => (x.phaseName, p)))).toMap - def followHard(node: Node): Node = collapsed.getOrElse(node.phasename, node) - - // find the longest path to the root node to assign as the level. - stack.iterator.drop(1).foreach { node => - var n = node - var level = 1 - while (n != root && n.after.nonEmpty) { - n = n.after.maxBy(edge => topoSort.get(followHard(edge.to))).to - level += 1 - } - node.level = level + for (edge <- node.before) { + collapseHardLinksAndLevels( edge.frm, lvl + 1) } - def phaseDebug = stack.toSeq.groupBy(_.level).toList.sortBy(_._1).map { case (level, nodes) => (level, nodes.sortBy(_.phasename).map(_.phaseobj.map(_.map(_.phaseName).mkString(":")).mkString(" ")).mkString(" "))}.mkString("\n") - debuglog("Phase assembly levels: " + phaseDebug) + + node.visited = false } /* Find all edges in the given graph that are hard links. For each hard link we @@ -263,16 +225,11 @@ trait PhaseAssembly { dump(3) - // collapse hard links into nodes - graph.collapseHardLinks() + // test for cycles, assign levels and collapse hard links into nodes + graph.collapseHardLinksAndLevels(graph.getNodeByPhase("parser"), 1) dump(4) - // test for cycles, assign levels - graph.assignLevelsAndDetectCycles(graph.getNodeByPhase("parser")) - - dump(5) - // assemble the compiler graph.compilerPhaseList() } diff --git a/test/benchmarks/src/main/scala/scala/tools/nsc/PhaseAssemblyBenchmark.scala b/test/benchmarks/src/main/scala/scala/tools/nsc/PhaseAssemblyBenchmark.scala index dbaeb446c089..b70c495ddc8b 100644 --- a/test/benchmarks/src/main/scala/scala/tools/nsc/PhaseAssemblyBenchmark.scala +++ b/test/benchmarks/src/main/scala/scala/tools/nsc/PhaseAssemblyBenchmark.scala @@ -19,7 +19,7 @@ class PhaseAssemblyBenchmark { var data: Data[_] = _ @Param(Array("1", "4", "8", "16")) - var size: Int = 16 + var size: Int = 0 @Setup def setup(): Unit = { @@ -47,8 +47,7 @@ class PhaseAssemblyBenchmark { val g = s.global val graph = g.phasesSetToDepGraph(s.components.reverse) graph.removeDanglingNodes() - graph.collapseHardLinks() - graph.assignLevelsAndDetectCycles(graph.getNodeByPhase("parser")) + graph.collapseHardLinksAndLevels(graph.getNodeByPhase("parser"), 1) graph } } diff --git a/test/files/neg/t7622-cyclic-dependency.check b/test/files/neg/t7622-cyclic-dependency.check index 81e3ecc6a48c..3546964f5f68 100644 --- a/test/files/neg/t7622-cyclic-dependency.check +++ b/test/files/neg/t7622-cyclic-dependency.check @@ -1 +1 @@ -error: Cycle in phase dependencies detected at cyclicdependency2, created phase-cycle.dot +error: Cycle in phase dependencies detected at cyclicdependency1, created phase-cycle.dot diff --git a/test/junit/scala/tools/nsc/PhaseAssemblyTest.scala b/test/junit/scala/tools/nsc/PhaseAssemblyTest.scala deleted file mode 100644 index a234565e585d..000000000000 --- a/test/junit/scala/tools/nsc/PhaseAssemblyTest.scala +++ /dev/null @@ -1,44 +0,0 @@ -/* - * Scala (https://www.scala-lang.org) - * - * Copyright EPFL and Lightbend, Inc. - * - * Licensed under Apache License 2.0 - * (http://www.apache.org/licenses/LICENSE-2.0). - * - * See the NOTICE file distributed with this work for - * additional information regarding copyright ownership. - */ - -package scala.tools.nsc - -import org.junit.Assert.assertEquals -import org.junit.Test - -class PhaseAssemblyTest { - @Test - def multipleRunsRightAfter(): Unit = { - val g = new Global(new Settings) - case class component(phaseName: String, override val runsRightAfter: Option[String], override val runsAfter: List[String], override val runsBefore: List[String]) extends SubComponent { - val global: g.type = g - override def newPhase(prev: Phase): Phase = ??? - } - - val N = 16 - val random = new scala.util.Random(123502L) - val names = Array.fill(N)("phase_" + random.nextInt(1024)) - val parserAndTerminal = List( - component("parser", None, Nil, Nil), - component("terminal", None, Nil, List(N.toString)) - ) - val components = List.tabulate(N)(i => component(names(i), Some(if (i == 0) "parser" else names(i - 1)), Nil, List("terminal"))) ::: parserAndTerminal - - val graph = g.phasesSetToDepGraph(components.reverse) - graph.removeDanglingNodes() - graph.collapseHardLinks() - graph.assignLevelsAndDetectCycles(graph.getNodeByPhase("parser")) - val result: List[String] = graph.compilerPhaseList().map(_.phaseName).filter(_.startsWith("phase_")) - assertEquals(names.toList, result) - } - -} From f85b3877f8af38868be2ab3b13742991a9d69edf Mon Sep 17 00:00:00 2001 From: Jason Zaugg Date: Tue, 3 Sep 2019 17:04:20 +1000 Subject: [PATCH 2/2] [backport] A more conservative fix for phase assembly performance. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Prune the recursion when we re-visit a node via a shorter path from the initial node. Before: ``` [info] # Run complete. Total time: 00:00:44 [info] Benchmark (size) Mode Cnt Score Error Units [info] PhaseAssemblyBenchmark.assemble 1 avgt 5 1153.587 ± 46.410 ns/op [info] PhaseAssemblyBenchmark.assemble 4 avgt 5 4055.886 ± 891.833 ns/op [info] PhaseAssemblyBenchmark.assemble 8 avgt 5 19886.933 ± 518.378 ns/op [info] PhaseAssemblyBenchmark.assemble 16 avgt 5 768377.075 ± 28072.668 ns/op ``` After: ``` [info] # Run complete. Total time: 00:00:44 [info] Benchmark (size) Mode Cnt Score Error Units [info] PhaseAssemblyBenchmark.assemble 1 avgt 5 1011.742 ± 164.448 ns/op [info] PhaseAssemblyBenchmark.assemble 4 avgt 5 2485.863 ± 482.310 ns/op [info] PhaseAssemblyBenchmark.assemble 8 avgt 5 4333.091 ± 199.798 ns/op [info] PhaseAssemblyBenchmark.assemble 16 avgt 5 9644.064 ± 468.399 ns/op ``` (cherry picked from commit 97177339c2af79228e5f5fb03b5f57fe92e98e42) --- src/compiler/scala/tools/nsc/PhaseAssembly.scala | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/compiler/scala/tools/nsc/PhaseAssembly.scala b/src/compiler/scala/tools/nsc/PhaseAssembly.scala index 496c96a68409..943559b7265a 100644 --- a/src/compiler/scala/tools/nsc/PhaseAssembly.scala +++ b/src/compiler/scala/tools/nsc/PhaseAssembly.scala @@ -110,8 +110,18 @@ trait PhaseAssembly { throw new FatalError(s"Cycle in phase dependencies detected at ${node.phasename}, created phase-cycle.dot") } - if (node.level < lvl) node.level = lvl - + val initLevel = node.level + val levelUp = initLevel < lvl + if (levelUp) { + node.level = lvl + } + if (initLevel != 0) { + if (!levelUp) { + // no need to revisit + node.visited = false + return + } + } var befores = node.before def hasHardLinks() = befores.exists(_.hard) while (hasHardLinks()) {