From 4ed8abd4fb7c010984d234278b6d2143a7579365 Mon Sep 17 00:00:00 2001 From: Grzegorz Kossakowski Date: Tue, 29 Oct 2013 12:00:08 +0100 Subject: [PATCH 1/2] More detailed logging of incremental compiler's invalidation logic. The following events are logged: * invalidation of source file due to macro definition * inclusion of dependency invalidated by inheritance; we log both nodes of dependency edge (dependent and dependency) The second bullet helps to understand what's going on in case of complex inheritance hierarchies like in Scala compiler. --- .../src/main/scala/sbt/inc/Incremental.scala | 40 ++++++++++++------- 1 file changed, 26 insertions(+), 14 deletions(-) diff --git a/compile/inc/src/main/scala/sbt/inc/Incremental.scala b/compile/inc/src/main/scala/sbt/inc/Incremental.scala index 8d6442ab2..1f71baa49 100644 --- a/compile/inc/src/main/scala/sbt/inc/Incremental.scala +++ b/compile/inc/src/main/scala/sbt/inc/Incremental.scala @@ -137,7 +137,7 @@ object Incremental { val oldApis = lastSources.toSeq map oldAPI val newApis = lastSources.toSeq map newAPI - val changes = (lastSources, oldApis, newApis).zipped.filter { (src, oldApi, newApi) => !sameSource(oldApi, newApi) } + val changes = (lastSources, oldApis, newApis).zipped.filter { (src, oldApi, newApi) => !sameSource(src, oldApi, newApi, log) } if (apiDebug(options) && changes.zipped.nonEmpty) { logApiChanges(changes, log, options) @@ -149,10 +149,15 @@ object Incremental new APIChanges(modifiedAPIs, changedNames) } - def sameSource(a: Source, b: Source): Boolean = { + def sameSource[T](src: T, a: Source, b: Source, log: Logger): Boolean = { // Clients of a modified source file (ie, one that doesn't satisfy `shortcutSameSource`) containing macros must be recompiled. val hasMacro = a.hasMacro || b.hasMacro - shortcutSameSource(a, b) || (!hasMacro && SameAPI(a,b)) + shortcutSameSource(a, b) || { + if (hasMacro) { + log.debug("API is considered to be modified because the following source file contains a macro: " + src) + false + } else SameAPI(a,b) + } } def shortcutSameSource(a: Source, b: Source): Boolean = !a.hash.isEmpty && !b.hash.isEmpty && sameCompilation(a.compilation, b.compilation) && (a.hash.deep equals b.hash.deep) @@ -198,6 +203,7 @@ object Incremental val inv = propagated ++ dups // ++ scopeInvalidations(previous.extAPI _, changes.modified, changes.names) val newlyInvalidated = inv -- recompiledSources + log.debug("All newly invalidated sources after taking into account (previously) recompiled sources:" + newlyInvalidated) if(newlyInvalidated.isEmpty) Set.empty else inv } @@ -212,7 +218,7 @@ object Incremental * if they are part of a cycle containing newly invalidated files . */ def transitiveDependencies(dependsOnSrc: File => Set[File], initial: Set[File], log: Logger): Set[File] = { - val transitiveWithInitial = transitiveDeps(initial)(dependsOnSrc) + val transitiveWithInitial = transitiveDeps(initial, log)(dependsOnSrc) val transitivePartial = includeInitialCond(initial, transitiveWithInitial, dependsOnSrc, log) log.debug("Final step, transitive dependencies:\n\t" + transitivePartial) transitivePartial @@ -263,7 +269,7 @@ object Incremental val externalInheritedR = relations.publicInherited.external val byExternalInherited = external flatMap externalInheritedR.reverse val internalInheritedR = relations.publicInherited.internal - val transitiveInherited = transitiveDeps(byExternalInherited)(internalInheritedR.reverse _) + val transitiveInherited = transitiveDeps(byExternalInherited, log)(internalInheritedR.reverse _) // Get the direct dependencies of all sources transitively invalidated by inheritance val directA = transitiveInherited flatMap relations.direct.internal.reverse @@ -281,7 +287,8 @@ object Incremental * 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("Invalidating by inheritance (transitively)...") + val transitiveInherited = transitiveDeps(initial, log)(publicInherited) log.debug("Invalidated by transitive public inheritance: " + transitiveInherited) val direct = transitiveInherited flatMap directDeps log.debug("Invalidated by direct dependency: " + direct) @@ -294,7 +301,7 @@ object Incremental { val newInv = currentInvalidations -- initial log.debug("New invalidations:\n\t" + newInv) - val transitiveOfNew = transitiveDeps(newInv)(allDeps) + val transitiveOfNew = transitiveDeps(newInv, log)(allDeps) val initialDependsOnNew = transitiveOfNew & initial log.debug("Previously invalidated, but (transitively) depend on new invalidations:\n\t" + initialDependsOnNew) newInv ++ initialDependsOnNew @@ -361,16 +368,21 @@ object Incremental def orEmpty(o: Option[Source]): Source = o getOrElse APIs.emptySource def orTrue(o: Option[Boolean]): Boolean = o getOrElse true - private[this] def transitiveDeps[T](nodes: Iterable[T])(dependencies: T => Iterable[T]): Set[T] = + private[this] def transitiveDeps[T](nodes: Iterable[T], log: Logger)(dependencies: T => Iterable[T]): Set[T] = { val xs = new collection.mutable.HashSet[T] - def all(ns: Iterable[T]): Unit = ns.foreach(visit) - def visit(n: T): Unit = - if (!xs.contains(n)) { - xs += n - all(dependencies(n)) + def all(from: T, tos: Iterable[T]): Unit = tos.foreach(to => visit(from, to)) + def visit(from: T, to: T): Unit = + if (!xs.contains(to)) { + log.debug(s"Including $to by $from") + xs += to + all(to, dependencies(to)) } - all(nodes) + log.debug("Initial set of included nodes: " + nodes) + nodes foreach { start => + xs += start + all(start, dependencies(start)) + } xs.toSet } From 4b43110a2c434ffa14825e7737bdf4f3114da664 Mon Sep 17 00:00:00 2001 From: Grzegorz Kossakowski Date: Tue, 29 Oct 2013 13:20:12 +0100 Subject: [PATCH 2/2] Represent api changes as values and cleanup APIChanges class. The main motivation behind this commit is to reify information about api changes that incremental compiler considers. We introduce a new sealed class `APIChange` that has (at the moment) two subtypes: * APIChangeDueToMacroDefinition - as the name explains, this represents the case where incremental compiler considers an api to be changed just because given source file contains a macro definition * SourceAPIChange - this represents the case of regular api change; at the moment it's just a simple wrapper around value representing source file but in the future it will get expanded to contain more detailed information about API changes (e.g. collection of changed name hashes) The APIChanges becomes just a collection of APIChange instances. In particular, I removed `names` field that seems to be a dead code in incremental compiler. The `NameChanges` class and methods that refer to it in `SameAPI` has been deprecated. The Incremental.scala has been adapted to changed signature of APIChanges class. The `sameSource` method returns representation of APIChange (if there's one) instead of just simple boolean. One notable change is that information about APIChanges is pushed deeper into invalidation logic. This will allow us to treat the APIChangeDueToMacroDefinition case properly once name hashing scheme arrives. This commit shouldn't change any behavior and is purely a refactoring. --- .../api/src/main/scala/xsbt/api/SameAPI.scala | 3 + .../inc/src/main/scala/sbt/inc/Changes.scala | 16 ++++- .../src/main/scala/sbt/inc/Incremental.scala | 70 +++++++++++-------- 3 files changed, 58 insertions(+), 31 deletions(-) diff --git a/compile/api/src/main/scala/xsbt/api/SameAPI.scala b/compile/api/src/main/scala/xsbt/api/SameAPI.scala index 2621189b3..ed0aaf276 100644 --- a/compile/api/src/main/scala/xsbt/api/SameAPI.scala +++ b/compile/api/src/main/scala/xsbt/api/SameAPI.scala @@ -8,6 +8,7 @@ import xsbti.api._ import Function.tupled import scala.collection.{immutable, mutable} +@deprecated("This class is not used in incremental compiler and will be removed in next major version.", "0.13.2") class NameChanges(val newTypes: Set[String], val removedTypes: Set[String], val newTerms: Set[String], val removedTerms: Set[String]) { override def toString = @@ -19,11 +20,13 @@ class NameChanges(val newTypes: Set[String], val removedTypes: Set[String], val object TopLevel { + @deprecated("The NameChanges class is deprecated and will be removed in next major version.", "0.13.2") def nameChanges(a: Iterable[Source], b: Iterable[Source]): NameChanges = { val api = (_: Source).api apiNameChanges(a map api, b map api) } /** Identifies removed and new top-level definitions by name. */ + @deprecated("The NameChanges class is deprecated and will be removed in next major version.", "0.13.2") def apiNameChanges(a: Iterable[SourceAPI], b: Iterable[SourceAPI]): NameChanges = { def changes(s: Set[String], t: Set[String]) = (s -- t, t -- s) diff --git a/compile/inc/src/main/scala/sbt/inc/Changes.scala b/compile/inc/src/main/scala/sbt/inc/Changes.scala index 11ff23651..3fce46738 100644 --- a/compile/inc/src/main/scala/sbt/inc/Changes.scala +++ b/compile/inc/src/main/scala/sbt/inc/Changes.scala @@ -8,11 +8,21 @@ import xsbt.api.NameChanges import java.io.File final case class InitialChanges(internalSrc: Changes[File], removedProducts: Set[File], binaryDeps: Set[File], external: APIChanges[String]) -final class APIChanges[T](val modified: Set[T], val names: NameChanges) +final class APIChanges[T](val apiChanges: Iterable[APIChange[T]]) { - override def toString = "API Changes: " + modified + "\n" + names + override def toString = "API Changes: " + apiChanges + def allModified: Iterable[T] = apiChanges.map(_.modified) } +sealed abstract class APIChange[T](val modified: T) +/** + * If we recompile a source file that contains a macro definition then we always assume that it's + * api has changed. The reason is that there's no way to determine if changes to macros implementation + * are affecting its users or not. Therefore we err on the side of caution. + */ +case class APIChangeDueToMacroDefinition[T](modified0: T) extends APIChange(modified0) +case class SourceAPIChange[T](modified0: T) extends APIChange(modified0) + trait Changes[A] { def added: Set[A] @@ -24,4 +34,4 @@ trait Changes[A] sealed abstract class Change(val file: File) final class Removed(f: File) extends Change(f) final class Added(f: File, newStamp: Stamp) extends Change(f) -final class Modified(f: File, oldStamp: Stamp, newStamp: Stamp) extends Change(f) \ No newline at end of file +final class Modified(f: File, oldStamp: Stamp, newStamp: Stamp) extends Change(f) diff --git a/compile/inc/src/main/scala/sbt/inc/Incremental.scala b/compile/inc/src/main/scala/sbt/inc/Incremental.scala index 1f71baa49..321a0b1c9 100644 --- a/compile/inc/src/main/scala/sbt/inc/Incremental.scala +++ b/compile/inc/src/main/scala/sbt/inc/Incremental.scala @@ -24,7 +24,7 @@ object Incremental val initialChanges = changedInitial(entry, sources, previous, current, forEntry, options, log) val binaryChanges = new DependencyChanges { val modifiedBinaries = initialChanges.binaryDeps.toArray - val modifiedClasses = initialChanges.external.modified.toArray + val modifiedClasses = initialChanges.external.allModified.toArray def isEmpty = modifiedBinaries.isEmpty && modifiedClasses.isEmpty } val initialInv = invalidateInitial(previous.relations, initialChanges, log) @@ -79,7 +79,7 @@ object Incremental val incChanges = changedIncremental(invalidated, previous.apis.internalAPI _, merged.apis.internalAPI _, log, options) debug("\nChanges:\n" + incChanges) val transitiveStep = options.transitiveStep - val incInv = invalidateIncremental(merged.relations, incChanges, invalidated, cycleNum >= transitiveStep, log) + val incInv = invalidateIncremental(merged.relations, merged.apis, incChanges, invalidated, cycleNum >= transitiveStep, log) cycle(incInv, allSources, emptyChanges, merged, doCompile, classfileManager, cycleNum+1, log, options) } private[this] def emptyChanges: DependencyChanges = new DependencyChanges { @@ -106,15 +106,20 @@ object Incremental * * NOTE: This method creates a new APIDiff instance on every invocation. */ - private def logApiChanges[T](changes: (collection.Set[T], Seq[Source], Seq[Source]), log: Logger, - options: IncOptions): Unit = { + private def logApiChanges[T](apiChanges: Iterable[APIChange[T]], oldAPIMapping: T => Source, + newAPIMapping: T => Source, log: Logger, options: IncOptions): Unit = { val contextSize = options.apiDiffContextSize try { val apiDiff = new APIDiff - changes.zipped foreach { - case (src, oldApi, newApi) => + apiChanges foreach { + case APIChangeDueToMacroDefinition(src) => + log.debug(s"Public API is considered to be changed because $src contains a macro definition.") + case SourceAPIChange(src) => + val oldApi = oldAPIMapping(src) + val newApi = newAPIMapping(src) val apiUnifiedPatch = apiDiff.generateApiDiff(src.toString, oldApi.api, newApi.api, contextSize) - log.debug("Detected a change in a public API:\n" + apiUnifiedPatch) + log.debug(s"Detected a change in a public API (${src.toString}):\n" + + apiUnifiedPatch) } } catch { case e: ClassNotFoundException => @@ -137,26 +142,32 @@ object Incremental { val oldApis = lastSources.toSeq map oldAPI val newApis = lastSources.toSeq map newAPI - val changes = (lastSources, oldApis, newApis).zipped.filter { (src, oldApi, newApi) => !sameSource(src, oldApi, newApi, log) } + val apiChanges = (lastSources, oldApis, newApis).zipped.flatMap { (src, oldApi, newApi) => sameSource(src, oldApi, newApi, log) } - if (apiDebug(options) && changes.zipped.nonEmpty) { - logApiChanges(changes, log, options) + if (apiDebug(options) && apiChanges.nonEmpty) { + logApiChanges(apiChanges, oldAPI, newAPI, log, options) } - val changedNames = TopLevel.nameChanges(changes._3, changes._2 ) - - val modifiedAPIs = changes._1.toSet - - new APIChanges(modifiedAPIs, changedNames) + new APIChanges(apiChanges) } - def sameSource[T](src: T, a: Source, b: Source, log: Logger): Boolean = { + def sameSource[T](src: T, a: Source, b: Source, log: Logger): Option[APIChange[T]] = { // Clients of a modified source file (ie, one that doesn't satisfy `shortcutSameSource`) containing macros must be recompiled. val hasMacro = a.hasMacro || b.hasMacro - shortcutSameSource(a, b) || { + if (shortcutSameSource(a, b)) { + None + } else { if (hasMacro) { - log.debug("API is considered to be modified because the following source file contains a macro: " + src) - false - } else SameAPI(a,b) + Some(APIChangeDueToMacroDefinition(src)) + } else sameAPI(src, a, b, log) + } + } + + def sameAPI[T](src: T, a: Source, b: Source, log: Logger): Option[SourceAPIChange[T]] = { + if (SameAPI(a,b)) + None + else { + val sourceApiChange = SourceAPIChange(src) + Some(sourceApiChange) } } @@ -188,14 +199,14 @@ object Incremental val (changed, unmodified) = inBoth.partition(existingModified) } - def invalidateIncremental(previous: Relations, changes: APIChanges[File], recompiledSources: Set[File], transitive: Boolean, log: Logger): Set[File] = + def invalidateIncremental(previous: Relations, apis: APIs, changes: APIChanges[File], recompiledSources: Set[File], transitive: Boolean, log: Logger): Set[File] = { val dependsOnSrc = previous.usesInternalSrc _ val propagated = if(transitive) - transitiveDependencies(dependsOnSrc, changes.modified, log) + transitiveDependencies(dependsOnSrc, changes.allModified.toSet, log) else - invalidateIntermediate(previous, changes.modified, log) + invalidateIntermediate(previous, changes, log) val dups = invalidateDuplicates(previous) if(dups.nonEmpty) @@ -231,12 +242,13 @@ 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 = invalidateByExternal(previous, changes.external.modified, log) //changes.external.modified.flatMap(previous.usesExternal) // ++ scopeInvalidations + val externalModifiedSources = changes.external.allModified.toSet + val byExtSrcDep = invalidateByExternal(previous, externalModifiedSources, log) //changes.external.modified.flatMap(previous.usesExternal) // ++ scopeInvalidations checkAbsolute(srcChanges.added.toList) log.debug( "\nInitial source changes: \n\tremoved:" + srcChanges.removed + "\n\tadded: " + srcChanges.added + "\n\tmodified: " + srcChanges.changed + "\nRemoved products: " + changes.removedProducts + - "\nModified external sources: " + changes.external.modified + + "\nModified external sources: " + externalModifiedSources + "\nModified binary dependencies: " + changes.binaryDeps + "\nInitial directly invalidated sources: " + srcDirect + "\n\nSources indirectly invalidated by:" + @@ -278,15 +290,16 @@ object Incremental 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 invalidateIntermediate(relations: Relations, changes: APIChanges[File], log: Logger): Set[File] = { def reverse(r: Relations.Source) = r.internal.reverse _ - invalidateSources(reverse(relations.direct), reverse(relations.publicInherited), modified, log) + invalidateSources(reverse(relations.direct), reverse(relations.publicInherited), changes, 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] = + private[this] def invalidateSources(directDeps: File => Set[File], publicInherited: File => Set[File], changes: APIChanges[File], log: Logger): Set[File] = { + val initial = changes.allModified.toSet log.debug("Invalidating by inheritance (transitively)...") val transitiveInherited = transitiveDeps(initial, log)(publicInherited) log.debug("Invalidated by transitive public inheritance: " + transitiveInherited) @@ -295,6 +308,7 @@ object Incremental 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] =