https://github.com/artemcm updated https://github.com/llvm/llvm-project/pull/88152
>From 1dd15b331795daaf2ac70cbd013f56f2d7677041 Mon Sep 17 00:00:00 2001 From: Artem Chikin <achi...@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 ++++++++++++++----- 3 files changed, 55 insertions(+), 13 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); +} _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits