Merge pull request #28642 from davidungar/markTransExt-returns-value

David Ungar · web-flow · commit b9a638756b04 · 2019-12-08T14:31:22.000-08:00
[Driver] Change `markTransitive` and `markExternal` to return values
diff --git a/include/swift/Driver/CoarseGrainedDependencyGraph.h b/include/swift/Driver/CoarseGrainedDependencyGraph.h
@@ -158,14 +158,13 @@ class CoarseGrainedDependencyGraphImpl {
 
   /// See CoarseGrainedDependencyGraph::markTransitive.
 
-  void markTransitive(SmallVectorImpl<const void *> &visited,
-                      const void *node, MarkTracerImpl *tracer = nullptr);
+  std::vector<const void*> markTransitive(const void *node, MarkTracerImpl *tracer = nullptr);
+
   bool markIntransitive(const void *node) {
     assert(Provides.count(node) && "node is not in the graph");
     return Marked.insert(node).second;
   }
-  void markExternal(SmallVectorImpl<const void *> &visited,
-                    StringRef externalDependency);
+  std::vector<const void*> markExternal(StringRef externalDependency);
 
 public:
   void forEachUnmarkedJobDirectlyDependentOnExternalSwiftdeps(
@@ -202,13 +201,14 @@ class CoarseGrainedDependencyGraph : public CoarseGrainedDependencyGraphImpl {
   using Traits = llvm::PointerLikeTypeTraits<T>;
   static_assert(Traits::NumLowBitsAvailable >= 0, "not a pointer-like type");
 
-  static void copyBack(SmallVectorImpl<T> &result,
-                       ArrayRef<const void *> rawNodes) {
-    result.reserve(result.size() + rawNodes.size());
+  static std::vector<T> copyBack(ArrayRef<const void *> rawNodes) {
+    std::vector<T> result;
+    result.reserve(rawNodes.size());
     std::transform(rawNodes.begin(), rawNodes.end(), std::back_inserter(result),
                    [](const void *rawNode) {
       return Traits::getFromVoidPointer(const_cast<void *>(rawNode));
     });
+    return result;
   }
 
 public:
@@ -286,23 +286,20 @@ class CoarseGrainedDependencyGraph : public CoarseGrainedDependencyGraphImpl {
   ///
   /// The traversal routines use
   /// \p visited to avoid endless recursion.
-  template <unsigned N>
-  void markTransitive(SmallVector<T, N> &visited, T node,
+  std::vector<T> markTransitive(T node,
                       MarkTracer *tracer = nullptr) {
-    SmallVector<const void *, N> rawMarked;
+    std::vector<const void *> rawMarked =
     CoarseGrainedDependencyGraphImpl::markTransitive(
-        rawMarked, Traits::getAsVoidPointer(node), tracer);
+        Traits::getAsVoidPointer(node), tracer);
     // FIXME: How can we avoid this copy?
-    copyBack(visited, rawMarked);
+    return copyBack(rawMarked);
   }
 
-  template <unsigned N>
-  void markExternal(SmallVector<T, N> &visited, StringRef externalDependency) {
-    SmallVector<const void *, N> rawMarked;
-    CoarseGrainedDependencyGraphImpl::markExternal(rawMarked,
-                                                   externalDependency);
+  std::vector<T>
+  markExternal(StringRef externalDependency) {
+    const auto rawMarked = CoarseGrainedDependencyGraphImpl::markExternal(externalDependency);
     // FIXME: How can we avoid this copy?
-    copyBack(visited, rawMarked);
+    return copyBack(rawMarked);
   }
 
   /// Marks \p node without marking any dependencies.
diff --git a/include/swift/Driver/FineGrainedDependencyDriverGraph.h b/include/swift/Driver/FineGrainedDependencyDriverGraph.h
@@ -304,8 +304,7 @@ class ModuleDepGraph {
   /// 2. Jobs not previously known to need dependencies reexamined after they
   /// are recompiled. Such jobs are added to the \ref cascadingJobs set, and
   /// accessed via \ref isMarked.
-  void markTransitive(
-      SmallVectorImpl<const driver::Job *> &consequentJobsToRecompile,
+  std::vector<const driver::Job*> markTransitive(
       const driver::Job *jobToBeRecompiled, const void *ignored = nullptr);
 
   /// "Mark" this node only.
@@ -316,8 +315,7 @@ class ModuleDepGraph {
 
   std::vector<StringRef> getExternalDependencies() const;
 
-  void markExternal(SmallVectorImpl<const driver::Job *> &uses,
-                    StringRef externalDependency);
+  std::vector<const driver::Job*> markExternal(StringRef externalDependency);
 
   void forEachUnmarkedJobDirectlyDependentOnExternalSwiftdeps(
       StringRef externalDependency, function_ref<void(const driver::Job *)> fn);
@@ -436,8 +434,7 @@ class ModuleDepGraph {
       std::unordered_set<const ModuleDepGraphNode *> &foundDependents,
       const ModuleDepGraphNode *definition);
 
-  void computeUniqueJobsFromNodes(
-      SmallVectorImpl<const driver::Job *> &uniqueJobs,
+  std::vector<const driver::Job*> computeUniqueJobsFromNodes(
       const std::unordered_set<const ModuleDepGraphNode *> &nodes);
 
   /// Record a visit to this node for later dependency printing
diff --git a/lib/Driver/CoarseGrainedDependencyGraph.cpp b/lib/Driver/CoarseGrainedDependencyGraph.cpp
@@ -297,13 +297,16 @@ CoarseGrainedDependencyGraphImpl::loadFromBuffer(const void *node,
                              interfaceHashCallback);
 }
 
-void CoarseGrainedDependencyGraphImpl::markExternal(
-    SmallVectorImpl<const void *> &visited, StringRef externalDependency) {
+std::vector<const void*> CoarseGrainedDependencyGraphImpl::markExternal(
+    StringRef externalDependency) {
+  std::vector<const void*> visited;
   forEachUnmarkedJobDirectlyDependentOnExternalSwiftdeps(
-      externalDependency, [&](const void *node) {
-        visited.push_back(node);
-        markTransitive(visited, node);
-      });
+                                                         externalDependency, [&](const void *node) {
+    visited.push_back(node);
+    for (const void* markedNode: markTransitive(node))
+      visited.push_back(markedNode);
+  });
+  return visited;
 }
 
 void CoarseGrainedDependencyGraphImpl::
@@ -322,9 +325,10 @@ void CoarseGrainedDependencyGraphImpl::
   }
 }
 
-void CoarseGrainedDependencyGraphImpl::markTransitive(
-    SmallVectorImpl<const void *> &visited, const void *node,
+std::vector<const void*> CoarseGrainedDependencyGraphImpl::markTransitive(
+    const void *node,
     MarkTracerImpl *tracer) {
+  std::vector <const void*> visited;
   assert(Provides.count(node) && "node is not in the graph");
   llvm::SpecificBumpPtrAllocator<MarkTracerImpl::Entry> scratchAlloc;
 
@@ -408,6 +412,7 @@ void CoarseGrainedDependencyGraphImpl::markTransitive(
       continue;
     record(next);
   }
+  return visited;
 }
 
 void CoarseGrainedDependencyGraphImpl::MarkTracerImpl::countStatsForNodeMarking(
diff --git a/lib/Driver/Compilation.cpp b/lib/Driver/Compilation.cpp
@@ -437,11 +437,9 @@ namespace driver {
     /// invalidated by any new dependency edges introduced by it. If reloading
     /// fails, this can cause deferred jobs to be immediately scheduled.
 
-    template <unsigned N>
-    void reloadAndRemarkDeps(const Job *FinishedCmd, int ReturnCode,
-                             SmallVector<const Job *, N> &Dependents,
+    std::vector<const Job*>
+    reloadAndRemarkDeps(const Job *FinishedCmd, int ReturnCode,
                              const bool forRanges) {
-
       const CommandOutput &Output = FinishedCmd->getOutput();
       StringRef DependenciesFile =
           Output.getAdditionalOutputForType(file_types::TY_SwiftDeps);
@@ -454,7 +452,7 @@ namespace driver {
         // not using either of those right now, and this logic should probably
         // be revisited when we are.
         assert(FinishedCmd->getCondition() == Job::Condition::Always);
-        return;
+        return {};
       }
       // If we have a dependency file /and/ the frontend task exited normally,
       // we can be discerning about what downstream files to rebuild.
@@ -470,61 +468,61 @@ namespace driver {
 
         switch (loadDepGraphFromPath(FinishedCmd, DependenciesFile,
                                      Comp.getDiags(), forRanges)) {
-        case CoarseGrainedDependencyGraphImpl::LoadResult::HadError:
-          if (ReturnCode == EXIT_SUCCESS) {
-            dependencyLoadFailed(DependenciesFile);
-            // Better try compiling whatever was waiting on more info.
-            for (const Job *Cmd : DeferredCommands)
-              scheduleCommandIfNecessaryAndPossible(Cmd);
-            DeferredCommands.clear();
-            Dependents.clear();
-          } // else, let the next build handle it.
+        case CoarseGrainedDependencyGraph::LoadResult::HadError:
+          if (ReturnCode != EXIT_SUCCESS)
+            // let the next build handle it.
+            break;
+          dependencyLoadFailed(DependenciesFile);
+          // Better try compiling whatever was waiting on more info.
+          for (const Job *Cmd : DeferredCommands)
+            scheduleCommandIfNecessaryAndPossible(Cmd);
+          DeferredCommands.clear();
           break;
-        case CoarseGrainedDependencyGraphImpl::LoadResult::UpToDate:
+
+        case CoarseGrainedDependencyGraph::LoadResult::UpToDate:
           if (!wasCascading)
             break;
           LLVM_FALLTHROUGH;
-        case CoarseGrainedDependencyGraphImpl::LoadResult::AffectsDownstream:
-          markTransitiveInDepGraph(Dependents, FinishedCmd, forRanges,
+        case CoarseGrainedDependencyGraph::LoadResult::AffectsDownstream:
+          return markTransitiveInDepGraph(FinishedCmd, forRanges,
                                    IncrementalTracer);
-          break;
         }
-        } else {
-          // If there's an abnormal exit (a crash), assume the worst.
-          switch (FinishedCmd->getCondition()) {
-          case Job::Condition::NewlyAdded:
-            // The job won't be treated as newly added next time. Conservatively
-            // mark it as affecting other jobs, because some of them may have
-            // completed already.
-            markTransitiveInDepGraph(Dependents, FinishedCmd, forRanges,
-                                     IncrementalTracer);
-            break;
-          case Job::Condition::Always:
-            // Any incremental task that shows up here has already been marked;
-            // we didn't need to wait for it to finish to start downstream
-            // tasks.
-            assert(isMarkedInDepGraph(FinishedCmd, forRanges));
-            break;
-          case Job::Condition::RunWithoutCascading:
-            // If this file changed, it might have been a non-cascading change
-            // and it might not. Unfortunately, the interface hash has been
-            // updated or compromised, so we don't actually know anymore; we
-            // have to conservatively assume the changes could affect other
-            // files.
-            markTransitiveInDepGraph(Dependents, FinishedCmd, forRanges,
-                                     IncrementalTracer);
-            break;
-          case Job::Condition::CheckDependencies:
-            // If the only reason we're running this is because something else
-            // changed, then we can trust the dependency graph as to whether
-            // it's a cascading or non-cascading change. That is, if whatever
-            // /caused/ the error isn't supposed to affect other files, and
-            // whatever /fixes/ the error isn't supposed to affect other files,
-            // then there's no need to recompile any other inputs. If either of
-            // those are false, we /do/ need to recompile other inputs.
-            break;
-          }
+        return {};
         }
+        // If there's an abnormal exit (a crash), assume the worst.
+        switch (FinishedCmd->getCondition()) {
+        case Job::Condition::NewlyAdded:
+          // The job won't be treated as newly added next time. Conservatively
+          // mark it as affecting other jobs, because some of them may have
+          // completed already.
+          return markTransitiveInDepGraph(FinishedCmd, forRanges,
+                                   IncrementalTracer);
+        case Job::Condition::Always:
+          // Any incremental task that shows up here has already been marked;
+          // we didn't need to wait for it to finish to start downstream
+          // tasks.
+          assert(isMarkedInDepGraph(FinishedCmd, forRanges));
+          break;
+        case Job::Condition::RunWithoutCascading:
+          // If this file changed, it might have been a non-cascading change
+          // and it might not. Unfortunately, the interface hash has been
+          // updated or compromised, so we don't actually know anymore; we
+          // have to conservatively assume the changes could affect other
+          // files.
+          return markTransitiveInDepGraph(FinishedCmd, forRanges,
+                                   IncrementalTracer);
+
+        case Job::Condition::CheckDependencies:
+          // If the only reason we're running this is because something else
+          // changed, then we can trust the dependency graph as to whether
+          // it's a cascading or non-cascading change. That is, if whatever
+          // /caused/ the error isn't supposed to affect other files, and
+          // whatever /fixes/ the error isn't supposed to affect other files,
+          // then there's no need to recompile any other inputs. If either of
+          // those are false, we /do/ need to recompile other inputs.
+          break;
+        }
+      return {};
     }
 
     /// Check to see if a job produced a zero-length serialized diagnostics
@@ -744,8 +742,7 @@ namespace driver {
                                     const bool forRanges) {
       if (!Comp.getIncrementalBuildEnabled())
         return {};
-      SmallVector<const Job *, 16> Dependents;
-      reloadAndRemarkDeps(FinishedCmd, ReturnCode, Dependents, forRanges);
+      auto Dependents = reloadAndRemarkDeps(FinishedCmd, ReturnCode, forRanges);
       CommandSet DepSet;
       for (const Job *Cmd : Dependents)
         DepSet.insert(Cmd);
@@ -1035,12 +1032,12 @@ namespace driver {
       const auto loadResult = loadDepGraphFromPath(Cmd, DependenciesFile,
                                                    Comp.getDiags(), forRanges);
       switch (loadResult) {
-      case CoarseGrainedDependencyGraphImpl::LoadResult::HadError:
+      case CoarseGrainedDependencyGraph::LoadResult::HadError:
         dependencyLoadFailed(DependenciesFile, /*Warn=*/true);
         return None;
-      case CoarseGrainedDependencyGraphImpl::LoadResult::UpToDate:
+      case CoarseGrainedDependencyGraph::LoadResult::UpToDate:
         return std::make_pair(Cmd->getCondition(), true);
-      case CoarseGrainedDependencyGraphImpl::LoadResult::AffectsDownstream:
+      case CoarseGrainedDependencyGraph::LoadResult::AffectsDownstream:
         if (Comp.getEnableFineGrainedDependencies()) {
           // The fine-grained graph reports a change, since it lumps new
           // files together with new "Provides".
@@ -1106,15 +1103,16 @@ namespace driver {
       }
     }
 
-    SmallVector<const Job *, 16> collectCascadedJobsFromDependencyGraph(
+    CommandSet collectCascadedJobsFromDependencyGraph(
         const CommandSet &InitialCascadingCommands, const bool forRanges) {
-      SmallVector<const Job *, 16> CascadedJobs;
+      CommandSet CascadedJobs;
       // 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 : InitialCascadingCommands) {
-        markTransitiveInDepGraph(CascadedJobs, Cmd, forRanges,
-                                 IncrementalTracer);
+        for (const auto *transitiveCmd: markTransitiveInDepGraph(Cmd, forRanges,
+                                 IncrementalTracer))
+          CascadedJobs.insert(transitiveCmd);
       }
       for (auto *transitiveCmd : CascadedJobs)
         noteBuilding(transitiveCmd, /*willBeBuilding=*/true,
@@ -1133,7 +1131,8 @@ namespace driver {
         // If the dependency has been modified since the oldest built file,
         // or if we can't stat it for some reason (perhaps it's been
         // deleted?), trigger rebuilds through the dependency graph.
-        markExternalInDepGraph(ExternallyDependentJobs, dependency, forRanges);
+        for (const Job * marked: markExternalInDepGraph(dependency, forRanges))
+          ExternallyDependentJobs.push_back(marked);
       });
       for (auto *externalCmd : ExternallyDependentJobs) {
         noteBuilding(externalCmd, /*willBeBuilding=*/true,
@@ -1574,15 +1573,12 @@ namespace driver {
       return Dependencies;
     }
 
-    template <unsigned N>
-    void markExternalInDepGraph(SmallVector<const driver::Job *, N> &uses,
-                                StringRef externalDependency,
+    std::vector<const Job*>
+    markExternalInDepGraph(StringRef externalDependency,
                                 const bool forRanges) {
-      if (Comp.getEnableFineGrainedDependencies())
-        getFineGrainedDepGraph(forRanges).markExternal(uses,
-                                                       externalDependency);
-      else
-        getDepGraph(forRanges).markExternal(uses, externalDependency);
+      return Comp.getEnableFineGrainedDependencies()
+        ? getFineGrainedDepGraph(forRanges).markExternal(externalDependency)
+        : getDepGraph(forRanges).markExternal(externalDependency);
     }
 
     bool markIntransitiveInDepGraph(const Job *Cmd, const bool forRanges) {
@@ -1600,15 +1596,13 @@ namespace driver {
                  : getDepGraph(forRanges).loadFromPath(Cmd, path, diags);
     }
 
-    template <unsigned N>
-    void markTransitiveInDepGraph(
-        SmallVector<const Job *, N> &visited, const Job *Cmd,
+    std::vector<const Job*> markTransitiveInDepGraph(
+        const Job *Cmd,
         const bool forRanges,
         CoarseGrainedDependencyGraph::MarkTracer *tracer = nullptr) {
-      if (Comp.getEnableFineGrainedDependencies())
-        getFineGrainedDepGraph(forRanges).markTransitive(visited, Cmd, tracer);
-      else
-        getDepGraph(forRanges).markTransitive(visited, Cmd, tracer);
+      return Comp.getEnableFineGrainedDependencies()
+        ? getFineGrainedDepGraph(forRanges).markTransitive(Cmd, tracer)
+        : getDepGraph(forRanges).markTransitive(Cmd, tracer);
     }
 
     void addIndependentNodeToDepGraph(const Job *Cmd, const bool forRanges) {
diff --git a/lib/Driver/FineGrainedDependencyDriverGraph.cpp b/lib/Driver/FineGrainedDependencyDriverGraph.cpp
diff --git a/unittests/Driver/CoarseGrainedDependencyGraphTests.cpp b/unittests/Driver/CoarseGrainedDependencyGraphTests.cpp
