Skip to content

Commit 24e869d

Browse files
artemcmartemcm
committed
[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'.
1 parent 5889874 commit 24e869d

File tree

4 files changed

+131
-70
lines changed

4 files changed

+131
-70
lines changed

‎clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -365,6 +365,10 @@ class DependencyScanningWorkerFilesystem
365365
/// false if not (i.e. this entry is not a file or its scan fails).
366366
bool ensureDirectiveTokensArePopulated(EntryRef Entry);
367367

368+
/// Check whether \p Path exists. By default checks cached result of \c
369+
/// status(), and falls back on FS if unable to do so.
370+
bool exists(const Twine &Path) override;
371+
368372
private:
369373
/// For a filename that's not yet associated with any entry in the caches,
370374
/// uses the underlying filesystem to either look up the entry based in the

‎clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -300,6 +300,17 @@ DependencyScanningWorkerFilesystem::status(const Twine &Path) {
300300
return Result->getStatus();
301301
}
302302

303+
bool DependencyScanningWorkerFilesystem::exists(const Twine &Path) {
304+
// While some VFS overlay filesystems may implement more-efficient
305+
// mechanisms for `exists` queries, `DependencyScanningWorkerFilesystem`
306+
// typically wraps `RealFileSystem` which does not specialize `exists`,
307+
// so it is not likely to benefit from such optimizations. Instead,
308+
// it is more-valuable to have this query go through the
309+
// cached-`status` code-path of the `DependencyScanningWorkerFilesystem`.
310+
llvm::ErrorOr<llvm::vfs::Status> Status = status(Path);
311+
return Status && Status->exists();
312+
}
313+
303314
namespace {
304315

305316
/// The VFS that is used by clang consumes the \c CachedFileSystemEntry using

‎clang/unittests/Tooling/DependencyScanning/DependencyScanningFilesystemTest.cpp

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88

99
#include "clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h"
1010
#include "llvm/ADT/SmallString.h"
11+
#include "llvm/ADT/Twine.h"
1112
#include "llvm/Support/VirtualFileSystem.h"
1213
#include "gtest/gtest.h"
1314

@@ -33,6 +34,28 @@ struct InstrumentingFilesystem
3334
return ProxyFileSystem::getRealPath(Path, Output);
3435
}
3536
};
37+
38+
39+
struct InstrumentingInMemoryFilesystem
40+
: llvm::RTTIExtends<InstrumentingInMemoryFilesystem,
41+
llvm::vfs::InMemoryFileSystem> {
42+
unsigned NumStatCalls = 0;
43+
unsigned NumExistsCalls = 0;
44+
45+
using llvm::RTTIExtends<InstrumentingInMemoryFilesystem,
46+
llvm::vfs::InMemoryFileSystem>::RTTIExtends;
47+
48+
llvm::ErrorOr<llvm::vfs::Status> status(const llvm::Twine &Path) override {
49+
++NumStatCalls;
50+
return InMemoryFileSystem::status(Path);
51+
}
52+
53+
bool exists(const llvm::Twine &Path) override {
54+
++NumExistsCalls;
55+
return InMemoryFileSystem::exists(Path);
56+
}
57+
};
58+
3659
} // namespace
3760

3861
TEST(DependencyScanningWorkerFilesystem, CacheStatusFailures) {
@@ -147,3 +170,26 @@ TEST(DependencyScanningFilesystem, RealPathAndStatusInvariants) {
147170
DepFS.status("/bar");
148171
}
149172
}
173+
174+
TEST(DependencyScanningFilesystem, CacheStatOnExists) {
175+
auto InMemoryInstrumentingFS =
176+
llvm::makeIntrusiveRefCnt<InstrumentingInMemoryFilesystem>();
177+
InMemoryInstrumentingFS->setCurrentWorkingDirectory("/");
178+
InMemoryInstrumentingFS->addFile("/foo", 0,
179+
llvm::MemoryBuffer::getMemBuffer(""));
180+
InMemoryInstrumentingFS->addFile("/bar", 0,
181+
llvm::MemoryBuffer::getMemBuffer(""));
182+
DependencyScanningFilesystemSharedCache SharedCache;
183+
DependencyScanningWorkerFilesystem DepFS(SharedCache,
184+
InMemoryInstrumentingFS);
185+
186+
DepFS.status("/foo");
187+
DepFS.status("/foo");
188+
DepFS.status("/bar");
189+
EXPECT_EQ(InMemoryInstrumentingFS->NumStatCalls, 2u);
190+
191+
DepFS.exists("/foo");
192+
DepFS.exists("/bar");
193+
EXPECT_EQ(InMemoryInstrumentingFS->NumStatCalls, 2u);
194+
EXPECT_EQ(InMemoryInstrumentingFS->NumExistsCalls, 0u);
195+
}

