AST: Refactor request evaluator dependency tracking a bit

Other than simplifying some code, the big improvement here is
that we 'freeze' the reference dependencies for a request down
to a simple vector. We only use a DenseSet to store dependencies
of active requests.

Author: slavapestov

include/swift/AST/Evaluator.h

@@ -271,8 +271,6 @@ class Evaluator {
            typename std::enable_if<Request::isEverCached>::type * = nullptr>
   llvm::Expected<typename Request::OutputType>
   operator()(const Request &request) {
-    evaluator::DependencyRecorder::StackRAII<Request> incDeps{recorder,
-                                                              request};
     // The request can be cached, but check a predicate to determine
     // whether this particular instance is cached. This allows more
     // fine-grained control over which instances get cache.
@@ -288,8 +286,6 @@ class Evaluator {
            typename std::enable_if<!Request::isEverCached>::type * = nullptr>
   llvm::Expected<typename Request::OutputType>
   operator()(const Request &request) {
-    evaluator::DependencyRecorder::StackRAII<Request> incDeps{recorder,
-                                                              request};
     return getResultUncached(request);
   }
 
@@ -366,13 +362,6 @@ class Evaluator {
           std::make_unique<CyclicalRequestError<Request>>(request, *this));
     }
 
-    // Make sure we remove this from the set of active requests once we're
-    // done.
-    SWIFT_DEFER {
-      assert(activeRequests.back() == activeReq);
-      activeRequests.pop_back();
-    };
-
     // Clear out the dependencies on this request; we're going to recompute
     // them now anyway.
     if (buildDependencyGraph)
@@ -384,8 +373,20 @@ class Evaluator {
     FrontendStatsTracer statsTracer = make_tracer(stats, request);
     if (stats) reportEvaluatedRequest(*stats, request);
 
+    recorder.beginRequest(activeReq);
+
     auto &&r = getRequestFunction<Request>()(request, *this);
-    reportEvaluatedResult<Request>(request, r);
+
+    recorder.endRequest(activeReq);
+
+    handleDependencySourceRequest<Request>(request);
+    handleDependencySinkRequest<Request>(request, r);
+
+    // Make sure we remove this from the set of active requests once we're
+    // done.
+    assert(activeRequests.back() == activeReq);
+    activeRequests.pop_back();
+
     return std::move(r);
   }
 
@@ -398,7 +399,8 @@ class Evaluator {
   getResultCached(const Request &request) {
     // If there is a cached result, return it.
     if (auto cached = request.getCachedResult()) {
-      reportEvaluatedResult<Request>(request, *cached);
+      recorder.replayCachedRequest(ActiveRequest(request));
+      handleDependencySinkRequest<Request>(request, *cached);
       return *cached;
     }
 
@@ -426,7 +428,8 @@ class Evaluator {
     auto known = cache.find_as(request);
     if (known != cache.end()) {
       auto r = known->second.template castTo<typename Request::OutputType>();
-      reportEvaluatedResult<Request>(request, r);
+      recorder.replayCachedRequest(ActiveRequest(request));
+      handleDependencySinkRequest<Request>(request, r);
       return r;
     }
 
@@ -441,22 +444,30 @@ class Evaluator {
   }
 
 private:
-  // Report the result of evaluating a request that is not a dependency sink.
   template <typename Request, typename std::enable_if<
                                   !Request::isDependencySink>::type * = nullptr>
-  void reportEvaluatedResult(const Request &r,
-                             const typename Request::OutputType &o) {
-    recorder.replay(activeRequests, ActiveRequest(r));
-  }
+  void handleDependencySinkRequest(const Request &r,
+                                   const typename Request::OutputType &o) {}
 
-  // Report the result of evaluating a request that is a dependency sink.
   template <typename Request,
             typename std::enable_if<Request::isDependencySink>::type * = nullptr>
-  void reportEvaluatedResult(const Request &r,
-                             const typename Request::OutputType &o) {
-    return recorder.record(activeRequests, [&r, &o](auto &c) {
-      return r.writeDependencySink(c, o);
-    });
+  void handleDependencySinkRequest(const Request &r,
+                                   const typename Request::OutputType &o) {
+    evaluator::DependencyCollector collector(recorder);
+    r.writeDependencySink(collector, o);
+  }
+
+  template <typename Request, typename std::enable_if<
+                                  !Request::isDependencySource>::type * = nullptr>
+  void handleDependencySourceRequest(const Request &r) {}
+
+  template <typename Request,
+            typename std::enable_if<Request::isDependencySource>::type * = nullptr>
+  void handleDependencySourceRequest(const Request &r) {
+    auto source = r.readDependencySource(recorder);
+    if (!source.isNull() && source.get()->isPrimary()) {
+      recorder.handleDependencySourceRequest(ActiveRequest(r), source.get());
+    }
   }
 
 public:

include/swift/AST/EvaluatorDependencies.h

@@ -23,6 +23,7 @@
 #include "swift/AST/SourceFile.h"
 #include "swift/Basic/NullablePtr.h"
 #include "llvm/ADT/PointerIntPair.h"
+#include <vector>
 
 namespace swift {
 
@@ -118,15 +119,12 @@ struct DependencyCollector {
     };
   };
 
-public:
-  using ReferenceSet = llvm::DenseSet<Reference, Reference::Info>;
-
 private:
   DependencyRecorder &parent;
-  ReferenceSet scratch;
 
 public:
-  explicit DependencyCollector(DependencyRecorder &parent) : parent(parent) {}
+  explicit DependencyCollector(DependencyRecorder &parent);
+  ~DependencyCollector();
 
 public:
   /// Registers a named reference from the current dependency scope to a member
@@ -168,10 +166,6 @@ struct DependencyCollector {
 public:
   /// Retrieves the dependency recorder that created this dependency collector.
   const DependencyRecorder &getRecorder() const { return parent; }
-
-  /// Returns \c true if this collector has not accumulated
-  /// any \c Reference objects.
-  bool empty() const { return scratch.empty(); }
 };
 
 /// A \c DependencyRecorder is an aggregator of named references discovered in a
@@ -180,64 +174,29 @@ struct DependencyRecorder {
   friend DependencyCollector;
 
 private:
-  /// A stack of dependency sources in the order they were evaluated.
-  llvm::SmallVector<evaluator::DependencySource, 8> dependencySources;
-  llvm::DenseMap<SourceFile *, DependencyCollector::ReferenceSet>
+  llvm::DenseMap<SourceFile *,
+                 llvm::DenseSet<DependencyCollector::Reference,
+                                DependencyCollector::Reference::Info>>
       fileReferences;
-  llvm::DenseMap<AnyRequest, DependencyCollector::ReferenceSet>
+  llvm::DenseMap<AnyRequest, std::vector<DependencyCollector::Reference>>
       requestReferences;
-  bool isRecording;
+  std::vector<llvm::DenseSet<DependencyCollector::Reference,
+                             DependencyCollector::Reference::Info>>
+      activeRequestReferences;
 
-public:
-  explicit DependencyRecorder() : isRecording{false} {};
-
-private:
-  /// Records the given \c Reference as a dependency of the current dependency
-  /// source.
-  ///
-  /// This is as opposed to merely collecting a \c Reference, which may just buffer
-  /// it for realization or replay later.
-  void realize(const DependencyCollector::Reference &ref);
+#ifndef NDEBUG
+  bool isRecording = false;
+#endif
 
 public:
-  /// Begins the recording of references by invoking the given continuation
-  /// with a fresh \c DependencyCollector object. This object should be used
-  /// to buffer dependency-relevant references to names looked up by a
-  /// given request.
-  ///
-  /// Recording only occurs for requests that are dependency sinks.
-  void record(const llvm::SetVector<swift::ActiveRequest> &stack,
-              llvm::function_ref<void(DependencyCollector &)> rec);
+  void beginRequest(const swift::ActiveRequest &req);
+  void endRequest(const swift::ActiveRequest &req);
+  void replayCachedRequest(const swift::ActiveRequest &req);
+  void handleDependencySourceRequest(const swift::ActiveRequest &req,
+                                     SourceFile *source);
 
-  /// Replays the \c Reference objects collected by a given cached request and
-  /// its sub-requests into the current dependency scope.
-  ///
-  /// Dependency replay ensures that cached requests do not "hide" names from
-  /// the active dependency scope. This would otherwise occur frequently in
-  /// batch mode, where cached requests effectively block the re-evaluation of
-  /// a large quantity of computations that perform name lookups by design.
-  ///
-  /// Replay need only occur for requests that are (separately) cached.
-  void replay(const llvm::SetVector<swift::ActiveRequest> &stack,
-              const swift::ActiveRequest &req);
 private:
-  /// Given the current stack of requests and a buffer of \c Reference objects
-  /// walk the active stack looking for the next-innermost cached request. If
-  /// found, insert the buffer of references into that request's known reference
-  /// set.
-  ///
-  /// This algorithm ensures that references propagate lazily up the request
-  /// graph from cached sub-requests to their cached parents. Once this process
-  /// completes, all cached requests in the request graph will see the
-  /// union of all references recorded while evaluating their sub-requests.
-  ///
-  /// This algorithm *must* be tail-called during
-  /// \c DependencyRecorder::record or \c DependencyRecorder::replay
-  /// or the corresponding set of references for the active dependency scope
-  /// will become incoherent.
-  void
-  unionNearestCachedRequest(ArrayRef<swift::ActiveRequest> stack,
-                            const DependencyCollector::ReferenceSet &scratch);
+  void recordDependency(const DependencyCollector::Reference &ref);
 
 public:
   using ReferenceEnumerator =
@@ -250,49 +209,6 @@ struct DependencyRecorder {
   /// of callers to ensure they are order-invariant or are sorting the result.
   void enumerateReferencesInFile(const SourceFile *SF,
                                  ReferenceEnumerator f) const ;
-
-public:
-  /// Returns the active dependency's source file, or \c nullptr if no
-  /// dependency source is active.
-  ///
-  /// The use of this accessor is strongly discouraged, as it implies that a
-  /// dependency sink is seeking to filter out names based on the files they
-  /// come from. Existing callers are being migrated to more reasonable ways
-  /// of judging the relevancy of a dependency.
-  evaluator::DependencySource getActiveDependencySourceOrNull() const {
-    if (dependencySources.empty())
-      return nullptr;
-    return dependencySources.front();
-  }
-
-public:
-  /// An RAII type that manages manipulating the evaluator's
-  /// dependency source stack. It is specialized to be zero-cost for
-  /// requests that are not dependency sources.
-  template <typename Request, typename = detail::void_t<>> struct StackRAII {
-    StackRAII(DependencyRecorder &DR, const Request &Req) {}
-  };
-
-  template <typename Request>
-  struct StackRAII<Request,
-                   typename std::enable_if<Request::isDependencySource>::type> {
-    NullablePtr<DependencyRecorder> Coll;
-    StackRAII(DependencyRecorder &coll, const Request &Req) {
-      auto Source = Req.readDependencySource(coll);
-      // If there is no source to introduce, bail. This can occur if
-      // a request originates in the context of a module.
-      if (Source.isNull() || !Source.get()->isPrimary()) {
-        return;
-      }
-      coll.dependencySources.emplace_back(Source);
-      Coll = &coll;
-    }
-
-    ~StackRAII() {
-      if (Coll.isNonNull())
-        Coll.get()->dependencySources.pop_back();
-    }
-  };
 };
 } // end namespace evaluator