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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Thanks! Looks good, rest of the nits are obvious or up to you.(



================
Comment at: clangd/index/Background.cpp:37
 
-BackgroundIndex::BackgroundIndex(Context BackgroundContext,
-                                 StringRef ResourceDir,
-                                 const FileSystemProvider &FSProvider,
-                                 ArrayRef<std::string> URISchemes,
-                                 size_t ThreadPoolSize)
+namespace {
+
----------------
nit: I think the moves of the helper functions (digest/digestFile/getAbsoluteFilePath) can be reverted now


================
Comment at: clangd/index/Background.h:45
+
+  using Factory =
+      llvm::unique_function<BackgroundIndexStorage *(llvm::StringRef)>;
----------------
Some docs? Maybe add a comment like
```
// The factory provides storage for each CDB. 
// It keeps ownership of the storage instances, and should manage caching itself.
// Factory must be threadsafe and never returns nullptr.
```


================
Comment at: clangd/index/Background.h:50
+  // CDBDirectory + ".clangd-index/" as the folder to save shards.
+  static Factory getDiskBackedStorageFactory();
+};
----------------
nit: `create` rather than `get` would be clearer that this returns an independent object each time.


================
Comment at: clangd/index/Background.h:103
 
+  // index storage
+  BackgroundIndexStorage::Factory IndexStorageFactory;
----------------
(nit: this comment seems redundant with the type/name)


================
Comment at: clangd/index/BackgroundIndexStorage.cpp:30
+  llvm::SmallString<128> ShardRootSS(ShardRoot);
+  llvm::sys::path::append(ShardRootSS, llvm::sys::path::filename(FilePath) +
+                                           llvm::toHex(digest(FilePath)) +
----------------
nit: can we put a separator like between the path and digest?
I'd suggest `.` so the file extension is most obviously preserved.

e.g. `Foo.cpp.12345.idx` vs `Foo.cpp_123456.idx` vs `Foo.cpp123456.idx'


================
Comment at: clangd/index/BackgroundIndexStorage.cpp:51
+    if (EC != OK) {
+      elog("Failed to create directory {0} for disk backed index storage: {1}",
+           DiskShardRoot, EC.message());
----------------
nit: just "for index storage"?
it's clearly disk backed if it's going into a directory!
(and the extra qualification may confuse users)


================
Comment at: clangd/index/BackgroundIndexStorage.cpp:79
+    OS << Shard;
+    return llvm::Error::success();
+  }
----------------
You're ignoring write errors here (and in fact errors may not have occurred yet due to buffering).
```
OS.close();
return OS.error();
```


================
Comment at: unittests/clangd/BackgroundIndexTests.cpp:100
+TEST(BackgroundIndexTest, ShardStorageWriteTest) {
+  class MemoryShardStorage : public BackgroundIndexStorage {
+    mutable std::mutex StorageMu;
----------------
Consider pulling this class to the top level, and use it any time you don't care about the storage (to avoid the need for DummyStorage)


================
Comment at: unittests/clangd/BackgroundIndexTests.cpp:111
+      std::lock_guard<std::mutex> Lock(StorageMu);
+      std::string &str = Storage[ShardIdentifier];
+      llvm::raw_string_ostream OS(str);
----------------
I think this can just be 
`Storage[ShardIdentifier] = llvm::to_string(Shard)`

(need to include ScopedPrinters.h)


================
Comment at: unittests/clangd/BackgroundIndexTests.cpp:121
+      if (Storage.find(ShardIdentifier) == Storage.end()) {
+        ADD_FAILURE() << "Shard " << ShardIdentifier << ": not found.";
+        return nullptr;
----------------
this one shouldn't be a failure I think - just return nullptr? (If the test cares about it, the test should depend on it more explicitly).

The corrupt index file is a more obvious "should never happen" case.


================
Comment at: unittests/clangd/BackgroundIndexTests.cpp:130
+      }
+      CacheHits++;
+      return llvm::make_unique<IndexFileIn>(std::move(*IndexFile));
----------------
You never actually assert on this value.

(I'm not sure you need to, but if not just remove it)


================
Comment at: unittests/clangd/BackgroundIndexTests.cpp:146
+  MemoryShardStorage MSS(Storage, CacheHits);
+  BackgroundIndexStorage::Factory MemoryStorageCreator = [&](llvm::StringRef) {
+    return &MSS;
----------------
seems clearer just to inline this in the constructor(, but up to you


================
Comment at: unittests/clangd/BackgroundIndexTests.cpp:162
+  EXPECT_EQ(Storage.size(), 2U);
+  EXPECT_NE(Storage.find(testPath("root/A.h")), Storage.end());
+  EXPECT_NE(Storage.find(testPath("root/A.cc")), Storage.end());
----------------
(even if the StringMap was encapsulated in the MemoryStorage, you can still write this assertion through the public interface: ASSERT_NE(Storage.loadShard("root/A.h"), None)

(you could even assert the actual stored symbols, but may not be worth it)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54269