Speed up compilation of ASTImporter

[rnk]

Summary

Avoid recursively instantiating importSeq. Use initializer list
expansion to stamp out a single instantiation of std::tuple of the
deduced sequence of types, and thread the error around that tuple type.
Avoids needlessly instantiating std::tuple N-1 times.

new time to compile: 0m25.985s
old time to compile: 0m35.563s

new obj size: 10,000kb
old obj size: 12,332kb

I found the slow TU by looking at ClangBuildAnalyzer results, and looked
at -ftime-trace for the file in chrome://tracing to find this.

Tested with: clang-cl, MSVC, and GCC.

[rnk]

• remove unintended hack to test MSVC

[merge_guards_bot]

Unit tests: pass. 62309 tests passed, 0 failed and 838 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[rnk]

The Expected<std::tuple<Ts...>> and std::tie<Ts...> instantiations are still expensive, and they seem avoidable. We could use this code pattern instead:

ExpectedType
ASTNodeImporter::VisitVariableArrayType(const VariableArrayType *T) {
  QualType ToElementType = T->getElementType();
  Expr *ToSizeExpr = T->getSizeExpr();
  SourceRange ToBracketsRange = T->getBracketsRange();
  if (Error E = importSeq(ToElementType, ToSizeExpr, ToBracketsRange))
    return E;

  return Importer.getToContext().getVariableArrayType(
      ToElementType, ToSizeExpr, T->getSizeModifier(),
      T->getIndexTypeCVRQualifiers(), ToBracketsRange);
}

Compare to the current pattern:
https://github.com/llvm/llvm-project/blob/master/clang/lib/AST/ASTImporter.cpp#L1176

importSeq would take its parameters by reference. One by one, it would replace them in place, or report an error.

[merge_guards_bot]

Unit tests: fail. 62308 tests passed, 1 failed and 838 were skipped.

failed: libc++.std/thread/futures/futures_async/async.pass.cpp

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[martong]

Thanks for the patch! Looks good to me!

clang/lib/AST/ASTImporter.cpp

192

Thanks for thinking about it. This way we can keep the original behavior i.e. the import is stopped on the first error.

clang/lib/AST/ASTImporter.cpp

class ASTNodeImporter : public TypeVisitor<ASTNodeImporter, ExpectedType>,
     // Import an Optional<T> by importing the contained T, if any.
     template<typename T>
     Expected<Optional<T>> import(Optional<T> From) {
       if (!From)
         return Optional<T>();
       return import(*From);
     }
 
-    template <class T>
-    Expected<std::tuple<T>>
-    importSeq(const T &From) {
-      Expected<T> ToOrErr = import(From);
-      if (!ToOrErr)
-        return ToOrErr.takeError();
-      return std::make_tuple<T>(std::move(*ToOrErr));
+    // Helper for error management in importSeq.
+    template <typename T> T checkImport(Error &Err, const T &From) {
+      // Don't attempt to import nodes if we hit an error earlier.
+      if (Err)
+        return T{};
+      Expected<T> MaybeVal = import(From);
+      if (!MaybeVal) {
+        Err = MaybeVal.takeError();
+        return T{};
+      }
+      return *MaybeVal;
     }
 
     // Import multiple objects with a single function call.
     // This should work for every type for which a variant of `import` exists.
     // The arguments are processed from left to right and import is stopped on
     // first error.
-    template <class THead, class... TTail>
-    Expected<std::tuple<THead, TTail...>>
-    importSeq(const THead &FromHead, const TTail &...FromTail) {
-      Expected<std::tuple<THead>> ToHeadOrErr = importSeq(FromHead);
-      if (!ToHeadOrErr)
-        return ToHeadOrErr.takeError();
-      Expected<std::tuple<TTail...>> ToTailOrErr = importSeq(FromTail...);
-      if (!ToTailOrErr)
-        return ToTailOrErr.takeError();
-      return std::tuple_cat(*ToHeadOrErr, *ToTailOrErr);
+    template <class... Args>
+    Expected<std::tuple<Args...>> importSeq(const Args &... args) {
+      Error E = Error::success();
+      std::tuple<Args...> Res{checkImport(E, args)...};
+      if (E)
+        return std::move(E);
+      return std::move(Res);
     }
 
     // Wrapper for an overload set.
     template <typename ToDeclT> struct CallOverloadedCreateFun {
       template <typename... Args>
       auto operator()(Args &&... args)
           -> decltype(ToDeclT::Create(std::forward<Args>(args)...)) {
         return ToDeclT::Create(std::forward<Args>(args)...);
       }
     };