[PATCH] D135801: [clang][Lexer] Speedup HeaderSearch when there are many HeaderMaps

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.

Thanks for working on this Troy, nice speed up.

A bit more context for reviewers: we're probably unique on how we use hmaps, we have tons of them in a single compiler invocation. The other users of hmaps I've seen don't use more than a handful. This means that general users shouldn't expect much disruption here.



================
Comment at: clang/include/clang/Lex/HeaderMap.h:52
+        Optional<StringRef> Key = getString(B.Key);
+        if (Key) {
+          Callback(Key.value());
----------------
Please remove the curly braces for this `if` and similar ones in the rest of the patch (if present).


================
Comment at: clang/include/clang/Lex/HeaderSearch.h:255
+  /// lets us avoid scanning them all to find a match.
+  llvm::StringMap<unsigned, llvm::BumpPtrAllocator> SearchDirHeaderMapIndex;
+
----------------
Did you end up getting data on the memory footprint of this map? It seems to me that it should be fine if header maps aren't much populated.


================
Comment at: clang/lib/Lex/HeaderSearch.cpp:391
+    auto Callback = [&](StringRef Filename) { Index.try_emplace(Filename, i); };
+
+    Dir.getHeaderMap()->forEachKey(Callback);
----------------
Remove this empty line.


================
Comment at: clang/lib/Lex/HeaderSearch.cpp:392
+
+    Dir.getHeaderMap()->forEachKey(Callback);
+  }
----------------
What happens if different header maps contain the same header name?



================
Comment at: clang/lib/Lex/HeaderSearch.cpp:1035
   } else {
-    // Otherwise, this is the first query, or the previous query didn't match
-    // our search start.  We will fill in our found location below, so prime the
-    // start point value.
     CacheLookup.reset(/*NewStartIt=*/NextIt);
   }
----------------
Looks like we could have only one call to `CacheLookup.reset` instead?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D135801/new/

https://reviews.llvm.org/D135801