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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
