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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/URI.h:131
+/// Resolves URI to file paths with cache.
+class URIToFileCache {
+public:
----------------
Maybe move it into the `Backgroud.cpp`, e.g. as a private member of `BackoundIndex`?
We don't have other use-case for it to the date and it doesn't really good like a good public API at this point, i.e. lacking docs on what it does and why it's useful, which are useful if we move it into a public header.


================
Comment at: clangd/index/Background.cpp:312
+      // Skip if file is already up to date.
+      auto DigestIt = IndexedFileDigests.try_emplace(Path);
+      if (!DigestIt.second && DigestIt.first->second == Hash)
----------------
kadircet wrote:
> ilya-biryukov wrote:
> > We still update digests and symbols non-atomically in that case. Could we do both under the same lock, similar to how it's done in `loadShards`?
> Moved update to the end of the loop, but now we might perform unnecessary work for building {symbol,ref}slabs for files that are already up-to-date. It shouldn't be too much of an issue, as a solution we can lock at the entrance and only check if the file was up-to-date, than we update at the exit. Or we can keep a snapshot.
LG, thanks. I don't think this extra work should pop up in the profiler, so we're good.


================
Comment at: clangd/index/Background.cpp:311
+             "Trying to update symbols for a file that hasn't been indexed");
+      // If we this is not the latest version of the file just skip.
+      if (LatestFileDigests[Path] != Hash)
----------------
NIT: s/we this/this/


================
Comment at: clangd/index/Background.cpp:312
+      // If we this is not the latest version of the file just skip.
+      if (LatestFileDigests[Path] != Hash)
+        continue;
----------------
An alternative strategy is to also update if the `IndexedFileDigests` does not contain a value.
That would ensure we populate **some** version of all files as fast as possible, but only update for the latest version. To me personally this looks like a reasonable right trade-off (and would require updating just a few lines of code)


================
Comment at: clangd/index/Background.cpp:319
+      DigestIt.first->second = Hash;
+      vlog("Update symbols in {0}", Path);
+      IndexedSymbols.update(Path, std::move(SS), std::move(RS));
----------------
This might be too verbose even for `vlog()`, i.e. this would produce thousands of messages for each TU. Maybe omit it?


================
Comment at: clangd/index/Background.cpp:435
+    for (const auto &P : LatestDigests)
+      LatestFileDigests[P.first] = P.second;
   }
----------------
I believe we want to avoid updating the digests for the dependencies if the digest for the main file has changed in the meantime.
Otherwise, we might record the stale versions of the dependency digests for the rest of the index lifetime.

Note that this might invalidate the assertion in `update() ` that checks all files have digests.


================
Comment at: clangd/index/Background.cpp:586
+  }
+  // FIXME: this should rebuild once-in-a-while, not after every file.
+  //       At that point we should use Dex, too.
----------------
Didn't we land the periodic rebuilds? Should this be obsolete now?


================
Comment at: clangd/index/Background.h:117
   llvm::StringMap<FileDigest> IndexedFileDigests; // Key is absolute file path.
+  llvm::StringMap<FileDigest> LatestFileDigests;  // Key is absolute file path.
   std::mutex DigestsMu;
----------------
NIT: maybe add a comment that we use this to decide which of the concurrently running indexing operations should take precedence?


================
Comment at: clangd/index/SymbolCollector.cpp:213
 bool isPreferredDeclaration(const NamedDecl &ND, index::SymbolRoleSet Roles) {
-  const auto& SM = ND.getASTContext().getSourceManager();
+  const auto &SM = ND.getASTContext().getSourceManager();
   return (Roles & static_cast<unsigned>(index::SymbolRole::Definition)) &&
----------------
NIT: look unrelated, maybe land as a separate NFC commit right away?
Not a big deal, though, feel free to ignore


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55224