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 <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 | 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")); _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits