[PATCH] D55224: [clangd] Introduce loading of shards within auto-index

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clangd/index/Background.cpp:259
+      // if this thread sees the older version but finishes later. This should
+      // be rare in practice.
+      DigestIt.first->second = Hash;
----------------
ilya-biryukov wrote:
> > "should be rare in practice"
> And yet, can we avoid this altogether?
> 
> Also, I believe it won't be rare. When processing multiple different TUs, we can easily run into the same header multiple times, possibly with the different contents.
> E.g. imagine the index for the whole of clang is being built and the user is editing `Sema.h` at the same time. We'll definitely be running into this case over and over again.
Well I am open to ideas, but currently there is no way of knowing whether this is the newer version for the file. Because only information we have is the digest is different than what we currently have and this doesn't yield any chronological information.


================
Comment at: clangd/index/Background.cpp:338
     std::lock_guard<std::mutex> Lock(DigestsMu);
-    if (IndexedFileDigests.lookup(AbsolutePath) == Hash) {
-      vlog("No need to index {0}, already up to date", AbsolutePath);
----------------
ilya-biryukov wrote:
> This looks like an important optimization. Did we move it to some other place? Why should we remove it?
We kind of moved it into loadShards logic by not running indexing for same file multiple times. I needed to delete this check since we might wanna run indexing on a TU, even if it is up-to-date, to get new symbols coming from one of it's includes.


================
Comment at: clangd/index/Background.cpp:365
   StringMap<FileDigest> FilesToUpdate;
+  // Main file always needs to be updated even if it has no symbols.
+  FilesToUpdate[AbsolutePath] = Hash;
----------------
ilya-biryukov wrote:
> The comment does not mention **why** should we update it. Is it because of the dependencies?
Nice catch, that was the case in one of my experiments but currently during the update we store shards for every source file that exists in include graph even if it has no symbols&refs. So it is no longer necessary.


================
Comment at: clangd/index/Background.cpp:386
   Action->EndSourceFile();
+  if (!Index.Symbols)
+    return createStringError(inconvertibleErrorCode(), "Uncompilable errors");
----------------
ilya-biryukov wrote:
> Was this error possible before this change too?
Yes and it got fixed by D55650, just deleting the check it will arrive once I rebase.


================
Comment at: clangd/index/Background.cpp:481
+          if (!Buf)
+            continue;
+          // If digests match then dependency doesn't need re-indexing.
----------------
ilya-biryukov wrote:
> maybe log the error? would be nice to somehow recover too, but not sure what we can do here.
Well, if this happens we don't have access to file's contents for some reason. I don't think we can do anything. We might actually want to skip loading these files into index, since they are most likely gone and symbols coming from them won't be accessible. WDYT?


================
Comment at: clangd/index/Background.cpp:508
+    BackgroundIndexStorage *IndexStorage = IndexStorageFactory(PI.SourceRoot);
+    auto Dependencies = loadShard(*Cmd, IndexStorage, LoadedShards);
+    for (const auto &Dependency : Dependencies) {
----------------
ilya-biryukov wrote:
> Maybe return a `vector<string>` with dependencies that need reindexing?  We ignore the dependencies that don't need to be reindexed anyway.
Yes we ignore them for now, but they might change in one of the consecutive checks. And if that happens we don't need to schedule another TU for re-indexing that dependency, since we mark it as already indexed.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55224