Merge pull request swiftlang#29089 from slavapestov/request-eval-perf-hack-5.2

Fix some low-hanging request evaluator performance fruit [5.2]

Author: slavapestov

include/swift/AST/Evaluator.h

@@ -187,6 +187,9 @@ class Evaluator {
   /// Whether to dump detailed debug info for cycles.
   bool debugDumpCycles;
 
+  /// Whether we're building a request dependency graph.
+  bool buildDependencyGraph;
+
   /// Used to report statistics about which requests were evaluated, if
   /// non-null.
   UnifiedStatsReporter *stats = nullptr;
@@ -237,7 +240,9 @@ class Evaluator {
 public:
   /// Construct a new evaluator that can emit cyclic-dependency
   /// diagnostics through the given diagnostics engine.
-  Evaluator(DiagnosticEngine &diags, bool debugDumpCycles=false);
+  Evaluator(DiagnosticEngine &diags,
+            bool debugDumpCycles,
+            bool buildDependencyGraph);
 
   /// Emit GraphViz output visualizing the request graph.
   void emitRequestEvaluatorGraphViz(llvm::StringRef graphVizPath);
@@ -253,26 +258,28 @@ class Evaluator {
   void registerRequestFunctions(Zone zone,
                                 ArrayRef<AbstractRequestFunction *> functions);
 
-  /// Evaluate the given request and produce its result,
-  /// consulting/populating the cache as required.
-  template<typename Request>
+  /// Retrieve the result produced by evaluating a request that can
+  /// be cached.
+  template<typename Request,
+           typename std::enable_if<Request::isEverCached>::type * = nullptr>
   llvm::Expected<typename Request::OutputType>
   operator()(const Request &request) {
-    // Check for a cycle.
-    if (checkDependency(getCanonicalRequest(request))) {
-      return llvm::Error(
-        llvm::make_unique<CyclicalRequestError<Request>>(request, *this));
-    }
+    // 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.
+    if (request.isCached())
+      return getResultCached(request);
 
-    // Make sure we remove this from the set of active requests once we're
-    // done.
-    SWIFT_DEFER {
-      assert(activeRequests.back().castTo<Request>() == request);
-      activeRequests.pop_back();
-    };
+    return getResultUncached(request);
+  }
 
-    // Get the result.
-    return getResult(request);
+  /// Retrieve the result produced by evaluating a request that
+  /// will never be cached.
+  template<typename Request,
+           typename std::enable_if<!Request::isEverCached>::type * = nullptr>
+  llvm::Expected<typename Request::OutputType>
+  operator()(const Request &request) {
+    return getResultUncached(request);
   }
 
   /// Evaluate a set of requests and return their results as a tuple.
@@ -340,37 +347,27 @@ class Evaluator {
   /// request to the \c activeRequests stack.
   bool checkDependency(const AnyRequest &request);
 
-  /// Retrieve the result produced by evaluating a request that can
-  /// be cached.
-  template<typename Request,
-           typename std::enable_if<Request::isEverCached>::type * = nullptr>
-  llvm::Expected<typename Request::OutputType>
-  getResult(const Request &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.
-    if (request.isCached())
-      return getResultCached(request);
-
-    return getResultUncached(request);
-  }
-
-  /// Retrieve the result produced by evaluating a request that
-  /// will never be cached.
-  template<typename Request,
-           typename std::enable_if<!Request::isEverCached>::type * = nullptr>
-  llvm::Expected<typename Request::OutputType>
-  getResult(const Request &request) {
-    return getResultUncached(request);
-  }
-
   /// Produce the result of the request without caching.
   template<typename Request>
   llvm::Expected<typename Request::OutputType>
   getResultUncached(const Request &request) {
+    // Check for a cycle.
+    if (checkDependency(getCanonicalRequest(request))) {
+      return llvm::Error(
+        llvm::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().castTo<Request>() == request);
+      activeRequests.pop_back();
+    };
+
     // Clear out the dependencies on this request; we're going to recompute
     // them now anyway.
-    dependencies.find_as(request)->second.clear();
+    if (buildDependencyGraph)
+      dependencies.find_as(request)->second.clear();
 
     PrettyStackTraceRequest<Request> prettyStackTrace(request);
 

include/swift/Basic/LangOptions.h

@@ -173,7 +173,10 @@ namespace swift {
 
     /// Whether to dump debug info for request evaluator cycles.
     bool DebugDumpCycles = false;
-    
+
+    /// Whether to build a request dependency graph for debugging.
+    bool BuildRequestDependencyGraph = false;
+
     /// Enable SIL type lowering
     bool EnableSubstSILFunctionTypesForFunctionValues = false;
 

include/swift/Option/FrontendOptions.td

@@ -246,8 +246,10 @@ def debug_forbid_typecheck_prefix : Separate<["-"], "debug-forbid-typecheck-pref
 
 def debug_cycles : Flag<["-"], "debug-cycles">,
   HelpText<"Print out debug dumps when cycles are detected in evaluation">;
+def build_request_dependency_graph : Flag<["-"], "build-request-dependency-graph">,
+  HelpText<"Build request dependency graph">;
 def output_request_graphviz : Separate<["-"], "output-request-graphviz">,
-  HelpText<"Emit GraphViz output visualizing the request graph">;
+  HelpText<"Emit GraphViz output visualizing the request dependency graph">;
 
 def debug_time_compilation : Flag<["-"], "debug-time-compilation">,
   HelpText<"Prints the time taken by each compilation phase">;

lib/AST/ASTContext.cpp

@@ -530,7 +530,9 @@ ASTContext::ASTContext(LangOptions &langOpts, TypeCheckerOptions &typeckOpts,
   : LangOpts(langOpts),
     TypeCheckerOpts(typeckOpts),
     SearchPathOpts(SearchPathOpts), SourceMgr(SourceMgr), Diags(Diags),
-    evaluator(Diags, langOpts.DebugDumpCycles),
+    evaluator(Diags,
+              langOpts.DebugDumpCycles,
+              langOpts.BuildRequestDependencyGraph),
     TheBuiltinModule(createBuiltinModule(*this)),
     StdlibModuleName(getIdentifier(STDLIB_NAME)),
     SwiftShimsModuleName(getIdentifier(SWIFT_SHIMS_NAME)),

lib/AST/Evaluator.cpp

@@ -62,8 +62,12 @@ void Evaluator::registerRequestFunctions(
   requestFunctionsByZone.push_back({zoneID, functions});
 }
 
-Evaluator::Evaluator(DiagnosticEngine &diags, bool debugDumpCycles)
-  : diags(diags), debugDumpCycles(debugDumpCycles) { }
+Evaluator::Evaluator(DiagnosticEngine &diags,
+                     bool debugDumpCycles,
+                     bool buildDependencyGraph)
+  : diags(diags),
+    debugDumpCycles(debugDumpCycles),
+    buildDependencyGraph(buildDependencyGraph) { }
 
 void Evaluator::emitRequestEvaluatorGraphViz(llvm::StringRef graphVizPath) {
   std::error_code error;
@@ -72,9 +76,11 @@ void Evaluator::emitRequestEvaluatorGraphViz(llvm::StringRef graphVizPath) {
 }
 
 bool Evaluator::checkDependency(const AnyRequest &request) {
-  // If there is an active request, record it's dependency on this request.
-  if (!activeRequests.empty())
-    dependencies[activeRequests.back()].push_back(request);
+  if (buildDependencyGraph) {
+    // If there is an active request, record it's dependency on this request.
+    if (!activeRequests.empty())
+      dependencies[activeRequests.back()].push_back(request);
+  }
 
   // Record this as an active request.
   if (activeRequests.insert(request)) {

lib/Frontend/CompilerInvocation.cpp

@@ -390,6 +390,9 @@ static bool ParseLangArgs(LangOptions &Opts, ArgList &Args,
   if (Args.getLastArg(OPT_debug_cycles))
     Opts.DebugDumpCycles = true;
 
+  if (Args.getLastArg(OPT_build_request_dependency_graph))
+    Opts.BuildRequestDependencyGraph = true;
+
   if (const Arg *A = Args.getLastArg(OPT_output_request_graphviz)) {
     Opts.RequestEvaluatorGraphVizPath = A->getValue();
   }

test/decl/class/circular_inheritance.swift

@@ -1,7 +1,7 @@
 // RUN: rm -rf %t/stats-dir
 // RUN: mkdir -p %t/stats-dir
 // RUN: %target-typecheck-verify-swift
-// RUN: not %target-swift-frontend -typecheck -debug-cycles %s -output-request-graphviz %t.dot -stats-output-dir %t/stats-dir 2> %t.cycles
+// RUN: not %target-swift-frontend -typecheck -debug-cycles %s -build-request-dependency-graph -output-request-graphviz %t.dot -stats-output-dir %t/stats-dir 2> %t.cycles
 // RUN: %FileCheck %s < %t.cycles
 // RUN: %FileCheck -check-prefix CHECK-DOT %s  < %t.dot
 

unittests/AST/ArithmeticEvaluator.cpp

@@ -210,7 +210,9 @@ TEST(ArithmeticEvaluator, Simple) {
 
   SourceManager sourceMgr;
   DiagnosticEngine diags(sourceMgr);
-  Evaluator evaluator(diags);
+  Evaluator evaluator(diags,
+                      /*debugDumpCycles=*/false,
+                      /*buildDependencyGraph=*/true);
   evaluator.registerRequestFunctions(Zone::ArithmeticEvaluator,
                                      arithmeticRequestFunctions);
 
@@ -333,7 +335,9 @@ TEST(ArithmeticEvaluator, Cycle) {
 
   SourceManager sourceMgr;
   DiagnosticEngine diags(sourceMgr);
-  Evaluator evaluator(diags);
+  Evaluator evaluator(diags,
+                      /*debugDumpCycles=*/false,
+                      /*buildDependencyGraph=*/false);
   evaluator.registerRequestFunctions(Zone::ArithmeticEvaluator,
                                      arithmeticRequestFunctions);