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

[Troy Johnson via Phabricator via cfe-commits]

troyj added inline comments.


================
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;
+
----------------
bruno wrote:
> 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.
Right, with ~15000 header maps, this map has ~92000 entries, so each map contributes about 6 unique keys. The patch does not increase the peak memory usage of Clang.


================
Comment at: clang/lib/Lex/HeaderSearch.cpp:392
+
+    Dir.getHeaderMap()->forEachKey(Callback);
+  }
----------------
bruno wrote:
> What happens if different header maps contain the same header name?
> 
The first one sticks because try_emplace doesn't insert if the key already exists. I'll add a comment.


================
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);
   }
----------------
bruno wrote:
> Looks like we could have only one call to `CacheLookup.reset` instead?
The reset call has a side effect of setting CacheLookup.MappedName to nullptr, so we can't call it unconditionally because the Hit case should not call it. The unpatched code combined the !SkipCache check and the Hit check, so it still called reset when SkipCache == true. I preserved that behavior because I think it's necessary. SkipCache seems to imply "don't consult the cache for this call" instead of "don't cache anything" so it still caches the result for a future LookupFile call that passes SkipCache == false.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135801