[PATCH] D53433: [clangd] auto-index stores symbols per-file instead of per-TU.

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

Made some more changes to make the code work in multiple threads (add mutex for digests, take snapshot of digests for each run etc). PTAL



================
Comment at: clangd/index/Background.cpp:311
   SPAN_ATTACH(Tracer, "refs", int(Refs.numRefs()));
-  // FIXME: partition the symbols by file rather than TU, to avoid duplication.
-  IndexedSymbols.update(AbsolutePath,
-                        llvm::make_unique<SymbolSlab>(std::move(Symbols)),
-                        llvm::make_unique<RefSlab>(std::move(Refs)));
-  FileHash[AbsolutePath] = Hash;
+  update(AbsolutePath, std::move(Symbols), std::move(Refs), FilesToUpdate);
+  IndexedFileDigests[AbsolutePath] = Hash;
----------------
sammccall wrote:
> ioeric wrote:
> > sammccall wrote:
> > > It looks like you never write the FilesToUpdate hashes back to the IndexedFileDigests, so we'll always update headers?
> > It's updated in `update` when slabs are updated.
> In that case, do we need to update IndexedFileDigests here too?
> 
> (I just realized there's an edge case: what if there are no symbols or references in a file? but I'm not sure if this is different between mainfile and headers, either)
The edge case seems to justify the update here. We want to make sure that hash for main files are always updated, even when there's no index data from it.


================
Comment at: clangd/index/FileIndex.cpp:140
+  }
+  case DuplicateHandling::PickOne: {
+    for (const auto &Slab : SymbolSlabs)
----------------
sammccall wrote:
> ioeric wrote:
> > sammccall wrote:
> > > since we're deduplicating above, we could deduplicate here too and remove the deduplicating logic from Dex (MemIndex gets it by mistake). That would avoid duplicated deduplication (!) in the Merge + Dex case, and seems a little cleaner.
> > > 
> > > Not something for this patch, but maybe add a FIXME.
> > Added the duduplication for the PickOne case. 
> Thanks! The FIXME is still warranted, to remove deduping from Dex.
Done. I removed it in this patch as it seems trivial enough.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53433