[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:52
+llvm::SmallString<128>
+getShardPathFromFilePath(llvm::SmallString<128> ShardRoot,
+                         llvm::StringRef FilePath) {
----------------
nit: these micro-optimizations with SmallString seem unlikely to be worthwhile - maybe just take StringRef and return string?


================
Comment at: clangd/index/Background.cpp:109
+
+SmallString<128> getAbsolutePath(const tooling::CompileCommand &Cmd) {
+  SmallString<128> AbsolutePath;
----------------
absoluteFilePath? (otherwise it's not clear what the path is to)


================
Comment at: clangd/index/Background.cpp:237
+  if (IndexStorage)
+    loadShards(IndexStorage.get(), Cmds);
+  else
----------------
this absolutely can't be done synchronously (it's much slower than calling getAllCompileCommands - this could block clangd for tens of seconds when opening the first file).

Can we drop shard-loading in BackgroundIndex from this patch?


================
Comment at: clangd/index/Background.cpp:256
   Queue.push_back(Bind(
-      [this](tooling::CompileCommand Cmd) {
+      [this](tooling::CompileCommand Cmd,
+             std::shared_ptr<BackgroundIndexStorage> IndexStorage) {
----------------
you can capture indexstorage by value


================
Comment at: clangd/index/Background.h:34
+private:
+  static std::function<std::shared_ptr<BackgroundIndexStorage>(llvm::StringRef)>
+      Factory;
----------------
I don't think we should be using a static mutable factory here.

I think I worded the previous comment about exposing `DiskBackedIndexStorage` badly, what I meant was something like: `static unique_ptr<BackgroundIndexStorage> BackgroundIndexStorage::createDiskStorage(StringRef dir)`

This would be separate from how the storage strategy is configured on the background index.



================
Comment at: clangd/index/Background.h:34
+private:
+  static std::function<std::shared_ptr<BackgroundIndexStorage>(llvm::StringRef)>
+      Factory;
----------------
sammccall wrote:
> I don't think we should be using a static mutable factory here.
> 
> I think I worded the previous comment about exposing `DiskBackedIndexStorage` badly, what I meant was something like: `static unique_ptr<BackgroundIndexStorage> BackgroundIndexStorage::createDiskStorage(StringRef dir)`
> 
> This would be separate from how the storage strategy is configured on the background index.
> 
do we need `shared_ptr` here?
ISTM either the BackgroundIndex is going to be responsible for caching (in which case it should receive unique_ptr) or the factory is responsible for caching (in which case it can own the functions, and the BackgroundIndex can get a non-owning raw pointer)


================
Comment at: clangd/index/Background.h:38
+public:
+  using FileDigest = decltype(llvm::SHA1::hash({}));
+
----------------
unused?


================
Comment at: clangd/index/Background.h:42
+  // retrieved later on with the same identifier.
+  virtual bool storeShard(llvm::StringRef ShardIdentifier,
+                          IndexFileOut Shard) const = 0;
----------------
Prefer llvm::Error over bool, or no return value if you don't think we ever want to handle it.


================
Comment at: clangd/index/Background.h:45
+
+  // Retrieves the shard if found, also returns hash for the source file that
+  // generated this shard.
----------------
doc stale


================
Comment at: clangd/index/Background.h:48
+  virtual llvm::Expected<IndexFileIn>
+  retrieveShard(llvm::StringRef ShardIdentifier) const = 0;
+
----------------
nit: I think "loadShard" would better emphasize the relationship with `storeShard`, as load/store is such a common pair. (read/write would similarly work).
Nothing wrong with "retrieve" per se, I just think the pairing is better.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54269