[PATCH] D98329: [clangd] Add cache for FID to Header mappings

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Some performance measurements...
(workload is PreambleCallback for clangd/XRefs.cpp in sync mode with a fixed baseline of 99b01cf28db9db1a3ec0e25367bd325b7aca6d43 <https://reviews.llvm.org/rG99b01cf28db9db1a3ec0e25367bd325b7aca6d43>, opt build without asserts)

baseline: 1.21s
baseline+D98242 <https://reviews.llvm.org/D98242>: 1.25s
baseline+D98242 <https://reviews.llvm.org/D98242>+D98329 <https://reviews.llvm.org/D98329>: 1.21s
baseline+D98371 <https://reviews.llvm.org/D98371>: 1.02s

So this patch does indeed help, and cancels out the extra work done in D98242 <https://reviews.llvm.org/D98242> with this workload at least.
Nevertheless I think the speedups from D98371 <https://reviews.llvm.org/D98371> clearly justify the more invasive approach to caching, so we should do something like that instead. (We don't need both as it caches at a slightly higher level).



================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:792
+      return HeaderToInclude[FID] =
+                 getIncludeHeader(S, SM.getFileID(SM.getIncludeLoc(FID)));
     // Conservatively refuse to insert #includes to files without guards.
----------------
this is correct but subtle: if we get to here we know the symbol does not have any special handling so it's safe to cache with the file Id alone


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98329