[PATCH] D54269: Introduce shard storage to auto-index.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clangd/index/Background.cpp:252
+
+    auto Hash = FilesToUpdate.lookup(Path);
+    // Put shards into storage for subsequent use.
----------------
kadircet wrote:
> sammccall wrote:
> > nit: i'd suggest doing the writes *after* updating the index, as the latter is user-facing
> but updating index consumes slabs. Is it worth making a copy?
Oh, true - probably not. Add a comment?


================
Comment at: clangd/index/Background.cpp:326
       return Error::success();
+    } else if (IndexShardStorage) { // Check if shard storage has the index.
+      auto Shard = IndexShardStorage->retrieveShard(AbsolutePath, Hash);
----------------
kadircet wrote:
> sammccall wrote:
> > I don't think this is the right place to be reading shards, vs on startup. It means we're doing extra IO whenever a file is reindexed, and more importantly we don't make the index contents available on startup (if any file needs reindexing, reads may get stuck behind it).
> > 
> > (as discussed, we can also leave reading out of this patch entirely)
> What exactly you mean by startup exactly from BackgroundIndex's perspective. Is it enqueueAll ?
Yes. (Or rather, a task scheduled there)


================
Comment at: clangd/index/Background.h:39
+  retrieveShard(llvm::StringRef ShardIdentifier, FileDigest Hash) const = 0;
+  virtual bool initialize(llvm::StringRef Directory) = 0;
+};
----------------
kadircet wrote:
> sammccall wrote:
> > Why not use the constructor? what does "directory" mean in the general case?
> Directory refers to the one specified in CompilationDatabase(which is usually the build directory?), sorry for the inconvenience.
> I wasn't sure about where we plan to instantiate BackgroundIndex. If you plan to do that initialization at a point in which we already know the build directory we can move that to constructor, especially only to the constructor of DiskBackedIndexStorage.
tooling::CompileCommand::WorkingDirectory? That doesn't seem especially relevant here.
Or the directory that the CDB was discovered in?

Yes, this seems to be only relevant to DiskBackedIndexStorage


================
Comment at: clangd/index/Background.h:51
                   const FileSystemProvider &, ArrayRef<std::string> URISchemes,
+                  std::unique_ptr<ShardStorage> IndexShardStorage = nullptr,
                   size_t ThreadPoolSize = llvm::hardware_concurrency());
----------------
kadircet wrote:
> sammccall wrote:
> > So we have several possible designs on a collision course:
> > 1. One instance of `BackgroundIndex` for clangd, which has a ShardStorage, which is used for every CDB
> > 2. One instance of `BackgroundIndex`, which has many ShardStorages, one-per-CDB
> > 3. Many instances of `BackgroundIndex`, one per CDB, each with a ShardStorage
> > 4. One `BackgroundIndex` for everything, and one ShardStorage for everything (storage in $HOME/.clangd-index)
> > 
> > What are we aiming for here?
> > 
> > The design of ShardStorage in this patch precludes 1, taking unique_ptr here precludes 2. 3 requires us to revisit some aspects of BackgroundIndex (which is possible). With 4 it isn't obvious how we do multi-config, or how users can manage the storage.
> > 
> > (I'd been assuming 1 or 2, but I don't think we ever discussed it)
> The aim was more like 2. But I forgot the fact that we could be working with multiple CDBs :D
> 
> So it might be better to just pass a factory to BackgroundIndex and let it create one ShardStorage for each CDB(which is distinguished by the build directory?) WDYT?
Yep, that works for me.


================
Comment at: clangd/index/Background.h:107
+// thread-safe.
+class DiskShardStorage : public ShardStorage {
+  mutable std::mutex DiskShardRootMu;
----------------
kadircet wrote:
> sammccall wrote:
> > This class can be hidden in the cpp file.
> But it needs to be passed into the constructor of BackgroundIndex, possibly by the ClangdServer, do you suggest hiding inside ClangdServer?
We can expose a factory function (e.g. static on ShardStorage)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54269