[SpecialCaseList] Remove TrigramIndex

ellishg · ellishg · commit 139067554276 · 2023-06-19T10:41:45.000-07:00
`TrigramIndex` was added back in https://reviews.llvm.org/D27188 as an optimization to make `SpecialCaseList::match()` faster. I've found that `TrigramIndex` actually makes the function slower and it has no functional use, so we can remove it. I grabbed the list of queries passed to `SpecialCaseList::match()` on a random very large file (`AArch64ISelLowering.cpp`) and measured the runtime to call `match()` on all of them with [this line](https://github.com/llvm/llvm-project/blob/8e1f820bb4eadf5c0704818f6063e0db1006e32d/llvm/lib/Support/SpecialCaseList.cpp#L64) disabled and then enabled. ``` $ hyperfine --warmup 3 'GTEST_FILTER="SpecialCaseListTest.Large" USE_TRIGRAMS=1 build/unittests/Support/SupportTests' 'GTEST_FILTER="SpecialCaseListTest.Large" USE_TRIGRAMS=0 build/unittests/Support/SupportTests' Benchmark 1: GTEST_FILTER="SpecialCaseListTest.Large" USE_TRIGRAMS=1 build/unittests/Support/SupportTests Time (mean ± σ): 575.9 ms ± 20.3 ms [User: 573.1 ms, System: 2.7 ms] Range (min … max): 555.5 ms … 620.0 ms 10 runs Benchmark 2: GTEST_FILTER="SpecialCaseListTest.Large" USE_TRIGRAMS=0 build/unittests/Support/SupportTests Time (mean ± σ): 283.4 ms ± 6.7 ms [User: 280.3 ms, System: 3.0 ms] Range (min … max): 277.0 ms … 294.9 ms 10 runs Summary 'GTEST_FILTER="SpecialCaseListTest.Large" USE_TRIGRAMS=0 build/unittests/Support/SupportTests' ran 2.03 ± 0.09 times faster than 'GTEST_FILTER="SpecialCaseListTest.Large" USE_TRIGRAMS=1 build/unittests/Support/SupportTests' ``` Using `perf` I found that most of the runtime in `TrigramIndex::isDefinitelyOut()` comes from a division operation that seems to come from `std::unordered_map`: https://github.com/llvm/llvm-project/blob/8e1f820bb4eadf5c0704818f6063e0db1006e32d/llvm/include/llvm/Support/TrigramIndex.h#L62 Removing `TrigramIndex` will make it easier to potentially switch to using `GlobPattern` instead of a full regex for `SpecialCaseList`. See discussion in https://reviews.llvm.org/D152762 for details. Reviewed By: MaskRay, #sanitizers, vitalybuka Differential Revision: https://reviews.llvm.org/D153171
diff --git a/llvm/include/llvm/Support/SpecialCaseList.h b/llvm/include/llvm/Support/SpecialCaseList.h
@@ -54,7 +54,6 @@
 
 #include "llvm/ADT/StringMap.h"
 #include "llvm/Support/Regex.h"
-#include "llvm/Support/TrigramIndex.h"
 #include <memory>
 #include <string>
 #include <vector>
@@ -128,7 +127,6 @@ class SpecialCaseList {
 
   private:
     StringMap<unsigned> Strings;
-    TrigramIndex Trigrams;
     std::vector<std::pair<std::unique_ptr<Regex>, unsigned>> RegExes;
   };
 
@@ -155,5 +153,4 @@ class SpecialCaseList {
 
 }  // namespace llvm
 
-#endif  // LLVM_SUPPORT_SPECIALCASELIST_H
-
+#endif // LLVM_SUPPORT_SPECIALCASELIST_H
diff --git a/llvm/include/llvm/Support/TrigramIndex.h b/llvm/include/llvm/Support/TrigramIndex.h
diff --git a/llvm/lib/Support/CMakeLists.txt b/llvm/lib/Support/CMakeLists.txt
@@ -226,7 +226,6 @@ add_llvm_component_library(LLVMSupport
   TimeProfiler.cpp
   Timer.cpp
   ToolOutputFile.cpp
-  TrigramIndex.cpp
   Twine.cpp
   TypeSize.cpp
   Unicode.cpp
diff --git a/llvm/lib/Support/SpecialCaseList.cpp b/llvm/lib/Support/SpecialCaseList.cpp
@@ -37,7 +37,6 @@ bool SpecialCaseList::Matcher::insert(std::string Regexp,
     Strings[Regexp] = LineNumber;
     return true;
   }
-  Trigrams.insert(Regexp);
 
   // Replace * with .*
   for (size_t pos = 0; (pos = Regexp.find('*', pos)) != std::string::npos;
@@ -61,8 +60,6 @@ unsigned SpecialCaseList::Matcher::match(StringRef Query) const {
   auto It = Strings.find(Query);
   if (It != Strings.end())
     return It->second;
-  if (Trigrams.isDefinitelyOut(Query))
-    return false;
   for (const auto &RegExKV : RegExes)
     if (RegExKV.first->match(Query))
       return RegExKV.second;
diff --git a/llvm/lib/Support/TrigramIndex.cpp b/llvm/lib/Support/TrigramIndex.cpp
diff --git a/llvm/unittests/Support/CMakeLists.txt b/llvm/unittests/Support/CMakeLists.txt
@@ -89,7 +89,6 @@ add_llvm_unittest(SupportTests
   TypeSizeTest.cpp
   TypeTraitsTest.cpp
   TrailingObjectsTest.cpp
-  TrigramIndexTest.cpp
   UnicodeTest.cpp
   VersionTupleTest.cpp
   VirtualFileSystemTest.cpp
diff --git a/llvm/unittests/Support/TrigramIndexTest.cpp b/llvm/unittests/Support/TrigramIndexTest.cpp
diff --git a/llvm/utils/gn/secondary/llvm/lib/Support/BUILD.gn b/llvm/utils/gn/secondary/llvm/lib/Support/BUILD.gn
@@ -141,7 +141,6 @@ static_library("Support") {
     "TimeProfiler.cpp",
     "Timer.cpp",
     "ToolOutputFile.cpp",
-    "TrigramIndex.cpp",
     "Twine.cpp",
     "TypeSize.cpp",
     "Unicode.cpp",
diff --git a/llvm/utils/gn/secondary/llvm/unittests/Support/BUILD.gn b/llvm/utils/gn/secondary/llvm/unittests/Support/BUILD.gn
@@ -89,7 +89,6 @@ unittest("SupportTests") {
     "TimerTest.cpp",
     "ToolOutputFileTest.cpp",
     "TrailingObjectsTest.cpp",
-    "TrigramIndexTest.cpp",
     "TypeNameTest.cpp",
     "TypeSizeTest.cpp",
     "TypeTraitsTest.cpp",
