[clang] [llvm] [clang][deps] Overload `Filesystem::exists` in `DependencyScanningFilesystem` to have it use cached `status` (PR #88152)

Artem Chikin via cfe-commits cfe-commits at lists.llvm.org
Fri Apr 12 13:50:57 PDT 2024


https://github.com/artemcm updated https://github.com/llvm/llvm-project/pull/88152

>From 24e869df273b9d75bb4fdf85f4ee8ab2ddbccc2c Mon Sep 17 00:00:00 2001
From: Artem Chikin <achikin at apple.com>
Date: Tue, 9 Apr 2024 09:37:09 -0700
Subject: [PATCH] [clang][deps] Overload 'Filesystem::exists' in
 'DependencyScanningFilesystem' to have it use cached `status`

As-is, calls to `exists()` fallback on the implementation in 'ProxyFileSystem::exists' which explicitly calls out to the underlying `FS`, which for the 'DependencyScanningFilesystem' (overlay) is the real underlying filesystem.

Instead, directly overloading 'exists' allows us to have it rely on the cached `status` behavior used elsewhere by the 'DependencyScanningFilesystem'.
---
 .../DependencyScanningFilesystem.h            |   4 +
 .../DependencyScanningFilesystem.cpp          |  11 ++
 .../DependencyScanningFilesystemTest.cpp      |  46 ++++++
 .../Support/VirtualFileSystemTest.cpp         | 140 +++++++++---------
 4 files changed, 131 insertions(+), 70 deletions(-)

diff --git a/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h b/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
index 8b6f149c7cb266..f7b4510d7f7beb 100644
--- a/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
+++ b/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
@@ -365,6 +365,10 @@ class DependencyScanningWorkerFilesystem
   /// false if not (i.e. this entry is not a file or its scan fails).
   bool ensureDirectiveTokensArePopulated(EntryRef Entry);
 
+  /// Check whether \p Path exists. By default checks cached result of \c
+  /// status(), and falls back on FS if unable to do so.
+  bool exists(const Twine &Path) override;
+
 private:
   /// For a filename that's not yet associated with any entry in the caches,
   /// uses the underlying filesystem to either look up the entry based in the
diff --git a/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp b/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
index 84185c931b552c..0cab17a3424406 100644
--- a/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
+++ b/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
@@ -300,6 +300,17 @@ DependencyScanningWorkerFilesystem::status(const Twine &Path) {
   return Result->getStatus();
 }
 
+bool DependencyScanningWorkerFilesystem::exists(const Twine &Path) {
+  // While some VFS overlay filesystems may implement more-efficient
+  // mechanisms for `exists` queries, `DependencyScanningWorkerFilesystem`
+  // typically wraps `RealFileSystem` which does not specialize `exists`,
+  // so it is not likely to benefit from such optimizations. Instead,
+  // it is more-valuable to have this query go through the
+  // cached-`status` code-path of the `DependencyScanningWorkerFilesystem`.
+  llvm::ErrorOr<llvm::vfs::Status> Status = status(Path);
+  return Status && Status->exists();
+}
+
 namespace {
 
 /// The VFS that is used by clang consumes the \c CachedFileSystemEntry using
diff --git a/clang/unittests/Tooling/DependencyScanning/DependencyScanningFilesystemTest.cpp b/clang/unittests/Tooling/DependencyScanning/DependencyScanningFilesystemTest.cpp
index 697b7d70ff035a..67f9ee1a3f97ce 100644
--- a/clang/unittests/Tooling/DependencyScanning/DependencyScanningFilesystemTest.cpp
+++ b/clang/unittests/Tooling/DependencyScanning/DependencyScanningFilesystemTest.cpp
@@ -8,6 +8,7 @@
 
 #include "clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h"
 #include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/Twine.h"
 #include "llvm/Support/VirtualFileSystem.h"
 #include "gtest/gtest.h"
 
@@ -33,6 +34,28 @@ struct InstrumentingFilesystem
     return ProxyFileSystem::getRealPath(Path, Output);
   }
 };
+
+
+struct InstrumentingInMemoryFilesystem
+    : llvm::RTTIExtends<InstrumentingInMemoryFilesystem,
+                        llvm::vfs::InMemoryFileSystem> {
+  unsigned NumStatCalls = 0;
+  unsigned NumExistsCalls = 0;
+
+  using llvm::RTTIExtends<InstrumentingInMemoryFilesystem,
+                          llvm::vfs::InMemoryFileSystem>::RTTIExtends;
+
+  llvm::ErrorOr<llvm::vfs::Status> status(const llvm::Twine &Path) override {
+    ++NumStatCalls;
+    return InMemoryFileSystem::status(Path);
+  }
+
+  bool exists(const llvm::Twine &Path) override {
+    ++NumExistsCalls;
+    return InMemoryFileSystem::exists(Path);
+  }
+};
+
 } // namespace
 
 TEST(DependencyScanningWorkerFilesystem, CacheStatusFailures) {
@@ -147,3 +170,26 @@ TEST(DependencyScanningFilesystem, RealPathAndStatusInvariants) {
     DepFS.status("/bar");
   }
 }
