Merge pull request swiftlang#20823 from davidungar/master-with-comments

[Driver] NFC: Added some comments around incremental compilation.

Author: David Ungar

include/swift/Driver/DependencyGraph.h

@@ -121,6 +121,7 @@ class DependencyGraphImpl {
   llvm::StringMap<std::pair<std::vector<DependencyEntryTy>, DependencyMaskTy>> Dependencies;
 
   /// The set of marked nodes.
+
   llvm::SmallPtrSet<const void *, 16> Marked;
 
   /// A list of all external dependencies that cannot be resolved from just this
@@ -153,6 +154,8 @@ class DependencyGraphImpl {
     (void)newlyInserted;
   }
 
+  /// See DependencyGraph::markTransitive.
+
   void markTransitive(SmallVectorImpl<const void *> &visited,
                       const void *node, MarkTracerImpl *tracer = nullptr);
   bool markIntransitive(const void *node) {
@@ -254,7 +257,7 @@ class DependencyGraph : public DependencyGraphImpl {
   /// Marks \p node and all nodes that depend on \p node, and places any nodes
   /// that get transitively marked into \p visited.
   ///
-  /// Nodes that have been previously marked are not included in \p newlyMarked,
+  /// Nodes that have been previously marked are not included in \p visited,
   /// nor are their successors traversed, <em>even if their "provides" set has
   /// been updated since it was marked.</em> (However, nodes that depend on the
   /// given \p node are always traversed.)
@@ -264,6 +267,14 @@ class DependencyGraph : public DependencyGraphImpl {
   ///
   /// If you want to see how each node gets added to \p visited, pass a local
   /// MarkTracer instance to \p tracer.
+  ///
+  /// Conservatively assumes that there exists a "cascading" edge into the
+  /// starting node. Therefore, mark the start. For each visited node, add it to
+  /// \p visited, and mark it if some incoming edge cascades. The start node is
+  /// NOT added to \p visited.
+  ///
+  /// The traversal routines use
+  /// \p visited to avoid endless recursion.
   template <unsigned N>
   void markTransitive(SmallVector<T, N> &visited, T node,
                       MarkTracer *tracer = nullptr) {

include/swift/Driver/Job.h

@@ -230,9 +230,17 @@ class CommandOutput {
 class Job {
 public:
   enum class Condition {
+    // There was no information about the previous build (i.e., an input map),
+    // or the map marked this Job as dirty or needing a cascading build.
+    // Be maximally conservative with dependencies.
     Always,
+    // The input changed, or this job was scheduled as non-cascading in the last
+    // build but didn't get to run.
     RunWithoutCascading,
+    // The best case: input didn't change, output exists.
+    // Only run if it depends on some other thing that changed.
     CheckDependencies,
+    // Run no matter what (but may or may not cascade).
     NewlyAdded
   };
 

lib/Driver/Compilation.cpp

@@ -206,9 +206,10 @@ namespace driver {
     /// Jobs that incremental-mode has decided it can skip.
     CommandSet DeferredCommands;
 
-    /// Jobs in the initial set with Condition::Always, or lacking existing
+    /// Jobs in the initial set with Condition::Always, and having an existing
     /// .swiftdeps files.
-    SmallVector<const Job *, 16> InitialOutOfDateCommands;
+    /// Set by scheduleInitialJobs and used only by scheduleAdditionalJobs.
+    SmallVector<const Job *, 16> InitialCascadingCommands;
 
     /// Dependency graph for deciding which jobs are dirty (need running)
     /// or clean (can be skipped).
@@ -382,7 +383,7 @@ namespace driver {
       DeferredCommands.clear();
     }
 
-    /// Helper that attmepts to reload a job's .swiftdeps file after the job
+    /// Helper that attempts to reload a job's .swiftdeps file after the job
     /// exits, and re-run transitive marking to ensure everything is properly
     /// invalidated by any new dependency edges introduced by it. If reloading
     /// fails, this can cause deferred jobs to be immediately scheduled.
@@ -406,6 +407,13 @@ namespace driver {
         // If we have a dependency file /and/ the frontend task exited normally,
         // we can be discerning about what downstream files to rebuild.
         if (ReturnCode == EXIT_SUCCESS || ReturnCode == EXIT_FAILURE) {
+          // "Marked" means that everything provided by this node (i.e. Job) is
+          // dirty. Thus any file using any of these provides must be
+          // recompiled. (Only non-private entities are output as provides.) In
+          // other words, this Job "cascades"; the need to recompile it causes
+          // other recompilations. It is possible that the current code marks
+          // things that do not need to be marked. Unecessary compilation would
+          // result if that were the case.
           bool wasCascading = DepGraph.isMarked(FinishedCmd);
 
           switch (DepGraph.loadFromPath(FinishedCmd, DependenciesFile)) {
@@ -715,7 +723,15 @@ namespace driver {
         switch (Condition) {
         case Job::Condition::Always:
           if (Comp.getIncrementalBuildEnabled() && !DependenciesFile.empty()) {
-            InitialOutOfDateCommands.push_back(Cmd);
+            // Ensure dependents will get recompiled.
+            InitialCascadingCommands.push_back(Cmd);
+            // Mark this job as cascading.
+            //
+            // It would probably be safe and simpler to markTransitive on the
+            // start nodes in the "Always" condition from the start instead of
+            // using markIntransitive and having later functions call
+            // markTransitive. That way markIntransitive would be an
+            // implementation detail of DependencyGraph.
             DepGraph.markIntransitive(Cmd);
           }
           LLVM_FALLTHROUGH;
@@ -740,7 +756,7 @@ namespace driver {
         // We scheduled all of the files that have actually changed. Now add the
         // files that haven't changed, so that they'll get built in parallel if
         // possible and after the first set of files if it's not.
-        for (auto *Cmd : InitialOutOfDateCommands) {
+        for (auto *Cmd : InitialCascadingCommands) {
           DepGraph.markTransitive(AdditionalOutOfDateCommands, Cmd,
                                   IncrementalTracer);
         }

lib/Driver/CompilationRecord.h

@@ -19,8 +19,8 @@ namespace swift {
 namespace driver {
 namespace compilation_record {
 
-/// Compilation record files (.swiftdeps files) are YAML files composed of these
-/// top-level keys.
+/// Compilation record files (-master.swiftdeps files) are YAML files composed
+/// of these top-level keys.
 enum class TopLevelKey {
   /// The key for the Swift compiler version used to produce the compilation
   /// record.