‎llvm/unittests/Support/VirtualFileSystemTest.cpp

Lines changed: 70 additions & 70 deletions
Original file line numberDiff line numberDiff line change
@@ -3443,51 +3443,51 @@ TEST(RedirectingFileSystemTest, ExternalPaths) {
34433443
TEST(RedirectingFileSystemTest, Exists) {
34443444
IntrusiveRefCntPtr<DummyFileSystem> Dummy(new NoStatusDummyFileSystem());
34453445
auto YAML =
3446-
MemoryBuffer::getMemBuffer("{\n"
3447-
" 'version': 0,\n"
3448-
" 'roots': [\n"
3449-
" {\n"
3450-
" 'type': 'directory-remap',\n"
3451-
" 'name': '/dremap',\n"
3452-
" 'external-contents': '/a',\n"
3453-
" },"
3454-
" {\n"
3455-
" 'type': 'directory-remap',\n"
3456-
" 'name': '/dmissing',\n"
3457-
" 'external-contents': '/dmissing',\n"
3458-
" },"
3459-
" {\n"
3460-
" 'type': 'directory',\n"
3461-
" 'name': '/both',\n"
3462-
" 'contents': [\n"
3463-
" {\n"
3464-
" 'type': 'file',\n"
3465-
" 'name': 'vfile',\n"
3466-
" 'external-contents': '/c'\n"
3467-
" }\n"
3468-
" ]\n"
3469-
" },\n"
3470-
" {\n"
3471-
" 'type': 'directory',\n"
3472-
" 'name': '/vdir',\n"
3473-
" 'contents': ["
3474-
" {\n"
3475-
" 'type': 'directory-remap',\n"
3476-
" 'name': 'dremap',\n"
3477-
" 'external-contents': '/b'\n"
3478-
" },\n"
3479-
" {\n"
3480-
" 'type': 'file',\n"
3481-
" 'name': 'missing',\n"
3482-
" 'external-contents': '/missing'\n"
3483-
" },\n"
3484-
" {\n"
3485-
" 'type': 'file',\n"
3486-
" 'name': 'vfile',\n"
3487-
" 'external-contents': '/c'\n"
3488-
" }]\n"
3489-
" }]\n"
3490-
"}");
3446+
MemoryBuffer::getMemBuffer("{\n"
3447+
" 'version': 0,\n"
3448+
" 'roots': [\n"
3449+
" {\n"
3450+
" 'type': 'directory-remap',\n"
3451+
" 'name': '/dremap',\n"
3452+
" 'external-contents': '/a',\n"
3453+
" },"
3454+
" {\n"
3455+
" 'type': 'directory-remap',\n"
3456+
" 'name': '/dmissing',\n"
3457+
" 'external-contents': '/dmissing',\n"
3458+
" },"
3459+
" {\n"
3460+
" 'type': 'directory',\n"
3461+
" 'name': '/both',\n"
3462+
" 'contents': [\n"
3463+
" {\n"
3464+
" 'type': 'file',\n"
3465+
" 'name': 'vfile',\n"
3466+
" 'external-contents': '/c'\n"
3467+
" }\n"
3468+
" ]\n"
3469+
" },\n"
3470+
" {\n"
3471+
" 'type': 'directory',\n"
3472+
" 'name': '/vdir',\n"
3473+
" 'contents': ["
3474+
" {\n"
3475+
" 'type': 'directory-remap',\n"
3476+
" 'name': 'dremap',\n"
3477+
" 'external-contents': '/b'\n"
3478+
" },\n"
3479+
" {\n"
3480+
" 'type': 'file',\n"
3481+
" 'name': 'missing',\n"
3482+
" 'external-contents': '/missing'\n"
3483+
" },\n"
3484+
" {\n"
3485+
" 'type': 'file',\n"
3486+
" 'name': 'vfile',\n"
3487+
" 'external-contents': '/c'\n"
3488+
" }]\n"
3489+
" }]\n"
3490+
"}");
34913491

34923492
Dummy->addDirectory("/a");
34933493
Dummy->addRegularFile("/a/foo");
@@ -3496,7 +3496,7 @@ TEST(RedirectingFileSystemTest, Exists) {
34963496
Dummy->addRegularFile("/both/foo");
34973497

34983498
auto Redirecting = vfs::RedirectingFileSystem::create(
3499-
std::move(YAML), nullptr, "", nullptr, Dummy);
3499+
std::move(YAML), nullptr, "", nullptr, Dummy);
35003500

35013501
EXPECT_TRUE(Redirecting->exists("/dremap"));
35023502
EXPECT_FALSE(Redirecting->exists("/dmissing"));
@@ -3514,22 +3514,22 @@ TEST(RedirectingFileSystemTest, Exists) {
35143514
TEST(RedirectingFileSystemTest, ExistsFallback) {
35153515
IntrusiveRefCntPtr<DummyFileSystem> Dummy(new NoStatusDummyFileSystem());
35163516
auto YAML =
3517-
MemoryBuffer::getMemBuffer("{\n"
3518-
" 'version': 0,\n"
3519-
" 'redirecting-with': 'fallback',\n"
3520-
" 'roots': [\n"
3521-
" {\n"
3522-
" 'type': 'file',\n"
3523-
" 'name': '/fallback',\n"
3524-
" 'external-contents': '/missing',\n"
3525-
" },"
3526-
" ]\n"
3527-
"}");
3517+
MemoryBuffer::getMemBuffer("{\n"
3518+
" 'version': 0,\n"
3519+
" 'redirecting-with': 'fallback',\n"
3520+
" 'roots': [\n"
3521+
" {\n"
3522+
" 'type': 'file',\n"
3523+
" 'name': '/fallback',\n"
3524+
" 'external-contents': '/missing',\n"
3525+
" },"
3526+
" ]\n"
3527+
"}");
35283528

35293529
Dummy->addRegularFile("/fallback");
35303530

35313531
auto Redirecting = vfs::RedirectingFileSystem::create(
3532-
std::move(YAML), nullptr, "", nullptr, Dummy);
3532+
std::move(YAML), nullptr, "", nullptr, Dummy);
35333533

35343534
EXPECT_TRUE(Redirecting->exists("/fallback"));
35353535
EXPECT_FALSE(Redirecting->exists("/missing"));
@@ -3538,23 +3538,23 @@ TEST(RedirectingFileSystemTest, ExistsFallback) {
35383538
TEST(RedirectingFileSystemTest, ExistsRedirectOnly) {
35393539
IntrusiveRefCntPtr<DummyFileSystem> Dummy(new NoStatusDummyFileSystem());
35403540
auto YAML =
3541-
MemoryBuffer::getMemBuffer("{\n"
3542-
" 'version': 0,\n"
3543-
" 'redirecting-with': 'redirect-only',\n"
3544-
" 'roots': [\n"
3545-
" {\n"
3546-
" 'type': 'file',\n"
3547-
" 'name': '/vfile',\n"
3548-
" 'external-contents': '/a',\n"
3549-
" },"
3550-
" ]\n"
3551-
"}");
3541+
MemoryBuffer::getMemBuffer("{\n"
3542+
" 'version': 0,\n"
3543+
" 'redirecting-with': 'redirect-only',\n"
3544+
" 'roots': [\n"
3545+
" {\n"
3546+
" 'type': 'file',\n"
3547+
" 'name': '/vfile',\n"
3548+
" 'external-contents': '/a',\n"
3549+
" },"
3550+
" ]\n"
3551+
"}");
35523552

35533553
Dummy->addRegularFile("/a");
35543554
Dummy->addRegularFile("/b");
35553555

35563556
auto Redirecting = vfs::RedirectingFileSystem::create(
3557-
std::move(YAML), nullptr, "", nullptr, Dummy);
3557+
std::move(YAML), nullptr, "", nullptr, Dummy);
35583558

35593559
EXPECT_FALSE(Redirecting->exists("/a"));
35603560
EXPECT_FALSE(Redirecting->exists("/b"));

0 commit comments

Comments
 (0)