From 658c3d06c4ae50cb18d0cbb26a4c32a858c5a248 Mon Sep 17 00:00:00 2001 From: Mark Harrah Date: Fri, 26 Apr 2013 22:35:27 -0400 Subject: [PATCH] Use public inherited dependencies in incremental compilation invalidation. 1. All parents of public/exported classes/modules/packages are tracked as 'publicInherited' dependencies. These are dealiased and normalized so that the dependency is on the actual underlying template and not the source enclosing the alias. 2. All CompilationUnit.depends dependencies are direct dependencies. These include inherited dependencies. 3. When invalidating changed internal sources, a. Invalidate all inherited dependencies, transitively and include the originally modified sources, b. Invalidate all direct dependencies of these sources, c. Exclude any sources that were compiled in the previous step unless they depend on a newly invalidated source. 4. Invalidate changed external sources in the same way as #3 but remove the external sources from the final set. Only public inheritance dependencies need to be considered because a template that is not accessible outside its source file and that inherits from another file can be handled as a normal, direct dependency. Because the template isn't public, changes to its API will not propagate outside of the source file. Several existing tests cover the correctness, especially: 1. transitive-a covers direct, transitive dependencies with inferred return types 2. transitive-b covers inherited, transitive dependencies with inferred return types There are two new tests, one that tests that public inherited dependencies are tracked and one that verifies the basic invalidation progression. More tests are needed to verify the improvements that this algorithm brings: 1. Inheritance-related dependencies are processed in one step to avoid the otherwise unavoidable several steps. 2. Only immediate direct dependencies are ever processed, which should in many typical cases avoid large invalidation sets. --- .../src/main/scala/sbt/inc/Incremental.scala | 106 +++++++++--------- .../scala/sbt/inc/StronglyConnected.scala | 51 --------- .../inherited-dependencies/A.scala | 4 + .../inherited-dependencies/B.scala | 19 ++++ .../inherited-dependencies/C.scala | 1 + .../inherited-dependencies/D.scala | 1 + .../inherited-dependencies/E.scala | 1 + .../inherited-dependencies/F.scala | 3 + .../inherited-dependencies/G.scala | 1 + .../inherited-dependencies/J.scala | 1 + .../inherited-dependencies/build.sbt | 27 +++++ .../inherited-dependencies/test | 1 + .../less-inter-inv/A.scala | 3 + .../less-inter-inv/B.scala | 1 + .../less-inter-inv/C.scala | 1 + .../less-inter-inv/D.scala | 4 + .../less-inter-inv/E.scala | 3 + .../less-inter-inv/build.sbt | 10 ++ .../less-inter-inv/changes/A2.scala | 3 + .../source-dependencies/less-inter-inv/test | 9 ++ 20 files changed, 149 insertions(+), 101 deletions(-) delete mode 100644 compile/inc/src/main/scala/sbt/inc/StronglyConnected.scala create mode 100644 sbt/src/sbt-test/source-dependencies/inherited-dependencies/A.scala create mode 100644 sbt/src/sbt-test/source-dependencies/inherited-dependencies/B.scala create mode 100644 sbt/src/sbt-test/source-dependencies/inherited-dependencies/C.scala create mode 100644 sbt/src/sbt-test/source-dependencies/inherited-dependencies/D.scala create mode 100644 sbt/src/sbt-test/source-dependencies/inherited-dependencies/E.scala create mode 100644 sbt/src/sbt-test/source-dependencies/inherited-dependencies/F.scala create mode 100644 sbt/src/sbt-test/source-dependencies/inherited-dependencies/G.scala create mode 100644 sbt/src/sbt-test/source-dependencies/inherited-dependencies/J.scala create mode 100644 sbt/src/sbt-test/source-dependencies/inherited-dependencies/build.sbt create mode 100644 sbt/src/sbt-test/source-dependencies/inherited-dependencies/test create mode 100644 sbt/src/sbt-test/source-dependencies/less-inter-inv/A.scala create mode 100644 sbt/src/sbt-test/source-dependencies/less-inter-inv/B.scala create mode 100644 sbt/src/sbt-test/source-dependencies/less-inter-inv/C.scala create mode 100644 sbt/src/sbt-test/source-dependencies/less-inter-inv/D.scala create mode 100644 sbt/src/sbt-test/source-dependencies/less-inter-inv/E.scala create mode 100644 sbt/src/sbt-test/source-dependencies/less-inter-inv/build.sbt create mode 100644 sbt/src/sbt-test/source-dependencies/less-inter-inv/changes/A2.scala create mode 100644 sbt/src/sbt-test/source-dependencies/less-inter-inv/test diff --git a/compile/inc/src/main/scala/sbt/inc/Incremental.scala b/compile/inc/src/main/scala/sbt/inc/Incremental.scala index 561ed15b7..2f7f568ba 100644 --- a/compile/inc/src/main/scala/sbt/inc/Incremental.scala +++ b/compile/inc/src/main/scala/sbt/inc/Incremental.scala @@ -77,7 +77,7 @@ object Incremental debug("********* Merged: \n" + merged.relations + "\n*********") val incChanges = changedIncremental(invalidated, previous.apis.internalAPI _, merged.apis.internalAPI _, options) - debug("Changes:\n" + incChanges) + debug("\nChanges:\n" + incChanges) val transitiveStep = options.transitiveStep val incInv = invalidateIncremental(merged.relations, incChanges, invalidated, cycleNum >= transitiveStep, log) cycle(incInv, allSources, emptyChanges, merged, doCompile, classfileManager, cycleNum+1, log, options) @@ -159,7 +159,7 @@ object Incremental if(transitive) transitiveDependencies(dependsOnSrc, changes.modified, log) else - invalidateStage2(dependsOnSrc, changes.modified, log) + invalidateIntermediate(previous, changes.modified, log) val dups = invalidateDuplicates(previous) if(dups.nonEmpty) @@ -176,53 +176,15 @@ object Incremental if(sources.size > 1) sources else Nil } toSet; - /** Only invalidates direct source dependencies. It excludes any sources that were recompiled during the previous run. - * Callers may want to augment the returned set with 'modified' or all sources recompiled up to this point. */ - def invalidateDirect(dependsOnSrc: File => Set[File], modified: Set[File]): Set[File] = - (modified flatMap dependsOnSrc) -- modified - - /** Invalidates transitive source dependencies including `modified`.*/ - @tailrec def invalidateTransitive(dependsOnSrc: File => Set[File], modified: Set[File], log: Logger): Set[File] = - { - val newInv = invalidateDirect(dependsOnSrc, modified) - log.debug("\tInvalidated direct: " + newInv) - if(newInv.isEmpty) modified else invalidateTransitive(dependsOnSrc, modified ++ newInv, log) - } - - /** Returns the transitive source dependencies of `initial`, excluding the files in `initial` in most cases. - * In three-stage incremental compilation, the `initial` files are the sources from step 2 that had API changes. - * Because strongly connected components (cycles) are included in step 2, the files with API changes shouldn't - * need to be compiled in step 3 if their dependencies haven't changed. If there are new cycles introduced after - * step 2, these can require step 2 sources to be included in step 3 recompilation. - */ + /** Returns the transitive source dependencies of `initial`. + * Because the intermediate steps do not pull in cycles, this result includes the initial files + * if they are part of a cycle containing newly invalidated files . */ def transitiveDependencies(dependsOnSrc: File => Set[File], initial: Set[File], log: Logger): Set[File] = { - // include any file that depends on included files - def recheck(included: Set[File], process: Set[File], excluded: Set[File]): Set[File] = - { - val newIncludes = (process flatMap dependsOnSrc) intersect excluded - if(newIncludes.isEmpty) - included - else - recheck(included ++ newIncludes, newIncludes, excluded -- newIncludes) - } - val transitiveOnly = transitiveDepsOnly(initial)(dependsOnSrc) - log.debug("Step 3 transitive dependencies:\n\t" + transitiveOnly) - val stage3 = recheck(transitiveOnly, transitiveOnly, initial) - log.debug("Step 3 sources from new step 2 source dependencies:\n\t" + (stage3 -- transitiveOnly)) - stage3 - } - - - def invalidateStage2(dependsOnSrc: File => Set[File], initial: Set[File], log: Logger): Set[File] = - { - val initAndImmediate = initial ++ initial.flatMap(dependsOnSrc) - log.debug("Step 2 changed sources and immdediate dependencies:\n\t" + initAndImmediate) - val components = sbt.inc.StronglyConnected(initAndImmediate)(dependsOnSrc) - log.debug("Non-trivial strongly connected components: " + components.filter(_.size > 1).mkString("\n\t", "\n\t", "")) - val inv = components.filter(initAndImmediate.exists).flatten - log.debug("Step 2 invalidated sources:\n\t" + inv) - inv + val transitiveWithInitial = transitiveDeps(initial)(dependsOnSrc) + val transitivePartial = includeInitialCond(initial, transitiveWithInitial, dependsOnSrc, log) + log.debug("Final step, transitive dependencies:\n\t" + transitivePartial) + transitivePartial } /** Invalidates sources based on initially detected 'changes' to the sources, products, and dependencies.*/ @@ -232,7 +194,7 @@ object Incremental val srcDirect = srcChanges.removed ++ srcChanges.removed.flatMap(previous.usesInternalSrc) ++ srcChanges.added ++ srcChanges.changed val byProduct = changes.removedProducts.flatMap(previous.produced) val byBinaryDep = changes.binaryDeps.flatMap(previous.usesBinary) - val byExtSrcDep = changes.external.modified.flatMap(previous.usesExternal) // ++ scopeInvalidations + val byExtSrcDep = invalidateByExternal(previous, changes.external.modified, log) //changes.external.modified.flatMap(previous.usesExternal) // ++ scopeInvalidations log.debug( "\nInitial source changes: \n\tremoved:" + srcChanges.removed + "\n\tadded: " + srcChanges.added + "\n\tmodified: " + srcChanges.changed + "\nRemoved products: " + changes.removedProducts + @@ -248,6 +210,51 @@ object Incremental srcDirect ++ byProduct ++ byBinaryDep ++ byExtSrcDep } + /** Sources invalidated by `external` sources in other projects according to the previous `relations`. */ + def invalidateByExternal(relations: Relations, external: Set[String], log: Logger): Set[File] = + { + // Propagate public inheritance dependencies transitively. + // This differs from normal because we need the initial crossing from externals to sources in this project. + val externalInheritedR = relations.publicInherited.external + val byExternalInherited = external flatMap externalInheritedR.reverse + val internalInheritedR = relations.publicInherited.internal + val transitiveInherited = transitiveDeps(byExternalInherited)(internalInheritedR.reverse _) + + // Get the direct dependencies of all sources transitively invalidated by inheritance + val directA = transitiveInherited flatMap relations.direct.internal.reverse + // Get the sources that directly depend on externals. This includes non-inheritance dependencies and is not transitive. + val directB = external flatMap relations.direct.external.reverse + transitiveInherited ++ directA ++ directB + } + /** Intermediate invalidation step: steps after the initial invalidation, but before the final transitive invalidation. */ + def invalidateIntermediate(relations: Relations, modified: Set[File], log: Logger): Set[File] = + { + def reverse(r: Relations.Source) = r.internal.reverse _ + invalidateSources(reverse(relations.direct), reverse(relations.publicInherited), modified, log) + } + /** Invalidates inheritance dependencies, transitively. Then, invalidates direct dependencies. Finally, excludes initial dependencies not + * included in a cycle with newly invalidated sources. */ + private[this] def invalidateSources(directDeps: File => Set[File], publicInherited: File => Set[File], initial: Set[File], log: Logger): Set[File] = + { + val transitiveInherited = transitiveDeps(initial)(publicInherited) + log.debug("Invalidated by transitive public inheritance: " + transitiveInherited) + val direct = transitiveInherited flatMap directDeps + log.debug("Invalidated by direct dependency: " + direct) + val all = transitiveInherited ++ direct + includeInitialCond(initial, all, f => directDeps(f) ++ publicInherited(f), log) + } + /** Conditionally include initial sources that are dependencies of newly invalidated sources. + ** Initial sources included in this step can be because of a cycle, but not always. */ + private[this] def includeInitialCond(initial: Set[File], currentInvalidations: Set[File], allDeps: File => Set[File], log: Logger): Set[File] = + { + val newInv = currentInvalidations -- initial + log.debug("New invalidations:\n\t" + newInv) + val transitiveOfNew = transitiveDeps(newInv)(allDeps) + val initialDependsOnNew = transitiveOfNew & initial + log.debug("Previously invalidated, but (transitively) depend on new invalidations:\n\t" + initialDependsOnNew) + newInv ++ initialDependsOnNew + } + def prune(invalidatedSrcs: Set[File], previous: Analysis): Analysis = prune(invalidatedSrcs, previous, ClassfileManager.deleteImmediately()) @@ -309,7 +316,7 @@ object Incremental def orEmpty(o: Option[Source]): Source = o getOrElse APIs.emptySource def orTrue(o: Option[Boolean]): Boolean = o getOrElse true - private[this] def transitiveDepsOnly[T](nodes: Iterable[T])(dependencies: T => Iterable[T]): Set[T] = + private[this] def transitiveDeps[T](nodes: Iterable[T])(dependencies: T => Iterable[T]): Set[T] = { val xs = new collection.mutable.HashSet[T] def all(ns: Iterable[T]): Unit = ns.foreach(visit) @@ -319,7 +326,6 @@ object Incremental all(dependencies(n)) } all(nodes) - xs --= nodes xs.toSet } diff --git a/compile/inc/src/main/scala/sbt/inc/StronglyConnected.scala b/compile/inc/src/main/scala/sbt/inc/StronglyConnected.scala deleted file mode 100644 index 89d8ac4a0..000000000 --- a/compile/inc/src/main/scala/sbt/inc/StronglyConnected.scala +++ /dev/null @@ -1,51 +0,0 @@ -package sbt.inc - -// stolen from Josh -object StronglyConnected -{ - def apply[N](nodes: Iterable[N])(dependencies: N => Iterable[N]): Set[Set[N]] = - { - val stack = new collection.mutable.Stack[N] - val onStack = new collection.mutable.HashSet[N] - val scc = new collection.mutable.ArrayBuffer[Set[N]] - val index = new collection.mutable.ArrayBuffer[N] - val lowLink = new collection.mutable.HashMap[N, Int] - - def tarjanImpl(v: N) - { - index += v - lowLink(v) = index.size-1 - stack.push(v) - onStack += v - for(n <- dependencies(v)) - { - if( !index.contains(n) ) - { - tarjanImpl(n) - lowLink(v) = math.min(lowLink(v), lowLink(n)) - } - else if(onStack(n)) - lowLink(v) = math.min(lowLink(v), index.indexOf(n)) - } - - if(lowLink(v) == index.indexOf(v)) - { - val components = new collection.mutable.ArrayBuffer[N] - def popLoop() - { - val popped = stack.pop() - onStack -= popped - components.append(popped) - if(popped != v) popLoop() - } - popLoop() - scc.append(components.toSet) - } - } - - for(node <- nodes) - if( !index.contains(node) ) - tarjanImpl(node) - scc.toSet - } -} diff --git a/sbt/src/sbt-test/source-dependencies/inherited-dependencies/A.scala b/sbt/src/sbt-test/source-dependencies/inherited-dependencies/A.scala new file mode 100644 index 000000000..30853bb78 --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/inherited-dependencies/A.scala @@ -0,0 +1,4 @@ +// T is a type constructor [x]C +// C extends D +// E verifies the core type gets pulled out +trait A extends B.T[Int] with (E[Int] @unchecked) diff --git a/sbt/src/sbt-test/source-dependencies/inherited-dependencies/B.scala b/sbt/src/sbt-test/source-dependencies/inherited-dependencies/B.scala new file mode 100644 index 000000000..9c6fbe046 --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/inherited-dependencies/B.scala @@ -0,0 +1,19 @@ +object B { + type T[x] = C +} + +class B { + // not public, so this shouldn't be tracked as an inherited dependency + private[this] class X extends D with E[Int] + + def x(i: Int) { + // not public, not an inherited dependency + trait Y extends D + } + + def y(j: Int) { + // not public + val w: D { def length: Int } = ??? + () + } +} \ No newline at end of file diff --git a/sbt/src/sbt-test/source-dependencies/inherited-dependencies/C.scala b/sbt/src/sbt-test/source-dependencies/inherited-dependencies/C.scala new file mode 100644 index 000000000..360899d9f --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/inherited-dependencies/C.scala @@ -0,0 +1 @@ +trait C extends D diff --git a/sbt/src/sbt-test/source-dependencies/inherited-dependencies/D.scala b/sbt/src/sbt-test/source-dependencies/inherited-dependencies/D.scala new file mode 100644 index 000000000..804e77004 --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/inherited-dependencies/D.scala @@ -0,0 +1 @@ +trait D extends G.P diff --git a/sbt/src/sbt-test/source-dependencies/inherited-dependencies/E.scala b/sbt/src/sbt-test/source-dependencies/inherited-dependencies/E.scala new file mode 100644 index 000000000..fa7c94867 --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/inherited-dependencies/E.scala @@ -0,0 +1 @@ +trait E[T] diff --git a/sbt/src/sbt-test/source-dependencies/inherited-dependencies/F.scala b/sbt/src/sbt-test/source-dependencies/inherited-dependencies/F.scala new file mode 100644 index 000000000..8c26474b5 --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/inherited-dependencies/F.scala @@ -0,0 +1,3 @@ +class F { + def q: C { def length: Int } = ??? +} \ No newline at end of file diff --git a/sbt/src/sbt-test/source-dependencies/inherited-dependencies/G.scala b/sbt/src/sbt-test/source-dependencies/inherited-dependencies/G.scala new file mode 100644 index 000000000..1fd92c068 --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/inherited-dependencies/G.scala @@ -0,0 +1 @@ +object G { trait P extends J } \ No newline at end of file diff --git a/sbt/src/sbt-test/source-dependencies/inherited-dependencies/J.scala b/sbt/src/sbt-test/source-dependencies/inherited-dependencies/J.scala new file mode 100644 index 000000000..62eeb6c96 --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/inherited-dependencies/J.scala @@ -0,0 +1 @@ +class J \ No newline at end of file diff --git a/sbt/src/sbt-test/source-dependencies/inherited-dependencies/build.sbt b/sbt/src/sbt-test/source-dependencies/inherited-dependencies/build.sbt new file mode 100644 index 000000000..15a6a4dba --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/inherited-dependencies/build.sbt @@ -0,0 +1,27 @@ +lazy val verifyDeps = taskKey[Unit]("verify inherited dependencies are properly extracted") + +verifyDeps := { + val a = compile.in(Compile).value + same(a.relations.publicInherited.internal.forwardMap, expectedDeps.forwardMap) +} + +lazy val expected = Seq( + "A" -> Seq("C", "D", "E", "G", "J"), + "B" -> Seq(), + "C" -> Seq("D", "G", "J"), + "D" -> Seq("G", "J"), + "E" -> Seq(), + "F" -> Seq("C", "D", "G", "J"), + "G" -> Seq("J"), + "J" -> Seq() +) +lazy val pairs = + expected.map { case (from,tos) => + (toFile(from), tos.map(toFile)) + } +lazy val expectedDeps = (Relation.empty[File,File] /: pairs) { case (r, (x,ys)) => r + (x,ys) } +def toFile(s: String) = file(s + ".scala").getAbsoluteFile + +def same[T](x: T, y: T) { + assert(x == y, s"\nActual: $x, \nExpected: $y") +} diff --git a/sbt/src/sbt-test/source-dependencies/inherited-dependencies/test b/sbt/src/sbt-test/source-dependencies/inherited-dependencies/test new file mode 100644 index 000000000..e5d477601 --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/inherited-dependencies/test @@ -0,0 +1 @@ +> verifyDeps diff --git a/sbt/src/sbt-test/source-dependencies/less-inter-inv/A.scala b/sbt/src/sbt-test/source-dependencies/less-inter-inv/A.scala new file mode 100644 index 000000000..a4f92f4fa --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/less-inter-inv/A.scala @@ -0,0 +1,3 @@ +class A { + def x = 3 +} diff --git a/sbt/src/sbt-test/source-dependencies/less-inter-inv/B.scala b/sbt/src/sbt-test/source-dependencies/less-inter-inv/B.scala new file mode 100644 index 000000000..a18aec3db --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/less-inter-inv/B.scala @@ -0,0 +1 @@ +class B extends A diff --git a/sbt/src/sbt-test/source-dependencies/less-inter-inv/C.scala b/sbt/src/sbt-test/source-dependencies/less-inter-inv/C.scala new file mode 100644 index 000000000..f6f5bb28f --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/less-inter-inv/C.scala @@ -0,0 +1 @@ +class C extends B diff --git a/sbt/src/sbt-test/source-dependencies/less-inter-inv/D.scala b/sbt/src/sbt-test/source-dependencies/less-inter-inv/D.scala new file mode 100644 index 000000000..55959c2a9 --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/less-inter-inv/D.scala @@ -0,0 +1,4 @@ +object D { + val c = new C + def x: String = c.x.toString +} diff --git a/sbt/src/sbt-test/source-dependencies/less-inter-inv/E.scala b/sbt/src/sbt-test/source-dependencies/less-inter-inv/E.scala new file mode 100644 index 000000000..f393ca20d --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/less-inter-inv/E.scala @@ -0,0 +1,3 @@ +object E extends App { + assert(D.x == "3") +} diff --git a/sbt/src/sbt-test/source-dependencies/less-inter-inv/build.sbt b/sbt/src/sbt-test/source-dependencies/less-inter-inv/build.sbt new file mode 100644 index 000000000..d23dff705 --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/less-inter-inv/build.sbt @@ -0,0 +1,10 @@ +import complete.DefaultParsers._ + +val checkIterations = inputKey[Unit]("Verifies the accumlated number of iterations of incremental compilation.") + +checkIterations := { + val expected: Int = (Space ~> NatBasic).parsed + val actual: Int = (compile in Compile).value.compilations.allCompilations.size + assert(expected == actual, s"Expected $expected compilations, got $actual") +} + diff --git a/sbt/src/sbt-test/source-dependencies/less-inter-inv/changes/A2.scala b/sbt/src/sbt-test/source-dependencies/less-inter-inv/changes/A2.scala new file mode 100644 index 000000000..acab4a1ae --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/less-inter-inv/changes/A2.scala @@ -0,0 +1,3 @@ +class A { + def x = "3" +} diff --git a/sbt/src/sbt-test/source-dependencies/less-inter-inv/test b/sbt/src/sbt-test/source-dependencies/less-inter-inv/test new file mode 100644 index 000000000..c6df5698e --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/less-inter-inv/test @@ -0,0 +1,9 @@ +# 1 iteration from initial full compile +> run +$ copy-file changes/A2.scala A.scala + +# 1 iteration for the initial changes +# 1 iteration to recompile all descendents and direct dependencies +# no further iteration, because APIs of directs don't change +> run +> checkIterations 3