+
+TEST(DependencyScanningFilesystem, CacheStatOnExists) {
+  auto InMemoryInstrumentingFS =
+      llvm::makeIntrusiveRefCnt<InstrumentingInMemoryFilesystem>();
+  InMemoryInstrumentingFS->setCurrentWorkingDirectory("/");
+  InMemoryInstrumentingFS->addFile("/foo", 0,
+                                   llvm::MemoryBuffer::getMemBuffer(""));
+  InMemoryInstrumentingFS->addFile("/bar", 0,
+                                   llvm::MemoryBuffer::getMemBuffer(""));
+  DependencyScanningFilesystemSharedCache SharedCache;
+  DependencyScanningWorkerFilesystem DepFS(SharedCache,
+                                           InMemoryInstrumentingFS);
+
+  DepFS.status("/foo");
+  DepFS.status("/foo");
+  DepFS.status("/bar");
+  EXPECT_EQ(InMemoryInstrumentingFS->NumStatCalls, 2u);
+
+  DepFS.exists("/foo");
+  DepFS.exists("/bar");
+  EXPECT_EQ(InMemoryInstrumentingFS->NumStatCalls, 2u);
+  EXPECT_EQ(InMemoryInstrumentingFS->NumExistsCalls, 0u);
+}
diff --git a/llvm/unittests/Support/VirtualFileSystemTest.cpp b/llvm/unittests/Support/VirtualFileSystemTest.cpp
index e9b4ac3d92e1dd..8cc444d9f2f6e0 100644
--- a/llvm/unittests/Support/VirtualFileSystemTest.cpp
+++ b/llvm/unittests/Support/VirtualFileSystemTest.cpp
@@ -3443,51 +3443,51 @@ TEST(RedirectingFileSystemTest, ExternalPaths) {
 TEST(RedirectingFileSystemTest, Exists) {
   IntrusiveRefCntPtr<DummyFileSystem> Dummy(new NoStatusDummyFileSystem());
   auto YAML =
-    MemoryBuffer::getMemBuffer("{\n"
-                               "  'version': 0,\n"
-                               "  'roots': [\n"
-                               "    {\n"
-                               "      'type': 'directory-remap',\n"
-                               "      'name': '/dremap',\n"
-                               "      'external-contents': '/a',\n"
-                               "    },"
-                               "    {\n"
-                               "      'type': 'directory-remap',\n"
-                               "      'name': '/dmissing',\n"
-                               "      'external-contents': '/dmissing',\n"
-                               "    },"
-                               "    {\n"
-                               "      'type': 'directory',\n"
-                               "      'name': '/both',\n"
-                               "      'contents': [\n"
-                               "        {\n"
-                               "          'type': 'file',\n"
-                               "          'name': 'vfile',\n"
-                               "          'external-contents': '/c'\n"
-                               "        }\n"
-                               "      ]\n"
-                               "    },\n"
-                               "    {\n"
-                               "      'type': 'directory',\n"
-                               "      'name': '/vdir',\n"
-                               "      'contents': ["
-                               "        {\n"
-                               "          'type': 'directory-remap',\n"
-                               "          'name': 'dremap',\n"
-                               "          'external-contents': '/b'\n"
-                               "        },\n"
-                               "        {\n"
-                               "          'type': 'file',\n"
-                               "          'name': 'missing',\n"
-                               "          'external-contents': '/missing'\n"
-                               "        },\n"
-                               "        {\n"
-                               "          'type': 'file',\n"
-                               "          'name': 'vfile',\n"
-                               "          'external-contents': '/c'\n"
-                               "        }]\n"
-                               "    }]\n"
-                               "}");
+      MemoryBuffer::getMemBuffer("{\n"
+                                 "  'version': 0,\n"
+                                 "  'roots': [\n"
+                                 "    {\n"
+                                 "      'type': 'directory-remap',\n"
+                                 "      'name': '/dremap',\n"
+                                 "      'external-contents': '/a',\n"
+                                 "    },"
+                                 "    {\n"
+                                 "      'type': 'directory-remap',\n"
+                                 "      'name': '/dmissing',\n"
+                                 "      'external-contents': '/dmissing',\n"
+                                 "    },"
+                                 "    {\n"
+                                 "      'type': 'directory',\n"
+                                 "      'name': '/both',\n"
+                                 "      'contents': [\n"
+                                 "        {\n"
+                                 "          'type': 'file',\n"
+                                 "          'name': 'vfile',\n"
+                                 "          'external-contents': '/c'\n"
+                                 "        }\n"
+                                 "      ]\n"
+                                 "    },\n"
+                                 "    {\n"
+                                 "      'type': 'directory',\n"
+                                 "      'name': '/vdir',\n"
+                                 "      'contents': ["
+                                 "        {\n"
+                                 "          'type': 'directory-remap',\n"
+                                 "          'name': 'dremap',\n"
+                                 "          'external-contents': '/b'\n"
+                                 "        },\n"
+                                 "        {\n"
+                                 "          'type': 'file',\n"
+                                 "          'name': 'missing',\n"
+                                 "          'external-contents': '/missing'\n"
+                                 "        },\n"
+                                 "        {\n"
+                                 "          'type': 'file',\n"
+                                 "          'name': 'vfile',\n"
+                                 "          'external-contents': '/c'\n"
+                                 "        }]\n"
+                                 "    }]\n"
+                                 "}");
 
   Dummy->addDirectory("/a");
   Dummy->addRegularFile("/a/foo");
@@ -3496,7 +3496,7 @@ TEST(RedirectingFileSystemTest, Exists) {
   Dummy->addRegularFile("/both/foo");
 
   auto Redirecting = vfs::RedirectingFileSystem::create(
-							std::move(YAML), nullptr, "", nullptr, Dummy);
+      std::move(YAML), nullptr, "", nullptr, Dummy);
 
   EXPECT_TRUE(Redirecting->exists("/dremap"));
   EXPECT_FALSE(Redirecting->exists("/dmissing"));
@@ -3514,22 +3514,22 @@ TEST(RedirectingFileSystemTest, Exists) {
 TEST(RedirectingFileSystemTest, ExistsFallback) {
   IntrusiveRefCntPtr<DummyFileSystem> Dummy(new NoStatusDummyFileSystem());
   auto YAML =
-    MemoryBuffer::getMemBuffer("{\n"
-                               "  'version': 0,\n"
-                               "  'redirecting-with': 'fallback',\n"
-                               "  'roots': [\n"
-                               "    {\n"
-                               "      'type': 'file',\n"
-                               "      'name': '/fallback',\n"
-                               "      'external-contents': '/missing',\n"
-                               "    },"
-                               "  ]\n"
-                               "}");
+      MemoryBuffer::getMemBuffer("{\n"
+                                 "  'version': 0,\n"
+                                 "  'redirecting-with': 'fallback',\n"
+                                 "  'roots': [\n"
+                                 "    {\n"
+                                 "      'type': 'file',\n"
+                                 "      'name': '/fallback',\n"
+                                 "      'external-contents': '/missing',\n"
+                                 "    },"
+                                 "  ]\n"
+                                 "}");
 
   Dummy->addRegularFile("/fallback");
 
   auto Redirecting = vfs::RedirectingFileSystem::create(
-							std::move(YAML), nullptr, "", nullptr, Dummy);
+      std::move(YAML), nullptr, "", nullptr, Dummy);
 
   EXPECT_TRUE(Redirecting->exists("/fallback"));
   EXPECT_FALSE(Redirecting->exists("/missing"));
@@ -3538,23 +3538,23 @@ TEST(RedirectingFileSystemTest, ExistsFallback) {
 TEST(RedirectingFileSystemTest, ExistsRedirectOnly) {
   IntrusiveRefCntPtr<DummyFileSystem> Dummy(new NoStatusDummyFileSystem());
   auto YAML =
-    MemoryBuffer::getMemBuffer("{\n"
-                               "  'version': 0,\n"
-                               "  'redirecting-with': 'redirect-only',\n"
-                               "  'roots': [\n"
-                               "    {\n"
-                               "      'type': 'file',\n"
-                               "      'name': '/vfile',\n"
-                               "      'external-contents': '/a',\n"
-                               "    },"
-                               "  ]\n"
-                               "}");
+      MemoryBuffer::getMemBuffer("{\n"
+                                 "  'version': 0,\n"
+                                 "  'redirecting-with': 'redirect-only',\n"
+                                 "  'roots': [\n"
+                                 "    {\n"
+                                 "      'type': 'file',\n"
+                                 "      'name': '/vfile',\n"
+                                 "      'external-contents': '/a',\n"
+                                 "    },"
+                                 "  ]\n"
+                                 "}");
 
   Dummy->addRegularFile("/a");
   Dummy->addRegularFile("/b");
 
   auto Redirecting = vfs::RedirectingFileSystem::create(
-							std::move(YAML), nullptr, "", nullptr, Dummy);
+      std::move(YAML), nullptr, "", nullptr, Dummy);
 
   EXPECT_FALSE(Redirecting->exists("/a"));
   EXPECT_FALSE(Redirecting->exists("/b"));



More information about the cfe-commits mailing list