[Support] Make llvm::Error faster

[zturner]

Summary

Whenever LLVM_ENABLE_ABI_BREAKING_CHECKS is enabled, which
is usually the case for example when asserts are enabled,
Error's destructor does some additional checking to make sure
that that it does not represent an error condition and that it
was checked.

However, this is -- by definition -- not the likely codepath.
Some profiling shows that at least with some compilers, simply
calling assertIsChecked -- in a release build with full
optimizations -- can account for up to 15% of the entire
runtime of the program, even though this function should almost
literally be a no-op.

The problem is that the assertIsChecked function can be considered
too big to inline depending on the compiler's inliner.  Since it's
unlikely to ever need to failure path though, we can move it out
of line and force it to not be inlined, so that the fast path
can be inlined.

In my test (using lld to link clang with CMAKE_BUILD_TYPE=Release
and LLVM_ENABLE_ASSERTIONS=ON), this reduces link time from 27
seconds to 23.5 seconds, which is a solid 15% gain.

[ruiu]

Nice. LGTM

llvm/include/llvm/Support/Error.h

284–285

I believe clang-format-diff would format it in one line.

[hans]

llvm/include/llvm/Support/Error.h

269

Would it be better to put this in the .cpp file instead? Since it's not supposed to be inlined, there's no point in declaring it inline, I think.

[hans]

llvm/include/llvm/Support/Error.h

269

Oh, and it's probably worth putting LLVM_ATTRIBUTE_NORETURN on it.

[lhames]

Nice!

