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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

oops, looks like i forgot to hit submit in the morning..



================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:228
+  auto Status = FS->status(IndexPath);
+  if (!Status || Status->equivalent(LastStatus))
+    return;
----------------
i beleive `equivalent` checks for inode number equivalence (at least on linux, using realfilesystem). That's not guranteed to change when you overwrite/modify a file.

Sorry if it felt like i meant this, but I was literally suggesting to use `!=` instead of `<=` when comparing modifcation time, and also incorporating size into the check.

Also please let me know if there's something i am missing with the `equivalent`.

It would be also nice to have some tests, as we are starting to add non-trivial functionality to server but not blocking for this patch. (i suppose it would've catched the equivalent problem)


================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:234
+       LastStatus.getLastModificationTime(), Status->getLastModificationTime());
+  std::unique_ptr<clang::clangd::SymbolIndex> NewIndex = loadIndex(IndexPath);
+  if (!NewIndex) {
----------------
i think we should update `LastStatus` here, as we are going to fail loading the index unless the file changes. so there's no need to retry loading the index if the file hasn't changed.


================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:236
+  if (!NewIndex) {
+    vlog("Failed to load new index. Old index will be served.");
+    return;
----------------
i believe, this should be an `elog`.


================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:240
+  Index.reset(std::move(NewIndex));
+  log("New index version loaded. Last modification time: {0}.",
+      Status->getLastModificationTime());
----------------
nit: maybe also print the size.


================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:318
+  if (!Status) {
+    elog("File {0} does not exist.", IndexPath);
+    return Status.getError().value();
----------------
nit: drop `File`


================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:335
+      std::this_thread::sleep_for(RefreshFrequency);
+      hotReload(*Index, llvm::StringRef(IndexPath), LastStatus, FS);
+    }
----------------
nit: i would first call `hotReload` and then sleep to get rid of one extra reload of the index when we are shutting down.


================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:342
+  HotReloadThread.join();
 }
----------------
nit: `return 0;` ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87450