[clang-tools-extra] 35ce741 - [clangd] Store paths as requested in PreambleStatCache

[Kadir Cetinkaya via cfe-commits]

Author: Kadir Cetinkaya
Date: 2023-05-23T14:30:58+02:00
New Revision: 35ce741ef3e3dd9db1da3ea0a06c565cb90f665a

URL: https://github.com/llvm/llvm-project/commit/35ce741ef3e3dd9db1da3ea0a06c565cb90f665a
DIFF: https://github.com/llvm/llvm-project/commit/35ce741ef3e3dd9db1da3ea0a06c565cb90f665a.diff

LOG: [clangd] Store paths as requested in PreambleStatCache

Underlying FS can store different file names inside the stat response
(e.g. symlinks resolved, absolute paths, dots removed). But we store path names
as requested inside the preamble,
https://github.com/llvm/llvm-project/blob/main/clang/lib/Serialization/ASTWriter.cpp#L1635.

This improves cache hit rates from ~30% to 90% in a build system that uses
symlinks.

Differential Revision: https://reviews.llvm.org/D151185

Added: 
    

Modified: 
    clang-tools-extra/clangd/FS.cpp
    clang-tools-extra/clangd/FS.h
    clang-tools-extra/clangd/unittests/FSTests.cpp

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clangd/FS.cpp b/clang-tools-extra/clangd/FS.cpp
index 3622d35d9d52..c67636dbf2d4 100644
--- a/clang-tools-extra/clangd/FS.cpp
+++ b/clang-tools-extra/clangd/FS.cpp
@@ -11,6 +11,7 @@
 #include "llvm/Support/Path.h"
 #include "llvm/Support/VirtualFileSystem.h"
 #include <optional>
+#include <utility>
 
 namespace clang {
 namespace clangd {
@@ -23,9 +24,10 @@ PreambleFileStatusCache::PreambleFileStatusCache(llvm::StringRef MainFilePath){
 }
 
 void PreambleFileStatusCache::update(const llvm::vfs::FileSystem &FS,
-                                     llvm::vfs::Status S) {
+                                     llvm::vfs::Status S,
+                                     llvm::StringRef File) {
   // Canonicalize path for later lookup, which is usually by absolute path.
-  llvm::SmallString<32> PathStore(S.getName());
+  llvm::SmallString<32> PathStore(File);
   if (FS.makeAbsolute(PathStore))
     return;
   llvm::sys::path::remove_dots(PathStore, /*remove_dot_dot=*/true);
@@ -72,14 +74,14 @@ PreambleFileStatusCache::getProducingFS(
       // many times (e.g. code completion) and the repeated status call is
       // likely to be cached in the underlying file system anyway.
       if (auto S = File->get()->status())
-        StatCache.update(getUnderlyingFS(), std::move(*S));
+        StatCache.update(getUnderlyingFS(), std::move(*S), Path.str());
       return File;
     }
 
     llvm::ErrorOr<llvm::vfs::Status> status(const llvm::Twine &Path) override {
       auto S = getUnderlyingFS().status(Path);
       if (S)
-        StatCache.update(getUnderlyingFS(), *S);
+        StatCache.update(getUnderlyingFS(), *S, Path.str());
       return S;
     }
 

diff  --git a/clang-tools-extra/clangd/FS.h b/clang-tools-extra/clangd/FS.h
index ea07de24bbae..827b465aed98 100644
--- a/clang-tools-extra/clangd/FS.h
+++ b/clang-tools-extra/clangd/FS.h
@@ -41,7 +41,8 @@ class PreambleFileStatusCache {
   /// corresponds to. The stat for the main file will not be cached.
   PreambleFileStatusCache(llvm::StringRef MainFilePath);
 
-  void update(const llvm::vfs::FileSystem &FS, llvm::vfs::Status S);
+  void update(const llvm::vfs::FileSystem &FS, llvm::vfs::Status S,
+              llvm::StringRef File);
 
   /// \p Path is a path stored in preamble.
   std::optional<llvm::vfs::Status> lookup(llvm::StringRef Path) const;

diff  --git a/clang-tools-extra/clangd/unittests/FSTests.cpp b/clang-tools-extra/clangd/unittests/FSTests.cpp
index 81129fec98c2..0b2bc688335d 100644
--- a/clang-tools-extra/clangd/unittests/FSTests.cpp
+++ b/clang-tools-extra/clangd/unittests/FSTests.cpp
@@ -37,18 +37,19 @@ TEST(FSTests, PreambleStatusCache) {
                       std::chrono::system_clock::now(), 0, 0, 1024,
                       llvm::sys::fs::file_type::regular_file,
                       llvm::sys::fs::all_all);
-  StatCache.update(*FS, S);
+  StatCache.update(*FS, S, "real");
   auto ConsumeFS = StatCache.getConsumingFS(FS);
-  auto Cached = ConsumeFS->status(testPath("fake"));
+  EXPECT_FALSE(ConsumeFS->status(testPath("fake")));
+  auto Cached = ConsumeFS->status(testPath("real"));
   EXPECT_TRUE(Cached);
-  EXPECT_EQ(Cached->getName(), testPath("fake"));
+  EXPECT_EQ(Cached->getName(), testPath("real"));
   EXPECT_EQ(Cached->getUniqueID(), S.getUniqueID());
 
-  // fake and temp/../fake should hit the same cache entry.
+  // real and temp/../real should hit the same cache entry.
   // However, the Status returned reflects the actual path requested.
-  auto CachedDotDot = ConsumeFS->status(testPath("temp/../fake"));
+  auto CachedDotDot = ConsumeFS->status(testPath("temp/../real"));
   EXPECT_TRUE(CachedDotDot);
-  EXPECT_EQ(CachedDotDot->getName(), testPath("temp/../fake"));
+  EXPECT_EQ(CachedDotDot->getName(), testPath("temp/../real"));
   EXPECT_EQ(CachedDotDot->getUniqueID(), S.getUniqueID());
 }