[PATCH] D87450: [clangd] Implement hot index reloading for clangd-index-server

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:71
 
 std::unique_ptr<clangd::SymbolIndex> openIndex(llvm::StringRef Index) {
   return loadIndex(Index, /*UseIndex=*/true);
----------------
why do we have this extra indirection?


================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:86
 
+  clangd::SwapIndex *Index;
+
----------------
why not make this private? also keep using `SymbolIndex`. also you can make this a reference now.


================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:224
+// whenever they become available.
+void hotReload(clangd::SwapIndex **Index, llvm::StringRef IndexPath) {
+  llvm::sys::TimePoint<> LastModificationTime =
----------------
why is this taking a double pointer? I think you can just use a reference here for `Index` (i.e. `clangd::SwapIndex &`


================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:226
+  llvm::sys::TimePoint<> LastModificationTime =
+      std::chrono::system_clock::now();
+  for (;; std::this_thread::sleep_for(std::chrono::seconds(90))) {
----------------
i think we should rather store the file_status from the original file in here. as we want to reload whenever files size/modification_time has changed.
it doesn't have to be moving forward let alone share the same timezone restrictions and such provided by system_clock::now.

Also it would be nice to use a VFS in here, that way we can move this logic into a helper in the future if other components need similar logic.


================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:227
+      std::chrono::system_clock::now();
+  for (;; std::this_thread::sleep_for(std::chrono::seconds(90))) {
+    llvm::sys::fs::file_status Status;
----------------
how's this loop terminated?

I would suggest factoring the loop body into a helper and then having a lambda for periodically running it, while looking for an exit signal.


================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:263
 
+  std::thread HotReloadThread(hotReload, &Service.Index, IndexPath);
+
----------------
i think `hotReload` thread creation should also be part of the main.


================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:266
   Server->Wait();
 }
 
----------------
you either need to `join` a thread before its destruction or `detach` it from the main thread.


================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:313
 
-  std::unique_ptr<clang::clangd::SymbolIndex> Index = openIndex(IndexPath);
+  std::unique_ptr<clang::clangd::SwapIndex> Index(
+      new clang::clangd::SwapIndex(openIndex(IndexPath)));
----------------
nit: `auto Index = std::make_unique<clang::clangd::SwapIndex>(openIndex...);`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87450