Skip to content

Commit f1c4624

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 c2b2cd0 commit f1c4624

File tree

6 files changed

+141
-73
lines changed

6 files changed

+141
-73
lines changed

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

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

313+
/// Check whether \p Path exists. By default checks cached result of \c
314+
/// status(), and falls back on FS if unable to do so.
315+
bool exists(const Twine &Path) override;
316+
313317
private:
314318
/// For a filename that's not yet associated with any entry in the caches,
315319
/// 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
@@ -270,6 +270,17 @@ DependencyScanningWorkerFilesystem::status(const Twine &Path) {
270270
return Result->getStatus();
271271
}
272272

273+
bool DependencyScanningWorkerFilesystem::exists(const Twine &Path) {
274+
// While some VFS overlay filesystems may implement more-efficient
275+
// mechanisms for `exists` queries, `DependencyScanningWorkerFilesystem`
276+
// typically wraps `RealFileSystem` which does not specialize `exists`,
277+
// so it is not likely to benefit from such optimizations. Instead,
278+
// it is more-valuable to have this query go through the
279+
// cached-`status` code-path of the `DependencyScanningWorkerFilesystem`.
280+
llvm::ErrorOr<llvm::vfs::Status> Status = status(Path);
281+
return Status && Status->exists();
282+
}
283+
273284
namespace {
274285

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

‎clang/unittests/Tooling/CMakeLists.txt‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@ add_clang_unittest(ToolingTests
1313
CastExprTest.cpp
1414
CommentHandlerTest.cpp
1515
CompilationDatabaseTest.cpp
16-
DependencyScannerTest.cpp
1716
DiagnosticsYamlTest.cpp
1817
ExecutionTest.cpp
1918
FixItTest.cpp
@@ -24,6 +23,8 @@ add_clang_unittest(ToolingTests
2423
LookupTest.cpp
2524
QualTypeNamesTest.cpp
2625
RangeSelectorTest.cpp
26+
DependencyScanning/DependencyScannerTest.cpp
27+
DependencyScanning/DependencyScanningFilesystemTest.cpp
2728
RecursiveASTVisitorTests/Attr.cpp
2829
RecursiveASTVisitorTests/BitfieldInitializer.cpp
2930
RecursiveASTVisitorTests/CallbacksLeaf.cpp

‎clang/unittests/Tooling/DependencyScannerTest.cpp‎ renamed to ‎clang/unittests/Tooling/DependencyScanning/DependencyScannerTest.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
//===- unittest/Tooling/ToolingTest.cpp - Tooling unit tests --------------===//
1+
//===- DependencyScannerTest.cpp - Tooling unit tests ---------------------===//
22
//
33
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
44
// See https://llvm.org/LICENSE.txt for license information.

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

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
//===- DependencyScanningFilesystemTest.cpp -------------------------------===//
2+
//
3+
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
4+
// See https://llvm.org/LICENSE.txt for license information.
5+
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
6+
//
7+
//===----------------------------------------------------------------------===//
8+
9+
#include "clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h"
10+
#include "llvm/ADT/Twine.h"
11+
#include "llvm/Support/VirtualFileSystem.h"
12+
#include "gtest/gtest.h"
13+
14+
using namespace clang::tooling::dependencies;
15+
16+
namespace {
17+
struct InstrumentingInMemoryFilesystem
18+
: llvm::RTTIExtends<InstrumentingInMemoryFilesystem,
19+
llvm::vfs::InMemoryFileSystem> {
20+
unsigned NumStatCalls = 0;
21+
22+
using llvm::RTTIExtends<InstrumentingInMemoryFilesystem,
23+
llvm::vfs::InMemoryFileSystem>::RTTIExtends;
24+
25+
llvm::ErrorOr<llvm::vfs::Status> status(const llvm::Twine &Path) override {
26+
++NumStatCalls;
27+
return InMemoryFileSystem::status(Path);
28+
}
29+
};
30+
} // namespace
31+
32+
TEST(DependencyScanningFilesystem, CacheStatOnExists) {
33+
auto InMemoryInstrumentingFS =
34+
llvm::makeIntrusiveRefCnt<InstrumentingInMemoryFilesystem>();
35+
InMemoryInstrumentingFS->setCurrentWorkingDirectory("/");
36+
InMemoryInstrumentingFS->addFile("/foo", 0,
37+
llvm::MemoryBuffer::getMemBuffer(""));
38+
InMemoryInstrumentingFS->addFile("/bar", 0,
39+
llvm::MemoryBuffer::getMemBuffer(""));
40+
DependencyScanningFilesystemSharedCache SharedCache;
41+
DependencyScanningWorkerFilesystem DepFS(SharedCache,
42+
InMemoryInstrumentingFS);
43+
44+
DepFS.status("/foo");
45+
DepFS.status("/foo");
46+
DepFS.status("/bar");
47+
EXPECT_EQ(InMemoryInstrumentingFS->NumStatCalls, 2u);
48+
49+
DepFS.exists("/foo");
50+
DepFS.exists("/bar");
51+
EXPECT_EQ(InMemoryInstrumentingFS->NumStatCalls, 2u);
52+
}

‎llvm/unittests/Support/VirtualFileSystemTest.cpp‎

Lines changed: 71 additions & 71 deletions
Original file line numberDiff line numberDiff line change
@@ -211,7 +211,7 @@ class NoStatusDummyFileSystem : public DummyFileSystem {
211211
}
212212

213213
bool exists(const Twine &Path) override {
214-
auto Status = DummyFileSystem::status(Path);
214+
llvm::ErrorOr<llvm::vfs::Status> Status = status(Path);
215215
return Status && Status->exists();
216216
}
217217
};
@@ -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)