[PATCH] D39761: [Support] Make llvm::Error faster

[Zachary Turner via Phabricator via llvm-commits]

zturner updated this revision to Diff 122020.
zturner added a comment.
Herald added a subscriber: hiraditya.

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).


https://reviews.llvm.org/D39761

Files:
  llvm/include/llvm/Support/Error.h
  llvm/lib/Support/Error.cpp


Index: llvm/lib/Support/Error.cpp
===================================================================
--- llvm/lib/Support/Error.cpp
+++ llvm/lib/Support/Error.cpp
@@ -91,6 +91,18 @@
   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) {}
 
Index: llvm/include/llvm/Support/Error.h
===================================================================
--- llvm/include/llvm/Support/Error.h
+++ llvm/include/llvm/Support/Error.h
@@ -246,18 +246,20 @@
   }
 
 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
   }
 
@@ -632,19 +634,26 @@
 #endif
   }
 
+#if LLVM_ENABLE_ABI_BREAKING_CHECKS
+  LLVM_ATTRIBUTE_NORETURN
+  LLVM_ATTRIBUTE_NOINLINE
+  void fatalUncheckedExpected() const {
+    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();
+  }
+#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();
-    }
+    if (LLVM_UNLIKELY(Unchecked))
+      fatalUncheckedExpected();
 #endif
   }
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D39761.122020.patch
Type: text/x-patch
Size: 3048 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20171108/ed9b8d8b/attachment.bin>