Optimize StringTableBuilder

[ruiu]

Summary

This is a patch to improve StringTableBuilder's performance. That class'
finalize function is very hot particularly in LLD because the function
does tail-merge strings for string tables or SHF_MERGE sections.

Generic std::sort-style sorter is not efficient for sorting strings.
The function implemented in this patch seems to be more efficient.

Here's a benchmark of LLD to link Clang with or without this patch.
The numbers are medians of 50 runs.

-O0
real 0m0.455s
real 0m0.430s (5.5% faster)

-O3
real 0m0.487s
real 0m0.452s (7.2% faster)

[• rafael]

-O0
real 0m0.455s
real 0m0.430s (5.5% faster)

-O3
real 0m0.487s
real 0m0.452s (7.2% faster)

Nice!

+static void qsort(StringPair Begin, StringPair End, int Pos) {
+ if (End - Begin < 2)
+ return;
+
+ // Safeguard for pathetic input.
+ if (Pos > 10000)
+ return;

We get here if two strings have a lot of characters in common, right?
This if then avoids running out of stack.

+ Partition items. Items in [Begin, P) are less than the pivot, [P, Q)
+ are the same as the pivot, and [Q, End) are greater than the pivot.

We are actually sorting the other way, no? Things larger than the
pivot go to be start of the array.

+ int Pivot = charTailAt(*Begin, Pos);
+ StringPair P = Begin;
+ StringPair Q = End;
+ for (StringPair **R = Begin + 1; R < Q;) {
+ int C = charTailAt(*R, Pos);
+ if (C > Pivot)
+ std::swap(*P++, *R++);
+ else if (C < Pivot)
+ std::swap(*--Q, *R);
+ else
+ R++;

}

• return SizeB - SizeA;

+ qsort(Begin, P, Pos);
+ if (Pivot != -1)
+ qsort(P, Q, Pos + 1);

This is the recursive call that can causes us to run out of stack. If
you write this as

qsort(Begin, P, Pos);
qsort(Q, End, Pos);
if (Pivot != -1)
loop back to stort P, Q, Pos + 1.

We should be able to remove the earlier check for Pos being too large.

Thanks you so much for ding this, it should help MC too!

Cheers,
Rafael

[ruiu]

Address review comments.

[grosbach]

Cool. AFAIK, the only reason for the array_pod_sort() before was "seems good enough for now."

lib/MC/StringTableBuilder.cpp

33

Can we use a different name? Function names that collide with the C standard library are bountiful opportunities for confusion later.

Relatedly, perhaps this helper function, or a generalized version of it, should go into STLExtras.h next to array_pod_sort?

lib/MC/StringTableBuilder.cpp

 #include "llvm/Support/Endian.h"
 
 #include <vector>
 
 using namespace llvm;
 
 StringTableBuilder::StringTableBuilder(Kind K) : K(K) {}
 
-static int compareBySuffix(std::pair<StringRef, size_t> *const *AP,
-                           std::pair<StringRef, size_t> *const *BP) {
-  StringRef A = (*AP)->first;
-  StringRef B = (*BP)->first;
-  size_t SizeA = A.size();
-  size_t SizeB = B.size();
-  size_t Len = std::min(SizeA, SizeB);
-  for (size_t I = 0; I < Len; ++I) {
-    char CA = A[SizeA - I - 1];
-    char CB = B[SizeB - I - 1];
-    if (CA != CB)
-      return CB - CA;
+typedef std::pair<StringRef, size_t> StringPair;
+
+// Returns the character at Pos from end of a string.
+static int charTailAt(StringPair *P, size_t Pos) {
+  StringRef S = P->first;
+  if (Pos >= S.size())
+    return -1;
+  return (unsigned char)S[S.size() - Pos - 1];
+}
+
+// Three-way radix quicksort. This is much faster than std::sort with strcmp
+// because it does not compare characters that we already know the same.
+static void qsort(StringPair **Begin, StringPair **End, int Pos) {
+tailcall:
+  if (End - Begin < 2)
+    return;
+
+  // Partition items. Items in [Begin, P) are greater than the pivot,
+  // [P, Q) are the same as the pivot, and [Q, End) are less than the pivot.
+  int Pivot = charTailAt(*Begin, Pos);
+  StringPair **P = Begin;
+  StringPair **Q = End;
+  for (StringPair **R = Begin + 1; R < Q;) {
+    int C = charTailAt(*R, Pos);
+    if (C > Pivot)
+      std::swap(*P++, *R++);
+    else if (C < Pivot)
+      std::swap(*--Q, *R);
+    else
+      R++;
+  }
+
+  qsort(Begin, P, Pos);
+  qsort(Q, End, Pos);
+  if (Pivot != -1) {
+    // qsort(P, Q, Pos + 1), but with tail call optimization.
+    Begin = P;
+    End = Q;
+    ++Pos;
+    goto tailcall;
   }
-  return SizeB - SizeA;
 }
 
 void StringTableBuilder::finalize() {
   std::vector<std::pair<StringRef, size_t> *> Strings;
   Strings.reserve(StringIndexMap.size());
   for (std::pair<StringRef, size_t> &P : StringIndexMap)
     Strings.push_back(&P);
 
-  array_pod_sort(Strings.begin(), Strings.end(), compareBySuffix);
+  if (!Strings.empty())
+    qsort(&Strings[0], &Strings[0] + Strings.size(), 0);
 
   switch (K) {
   case RAW:
     break;
   case ELF:
   case MachO:
     // Start the table with a NUL byte.
     StringTable += '\x00';