[PATCH] D64712: [clangd] Refactor background-index shard loading

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/index/BackgroundIndexLoader.cpp:93
+
+void BackgroundIndexLoader::load(PathRef MainFile,
+                                 BackgroundIndexStorage *Storage) {
----------------
sammccall wrote:
> This handles only one main file, I can't see where you skip loading shards for headers that were loaded for a previous file
The real loading happens down the line at `const CachedShard &CS = loadShard(ShardIdentifier, Storage);`, which returns shard from cache if it was already loaded.


================
Comment at: clang-tools-extra/clangd/index/BackgroundIndexLoader.cpp:126
+    assert(TUsIt != FileToTUs.end() && "No TU registered for the shard");
+    Result.back().DependentTUs = std::move(TUsIt->second);
+  }
----------------
sammccall wrote:
> I don't think this works, the storage for these StringRefs is the LoadedShards you just moved from.
> (In practice, maybe you get away with it if the strings are longer than the SSO?)
> 
> 
Yes you are right, I don't see a good way out of it with current data model. I suppose we can keep an "intern table :)" for the strings, but then LoadedShards would be meaningless without that intern table so we would end up returning another struct from `loadIndexShards` that will wrap `std::vector<LoadedShards>` and the intern table(but we tried to mitigate from that struct in previous iteration).

So instead I am changing the model to return only the first translation unit and actually this makes a lot of sense when we think about shards for standard library, in a code base like chromium vector could easily have ~40k dependent TUs and even if we just returned a `vector<StringRef>` it would still be around 640kB, and we have a lot of standard library headers. this can quite easily add hundreds of MB to clangd's memory footprint while loading shards.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64712