Fixes for fine-grained dependencies

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/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

@@ -449,7 +449,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,10 +485,11 @@ 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.
@@ -501,7 +502,7 @@ namespace driver {
           break;
 
         case CoarseGrainedDependencyGraph::LoadResult::UpToDate:
-          if (!wasKnownToNeedRunning)
+          if (!wasKnownToCascade)
             break;
           LLVM_FALLTHROUGH;
         case CoarseGrainedDependencyGraph::LoadResult::AffectsDownstream:
@@ -1092,6 +1093,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
+          // any dependent jobs to be run if they haven't already been.
+          //
+          // TODO: I think this is overly tricky
           markIntransitiveInDepGraph(Cmd, forRanges);
         }
         LLVM_FALLTHROUGH;