Merge pull request #29246 from davidungar/fine-grained-off-but-w-tests-rb

[Incremental] Fixes for fine-grained dependencies + tests for them, but off-by-default

Author: David Ungar

include/swift/AST/FineGrainedDependencies.h

@@ -785,6 +785,8 @@ class SourceFileDepGraph {
                llvm::StringMap<std::vector<std::pair<std::string, std::string>>>
                    compoundNamesByRDK);
 
+  static constexpr char noncascadingOrPrivatePrefix = '#';
+
   /// Nodes are owned by the graph.
   ~SourceFileDepGraph() {
     forEachNode([&](SourceFileDepGraphNode *n) { delete n; });
@@ -793,7 +795,7 @@ class SourceFileDepGraph {
   /// Goes at the start of an emitted YAML file to help tools recognize it.
   /// May vary in the future according to version, etc.
   std::string yamlProlog(const bool hadCompilationError) const {
-    return std::string("# Experimental\n") +
+    return std::string("# Fine-grained v0\n") +
            (!hadCompilationError ? ""
                                  : "# Dependencies are unknown because a "
                                    "compilation error occurred.\n");

include/swift/Driver/Compilation.h

@@ -529,8 +529,9 @@ class Compilation {
   /// sequence of inputs the driver was initially invoked with.
   ///
   /// Also use to write out information in a consistent order.
+  template <typename JobCollection>
   void sortJobsToMatchCompilationInputs(
-      ArrayRef<const Job *> unsortedJobs,
+      const JobCollection &unsortedJobs,
       SmallVectorImpl<const Job *> &sortedJobs) const;
 
 private:

include/swift/Driver/FineGrainedDependencyDriverGraph.h

@@ -164,10 +164,12 @@ class ModuleDepGraph {
   std::unordered_set<std::string> externalDependencies;
 
   /// The new version of "Marked."
-  /// Aka "isMarked". Holds the swiftDeps paths for jobs the driver has or will
-  /// schedule.
-  /// TODO: Move scheduledJobs out of the graph, ultimately.
-  std::unordered_set<std::string> swiftDepsOfJobsThatNeedRunning;
+  /// Aka "isMarked".
+  /// If  job is in here, all of its dependent jobs have already been searched
+  /// for jobs that depend on them, OR the job is about to be scheduled and
+  /// we'll need to run all dependent jobs after it completes. (See the call to
+  /// \c markIntransitive in \c shouldScheduleCompileJobAccordingToCondition.)
+  std::unordered_set<std::string> swiftDepsOfMarkedJobs;
 
   /// Keyed by swiftdeps filename, so we can get back to Jobs.
   std::unordered_map<std::string, const driver::Job *> jobsBySwiftDeps;
@@ -307,6 +309,10 @@ class ModuleDepGraph {
   /// For the dot file.
   std::string getGraphID() const { return "driver"; }
 
+  void forCorrespondingImplementationOfProvidedInterface(
+      const ModuleDepGraphNode *,
+      function_ref<void(ModuleDepGraphNode *)>) const;
+
   void forEachUseOf(const ModuleDepGraphNode *def,
                     function_ref<void(const ModuleDepGraphNode *use)>);
 
@@ -326,6 +332,8 @@ class ModuleDepGraph {
   /// Interface to status quo code in the driver.
   bool isMarked(const driver::Job *) const;
 
+  bool isSwiftDepsMarked(StringRef swiftDeps) const;
+
   /// Given a "cascading" job, that is a job whose dependents must be recompiled
   /// when this job is recompiled, Compute two sets of jobs:
   /// 1. Return value (via visited) is the set of jobs needing recompilation
@@ -334,9 +342,9 @@ class ModuleDepGraph {
   /// are recompiled. Such jobs are added to the \ref scheduledJobs set, and
   /// accessed via \ref isMarked.
   ///
-  /// Only return jobs marked that were previously unmarked. Not required for
-  /// the driver because it won't run a job twice, but required for the unit
-  /// test.
+  /// Returns jobs to be run because of changes to any/ever node in the
+  /// argument. Only return jobs marked that were previously unmarked, assuming
+  /// previously marked jobs are already scheduled.
   std::vector<const driver::Job*> markTransitive(
       const driver::Job *jobToBeRecompiled, const void *ignored = nullptr);
 
@@ -413,7 +421,7 @@ class ModuleDepGraph {
   /// Integrate a SourceFileDepGraph into the receiver.
   /// Integration happens when the driver needs to read SourceFileDepGraph.
   CoarseGrainedDependencyGraphImpl::LoadResult
-  integrate(const SourceFileDepGraph &);
+  integrate(const SourceFileDepGraph &, StringRef swiftDepsOfJob);
 
   enum class LocationOfPreexistingNode { nowhere, here, elsewhere };
 
@@ -431,7 +439,8 @@ class ModuleDepGraph {
   bool
   integrateSourceFileDepGraphNode(const SourceFileDepGraph &g,
                                   const SourceFileDepGraphNode *integrand,
-                                  const PreexistingNodeIfAny preexistingMatch);
+                                  const PreexistingNodeIfAny preexistingMatch,
+                                  StringRef swiftDepsOfJob);
 
   /// Integrate the \p integrand, a node that represents a Decl in the swiftDeps
   /// file being integrated. \p preexistingNodeInPlace holds the node
@@ -442,7 +451,7 @@ class ModuleDepGraph {
   /// ModuleDepGraphNode.
   std::pair<bool, ModuleDepGraphNode *>
   integrateSourceFileDeclNode(const SourceFileDepGraphNode *integrand,
-                              StringRef swiftDepsOfSourceFileGraph,
+                              StringRef swiftDepsOfJob,
                               const PreexistingNodeIfAny preexistingMatch);
 
   /// Create a brand-new ModuleDepGraphNode to integrate \p integrand.
@@ -463,14 +472,25 @@ class ModuleDepGraph {
   /// Given a definition node, and a list of already found dependents,
   /// recursively add transitive closure of dependents of the definition
   /// into the already found dependents.
+  ///
+  /// \param foundDependents gets filled out with all dependent nodes found
+  /// \param definition the starting definition
+  /// \param shouldConsiderUse returns true if a use should be considered
   void findDependentNodes(
       std::unordered_set<const ModuleDepGraphNode *> &foundDependents,
-      const ModuleDepGraphNode *definition);
+      const ModuleDepGraphNode *definition,
+      function_ref<bool(const ModuleDepGraphNode *use)> shouldConsiderUse);
 
   /// Givien a set of nodes, return the set of swiftDeps for the jobs those
   /// nodes are in.
-  llvm::StringSet<> computeSwiftDepsFromInterfaceNodes(
-      ArrayRef<const ModuleDepGraphNode *> nodes);
+  std::vector<std::string>
+  computeSwiftDepsFromNodes(ArrayRef<const ModuleDepGraphNode *> nodes) const;
+
+  std::vector<const driver::Job *>
+  getUnmarkedJobsFrom(const ArrayRef<const ModuleDepGraphNode *> nodes) const;
+
+  /// Mark any jobs for these nodes
+  void markJobsFrom(ArrayRef<const ModuleDepGraphNode *>);
 
   /// Record a visit to this node for later dependency printing
   size_t traceArrival(const ModuleDepGraphNode *visitedNode);
@@ -483,8 +503,8 @@ class ModuleDepGraph {
       const driver::Job *dependentJob);
 
   /// Return true if job was not scheduled before
-  bool recordJobNeedsRunning(StringRef swiftDeps) {
-    return swiftDepsOfJobsThatNeedRunning.insert(swiftDeps).second;
+  bool markJobViaSwiftDeps(StringRef swiftDeps) {
+    return swiftDepsOfMarkedJobs.insert(swiftDeps).second;
   }
 
   /// For debugging and visualization, write out the graph to a dot file.

lib/AST/FineGrainedDependenciesSourceFileDepGraphConstructor.cpp

@@ -585,14 +585,17 @@ class SourceFileDepGraphConstructor {
       dynamicLookupDepends.push_back(std::make_pair(p.getFirst().userFacingName(), p.getSecond()));
 
     std::vector<std::pair<std::tuple<std::string, std::string, bool>, bool>> memberDepends;
-    for (const auto &p: SF->getReferencedNameTracker()->getUsedMembers())
+    for (const auto &p: SF->getReferencedNameTracker()->getUsedMembers()) {
+      const auto &member = p.getFirst().second;
+      StringRef emptyOrUserFacingName = member.empty() ? "" : member.userFacingName();
       memberDepends.push_back(
         std::make_pair(
           std::make_tuple(
             mangleTypeAsContext(p.getFirst().first),
-            p.getFirst().second.userFacingName(),
+            emptyOrUserFacingName,
             declIsPrivate(p.getFirst().first)),
           p.getSecond()));
+    }
 
     return SourceFileDepGraphConstructor(
       swiftDeps,
@@ -810,7 +813,8 @@ bool swift::fine_grained_dependencies::emitReferenceDependencies(
         return false;
       });
 
-  assert(g.verifyReadsWhatIsWritten(outputPath));
+  // If path is stdout, cannot read it back, so check for "-"
+  assert(outputPath == "-" || g.verifyReadsWhatIsWritten(outputPath));
 
   if (alsoEmitDotFile) {
     std::string dotFileName = outputPath.str() + ".dot";
@@ -825,44 +829,47 @@ bool swift::fine_grained_dependencies::emitReferenceDependencies(
 //==============================================================================
 // Entry point from the unit tests
 //==============================================================================
+static StringRef stripPrefix(const StringRef name) {
+  return name.ltrim(SourceFileDepGraph::noncascadingOrPrivatePrefix);
+}
 
 static std::vector<ContextNameFingerprint>
 getBaseNameProvides(ArrayRef<std::string> simpleNames) {
   std::vector<ContextNameFingerprint> result;
   for (StringRef n : simpleNames)
-    result.push_back(ContextNameFingerprint("", n.str(), None));
+    result.push_back(ContextNameFingerprint("", stripPrefix(n).str(), None));
   return result;
 }
 
 static std::vector<ContextNameFingerprint>
 getMangledHolderProvides(ArrayRef<std::string> simpleNames) {
   std::vector<ContextNameFingerprint> result;
   for (StringRef n : simpleNames)
-    result.push_back(ContextNameFingerprint(n.str(), "", None));
+    result.push_back(ContextNameFingerprint(stripPrefix(n).str(), "", None));
   return result;
 }
 
 static std::vector<ContextNameFingerprint> getCompoundProvides(
     ArrayRef<std::pair<std::string, std::string>> compoundNames) {
   std::vector<ContextNameFingerprint> result;
   for (const auto &p : compoundNames)
-    result.push_back(ContextNameFingerprint(p.first, p.second, None));
+    result.push_back(ContextNameFingerprint(stripPrefix(p.first),
+                                            stripPrefix(p.second), None));
   return result;
 }
 
-// Use '_' as a prefix indicating non-cascading
-static bool cascades(const std::string &s) { return s.empty() || s[0] != '_'; }
+static bool cascades(const std::string &s) { return s.empty() || s[0] != SourceFileDepGraph::noncascadingOrPrivatePrefix; }
 
 // Use '_' as a prefix for a file-private member
 static bool isPrivate(const std::string &s) {
-  return !s.empty() && s[0] == '_';
+  return !s.empty() && s[0] == SourceFileDepGraph::noncascadingOrPrivatePrefix;
 }
 
 static std::vector<std::pair<std::string, bool>>
 getSimpleDepends(ArrayRef<std::string> simpleNames) {
   std::vector<std::pair<std::string, bool>> result;
   for (std::string n : simpleNames)
-    result.push_back({n, cascades((n))});
+    result.push_back({stripPrefix(n), cascades((n))});
   return result;
 }
 
@@ -881,14 +888,16 @@ getCompoundDepends(
     // (On Linux, the compiler needs more verbosity than:
     //  result.push_back({{n, "", false}, cascades(n)});
     result.push_back(
-        std::make_pair(std::make_tuple(n, std::string(), false), cascades(n)));
+        std::make_pair(std::make_tuple(stripPrefix(n), std::string(), false), cascades(n)));
   }
   for (auto &p : compoundNames) {
     // Likewise, for Linux expand the following out:
     //    result.push_back(
     //        {{p.first, p.second, isPrivate(p.second)}, cascades(p.first)});
     result.push_back(
-        std::make_pair(std::make_tuple(p.first, p.second, isPrivate(p.second)),
+        std::make_pair(std::make_tuple(stripPrefix(p.first),
+                                       stripPrefix(p.second),
+                                       isPrivate(p.second)),
                        cascades(p.first)));
   }
   return result;

lib/Driver/Compilation.cpp

@@ -355,6 +355,15 @@ namespace driver {
       PendingExecution.insert(Cmd);
     }
 
+    // Sort for ease of testing
+    template <typename Jobs>
+    void scheduleCommandsInSortedOrder(const Jobs &jobs) {
+      llvm::SmallVector<const Job *, 16> sortedJobs;
+      Comp.sortJobsToMatchCompilationInputs(jobs, sortedJobs);
+      for (const Job *Cmd : sortedJobs)
+        scheduleCommandIfNecessaryAndPossible(Cmd);
+    }
+
     void addPendingJobToTaskQueue(const Job *Cmd) {
       // FIXME: Failing here should not take down the whole process.
       bool success =
@@ -393,8 +402,7 @@ namespace driver {
                        << LogJobArray(AllBlocked) << "\n";
         }
         BlockingCommands.erase(BlockedIter);
-        for (auto *Blocked : AllBlocked)
-          scheduleCommandIfNecessaryAndPossible(Blocked);
+        scheduleCommandsInSortedOrder(AllBlocked);
       }
     }
 
@@ -449,7 +457,7 @@ namespace driver {
                                  diag::warn_unable_to_load_dependencies,
                                  DependenciesFile);
       Comp.disableIncrementalBuild(
-          Twine("malformed swift dependencies file ' ") + DependenciesFile +
+          Twine("malformed swift dependencies file '") + DependenciesFile +
           "'");
     }
 
@@ -485,23 +493,23 @@ namespace driver {
         // 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 wasKnownToNeedRunning = isMarkedInDepGraph(FinishedCmd, forRanges);
+        bool wasKnownToCascade = isMarkedInDepGraph(FinishedCmd, forRanges);
 
-        switch (loadDepGraphFromPath(FinishedCmd, DependenciesFile,
-                                     Comp.getDiags(), forRanges)) {
+        const auto loadResult = loadDepGraphFromPath(
+            FinishedCmd, DependenciesFile, Comp.getDiags(), forRanges);
+        switch (loadResult) {
         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);
+          scheduleCommandsInSortedOrder(DeferredCommands);
           DeferredCommands.clear();
           break;
 
         case CoarseGrainedDependencyGraph::LoadResult::UpToDate:
-          if (!wasKnownToNeedRunning)
+          if (!wasKnownToCascade)
             break;
           LLVM_FALLTHROUGH;
         case CoarseGrainedDependencyGraph::LoadResult::AffectsDownstream:
@@ -695,18 +703,9 @@ namespace driver {
       noteBuildingJobs(DependentsInEffect, useRangesForScheduling,
                        "because of dependencies discovered later");
 
-      // Sort dependents for more deterministic behavior
-      llvm::SmallVector<const Job *, 16> UnsortedDependents;
-      for (const Job *j : DependentsInEffect)
-        UnsortedDependents.push_back(j);
-      llvm::SmallVector<const Job *, 16> SortedDependents;
-      Comp.sortJobsToMatchCompilationInputs(UnsortedDependents,
-                                            SortedDependents);
-
-      for (const Job *Cmd : SortedDependents) {
+      scheduleCommandsInSortedOrder(DependentsInEffect);
+      for (const Job *Cmd : DependentsInEffect)
         DeferredCommands.erase(Cmd);
-        scheduleCommandIfNecessaryAndPossible(Cmd);
-      }
       return TaskFinishedResponse::ContinueExecution;
     }
 
@@ -1092,6 +1091,12 @@ namespace driver {
           // using markIntransitive and having later functions call
           // markTransitive. That way markIntransitive would be an
           // implementation detail of CoarseGrainedDependencyGraph.
+          //
+          // As it stands, after this job finishes, this mark will tell the code
+          // that this job was known to be "cascading". That knowledge will
+          // cause any dependent jobs to be run if it hasn't already been.
+          //
+          // TODO: I think this is overly tricky
           markIntransitiveInDepGraph(Cmd, forRanges);
         }
         LLVM_FALLTHROUGH;
@@ -2080,17 +2085,22 @@ void Compilation::addDependencyPathOrCreateDummy(
   }
 }
 
+template <typename JobCollection>
 void Compilation::sortJobsToMatchCompilationInputs(
-    const ArrayRef<const Job *> unsortedJobs,
+    const JobCollection &unsortedJobs,
     SmallVectorImpl<const Job *> &sortedJobs) const {
   llvm::DenseMap<StringRef, const Job *> jobsByInput;
   for (const Job *J : unsortedJobs) {
-    const CompileJobAction *CJA = cast<CompileJobAction>(&J->getSource());
-    const InputAction *IA = CJA->findSingleSwiftInput();
-    auto R =
-        jobsByInput.insert(std::make_pair(IA->getInputArg().getValue(), J));
-    assert(R.second);
-    (void)R;
+    // Only worry about sorting compilation jobs
+    if (const CompileJobAction *CJA =
+            dyn_cast<CompileJobAction>(&J->getSource())) {
+      const InputAction *IA = CJA->findSingleSwiftInput();
+      auto R =
+          jobsByInput.insert(std::make_pair(IA->getInputArg().getValue(), J));
+      assert(R.second);
+      (void)R;
+    } else
+      sortedJobs.push_back(J);
   }
   for (const InputPair &P : getInputFiles()) {
     auto I = jobsByInput.find(P.second->getValue());
@@ -2099,3 +2109,8 @@ void Compilation::sortJobsToMatchCompilationInputs(
     }
   }
 }
+
+template void
+Compilation::sortJobsToMatchCompilationInputs<ArrayRef<const Job *>>(
+    const ArrayRef<const Job *> &,
+    SmallVectorImpl<const Job *> &sortedJobs) const;