dyno: changes to improve --dyno-scope-bundled performance (chapel-l…

…ang#25090)

This PR makes a number of changes that serve to improve the performance
of `chpl` under `--dyno-scope-bundled.` They are as follows:

* Disable query tracing. This is currently done system-wide while this
PR is a draft, but a merge-ready version of this PR should have some
kind of build switch or configuration to disable query tracing when a
release build is being created.
* Deliberately leak the memory associated with the Dyno context. This
one might be a bit controversial. The Dyno context's destruct -- thanks
to all the memoized queries -- takes 0.1s to run. We only ever `delete`
it on `clean_exit`, which means that the OS is about to reclaim memory
anyway.
* Reduce usage of `parentAst` in identifier lookups (specifically, the
logic that checks for, and silences, warnings about nodes-in-calls
(which are handled by call resolution). We already track if we're in the
called expression of a call, so we can re-use that. This saves the --
costly -- `parentAst` lookups.
* Adjust logic in `validateAndSetToId` to use 'ID::parentSymbolId`
instead of the `idToParentId` query; the former does not require any AST
knowledge, which also reduces the reliance on `idToAst`. It also helps
skip some intermediate (non-scope) IDs while traversing.
* Slightly re-order the checks in `astToAttributeGroup` to avoid as many
`parentAst` lookups as possible.
* Remove the logic that looks up `super` in scopes, since `super` is a
reserved keyword with special meaning.
* In certain eligible conditions, skip checking scopes in favor of
returning a ("found") ID() when looking up a well-known reserved name.
E.g., looking up `int` cannot result in matches anywhere but the global
scope (since `int` is a reserved keyword), so there's no reason to look
it up in user modules.
* Rewrite the computation for transitively finding method receivers from
inheritance chains to be recursive and re-use previous results.

The net result is that the following invocation:

```
chpl --no-compiler-driver --dyno-scope-bundled examples/hello.chpl --stop-after-pass=parseAndConvertUast 
```

Went from 1.3 seconds to 0.9 seconds on my local machine. This is a step
forward, but is far from the 0.4s of the `--no-dyno-scope-resolve
--stop-after-pass=scopeResolve` version of this command.

## Other notes
* Unsurprisingly, most of the time (900ms / 1.3s of the original
execution) is spent in `doLookupInScope`
* From there, `scopeResolveFunction` takes 75% of the total execution
time of `parseAndConvertUast`
* According to query timings, the `resolveVisibilityStmtsQuery` is still
among the slowest self-queries we have (i.e., excluding child execution
times). This might be unsurprising since it runs the non-query
`doLookupInScope`.
* Noticed some minor overheads from constructing QueryMapResults. Tried
switching them to use small pointer sets and small vectors, but that
hurt instead of helping.

At this point, it seems like we might need some sort of lookup cache,
since identifiers are looked up within modules many times. This is
complicated by the fact that instead of "checked module for X", we have
"checked module for X given the following filter flags", which makes it
harder to detect when we can be sure that a symbol doesn't exist in a
scope.

Reviewed by @mppf -- thanks!

## Testing
- [x] paratest

compiler/include/driver.h

@@ -169,6 +169,7 @@ extern bool fDriverCompilationPhase;
 extern bool fDriverMakeBinaryPhase;
 extern char driverTmpDir[FILENAME_MAX];
 // end compiler driver control flags
+extern bool fExitLeaks;
 extern bool fPrintAllCandidates;
 extern bool fPrintCallGraph;
 extern bool fPrintCallStackOnError;
@@ -322,6 +323,7 @@ extern bool fDynoScopeResolve;
 extern bool fDynoScopeProduction;
 extern bool fDynoScopeBundled;
 extern bool fDynoDebugTrace;
+extern bool fDynoDebugPrintParsedFiles;
 extern bool fDynoVerifySerialization;
 extern bool fDynoGenLib;
 extern bool fDynoGenStdLib;

compiler/main/driver.cpp

@@ -151,6 +151,7 @@ bool fDriverDoMonolithic = false;
 bool driverDebugPhaseSpecified = false;
 // Tmp dir path managed by compiler driver
 char driverTmpDir[FILENAME_MAX] = "";
+bool fExitLeaks = true;
 bool fLibraryCompile = false;
 bool fLibraryFortran = false;
 bool fLibraryMakefile = false;
@@ -375,6 +376,7 @@ bool fDynoScopeResolve = true;
 bool fDynoScopeProduction = true;
 bool fDynoScopeBundled = false;
 bool fDynoDebugTrace = false;
+bool fDynoDebugPrintParsedFiles = false;
 bool fDynoVerifySerialization = false;
 bool fDynoGenLib = false;
 bool fDynoGenStdLib = false;
@@ -1439,6 +1441,7 @@ static ArgumentDescription arg_desc[] = {
  {"driver-compilation-phase", ' ', NULL, "Run driver compilation phase (internal use flag)", "F", &fDriverCompilationPhase, NULL, setSubInvocation},
  {"driver-makebinary-phase", ' ', NULL, "Run driver makeBinary phase (internal use flag)", "F", &fDriverMakeBinaryPhase, NULL, setSubInvocation},
  {"driver-debug-phase", ' ', "<phase>", "Specify driver phase to run when debugging: compilation, makeBinary, all", "S", NULL, NULL, setDriverDebugPhase},
+ {"exit-leaks", ' ', NULL, "[Don't] leak memory on exit", "N", &fExitLeaks, NULL, NULL},
  {"gdb", ' ', NULL, "Run compiler in gdb", "F", &fRungdb, NULL, NULL},
  {"lldb", ' ', NULL, "Run compiler in lldb", "F", &fRunlldb, NULL, NULL},
  {"interprocedural-alias-analysis", ' ', NULL, "Enable [disable] interprocedural alias analysis", "n", &fNoInterproceduralAliasAnalysis, NULL, NULL},
@@ -1508,6 +1511,7 @@ static ArgumentDescription arg_desc[] = {
  {"dyno-scope-production", ' ', NULL, "Enable [disable] using both dyno and production scope resolution", "N", &fDynoScopeProduction, "CHPL_DYNO_SCOPE_PRODUCTION", NULL},
  {"dyno-scope-bundled", ' ', NULL, "Enable [disable] using dyno to scope resolve bundled modules", "N", &fDynoScopeBundled, "CHPL_DYNO_SCOPE_BUNDLED", NULL},
  {"dyno-debug-trace", ' ', NULL, "Enable [disable] debug-trace output when using dyno compiler library", "N", &fDynoDebugTrace, "CHPL_DYNO_DEBUG_TRACE", NULL},
+ {"dyno-debug-print-parsed-files", ' ', NULL, "Enable [disable] printing all files that were parsed by Dyno", "N", &fDynoDebugPrintParsedFiles, "CHPL_DYNO_DEBUG_PRINT_PARSED_FILES", NULL},
  {"dyno-break-on-hash", ' ' , NULL, "Break when query with given hash value is executed when using dyno compiler library", "X", &fDynoBreakOnHash, "CHPL_DYNO_BREAK_ON_HASH", NULL},
  {"dyno-gen-lib", ' ', "<path>", "Specify files named on the command line should be saved into a .dyno library", "P", NULL, NULL, addDynoGenLib},
  {"dyno-gen-std", ' ', NULL, "Generate a .dyno library file for the standard library", "F", &fDynoGenStdLib, NULL, setDynoGenStdLib},

compiler/passes/parseAndConvert.cpp

@@ -1439,5 +1439,15 @@ void parseAndConvertUast() {
   // Revert to using the default error handler now.
   gContext->installErrorHandler(nullptr);
 
+  if (fDynoDebugPrintParsedFiles) {
+    std::set<std::string> files;
+    auto filesUstr = chpl::parsing::introspectParsedFiles(gContext);
+    for (auto ustr : filesUstr) {
+      files.insert(ustr.str());
+    }
+    for (auto file : files) {
+      fprintf(stderr, "Parsed file: %s\n", file.c_str());
+    }
+  }
   parsed = true;
 }

compiler/util/misc.cpp

@@ -1117,7 +1117,12 @@ void clean_exit(int status) {
 
   cleanup_for_exit();
 
-  delete gContext;
+  if (fExitLeaks) {
+    // The context's destructor takes a while, and we're about to exit anyway,
+    // so deliberately leak it.
+  } else {
+    delete gContext;
+  }
   gContext = nullptr;
 
   if (gGenInfo) {

frontend/include/chpl/framework/query-impl.h

@@ -27,8 +27,14 @@
 #include "chpl/framework/stringify-functions.h"
 
 #ifndef CHPL_QUERY_TIMING_AND_TRACE_ENABLED
+#ifdef NDEBUG
+// release mode
+#define CHPL_QUERY_TIMING_AND_TRACE_ENABLED 0
+#else
+// debug mode
 #define CHPL_QUERY_TIMING_AND_TRACE_ENABLED 1
 #endif
+#endif
 
 /**
   This file should be included by .cpp files implementing queries.
@@ -617,7 +623,7 @@ Context::querySetterUpdateResult(
 
 #else
 
-#define QUERY_BEGIN_TIMING()
+#define QUERY_BEGIN_TIMING(context__)
 
 #endif
 

frontend/include/chpl/resolution/scope-queries.h

@@ -168,9 +168,12 @@ namespace resolution {
 
   /**
     Resolve the uses and imports in a given scope.
+
+    If 'skipPrivate' is set, avoids resolving visibility statements that
+    only expose scope-private symbols. This helps avoid unnecessary work.
   */
   const ResolvedVisibilityScope*
-  resolveVisibilityStmts(Context* context, const Scope* scope);
+  resolveVisibilityStmts(Context* context, const Scope* scope, bool skipPrivate = false);
 
   /**
     Return the scope for the automatically included 'ChapelStandard' module,

frontend/lib/parsing/parsing-queries.cpp

@@ -1439,9 +1439,16 @@ const uast::AttributeGroup*
 astToAttributeGroup(Context* context, const uast::AstNode* ast) {
   const uast::AttributeGroup* ret = nullptr;
   if (ast) {
+    // If we find an attribute group on the AST, return it.
+    if (auto ag = ast->attributeGroup()) return ag;
+
+    // Right now, only Variables and TupleDecls can inherit attributes
+    // from enclosing MultiDecls or TupleDecls.
+    if (!ast->isVariable() && !ast->isTupleDecl()) return nullptr;
+
+    // handle nesting: what if we're a Variable inside a MultiDecl or TupleDecl?
     auto parent = parentAst(context, ast);
-    bool done = ast->isMultiDecl() || !parent ||
-                (!parent->isTupleDecl() && !parent->isMultiDecl());
+    bool done = !parent || (!parent->isTupleDecl() && !parent->isMultiDecl());
     // recurse if not done
     return done
            ? ast->attributeGroup()

frontend/lib/resolution/Resolver.cpp

@@ -512,60 +512,60 @@ gatherParentClassScopesForScopeResolving(Context* context, ID classDeclId) {
 
   std::vector<const Scope*> result;
 
-  ID curId = classDeclId;
-
   bool encounteredError = false;
-  while (!curId.isEmpty() && !encounteredError) {
-    auto ast = parsing::idToAst(context, curId);
-    if (!ast) break;
-
-    auto c = ast->toClass();
-    if (!c || c->numInheritExprs() == 0) break;
-
-    const uast::AstNode* lastParentClass = nullptr;
-    for (auto inheritExpr : c->inheritExprs()) {
-      // Resolve the parent class type expression
-      ResolutionResultByPostorderID r;
-      auto visitor =
-        Resolver::createForParentClassScopeResolve(context, c, r);
-      // Parsing excludes non-identifiers as parent class expressions.
-      //
-      // Intended to avoid calling methodReceiverScopes() recursively.
-      // Uses the empty 'savecReceiverScopes' because the class expression
-      // can't be a method anyways.
-      bool ignoredMarkedGeneric = false;
-      auto ident = Class::getInheritExprIdent(inheritExpr,
-                                              ignoredMarkedGeneric);
-      visitor.resolveIdentifier(ident, visitor.savedReceiverScopes);
-
-
-      ResolvedExpression& re = r.byAst(ident);
-      if (re.toId().isEmpty()) {
-        context->error(inheritExpr, "invalid parent class expression");
+  auto ast = parsing::idToAst(context, classDeclId);
+  if (!ast) return QUERY_END(result);
+
+  auto c = ast->toClass();
+  if (!c || c->numInheritExprs() == 0) return QUERY_END(result);
+
+  const uast::AstNode* lastParentClass = nullptr;
+  ID parentClassDeclId;
+  for (auto inheritExpr : c->inheritExprs()) {
+    // Resolve the parent class type expression
+    ResolutionResultByPostorderID r;
+    auto visitor =
+      Resolver::createForParentClassScopeResolve(context, c, r);
+    // Parsing excludes non-identifiers as parent class expressions.
+    //
+    // Intended to avoid calling methodReceiverScopes() recursively.
+    // Uses the empty 'savecReceiverScopes' because the class expression
+    // can't be a method anyways.
+    bool ignoredMarkedGeneric = false;
+    auto ident = Class::getInheritExprIdent(inheritExpr,
+                                            ignoredMarkedGeneric);
+    visitor.resolveIdentifier(ident, visitor.savedReceiverScopes);
+
+
+    ResolvedExpression& re = r.byAst(ident);
+    if (re.toId().isEmpty()) {
+      context->error(inheritExpr, "invalid parent class expression");
+      encounteredError = true;
+      break;
+    } else if (parsing::idToTag(context, re.toId()) == uast::asttags::Interface) {
+      // this is an interface; ignore it for the purposes of parent scopes.
+    } else {
+      if (lastParentClass) {
+        reportInvalidMultipleInheritance(context, c, lastParentClass, inheritExpr);
         encounteredError = true;
         break;
-      } else if (parsing::idToTag(context, re.toId()) == uast::asttags::Interface) {
-        // this is an interface; ignore it for the purposes of parent scopes.
-      } else {
-        if (lastParentClass) {
-          reportInvalidMultipleInheritance(context, c, lastParentClass, inheritExpr);
-          encounteredError = true;
-          break;
-        }
-        lastParentClass = inheritExpr;
-
-        result.push_back(scopeForId(context, re.toId()));
-        curId = re.toId();
-        // keep going through the list of parent expressions. hitting
-        // another parent expression that's a class after this point
-        // will result in an error. When we're done with other parent
-        // expressions, the loop will continue to searching for the
-        // parent classes of this parent class.
       }
+      lastParentClass = inheritExpr;
+
+      result.push_back(scopeForId(context, re.toId()));
+      parentClassDeclId = re.toId();
+      // keep going through the list of parent expressions. hitting
+      // another parent expression that's a class after this point
+      // will result in an error. When we're done with other parent
+      // expressions, the loop will continue to searching for the
+      // parent classes of this parent class.
     }
+  }
 
-    // only interfaces found, no need to look for more parents.
-    if (!lastParentClass) break;
+  if (!encounteredError && !parentClassDeclId.isEmpty()) {
+    const auto& parentScopes =
+      gatherParentClassScopesForScopeResolving(context, parentClassDeclId);
+    result.insert(result.end(), parentScopes.begin(), parentScopes.end());
   }
 
   return QUERY_END(result);
@@ -2666,6 +2666,13 @@ Resolver::lookupIdentifier(const Identifier* ident,
   const Scope* scope = scopeStack.back();
   outParenlessOverloadInfo = ParenlessOverloadInfo();
 
+  if (ident->name() == USTR("super")) {
+    // Super is a keyword, and should't be looked up in scopes. Return
+    // an empty ID to indicate that this identifier points to something,
+    // but that something has a special meaning.
+    return { BorrowedIdsWithName::createWithBuiltinId() };
+  }
+
   bool resolvingCalledIdent = nearestCalledExpression() == ident;
 
   LookupConfig config = IDENTIFIER_LOOKUP_CONFIG;
@@ -2696,13 +2703,7 @@ Resolver::lookupIdentifier(const Identifier* ident,
   // and probably a few other features.
   if (!scopeResolveOnly) {
     if (notFound) {
-      // If this identifier is 'super' and we couldn't find something it refers
-      // to, it could stand for 'this.super'. But in that case, no need to
-      // issue an error.
-      if (isPotentialSuper(ident)) {
-        // We found a single ID, and it's just 'super'.
-        return { BorrowedIdsWithName::createWithBuiltinId() };
-      } else if (!resolvingCalledIdent) {
+      if (!resolvingCalledIdent) {
         auto pair = namesWithErrorsEmitted.insert(ident->name());
         if (pair.second) {
           // insertion took place so emit the error
@@ -2722,13 +2723,13 @@ Resolver::lookupIdentifier(const Identifier* ident,
 
 void Resolver::validateAndSetToId(ResolvedExpression& r,
                                   const AstNode* node,
-                                  const ID& id) {
-  r.setToId(id);
-  if (id.isEmpty()) return;
-  if (id.isFabricatedId()) return;
+                                  const ID& toId) {
+  r.setToId(toId);
+  if (toId.isEmpty()) return;
+  if (toId.isFabricatedId()) return;
 
   // Validate the newly set to ID.
-  auto idTag = parsing::idToTag(context, id);
+  auto idTag = parsing::idToTag(context, toId);
 
   // It shouldn't refer to a module unless the node is an identifier in one of
   // the places where module references are allowed (e.g. imports).
@@ -2741,7 +2742,7 @@ void Resolver::validateAndSetToId(ResolvedExpression& r,
           asttags::isDot(parentTag)) {
         // OK
       } else {
-        auto toAst = parsing::idToAst(context, id);
+        auto toAst = parsing::idToAst(context, toId);
         auto mod = toAst->toModule();
         auto parentAst = parsing::idToAst(context, parentId);
         CHPL_REPORT(context, ModuleAsVariable, node, parentAst, mod);
@@ -2750,33 +2751,32 @@ void Resolver::validateAndSetToId(ResolvedExpression& r,
   }
 
   // If we're in a nested class, it shouldn't refer to an outer class' field.
-  auto scope = scopeForId(context, id);
-  auto parentId = scope->id();
+  auto parentId =
+    Builder::astTagIndicatesNewIdScope(idTag) ? toId : toId.parentSymbolId(context);
   auto parentTag = parsing::idToTag(context, parentId);
   if (asttags::isAggregateDecl(parentTag) &&
       parentId.contains(node->id()) &&
-      parentId != id /* It's okay to refer to the record itself */) {
+      parentId != toId /* It's okay to refer to the record itself */) {
     // Referring to a field of a class that's surrounding the current node.
     // Loop upwards looking for a composite type.
-    auto searchId = parsing::idToParentId(context, node->id());
+    auto searchId = node->id().parentSymbolId(context);
     while (!searchId.isEmpty()) {
-      auto searchTag = parsing::idToTag(context, searchId);
       if (searchId == parentId) {
         // We found the aggregate type in which the to-ID is declared,
         // so there's no nested class issues.
         break;
-      } else if (asttags::isAggregateDecl(searchTag)) {
+      } else if (asttags::isAggregateDecl(parsing::idToTag(context, searchId))) {
         auto parentAst = parsing::idToAst(context, parentId);
         auto searchAst = parsing::idToAst(context, searchId);
         auto searchAD = searchAst->toAggregateDecl();
         // It's an error!
         CHPL_REPORT(context, NestedClassFieldRef, parentAst->toAggregateDecl(),
-                    searchAD, node, id);
+                    searchAD, node, toId);
         break;
       }
 
       // Move on to the surrounding ID.
-      searchId = parsing::idToParentId(context, searchId);
+      searchId = searchId.parentSymbolId(context);
     }
   }
 }
@@ -2799,19 +2799,6 @@ void Resolver::validateAndSetMostSpecific(ResolvedExpression& r,
   r.setMostSpecific(mostSpecific);
 }
 
-static bool isCalledExpression(Resolver* rv, const AstNode* ast) {
-  if (!ast) return false;
-
-  auto p = parsing::parentAst(rv->context, ast);
-  if (!p) return false;
-
-  if (auto call = p->toCall())
-    if (auto ce = call->calledExpression())
-      return ce == ast;
-
-  return false;
-}
-
 static void maybeEmitWarningsForId(Resolver* rv, QualifiedType qt,
                                    const AstNode* astMention,
                                    ID idTarget) {
@@ -2833,7 +2820,11 @@ static void maybeEmitWarningsForId(Resolver* rv, QualifiedType qt,
   if (qt.kind() == QualifiedType::PARENLESS_FUNCTION) return;
 
   bool emitUnstableAndDeprecationWarnings = true;
-  if (isCalledExpression(rv, astMention)) {
+  if (rv->nearestCalledExpression() == astMention) {
+    // Note: the above conditional assumes that the astMention is
+    // currently the thing being traversed (which is what makes
+    // nearestCalledExpression() sufficient).
+    //
     // For functions, do not warn, since call resolution will take
     // care of that.  However, if we're referring to other symbol
     // kinds, we know right now that a deprecation warning should be