Skip to content

Commit 8e98fd0

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 12621dc commit 8e98fd0

File tree

4 files changed

+123
-70
lines changed

4 files changed

+123
-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: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,22 @@ struct InstrumentingFilesystem
3333
return ProxyFileSystem::getRealPath(Path, Output);
3434
}
3535
};
36+
37+
38+
struct InstrumentingInMemoryFilesystem
39+
: llvm::RTTIExtends<InstrumentingInMemoryFilesystem,
40+
llvm::vfs::InMemoryFileSystem> {
41+
unsigned NumStatCalls = 0;
42+
43+
using llvm::RTTIExtends<InstrumentingInMemoryFilesystem,
44+
llvm::vfs::InMemoryFileSystem>::RTTIExtends;
45+
46+
llvm::ErrorOr<llvm::vfs::Status> status(const llvm::Twine &Path) override {
47+
++NumStatCalls;
48+
return InMemoryFileSystem::status(Path);
49+
}
50+
};
51+
3652
} // namespace
3753

3854
TEST(DependencyScanningWorkerFilesystem, CacheStatusFailures) {
@@ -150,3 +166,25 @@ TEST(DependencyScanningFilesystem, RealPathAndStatusInvariants) {
150166
DepFS.status("/bar");
151167
}
152168
}
169+
170+
TEST(DependencyScanningFilesystem, CacheStatOnExists) {
171+
auto InMemoryInstrumentingFS =
172+
llvm::makeIntrusiveRefCnt<InstrumentingInMemoryFilesystem>();
173+
InMemoryInstrumentingFS->setCurrentWorkingDirectory("/");
174+
InMemoryInstrumentingFS->addFile("/foo", 0,
175+
llvm::MemoryBuffer::getMemBuffer(""));
176+
InMemoryInstrumentingFS->addFile("/bar", 0,
177+
llvm::MemoryBuffer::getMemBuffer(""));
178+
DependencyScanningFilesystemSharedCache SharedCache;
179+
DependencyScanningWorkerFilesystem DepFS(SharedCache,
180+
InMemoryInstrumentingFS);
181+
182+
DepFS.status("/foo");
183+
DepFS.status("/foo");
184+
DepFS.status("/bar");
185+
EXPECT_EQ(InMemoryInstrumentingFS->NumStatCalls, 2u);
186+
187+
DepFS.exists("/foo");
188+
DepFS.exists("/bar");
189+
EXPECT_EQ(InMemoryInstrumentingFS->NumStatCalls, 2u);
190+
}

‎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)