[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 14:06:54 PDT 2024
https://github.com/artemcm updated https://github.com/llvm/llvm-project/pull/88152
>From 8164aaf2e93dde4761789e6d574088d1cb0e97fc 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 | 53 +++++--
.../Support/VirtualFileSystemTest.cpp | 140 +++++++++---------
4 files changed, 125 insertions(+), 83 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..256dd980b2118f 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"
@@ -15,31 +16,36 @@ using namespace clang::tooling::dependencies;
namespace {
struct InstrumentingFilesystem
- : llvm::RTTIExtends<InstrumentingFilesystem, llvm::vfs::ProxyFileSystem> {
+ : llvm::RTTIExtends<InstrumentingFilesystem, llvm::vfs::InMemoryFileSystem> {
unsigned NumStatusCalls = 0;
unsigned NumGetRealPathCalls = 0;
+ unsigned NumExistsCalls = 0;
using llvm::RTTIExtends<InstrumentingFilesystem,
- llvm::vfs::ProxyFileSystem>::RTTIExtends;
+ llvm::vfs::InMemoryFileSystem>::RTTIExtends;
llvm::ErrorOr<llvm::vfs::Status> status(const llvm::Twine &Path) override {
++NumStatusCalls;
- return ProxyFileSystem::status(Path);
+ return InMemoryFileSystem::status(Path);
}
std::error_code getRealPath(const llvm::Twine &Path,
llvm::SmallVectorImpl<char> &Output) override {
++NumGetRealPathCalls;
- return ProxyFileSystem::getRealPath(Path, Output);
+ return InMemoryFileSystem::getRealPath(Path, Output);
+ }
+
+ bool exists(const llvm::Twine &Path) override {
+ ++NumExistsCalls;
+ return InMemoryFileSystem::exists(Path);
}
};
+
} // namespace
TEST(DependencyScanningWorkerFilesystem, CacheStatusFailures) {
- auto InMemoryFS = llvm::makeIntrusiveRefCnt<llvm::vfs::InMemoryFileSystem>();
-
auto InstrumentingFS =
- llvm::makeIntrusiveRefCnt<InstrumentingFilesystem>(InMemoryFS);
+ llvm::makeIntrusiveRefCnt<InstrumentingFilesystem>();
DependencyScanningFilesystemSharedCache SharedCache;
DependencyScanningWorkerFilesystem DepFS(SharedCache, InstrumentingFS);
@@ -59,13 +65,11 @@ TEST(DependencyScanningWorkerFilesystem, CacheStatusFailures) {
}
TEST(DependencyScanningFilesystem, CacheGetRealPath) {
- auto InMemoryFS = llvm::makeIntrusiveRefCnt<llvm::vfs::InMemoryFileSystem>();
- InMemoryFS->setCurrentWorkingDirectory("/");
- InMemoryFS->addFile("/foo", 0, llvm::MemoryBuffer::getMemBuffer(""));
- InMemoryFS->addFile("/bar", 0, llvm::MemoryBuffer::getMemBuffer(""));
-
auto InstrumentingFS =
- llvm::makeIntrusiveRefCnt<InstrumentingFilesystem>(InMemoryFS);
+ llvm::makeIntrusiveRefCnt<InstrumentingFilesystem>();
+ InstrumentingFS->setCurrentWorkingDirectory("/");
+ InstrumentingFS->addFile("/foo", 0, llvm::MemoryBuffer::getMemBuffer(""));
+ InstrumentingFS->addFile("/bar", 0, llvm::MemoryBuffer::getMemBuffer(""));
DependencyScanningFilesystemSharedCache SharedCache;
DependencyScanningWorkerFilesystem DepFS(SharedCache, InstrumentingFS);
@@ -147,3 +151,26 @@ TEST(DependencyScanningFilesystem, RealPathAndStatusInvariants) {
DepFS.status("/bar");
}
}
+
+TEST(DependencyScanningFilesystem, CacheStatOnExists) {
+ auto InMemoryInstrumentingFS =
+ llvm::makeIntrusiveRefCnt<InstrumentingFilesystem>();
+ 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->NumStatusCalls, 2u);
+
+ DepFS.exists("/foo");
+ DepFS.exists("/bar");
+ EXPECT_EQ(InMemoryInstrumentingFS->NumStatusCalls, 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