[PATCH] D77732: [clangd] Shard preamble symbols in dynamic index

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:97
+/// paths used by \p FileSyms.
+void shardSlabToFiles(const SymbolSlab &Syms, const RelationSlab &Rels,
+                      FileSymbols &FileSyms, llvm::StringRef HintPath) {
----------------
kadircet wrote:
> sammccall wrote:
> > This screams out to be shared with BackgroundIndex::update(). Is it very hard to do?
> > 
> > Things that are different:
> >  - no duplication of symbol into defining file (why?)
> >  - refs not gathered here (but I guess empty input -> empty output?)
> >  - include graph not gathered here (same)
> >  - this holds 3 copies of all the data at peak, other impl holds 2
> > 
> > I imagine factoring out a function that returns a StringMap<File> like in BackgroundIndex::Update, but File has a build() function that returns an IndexFileIn or so.
> > (Ooh, is depending on IndexFileIn a layering violation? Maybe we should move that into another header. Duplicating the struct is probably OK for now)
> In backgroundindex we "prefilter" symbols and only duplicate the ones coming from modified files, whereas in this one we duplicate every symbol no matter what. I didn't want to change backgroundindex into post filtering mode (I suppose that's what you mean by the last item).
> 
> as for the first bullet point, duplicating in case of preamble symbols wouldn't matter since we don't merge(rather pick one) while building the index for preamblesymbols. I suppose storing them twice shouldn't be a huge issue.
> 
> Depending to `IndexFileIn` in "Index.h" would be a violation, but depending on it in "FileIndex.h" is fine.
> 
> I am happy to refactor a `StringMap<File> shardIndexToFiles(IndexFileIn, HintPath)` into "FileIndex.h" that can be used by both backgroundindex and this, if you are OK with above mentioned regressions.
implemented a version of this that only copies data when requested. Hence the pre/post filtering argument is gone.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77732