Aside: getChecked() should be a no-op in release builds, but we do still check the error pointer for equivalence with uncaught exceptions. (I'm open to revisiting that idea: we also have better error checking validation than exceptions provide).

So does the 15% speed-up mean that you're now spending ~0% in Error destructors?

[zturner]

I went ahead and applied the same optimization for llvm::Expected, and also incorporated other suggestions. Even though Expected wasn't showing up on the profile, this only arose because we happened to be a heavy user of Error. If someone else happened to be a heavy user of Expected, we'd see the same issues arise.

To answer @lhames question about the Error destructor, It's hard for me to check the total time in Error's destructor, or at least I don't know how to do it with this particular profiler. I can either get exclusive samples, in which case the number won't account for nested calls inside of the destructor, or I can get inclusive samples, in which case I'll need to find every callsite of an Error destructor. I'm sure there's a way, I just don't know this profiler tht well.

What I can say though is that before the patch, on one particular run lld-link had 23,271 total samples, 2,017 samples of which were in assertUnchecked. After the patch, lld-link had 25,493 total samples, 0 of which were in assertUnchecked (i.e. it wasn't even on the profile).

[ruiu]

llvm/include/llvm/Support/Error.h

640

Maybe you should move this to a .cpp file as well?

[hans]

lgtm

Maybe mention llvm::Expected in the commit message too.

[hans]

r317824

llvm/include/llvm/Support/Error.h

public:
   /// value.
   const void* dynamicClassID() const {
     if (!getPtr())
       return nullptr;
     return getPtr()->dynamicClassID();
   }
 
 private:
+#if LLVM_ENABLE_ABI_BREAKING_CHECKS
+  // assertIsChecked() happens very frequently, but under normal circumstances
+  // is supposed to be a no-op.  So we want it to be inlined, but having a bunch
+  // of debug prints can cause the function to be too large for inlining.  So
+  // it's important that we define this function out of line so that it can't be
+  // inlined.
+  LLVM_ATTRIBUTE_NORETURN
+  void fatalUncheckedError() const;
+#endif
+
   void assertIsChecked() {
 #if LLVM_ENABLE_ABI_BREAKING_CHECKS
-    if (!getChecked() || getPtr()) {
-      dbgs() << "Program aborted due to an unhandled Error:\n";
-      if (getPtr())
-        getPtr()->log(dbgs());
-      else
-        dbgs()
-            << "Error value was Success. (Note: Success values must still be "
-               "checked prior to being destroyed).\n";
-      abort();
-    }
+    if (LLVM_UNLIKELY(!getChecked() || getPtr()))
+      fatalUncheckedError();
 #endif
   }
 
   ErrorInfoBase *getPtr() const {
     return reinterpret_cast<ErrorInfoBase*>(
              reinterpret_cast<uintptr_t>(Payload) &
              ~static_cast<uintptr_t>(0x1));
   }
 
   void setPtr(ErrorInfoBase *EI) {
 #if LLVM_ENABLE_ABI_BREAKING_CHECKS
     Payload = reinterpret_cast<ErrorInfoBase*>(
                 (reinterpret_cast<uintptr_t>(EI) &
                  ~static_cast<uintptr_t>(0x1)) |
                 (reinterpret_cast<uintptr_t>(Payload) & 0x1));
 #else
     Payload = EI;
 #endif
   }
 
   bool getChecked() const {
 #if LLVM_ENABLE_ABI_BREAKING_CHECKS
     return (reinterpret_cast<uintptr_t>(Payload) & 0x1) == 0;
 #else
     return true;
 #endif
   }
 
   void setChecked(bool V) {
     Payload = reinterpret_cast<ErrorInfoBase*>(
                 (reinterpret_cast<uintptr_t>(Payload) &
#endif
 
   // Used by ExpectedAsOutParameter to reset the checked flag.
   void setUnchecked() {
 #if LLVM_ENABLE_ABI_BREAKING_CHECKS
     Unchecked = true;
 #endif
   }
 
-  void assertIsChecked() {
 #if LLVM_ENABLE_ABI_BREAKING_CHECKS
-    if (Unchecked) {
-      dbgs() << "Expected<T> must be checked before access or destruction.\n";
-      if (HasError) {
-        dbgs() << "Unchecked Expected<T> contained error:\n";
-        (*getErrorStorage())->log(dbgs());
-      } else
-        dbgs() << "Expected<T> value was in success state. (Note: Expected<T> "
-                  "values in success mode must still be checked prior to being "
-                  "destroyed).\n";
-      abort();
-    }
+  LLVM_ATTRIBUTE_NORETURN
+  LLVM_ATTRIBUTE_NOINLINE
+  void fatalUncheckedExpected() const {
 #endif
+
+  void assertIsChecked() {
+#if LLVM_ENABLE_ABI_BREAKING_CHECKS
+    if (LLVM_UNLIKELY(Unchecked))
+      fatalUncheckedExpected();
+#endif
   }
 
   union {
     AlignedCharArrayUnion<storage_type> TStorage;
     AlignedCharArrayUnion<error_type> ErrorStorage;
   };
   bool HasError : 1;
 #if LLVM_ENABLE_ABI_BREAKING_CHECKS

llvm/lib/Support/Error.cpp

std::error_code errorToErrorCode(Error Err) {
   handleAllErrors(std::move(Err), [&](const ErrorInfoBase &EI) {
     EC = EI.convertToErrorCode();
   });
   if (EC == inconvertibleErrorCode())
     report_fatal_error(EC.message());
   return EC;
 }
 
+#if LLVM_ENABLE_ABI_BREAKING_CHECKS
+void Error::fatalUncheckedError() const {
+  dbgs() << "Program aborted due to an unhandled Error:\n";
+  if (getPtr())
+    getPtr()->log(dbgs());
+  else
+    dbgs() << "Error value was Success. (Note: Success values must still be "
+              "checked prior to being destroyed).\n";
+  abort();
+}
+#endif
+
 StringError::StringError(const Twine &S, std::error_code EC)
     : Msg(S.str()), EC(EC) {}
 
 void StringError::log(raw_ostream &OS) const { OS << Msg; }
 
 std::error_code StringError::convertToErrorCode() const {
   return EC;
 }