[clang] [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 14:21:15 PDT 2024
https://github.com/artemcm updated https://github.com/llvm/llvm-project/pull/88152
>From b7c693011abd785bec0a58fe4b6011a84f1ea329 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 | 30 +++++++++++++++++++
3 files changed, 45 insertions(+)
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..71447f43f1e6ff 100644
--- a/clang/unittests/Tooling/DependencyScanning/DependencyScanningFilesystemTest.cpp
+++ b/clang/unittests/Tooling/DependencyScanning/DependencyScanningFilesystemTest.cpp
@@ -18,6 +18,7 @@ struct InstrumentingFilesystem
: llvm::RTTIExtends<InstrumentingFilesystem, llvm::vfs::ProxyFileSystem> {
unsigned NumStatusCalls = 0;
unsigned NumGetRealPathCalls = 0;
+ unsigned NumExistsCalls = 0;
using llvm::RTTIExtends<InstrumentingFilesystem,
llvm::vfs::ProxyFileSystem>::RTTIExtends;
@@ -32,6 +33,11 @@ struct InstrumentingFilesystem
++NumGetRealPathCalls;
return ProxyFileSystem::getRealPath(Path, Output);
}
+
+ bool exists(const llvm::Twine &Path) override {
+ ++NumExistsCalls;
+ return ProxyFileSystem::exists(Path);
+ }
};
} // namespace
@@ -147,3 +153,27 @@ TEST(DependencyScanningFilesystem, RealPathAndStatusInvariants) {
DepFS.status("/bar");
}
}
+
+TEST(DependencyScanningFilesystem, CacheStatOnExists) {
+ auto InMemoryFS = llvm::makeIntrusiveRefCnt<llvm::vfs::InMemoryFileSystem>();
+ auto InstrumentingFS =
+ llvm::makeIntrusiveRefCnt<InstrumentingFilesystem>(InMemoryFS);
+ InMemoryFS->setCurrentWorkingDirectory("/");
+ InMemoryFS->addFile("/foo", 0,
+ llvm::MemoryBuffer::getMemBuffer(""));
+ InMemoryFS->addFile("/bar", 0,
+ llvm::MemoryBuffer::getMemBuffer(""));
+ DependencyScanningFilesystemSharedCache SharedCache;
+ DependencyScanningWorkerFilesystem DepFS(SharedCache,
+ InstrumentingFS);
+
+ DepFS.status("/foo");
+ DepFS.status("/foo");
+ DepFS.status("/bar");
+ EXPECT_EQ(InstrumentingFS->NumStatusCalls, 2u);
+
+ DepFS.exists("/foo");
+ DepFS.exists("/bar");
+ EXPECT_EQ(InstrumentingFS->NumStatusCalls, 2u);
+ EXPECT_EQ(InstrumentingFS->NumExistsCalls, 0u);
+}
More information about the cfe-commits
mailing list